* [PATCH] ata: pata_sil680: fix result type of sil680_sel{dev|reg}()
@ 2022-04-09 20:29 Sergey Shtylyov
2022-04-10 23:47 ` Damien Le Moal
0 siblings, 1 reply; 4+ messages in thread
From: Sergey Shtylyov @ 2022-04-09 20:29 UTC (permalink / raw)
To: Damien Le Moal, linux-ide
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;
}
@@ -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);
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ata: pata_sil680: fix result type of sil680_sel{dev|reg}()
2022-04-09 20:29 [PATCH] ata: pata_sil680: fix result type of sil680_sel{dev|reg}() Sergey Shtylyov
@ 2022-04-10 23:47 ` Damien Le Moal
2022-04-11 20:42 ` Sergey Shtylyov
0 siblings, 1 reply; 4+ messages in thread
From: Damien Le Moal @ 2022-04-10 23:47 UTC (permalink / raw)
To: Sergey Shtylyov, linux-ide
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ata: pata_sil680: fix result type of sil680_sel{dev|reg}()
2022-04-10 23:47 ` Damien Le Moal
@ 2022-04-11 20:42 ` Sergey Shtylyov
2022-04-11 22:49 ` Damien Le Moal
0 siblings, 1 reply; 4+ messages in thread
From: Sergey Shtylyov @ 2022-04-11 20:42 UTC (permalink / raw)
To: Damien Le Moal, linux-ide
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ata: pata_sil680: fix result type of sil680_sel{dev|reg}()
2022-04-11 20:42 ` Sergey Shtylyov
@ 2022-04-11 22:49 ` Damien Le Moal
0 siblings, 0 replies; 4+ messages in thread
From: Damien Le Moal @ 2022-04-11 22:49 UTC (permalink / raw)
To: Sergey Shtylyov, linux-ide
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
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-04-11 22:49 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-04-09 20:29 [PATCH] ata: pata_sil680: fix result type of sil680_sel{dev|reg}() Sergey Shtylyov
2022-04-10 23:47 ` Damien Le Moal
2022-04-11 20:42 ` Sergey Shtylyov
2022-04-11 22:49 ` Damien Le Moal
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox