Re: [PATCH] ata: pata_sil680: fix result type of sil680_sel{dev|reg}()

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

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


-- 
Damien Le Moal
Western Digital Research