On 4/12/22 05:42, Sergey Shtylyov wrote:
> Hello!
>
> On 4/11/22 2:47 AM, Damien Le Moal wrote:
>
>>> sil680_sel{dev|reg}() return a PCI config space address but needlessly
>>> use the *unsigned long* type for that, whereas the PCI config space
>>> accessors take *int* for the address parameter. Switch these functions
>>> to returning *int*, updating the local variables at their call sites.
>>> Add the empty lines after some declarations, while at it...
>>>
>>> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
>>> analysis tool.
>>>
>>> Signed-off-by: Sergey Shtylyov <s.shtylyov@xxxxxx>
>>>
>>> ---
>>> This patch is against the 'for-next' branch of Damien Le Moal's 'libata.git'
>>> repo.
>>>
>>> drivers/ata/pata_sil680.c | 27 +++++++++++++++------------
>>> 1 file changed, 15 insertions(+), 12 deletions(-)
>>>
>>> Index: libata/drivers/ata/pata_sil680.c
>>> ===================================================================
>>> --- libata.orig/drivers/ata/pata_sil680.c
>>> +++ libata/drivers/ata/pata_sil680.c
>>> @@ -47,9 +47,10 @@
>>> * criticial.
>>> */
>>>
>>> -static unsigned long sil680_selreg(struct ata_port *ap, int r)
>>> +static int sil680_selreg(struct ata_port *ap, int r)
>>> {
>>> - unsigned long base = 0xA0 + r;
>>> + int base = 0xA0 + r;
>>> +
>>> base += (ap->port_no << 4);
>>> return base;
>>
>> The variable "base" is rather useless here... A simple:
>>
>> return 0xA0 + r + (ap->port_no << 4);
>>
>> would work too and is a lot cleaner.
>
> Yes, probably... but it's a matter of a separate patch, I think.
> Note that both functions are inlined by gcc.
No need for a separate patch. Your patch already changes the base variable
type. It may as well remove it :)
>
> [...]
>
> MBR, Sergey
--
Damien Le Moal
Western Digital Research
