[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 5/6] hw/scsi/megasas: Silent GCC9 duplicated-cond warning
From: |
Paolo Bonzini |
Subject: |
Re: [PATCH 5/6] hw/scsi/megasas: Silent GCC9 duplicated-cond warning |
Date: |
Tue, 7 Jan 2020 15:56:40 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.1 |
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
- Re: [PATCH 5/6] hw/scsi/megasas: Silent GCC9 duplicated-cond warning,
Paolo Bonzini <=