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@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... 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; -- Damien Le Moal Western Digital Research
