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

[Paolo Bonzini]

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;

Paolo