[PATCH v2 0/3] scsi: aic7xxx: fix some problem of return value

[PATCH v2 0/3] scsi: aic7xxx: fix some problem of return value

[Su Hui]

v2:
 - fix some problems and split v1 patch into this patch set.(Thanks to
   Dan)

v1:
 - https://lore.kernel.org/all/20231130024122.1193324-1-suhui@nfschina.com/

Su Hui (3):
  scsi: aic7xxx: return negative error codes in
    ahc_linux_register_host()
  scsi: aic7xxx: return ahc_linux_register_host()'s value rather than
    zero
  scsi: aic7xxx: return negative error codes in aic7770_probe()

 drivers/scsi/aic7xxx/aic7770_osm.c     | 6 +++---
 drivers/scsi/aic7xxx/aic7xxx_osm.c     | 2 +-
 drivers/scsi/aic7xxx/aic7xxx_osm_pci.c | 3 +--
 3 files changed, 5 insertions(+), 6 deletions(-)

-- 
2.30.2

[PATCH v2 1/3] scsi: aic7xxx: return negative error codes in ahc_linux_register_host()

[Su Hui]

ahc_linux_register_host() return both positive and negative error code,
it's better to make this function only return negative error codes.

Signed-off-by: Su Hui <suhui@nfschina.com>
---
 drivers/scsi/aic7xxx/aic7xxx_osm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/aic7xxx/aic7xxx_osm.c b/drivers/scsi/aic7xxx/aic7xxx_osm.c
index 4ae0a1c4d374..b0c4f2345321 100644
--- a/drivers/scsi/aic7xxx/aic7xxx_osm.c
+++ b/drivers/scsi/aic7xxx/aic7xxx_osm.c
@@ -1085,7 +1085,7 @@ ahc_linux_register_host(struct ahc_softc *ahc, struct scsi_host_template *templa
 	template->name = ahc->description;
 	host = scsi_host_alloc(template, sizeof(struct ahc_softc *));
 	if (host == NULL)
-		return (ENOMEM);
+		return -ENOMEM;
 
 	*((struct ahc_softc **)host->hostdata) = ahc;
 	ahc->platform_data->host = host;
-- 
2.30.2

[PATCH v2 2/3] scsi: aic7xxx: return ahc_linux_register_host()'s value rather than zero

[Su Hui]

ahc_linux_register_host() can return error code if failed.
So ahc_linux_pci_dev_probe() should return the value of
ahc_linux_register_host() rather than zero.

And the last patch made ahc_linux_register_host() return negative error
codes, which makes sure ahc_linux_pci_dev_probe() returns negative error
codes.

Signed-off-by: Su Hui <suhui@nfschina.com>
---
 drivers/scsi/aic7xxx/aic7xxx_osm_pci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c b/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
index a07e94fac673..198440dc0918 100644
--- a/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
+++ b/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
@@ -241,8 +241,7 @@ ahc_linux_pci_dev_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		ahc_linux_pci_inherit_flags(ahc);
 
 	pci_set_drvdata(pdev, ahc);
-	ahc_linux_register_host(ahc, &aic7xxx_driver_template);
-	return (0);
+	return ahc_linux_register_host(ahc, &aic7xxx_driver_template);
 }
 
 /******************************* PCI Routines *********************************/
-- 
2.30.2

[PATCH v2 3/3] scsi: aic7xxx: return negative error codes in aic7770_probe()

[Su Hui]

aic7770_config() returns both negative and positive error code.
it's better to make aic7770_probe() only return negative error codes.

And the previous patch made ahc_linux_register_host() return negative error
codes, which makes sure aic7770_probe() returns negative error codes.

Signed-off-by: Su Hui <suhui@nfschina.com>
---
 drivers/scsi/aic7xxx/aic7770_osm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/aic7xxx/aic7770_osm.c b/drivers/scsi/aic7xxx/aic7770_osm.c
index bdd177e3d762..a19cdd87c453 100644
--- a/drivers/scsi/aic7xxx/aic7770_osm.c
+++ b/drivers/scsi/aic7xxx/aic7770_osm.c
@@ -87,17 +87,17 @@ aic7770_probe(struct device *dev)
 	sprintf(buf, "ahc_eisa:%d", eisaBase >> 12);
 	name = kstrdup(buf, GFP_ATOMIC);
 	if (name == NULL)
-		return (ENOMEM);
+		return -ENOMEM;
 	ahc = ahc_alloc(&aic7xxx_driver_template, name);
 	if (ahc == NULL)
-		return (ENOMEM);
+		return -ENOMEM;
 	ahc->dev = dev;
 	error = aic7770_config(ahc, aic7770_ident_table + edev->id.driver_data,
 			       eisaBase);
 	if (error != 0) {
 		ahc->bsh.ioport = 0;
 		ahc_free(ahc);
-		return (error);
+		return error < 0 ? error : -error;
 	}
 
  	dev_set_drvdata(dev, ahc);
-- 
2.30.2

Re: [PATCH v2 3/3] scsi: aic7xxx: return negative error codes in aic7770_probe()

[Christophe JAILLET]

Le 01/12/2023 à 03:59, Su Hui a écrit :
> aic7770_config() returns both negative and positive error code.
> it's better to make aic7770_probe() only return negative error codes.
> 
> And the previous patch made ahc_linux_register_host() return negative error
> codes, which makes sure aic7770_probe() returns negative error codes.
> 
> Signed-off-by: Su Hui <suhui@nfschina.com>
> ---
>   drivers/scsi/aic7xxx/aic7770_osm.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/aic7xxx/aic7770_osm.c b/drivers/scsi/aic7xxx/aic7770_osm.c
> index bdd177e3d762..a19cdd87c453 100644
> --- a/drivers/scsi/aic7xxx/aic7770_osm.c
> +++ b/drivers/scsi/aic7xxx/aic7770_osm.c
> @@ -87,17 +87,17 @@ aic7770_probe(struct device *dev)
>   	sprintf(buf, "ahc_eisa:%d", eisaBase >> 12);
>   	name = kstrdup(buf, GFP_ATOMIC);
>   	if (name == NULL)
> -		return (ENOMEM);
> +		return -ENOMEM;
>   	ahc = ahc_alloc(&aic7xxx_driver_template, name);
>   	if (ahc == NULL)

Unrelated to your fix, but 'name' is leaking here.

Also, kasprintf() could be used to avoid buf+sprintf()+kstrdup()

The GFP_ATOMIC in the allocation could certainly also be just a GFP_KERNEL.

CJ

> -		return (ENOMEM);
> +		return -ENOMEM;
>   	ahc->dev = dev;
>   	error = aic7770_config(ahc, aic7770_ident_table + edev->id.driver_data,
>   			       eisaBase);
>   	if (error != 0) {
>   		ahc->bsh.ioport = 0;
>   		ahc_free(ahc);
> -		return (error);
> +		return error < 0 ? error : -error;
>   	}
>   
>    	dev_set_drvdata(dev, ahc);

Re: [PATCH v2 3/3] scsi: aic7xxx: return negative error codes in aic7770_probe()

[Marion & Christophe JAILLET]

Le 03/12/2023 à 11:34, Christophe JAILLET a écrit :
> Le 01/12/2023 à 03:59, Su Hui a écrit :
>> aic7770_config() returns both negative and positive error code.
>> it's better to make aic7770_probe() only return negative error codes.
>>
>> And the previous patch made ahc_linux_register_host() return negative 
>> error
>> codes, which makes sure aic7770_probe() returns negative error codes.
>>
>> Signed-off-by: Su Hui <suhui@nfschina.com>
>> ---
>>   drivers/scsi/aic7xxx/aic7770_osm.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/scsi/aic7xxx/aic7770_osm.c 
>> b/drivers/scsi/aic7xxx/aic7770_osm.c
>> index bdd177e3d762..a19cdd87c453 100644
>> --- a/drivers/scsi/aic7xxx/aic7770_osm.c
>> +++ b/drivers/scsi/aic7xxx/aic7770_osm.c
>> @@ -87,17 +87,17 @@ aic7770_probe(struct device *dev)
>>       sprintf(buf, "ahc_eisa:%d", eisaBase >> 12);
>>       name = kstrdup(buf, GFP_ATOMIC);
>>       if (name == NULL)
>> -        return (ENOMEM);
>> +        return -ENOMEM;
>>       ahc = ahc_alloc(&aic7xxx_driver_template, name);
>>       if (ahc == NULL)
> 
> Unrelated to your fix, but 'name' is leaking here.

Oups, no, ahc_alloc() handles it.
Really strange API!

CJ

> 
> Also, kasprintf() could be used to avoid buf+sprintf()+kstrdup()
> 
> The GFP_ATOMIC in the allocation could certainly also be just a GFP_KERNEL.
> 
> CJ
> 
>> -        return (ENOMEM);
>> +        return -ENOMEM;
>>       ahc->dev = dev;
>>       error = aic7770_config(ahc, aic7770_ident_table + 
>> edev->id.driver_data,
>>                      eisaBase);
>>       if (error != 0) {
>>           ahc->bsh.ioport = 0;
>>           ahc_free(ahc);
>> -        return (error);
>> +        return error < 0 ? error : -error;
>>       }
>>        dev_set_drvdata(dev, ahc);
> 
> 

Re: [PATCH v2 0/3] scsi: aic7xxx: fix some problem of return value

[Dan Carpenter]

On Fri, Dec 01, 2023 at 10:59:53AM +0800, Su Hui wrote:
> v2:
>  - fix some problems and split v1 patch into this patch set.(Thanks to
>    Dan)
> 
> v1:
>  - https://lore.kernel.org/all/20231130024122.1193324-1-suhui@nfschina.com/
> 

Would have been better with Fixes tags probably.  Otherwise, it looks
good to me.

Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>

regards,
dan carpenter

Re: [PATCH v2 0/3] scsi: aic7xxx: fix some problem of return value

[Su Hui]

On 2023/12/1 15:53, Dan Carpenter wrote:
> On Fri, Dec 01, 2023 at 10:59:53AM +0800, Su Hui wrote:
>> v2:
>>   - fix some problems and split v1 patch into this patch set.(Thanks to
>>     Dan)
>>
>> v1:
>>   - https://lore.kernel.org/all/20231130024122.1193324-1-suhui@nfschina.com/
>>
> Would have been better with Fixes tags probably.  Otherwise, it looks
> good to me.

Hi, Dan

Sorry for the late reply.

I'm not sure if it's worth to add Fixes tags.
These codes are very old which come from "Linux-2.6.12-rc2".
It's seems like a cleanup or improvement.

Umm, should I send v3 patches to add Fixes tags?

Su Hui

Re: [PATCH v2 0/3] scsi: aic7xxx: fix some problem of return value

[Dan Carpenter]

On Tue, Dec 05, 2023 at 11:33:36AM +0800, Su Hui wrote:
> On 2023/12/1 15:53, Dan Carpenter wrote:
> > On Fri, Dec 01, 2023 at 10:59:53AM +0800, Su Hui wrote:
> > > v2:
> > >   - fix some problems and split v1 patch into this patch set.(Thanks to
> > >     Dan)
> > > 
> > > v1:
> > >   - https://lore.kernel.org/all/20231130024122.1193324-1-suhui@nfschina.com/
> > > 
> > Would have been better with Fixes tags probably.  Otherwise, it looks
> > good to me.
> 
> Hi, Dan
> 
> Sorry for the late reply.
> 
> I'm not sure if it's worth to add Fixes tags.
> These codes are very old which come from "Linux-2.6.12-rc2".

I know some people use Fixes tags to point to Linux-2.6.12-rc2 but
other people don't like it...  Or they didn't like it back in the day,
I'm not sure now.

> It's seems like a cleanup or improvement.

It's definitely a Fix.  It affects runtime.

> 
> Umm, should I send v3 patches to add Fixes tags?

I don't really care, I guess.  Probably yes?  Not a lot of people use
aic7xxx these days so from a practical perspective it's not super
important either way.

regards,
dan carpenter

Re: [PATCH v2 0/3] scsi: aic7xxx: fix some problem of return value

[Su Hui]

On 2023/12/5 16:14, Dan Carpenter wrote:
> On Tue, Dec 05, 2023 at 11:33:36AM +0800, Su Hui wrote:
>> On 2023/12/1 15:53, Dan Carpenter wrote:
>>> On Fri, Dec 01, 2023 at 10:59:53AM +0800, Su Hui wrote:
>>>> v2:
>>>>    - fix some problems and split v1 patch into this patch set.(Thanks to
>>>>      Dan)
>>>>
>>>> v1:
>>>>    - https://lore.kernel.org/all/20231130024122.1193324-1-suhui@nfschina.com/
>>>>
>>> Would have been better with Fixes tags probably.  Otherwise, it looks
>>> good to me.
>> Hi, Dan
>>
>> Sorry for the late reply.
>>
>> I'm not sure if it's worth to add Fixes tags.
>> These codes are very old which come from "Linux-2.6.12-rc2".
> I know some people use Fixes tags to point to Linux-2.6.12-rc2 but
> other people don't like it...  Or they didn't like it back in the day,
> I'm not sure now.
>
>> It's seems like a cleanup or improvement.
> It's definitely a Fix.  It affects runtime.
>
>> Umm, should I send v3 patches to add Fixes tags?
> I don't really care, I guess.  Probably yes?  Not a lot of people use
> aic7xxx these days so from a practical perspective it's not super
> important either way.

Got it.  Maybe it's right to omit these old Fixes tags.

Su Hui

Re: [PATCH v2 0/3] scsi: aic7xxx: fix some problem of return value

[Martin K. Petersen]

Su,

> v2:
>  - fix some problems and split v1 patch into this patch set.(Thanks to
>    Dan)

Applied to 6.8/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2 0/3] scsi: aic7xxx: fix some problem of return value

[Martin K. Petersen]

On Fri, 01 Dec 2023 10:59:53 +0800, Su Hui wrote:

> v2:
>  - fix some problems and split v1 patch into this patch set.(Thanks to
>    Dan)
> 
> v1:
>  - https://lore.kernel.org/all/20231130024122.1193324-1-suhui@nfschina.com/
> 
> [...]

Applied to 6.8/scsi-queue, thanks!

[1/3] scsi: aic7xxx: return negative error codes in ahc_linux_register_host()
      https://git.kernel.org/mkp/scsi/c/573eb4a3410a
[2/3] scsi: aic7xxx: return ahc_linux_register_host()'s value rather than zero
      https://git.kernel.org/mkp/scsi/c/70dfaf84ec77
[3/3] scsi: aic7xxx: return negative error codes in aic7770_probe()
      https://git.kernel.org/mkp/scsi/c/aef6ac123609

-- 
Martin K. Petersen	Oracle Linux Engineering