| Message ID | 9fceaba7-22e0-8cb2-fc69-04c0b8ece7db@omp.ru |
|---|---|
| State | New |
| Headers | show |
| Series | ata: pata_sil680: fix result type of sil680_sel{dev|reg}() | expand |
On 4/10/22 05:29, Sergey Shtylyov 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@omp.ru> > > --- > 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. > } > @@ -65,9 +66,10 @@ static unsigned long sil680_selreg(struc > * the unit shift. > */ > > -static unsigned long sil680_seldev(struct ata_port *ap, struct ata_device *adev, int r) > +static int sil680_seldev(struct ata_port *ap, struct ata_device *adev, int r) > { > - unsigned long base = 0xA0 + r; > + int base = 0xA0 + r; > + > base += (ap->port_no << 4); > base |= adev->devno ? 2 : 0; > return base; > @@ -85,8 +87,9 @@ static unsigned long sil680_seldev(struc > static int sil680_cable_detect(struct ata_port *ap) > { > struct pci_dev *pdev = to_pci_dev(ap->host->dev); > - unsigned long addr = sil680_selreg(ap, 0); > + int addr = sil680_selreg(ap, 0); > u8 ata66; > + > pci_read_config_byte(pdev, addr, &ata66); > if (ata66 & 1) > return ATA_CBL_PATA80; > @@ -113,9 +116,9 @@ static void sil680_set_piomode(struct at > 0x328A, 0x2283, 0x1281, 0x10C3, 0x10C1 > }; > > - unsigned long tfaddr = sil680_selreg(ap, 0x02); > - unsigned long addr = sil680_seldev(ap, adev, 0x04); > - unsigned long addr_mask = 0x80 + 4 * ap->port_no; > + int tfaddr = sil680_selreg(ap, 0x02); > + int addr = sil680_seldev(ap, adev, 0x04); > + int addr_mask = 0x80 + 4 * ap->port_no; > struct pci_dev *pdev = to_pci_dev(ap->host->dev); > int pio = adev->pio_mode - XFER_PIO_0; > int lowest_pio = pio; > @@ -165,9 +168,9 @@ static void sil680_set_dmamode(struct at > static const u16 dma_table[3] = { 0x2208, 0x10C2, 0x10C1 }; > > struct pci_dev *pdev = to_pci_dev(ap->host->dev); > - unsigned long ma = sil680_seldev(ap, adev, 0x08); > - unsigned long ua = sil680_seldev(ap, adev, 0x0C); > - unsigned long addr_mask = 0x80 + 4 * ap->port_no; > + int ma = sil680_seldev(ap, adev, 0x08); > + int ua = sil680_seldev(ap, adev, 0x0C); > + int addr_mask = 0x80 + 4 * ap->port_no; > int port_shift = adev->devno * 4; > u8 scsc, mode; > u16 multi, ultra; > @@ -219,7 +222,7 @@ static void sil680_sff_exec_command(stru > static bool sil680_sff_irq_check(struct ata_port *ap) > { > struct pci_dev *pdev = to_pci_dev(ap->host->dev); > - unsigned long addr = sil680_selreg(ap, 1); > + int addr = sil680_selreg(ap, 1); > u8 val; > > pci_read_config_byte(pdev, addr, &val);
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@omp.ru> >> >> --- >> 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. [...] MBR, Sergey
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@omp.ru> >>> >>> --- >>> 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
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; } @@ -65,9 +66,10 @@ static unsigned long sil680_selreg(struc * the unit shift. */ -static unsigned long sil680_seldev(struct ata_port *ap, struct ata_device *adev, int r) +static int sil680_seldev(struct ata_port *ap, struct ata_device *adev, int r) { - unsigned long base = 0xA0 + r; + int base = 0xA0 + r; + base += (ap->port_no << 4); base |= adev->devno ? 2 : 0; return base; @@ -85,8 +87,9 @@ static unsigned long sil680_seldev(struc static int sil680_cable_detect(struct ata_port *ap) { struct pci_dev *pdev = to_pci_dev(ap->host->dev); - unsigned long addr = sil680_selreg(ap, 0); + int addr = sil680_selreg(ap, 0); u8 ata66; + pci_read_config_byte(pdev, addr, &ata66); if (ata66 & 1) return ATA_CBL_PATA80; @@ -113,9 +116,9 @@ static void sil680_set_piomode(struct at 0x328A, 0x2283, 0x1281, 0x10C3, 0x10C1 }; - unsigned long tfaddr = sil680_selreg(ap, 0x02); - unsigned long addr = sil680_seldev(ap, adev, 0x04); - unsigned long addr_mask = 0x80 + 4 * ap->port_no; + int tfaddr = sil680_selreg(ap, 0x02); + int addr = sil680_seldev(ap, adev, 0x04); + int addr_mask = 0x80 + 4 * ap->port_no; struct pci_dev *pdev = to_pci_dev(ap->host->dev); int pio = adev->pio_mode - XFER_PIO_0; int lowest_pio = pio; @@ -165,9 +168,9 @@ static void sil680_set_dmamode(struct at static const u16 dma_table[3] = { 0x2208, 0x10C2, 0x10C1 }; struct pci_dev *pdev = to_pci_dev(ap->host->dev); - unsigned long ma = sil680_seldev(ap, adev, 0x08); - unsigned long ua = sil680_seldev(ap, adev, 0x0C); - unsigned long addr_mask = 0x80 + 4 * ap->port_no; + int ma = sil680_seldev(ap, adev, 0x08); + int ua = sil680_seldev(ap, adev, 0x0C); + int addr_mask = 0x80 + 4 * ap->port_no; int port_shift = adev->devno * 4; u8 scsc, mode; u16 multi, ultra; @@ -219,7 +222,7 @@ static void sil680_sff_exec_command(stru static bool sil680_sff_irq_check(struct ata_port *ap) { struct pci_dev *pdev = to_pci_dev(ap->host->dev); - unsigned long addr = sil680_selreg(ap, 1); + int addr = sil680_selreg(ap, 1); u8 val; pci_read_config_byte(pdev, addr, &val);
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@omp.ru> --- 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(-)