Skip to contentSkip to conversation

ata: pata_sil680: fix result type of sil680_sel{dev|reg}()

PATCH In review Rand Deeb 2 participants 4 messages Apr 19 – Apr 28

Subsystems (2)
Stable branch libata
lore
Share & tools
lore

Rand Deeb

author
#0 lore raw

PATCH 1/1 ata: pata_sil680: fix result type of sil680_sel{dev|reg}()

From: Sergey Shtylyov <[email protected]>

[ Upstream commit dafbbf5c57dd6ae01d20b894bc2200e9d9834c4e ]

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. Get rid of the 'base' local variables in these functions, while at it...

Found by Linux Verification Center (linuxtesting.org) with the SVACE static analysis tool.

Signed-off-by Sergey Shtylyov <[email protected]>
Signed-off-by Damien Le Moal <[email protected]>
Signed-off-by Rand Deeb <[email protected]>
---
 drivers/ata/pata_sil680.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)
drivers/ata/pata_sil680.c +13 -17
index 7ab9aea3b..fe60f884b 100644
@@ -47,11 +47,9 @@
  *	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;
-	base += (ap->port_no << 4);
-	return base;
+	return 0xA0 + (ap->port_no << 4) + r;
 }
 
 /**

@@ -64,12 +62,9 @@ static unsigned long sil680_selreg(struct ata_port *ap, int r)
  *	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;
-	base += (ap->port_no << 4);
-	base |= adev->devno ? 2 : 0;
-	return base;
+	return 0xA0 + (ap->port_no << 4) + r + (adev->devno << 1);
 }
 
 

@@ -84,8 +79,9 @@ static unsigned long sil680_seldev(struct ata_port *ap, struct ata_device *adev,
 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;

@@ -112,9 +108,9 @@ static void sil680_set_piomode(struct ata_port *ap, struct ata_device *adev)
 		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;

@@ -164,9 +160,9 @@ static void sil680_set_dmamode(struct ata_port *ap, struct ata_device *adev)
 	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 +215,7 @@ static void sil680_sff_exec_command(struct ata_port *ap,
 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);
Signature · 1 line
2.43.0
#1 lore raw
Quoted 41 lines · Rand Deeb

From: Sergey Shtylyov <[email protected]>

[ Upstream commit dafbbf5c57dd6ae01d20b894bc2200e9d9834c4e ]

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. Get rid of the 'base' local variables in these functions, while at it...

Found by Linux Verification Center (linuxtesting.org) with the SVACE static analysis tool.

Signed-off-by Sergey Shtylyov <[email protected]>
Signed-off-by Damien Le Moal <[email protected]>
Signed-off-by Rand Deeb <[email protected]>
---
 drivers/ata/pata_sil680.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)
drivers/ata/pata_sil680.c +2 -4
index 7ab9aea3b..fe60f884b 100644
@@ -47,11 +47,9 @@
  *     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;
-       base += (ap->port_no << 4);
-       return base;
+       return 0xA0 + (ap->port_no << 4) + r;
 }
 
 /**

@@ -64,12 +62,9 @@ static unsigned long sil680_selreg(struct ata_port *ap, int r)
  *     the unit shift.
  */

drivers/ata/pata_sil680.c static unsigned long sil680_selreg(struct ata_port *ap, int r) · line 68

Quoted 10 lines
drivers/ata/pata_sil680.c +2 -5
 
-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;
-       base += (ap->port_no << 4);
-       base |= adev->devno ? 2 : 0;
-       return base;
+       return 0xA0 + (ap->port_no << 4) + r + (adev->devno << 1);
 }

And why exactly is this needed in 5.10.y?

[...]
MBR, Sergey

author
#2 lore raw
Quoted 57 lines · Sergey Shtylyov
Quoted 41 lines · Rand Deeb

From: Sergey Shtylyov <[email protected]>

[ Upstream commit dafbbf5c57dd6ae01d20b894bc2200e9d9834c4e ]

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. Get rid of the 'base' local variables in these functions, while at it...

Found by Linux Verification Center (linuxtesting.org) with the SVACE static analysis tool.

Signed-off-by Sergey Shtylyov <[email protected]>
Signed-off-by Damien Le Moal <[email protected]>
Signed-off-by Rand Deeb <[email protected]>
---
 drivers/ata/pata_sil680.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)
drivers/ata/pata_sil680.c +2 -4
index 7ab9aea3b..fe60f884b 100644
@@ -47,11 +47,9 @@
  *     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;
-       base += (ap->port_no << 4);
-       return base;
+       return 0xA0 + (ap->port_no << 4) + r;
 }
 
 /**

@@ -64,12 +62,9 @@ static unsigned long sil680_selreg(struct ata_port *ap, int r)
  *     the unit shift.
  */

drivers/ata/pata_sil680.c static unsigned long sil680_selreg(struct ata_port *ap, int r) · line 68

Quoted 10 lines
drivers/ata/pata_sil680.c +2 -5
 
-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;
-       base += (ap->port_no << 4);
-       base |= adev->devno ? 2 : 0;
-       return base;
+       return 0xA0 + (ap->port_no << 4) + r + (adev->devno << 1);
 }

And why exactly is this needed in 5.10.y?

Quoted 3 lines

[...]
MBR, Sergey

Hi Sergey,

This is a direct backport of upstream commit dafbbf5c57dd.

It fixes a type mismatch between the helper functions and the PCI config accessors (which expect int), as identified by static analysis (SVACE).

The intent is to keep the code consistent and avoid issues flagged by tooling.

If this is not considered appropriate for 5.10.y, I’m fine with dropping it.

Thanks

#3 lore raw

On 4/28/26 11:15 AM, Rand Deeb wrote:

[...]

Quoted 54 lines
Quoted 54 lines
Quoted 41 lines

From: Sergey Shtylyov <[email protected]>

[ Upstream commit dafbbf5c57dd6ae01d20b894bc2200e9d9834c4e ]

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. Get rid of the 'base' local variables in these functions, while at it...

Found by Linux Verification Center (linuxtesting.org) with the SVACE static analysis tool.

Signed-off-by Sergey Shtylyov <[email protected]>
Signed-off-by Damien Le Moal <[email protected]>
Signed-off-by Rand Deeb <[email protected]>
---
 drivers/ata/pata_sil680.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)
drivers/ata/pata_sil680.c +2 -4
index 7ab9aea3b..fe60f884b 100644
@@ -47,11 +47,9 @@
  *     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;
-       base += (ap->port_no << 4);
-       return base;
+       return 0xA0 + (ap->port_no << 4) + r;
 }
 
 /**

@@ -64,12 +62,9 @@ static unsigned long sil680_selreg(struct ata_port *ap, int r)
  *     the unit shift.
  */

drivers/ata/pata_sil680.c static unsigned long sil680_selreg(struct ata_port *ap, int r) · line 68

Quoted 10 lines
drivers/ata/pata_sil680.c +2 -5
 
-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;
-       base += (ap->port_no << 4);
-       base |= adev->devno ? 2 : 0;
-       return base;
+       return 0xA0 + (ap->port_no << 4) + r + (adev->devno << 1);
 }

And why exactly is this needed in 5.10.y?

Quoted 7 lines
Quoted 3 lines

[...]
MBR, Sergey

Hi Sergey,

This is a direct backport of upstream commit dafbbf5c57dd.

I figured. :-)

Quoted 3 lines

It fixes a type mismatch between the helper functions and the PCI config accessors (which expect int), as identified by static analysis (SVACE).

Thank you for reminding me what my patch does... :-)

Quoted 2 lines

The intent is to keep the code consistent and avoid issues flagged by tooling.

What tooling, Svace?

Quoted 2 lines

If this is not considered appropriate for 5.10.y, I’m fine with dropping it.

If I considered this a fix for something, I'd have added
the Fixes tag -- then it would be worthy of backporting. Now it's not, I think...

Quoted 1 line

Thanks

MBR, Sergey