On 4/12/22 2:03 AM, Damien Le Moal wrote:
>> The *unsigned long* variable 'T' is initialized with an *int* value
>> (luckily always positive) -- to avoid that, declare the 'via_clock'
>> variable as *unsigned int* and make the divisible constant *unsigned*
>> too. While at it, make the 'via_clock' and 'T' variables *const* as
>> they are never re-assigned after initialization -- the object code
>> remains the same as gcc previously used copy propagation anyway...
>>
>> 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_via.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> Index: libata/drivers/ata/pata_via.c
>> ===================================================================
>> --- libata.orig/drivers/ata/pata_via.c
>> +++ libata/drivers/ata/pata_via.c
>> @@ -248,8 +248,8 @@ static void via_do_set_mode(struct ata_p
>> struct pci_dev *pdev = to_pci_dev(ap->host->dev);
>> struct ata_device *peer = ata_dev_pair(adev);
>> struct ata_timing t, p;
>> - static int via_clock = 33333; /* Bus clock in kHZ */
>> - unsigned long T = 1000000000 / via_clock;
>> + const unsigned int via_clock = 33333; /* Bus clock in kHz */
>> + const unsigned long T = 1000000000U / via_clock;
>
> This looks OK, but via_clock is never used apart from here. Why even
> bother having a variable ? I suspect this was a mean of documenting the
> value meaning. To really clean this, I would define T as a macro...
I think *const* is preferable...
> But looking at other pata drivers, they all do something similar, and many
> of them have the same type issue. E.g. pata_amd:
>
> int T, UT;
> const int amd_clock = 33333; /* KHz. */
> u8 t;
>
> T = 1000000000 / amd_clock;
> UT = T;
>
> Also, ata_timing_compute() takes int as argument. So I do not think that
Ah, I failed to check that! The code cited above is correct then...
> the type change is mandated, unless that function is changed too, but that
We should change T's type to *int* then...
> could lead to a very large set of changes. Unless these are causing
> problems, I am tempted to leave everything as is (apart for the clearly
> wrong "static" declaration of via_clock).
I'll prepare v2 then...
MBR, Sergey
