diff mbox series

[v2] hw/scsi/megasas: Silent GCC duplicated-cond warning

Message ID 20230328210126.16282-1-philmd@linaro.org
State Superseded
Headers show
Series [v2] hw/scsi/megasas: Silent GCC duplicated-cond warning | expand

Commit Message

Philippe Mathieu-Daudé March 28, 2023, 9:01 p.m. UTC 
From: Philippe Mathieu-Daudé <philmd@redhat.com>

GCC9 is confused when building with CFLAG -O3:

  hw/scsi/megasas.c: In function ‘megasas_scsi_realize’:
  hw/scsi/megasas.c:2387:26: error: duplicated ‘if’ condition [-Werror=duplicated-cond]
   2387 |     } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
  hw/scsi/megasas.c:2385:19: note: previously used here
   2385 |     if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
  cc1: all warnings being treated as errors

When this device was introduced in commit e8f943c3bcc, the author
cared about modularity, using a definition for the firmware limit.

However if the firmware limit isn't changed (MEGASAS_MAX_SGE = 128),
the code ends doing the same check twice.

Per the maintainer [*]:

> The original code assumed that one could change MFI_PASS_FRAME_SIZE,
> but it turned out not to be possible as it's being hardcoded in the
> drivers themselves (even though the interface provides mechanisms to
> query it). So we can remove the duplicate lines.

Add the 'MEGASAS_MIN_SGE' definition for the '64' magic value,
slightly rewrite the condition check to simplify a bit the logic
and remove the unnecessary / duplicated check.

[*] https://lore.kernel.org/qemu-devel/e0029fc5-882f-1d63-15e3-1c3dbe9b6a2c@suse.de/

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
v1: https://lore.kernel.org/qemu-devel/20191217173425.5082-6-philmd@redhat.com/
---
 hw/scsi/megasas.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

Comments

Philippe Mathieu-Daudé June 5, 2023, 1:44 p.m. UTC | #1 
ping?

On 28/3/23 23:01, Philippe Mathieu-Daudé wrote:
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> GCC9 is confused when building with CFLAG -O3:
> 
>    hw/scsi/megasas.c: In function ‘megasas_scsi_realize’:
>    hw/scsi/megasas.c:2387:26: error: duplicated ‘if’ condition [-Werror=duplicated-cond]
>     2387 |     } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
>    hw/scsi/megasas.c:2385:19: note: previously used here
>     2385 |     if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
>    cc1: all warnings being treated as errors
> 
> When this device was introduced in commit e8f943c3bcc, the author
> cared about modularity, using a definition for the firmware limit.
> 
> However if the firmware limit isn't changed (MEGASAS_MAX_SGE = 128),
> the code ends doing the same check twice.
> 
> Per the maintainer [*]:
> 
>> The original code assumed that one could change MFI_PASS_FRAME_SIZE,
>> but it turned out not to be possible as it's being hardcoded in the
>> drivers themselves (even though the interface provides mechanisms to
>> query it). So we can remove the duplicate lines.
> 
> Add the 'MEGASAS_MIN_SGE' definition for the '64' magic value,
> slightly rewrite the condition check to simplify a bit the logic
> and remove the unnecessary / duplicated check.
> 
> [*] https://lore.kernel.org/qemu-devel/e0029fc5-882f-1d63-15e3-1c3dbe9b6a2c@suse.de/
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> v1: https://lore.kernel.org/qemu-devel/20191217173425.5082-6-philmd@redhat.com/
> ---
>   hw/scsi/megasas.c | 16 ++++++++++------
>   1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index 9cbbb16121..32c70c9e99 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -42,6 +42,7 @@
>   #define MEGASAS_MAX_FRAMES 2048         /* Firmware limit at 65535 */
>   #define MEGASAS_DEFAULT_FRAMES 1000     /* Windows requires this */
>   #define MEGASAS_GEN2_DEFAULT_FRAMES 1008     /* Windows requires this */
> +#define MEGASAS_MIN_SGE 64
>   #define MEGASAS_MAX_SGE 128             /* Firmware limit */
>   #define MEGASAS_DEFAULT_SGE 80
>   #define MEGASAS_MAX_SECTORS 0xFFFF      /* No real limit */
> @@ -2356,6 +2357,7 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
>       MegasasState *s = MEGASAS(dev);
>       MegasasBaseClass *b = MEGASAS_GET_CLASS(s);
>       uint8_t *pci_conf;
> +    uint32_t sge;
>       int i, bar_type;
>       Error *err = NULL;
>       int ret;
> @@ -2424,13 +2426,15 @@ 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) {
> -        s->fw_sge = 128 - MFI_PASS_FRAME_SIZE;
> -    } else {
> -        s->fw_sge = 64 - MFI_PASS_FRAME_SIZE;
> +
> +    sge = s->fw_sge + MFI_PASS_FRAME_SIZE;
> +    if (sge < MEGASAS_MIN_SGE) {
> +        sge = MEGASAS_MIN_SGE;
> +    } else if (sge >= MEGASAS_MAX_SGE) {
> +        sge = MEGASAS_MAX_SGE;
>       }
> +    s->fw_sge = sge - MFI_PASS_FRAME_SIZE;
> +
>       if (s->fw_cmds > MEGASAS_MAX_FRAMES) {
>           s->fw_cmds = MEGASAS_MAX_FRAMES;
>       }
Hannes Reinecke June 5, 2023, 3:10 p.m. UTC | #2 
On 6/5/23 15:44, Philippe Mathieu-Daudé wrote:
> ping?
> 
> On 28/3/23 23:01, Philippe Mathieu-Daudé wrote:
>> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
>> GCC9 is confused when building with CFLAG -O3:
>>
>>    hw/scsi/megasas.c: In function ‘megasas_scsi_realize’:
>>    hw/scsi/megasas.c:2387:26: error: duplicated ‘if’ condition 
>> [-Werror=duplicated-cond]
>>     2387 |     } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
>>    hw/scsi/megasas.c:2385:19: note: previously used here
>>     2385 |     if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
>>    cc1: all warnings being treated as errors
>>
>> When this device was introduced in commit e8f943c3bcc, the author
>> cared about modularity, using a definition for the firmware limit.
>>
>> However if the firmware limit isn't changed (MEGASAS_MAX_SGE = 128),
>> the code ends doing the same check twice.
>>
>> Per the maintainer [*]:
>>
>>> The original code assumed that one could change MFI_PASS_FRAME_SIZE,
>>> but it turned out not to be possible as it's being hardcoded in the
>>> drivers themselves (even though the interface provides mechanisms to
>>> query it). So we can remove the duplicate lines.
>>
>> Add the 'MEGASAS_MIN_SGE' definition for the '64' magic value,
>> slightly rewrite the condition check to simplify a bit the logic
>> and remove the unnecessary / duplicated check.
>>
>> [*] 
>> https://lore.kernel.org/qemu-devel/e0029fc5-882f-1d63-15e3-1c3dbe9b6a2c@suse.de/
>>
>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> v1: 
>> https://lore.kernel.org/qemu-devel/20191217173425.5082-6-philmd@redhat.com/
>> ---
>>   hw/scsi/megasas.c | 16 ++++++++++------
>>   1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
>> index 9cbbb16121..32c70c9e99 100644
>> --- a/hw/scsi/megasas.c
>> +++ b/hw/scsi/megasas.c
>> @@ -42,6 +42,7 @@
>>   #define MEGASAS_MAX_FRAMES 2048         /* Firmware limit at 65535 */
>>   #define MEGASAS_DEFAULT_FRAMES 1000     /* Windows requires this */
>>   #define MEGASAS_GEN2_DEFAULT_FRAMES 1008     /* Windows requires 
>> this */
>> +#define MEGASAS_MIN_SGE 64
>>   #define MEGASAS_MAX_SGE 128             /* Firmware limit */
>>   #define MEGASAS_DEFAULT_SGE 80
>>   #define MEGASAS_MAX_SECTORS 0xFFFF      /* No real limit */
>> @@ -2356,6 +2357,7 @@ static void megasas_scsi_realize(PCIDevice *dev, 
>> Error **errp)
>>       MegasasState *s = MEGASAS(dev);
>>       MegasasBaseClass *b = MEGASAS_GET_CLASS(s);
>>       uint8_t *pci_conf;
>> +    uint32_t sge;
>>       int i, bar_type;
>>       Error *err = NULL;
>>       int ret;
>> @@ -2424,13 +2426,15 @@ 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) {
>> -        s->fw_sge = 128 - MFI_PASS_FRAME_SIZE;
>> -    } else {
>> -        s->fw_sge = 64 - MFI_PASS_FRAME_SIZE;
>> +
>> +    sge = s->fw_sge + MFI_PASS_FRAME_SIZE;
>> +    if (sge < MEGASAS_MIN_SGE) {
>> +        sge = MEGASAS_MIN_SGE;
>> +    } else if (sge >= MEGASAS_MAX_SGE) {
>> +        sge = MEGASAS_MAX_SGE;
>>       }
>> +    s->fw_sge = sge - MFI_PASS_FRAME_SIZE;
>> +
>>       if (s->fw_cmds > MEGASAS_MAX_FRAMES) {
>>           s->fw_cmds = MEGASAS_MAX_FRAMES;
>>       }
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
diff mbox series

Patch

diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 9cbbb16121..32c70c9e99 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -42,6 +42,7 @@ 
 #define MEGASAS_MAX_FRAMES 2048         /* Firmware limit at 65535 */
 #define MEGASAS_DEFAULT_FRAMES 1000     /* Windows requires this */
 #define MEGASAS_GEN2_DEFAULT_FRAMES 1008     /* Windows requires this */
+#define MEGASAS_MIN_SGE 64
 #define MEGASAS_MAX_SGE 128             /* Firmware limit */
 #define MEGASAS_DEFAULT_SGE 80
 #define MEGASAS_MAX_SECTORS 0xFFFF      /* No real limit */
@@ -2356,6 +2357,7 @@  static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
     MegasasState *s = MEGASAS(dev);
     MegasasBaseClass *b = MEGASAS_GET_CLASS(s);
     uint8_t *pci_conf;
+    uint32_t sge;
     int i, bar_type;
     Error *err = NULL;
     int ret;
@@ -2424,13 +2426,15 @@  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) {
-        s->fw_sge = 128 - MFI_PASS_FRAME_SIZE;
-    } else {
-        s->fw_sge = 64 - MFI_PASS_FRAME_SIZE;
+
+    sge = s->fw_sge + MFI_PASS_FRAME_SIZE;
+    if (sge < MEGASAS_MIN_SGE) {
+        sge = MEGASAS_MIN_SGE;
+    } else if (sge >= MEGASAS_MAX_SGE) {
+        sge = MEGASAS_MAX_SGE;
     }
+    s->fw_sge = sge - MFI_PASS_FRAME_SIZE;
+
     if (s->fw_cmds > MEGASAS_MAX_FRAMES) {
         s->fw_cmds = MEGASAS_MAX_FRAMES;
     }