ata: pata_via: fix sloppy typing in via_do_set_mode()

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@omp.ru>

---
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(-)

On 4/12/22 05:34, Sergey Shtylyov 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@omp.ru>
> 
> ---
> 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...

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
the type change is mandated, unless that function is changed too, but that
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).


>  	unsigned long UT = T;
>  	int ut;
>  	int offset = 3 - (2*ap->port_no) - adev->devno;

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@omp.ru>
>>
>> ---
>> 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