Re: [PATCH 5/6] hw/scsi/megasas: Silent GCC9 duplicated-cond warning

[Philippe Mathieu-Daudé]

On 12/18/19 5:03 AM, Richard Henderson wrote:
> On 12/17/19 7:34 AM, Philippe Mathieu-Daudé wrote:
> > 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.
> > If we modify the limit, the code is valid. Add a check if the
> > definition got modified to a bigger limit.
> >
> > Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> > ---
> > Cc: Hannes Reinecke <address@hidden>
> > Cc: Paolo Bonzini <address@hidden>
> > Cc: Fam Zheng <address@hidden>
> > Cc: address@hidden
> > ---
> >   hw/scsi/megasas.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> > index de9bd20887..ece1601b66 100644
> > --- a/hw/scsi/megasas.c
> > +++ b/hw/scsi/megasas.c
> > @@ -2382,7 +2382,8 @@ 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) {
> > +    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;
---