[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 5/6] hw/scsi/megasas: Silent GCC9 duplicated-cond warning
From: |
Richard Henderson |
Subject: |
Re: [PATCH 5/6] hw/scsi/megasas: Silent GCC9 duplicated-cond warning |
Date: |
Wed, 18 Dec 2019 09:02:51 -1000 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 |
On 12/18/19 7:52 AM, Philippe Mathieu-Daudé wrote:
>>> - if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
>>> + if (MEGASAS_MAX_SGE > 128
>>> + && s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
>>> s->fw_sge = MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE;
>>> } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
>>> s->fw_sge = 128 - MFI_PASS_FRAME_SIZE;
>>>
>>
>> I'm not keen on this. It looks to me like the raw 128 case should be removed
>> -- surely that's the point of the symbolic constant. But I'll defer if a
>> maintainer disagrees.
>
> Is that approach acceptable?
>
> -- >8 --
> @@ -54,6 +54,9 @@
> #define MEGASAS_FLAG_USE_QUEUE64 1
> #define MEGASAS_MASK_USE_QUEUE64 (1 << MEGASAS_FLAG_USE_QUEUE64)
>
> +QEMU_BUILD_BUG_MSG(MEGASAS_MAX_SGE > 128,
> + "Firmware limit too big for this device model");
> +
> static const char *mfi_frame_desc[] = {
> "MFI init", "LD Read", "LD Write", "LD SCSI", "PD SCSI",
> "MFI Doorbell", "MFI Abort", "MFI SMP", "MFI Stop"};
> @@ -2382,9 +2385,7 @@ static void megasas_scsi_realize(PCIDevice *dev, Error
> **errp)
> if (!s->hba_serial) {
> s->hba_serial = g_strdup(MEGASAS_HBA_SERIAL);
> }
> - if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
> - s->fw_sge = MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE;
> - } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
> + if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
> s->fw_sge = 128 - MFI_PASS_FRAME_SIZE;
> } else {
> s->fw_sge = 64 - MFI_PASS_FRAME_SIZE;
Eh. I suppose. But what's the point of leaving the hard-coded 128? There's
certainly something I'm missing here...
r~