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

[Hannes Reinecke <hare@suse.de>]

On 1/7/20 3:56 PM, Paolo Bonzini wrote:
> On 18/12/19 05:03, Richard Henderson wrote:
>>>      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;
>>>      } else {
>>>          s->fw_sge = 64 - 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.
> 
> I don't really understand this chain of ifs.  Hannes, does it make sense
> to just remove the "if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE)" case,
> or does Phil's variation (quoted in the patch fragment above) makes sense?
> 
> Or perhaps this rewrite:
> 
>         max_sge = s->fw_sge + MFI_PASS_FRAME_SIZE;
>         if (max_sge < MEGASAS_MAX_SGE) {
>             if (max_sge < 64) {
>                 error(...);
>             } else {
>                 max_sge = max_sge < 128 ? 64 : 128;
>             }
>         }
> 	s->fw_sge = max_sge - MFI_PASS_FRAME_SIZE;
> 
Yeah, do it.
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.
But then I prefer to stick with the define, and avoid having to check
the magic '128' value directly; rather use the define throughout the code.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer