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

[Richard Henderson <richard.henderson@linaro.org>]

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~