[PATCH] cciss: return 0 from driver probe function on success, not 1

[PATCH] cciss: return 0 from driver probe function on success, not 1

[Stephen M. Cameron]

Just resending the patch with a better change log message (as requested
by Andrew Morton) and cc'ing stable@vger.kernel.org, (as I originally
had intended.)

---

Stephen M. Cameron (1):
      cciss: return 0 from driver probe function on success, not 1


 drivers/block/cciss.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

-- 
-- steve

[PATCH] cciss: return 0 from driver probe function on success, not 1

[Stephen M. Cameron]

From: Stephen M. Cameron <scameron@beardog.cce.hp.com>

A return value of 1 is interpreted as an error.  See pci_driver.
in local_pci_probe().  If you're wondering how this ever could
have worked, it's because it used to be the case that only return
values less than zero were interpreted as failure.  But even in
the current kernel if the driver registers its various entry
points with the kernel, and then returns a value which is
interpreted as failure, those registrations aren't undone, so
the driver still mostly works.  However, the driver's remove
function wouldn't be called on rmmod, and pci power management
functions wouldn't work.  In the case of Smart Array, since it
has a battery backed cache (or else no cache) even if the driver
is not shut down properly as long as there is no outstanding
i/o, nothing too bad happens, which is why it took so long to
notice.

Requesting backport to stable because the change to pci-driver.c
which requires driver probe functions to return 0 occurred between
2.6.35 and 2.6.36 (the pci power management breakage) and again
between 3.7 and 3.8 (pci_dev->driver getting set to NULL in
local_pci_probe() preventing driver remove function from being
called on rmmod.)

Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
Cc: stable@vger.kernel.org
---
 drivers/block/cciss.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
index edfa251..0c004ac 100644
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -5183,7 +5183,7 @@ reinit_after_soft_reset:
 	rebuild_lun_table(h, 1, 0);
 	cciss_engage_scsi(h);
 	h->busy_initializing = 0;
-	return 1;
+	return 0;
 
 clean4:
 	cciss_free_cmd_pool(h);

Re: [PATCH] cciss: return 0 from driver probe function on success, not 1

[Jens Axboe]

On 11/01/2013 10:06 AM, Stephen M. Cameron wrote:
> From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> 
> A return value of 1 is interpreted as an error.  See pci_driver.
> in local_pci_probe().  If you're wondering how this ever could
> have worked, it's because it used to be the case that only return
> values less than zero were interpreted as failure.  But even in
> the current kernel if the driver registers its various entry
> points with the kernel, and then returns a value which is
> interpreted as failure, those registrations aren't undone, so
> the driver still mostly works.  However, the driver's remove
> function wouldn't be called on rmmod, and pci power management
> functions wouldn't work.  In the case of Smart Array, since it
> has a battery backed cache (or else no cache) even if the driver
> is not shut down properly as long as there is no outstanding
> i/o, nothing too bad happens, which is why it took so long to
> notice.
> 
> Requesting backport to stable because the change to pci-driver.c
> which requires driver probe functions to return 0 occurred between
> 2.6.35 and 2.6.36 (the pci power management breakage) and again
> between 3.7 and 3.8 (pci_dev->driver getting set to NULL in
> local_pci_probe() preventing driver remove function from being
> called on rmmod.)

The original patch went in two days ago, so it's a little difficult for
me to update it unfortunately. But you can include lots of this in the
pci-driver.c warning print patch instead.

-- 
Jens Axboe

[PATCH] cciss: return 0 from driver probe function on success, not 1

[Stephen M. Cameron]

From: Stephen M. Cameron <scameron@beardog.cce.hp.com>

A return value of 1 is interpreted as an error

Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
---
 drivers/block/cciss.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
index edfa251..0c004ac 100644
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -5183,7 +5183,7 @@ reinit_after_soft_reset:
 	rebuild_lun_table(h, 1, 0);
 	cciss_engage_scsi(h);
 	h->busy_initializing = 0;
-	return 1;
+	return 0;
 
 clean4:
 	cciss_free_cmd_pool(h);

Re: [PATCH] cciss: return 0 from driver probe function on success, not 1

[Jens Axboe]

On 10/29/2013 12:41 PM, Stephen M. Cameron wrote:
> From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> 
> A return value of 1 is interpreted as an error
> 
> Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> ---
>  drivers/block/cciss.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
> index edfa251..0c004ac 100644
> --- a/drivers/block/cciss.c
> +++ b/drivers/block/cciss.c
> @@ -5183,7 +5183,7 @@ reinit_after_soft_reset:
>  	rebuild_lun_table(h, 1, 0);
>  	cciss_engage_scsi(h);
>  	h->busy_initializing = 0;
> -	return 1;
> +	return 0;
>  
>  clean4:
>  	cciss_free_cmd_pool(h);
> 

How did this ever work?

-- 
Jens Axboe

Re: [PATCH] cciss: return 0 from driver probe function on success, not 1

[Andrew Morton]

On Tue, 29 Oct 2013 12:58:09 -0600 Jens Axboe <axboe@kernel.dk> wrote:

> On 10/29/2013 12:41 PM, Stephen M. Cameron wrote:
> > From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> > 
> > A return value of 1 is interpreted as an error
> > 
> > Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> > ---
> >  drivers/block/cciss.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
> > index edfa251..0c004ac 100644
> > --- a/drivers/block/cciss.c
> > +++ b/drivers/block/cciss.c
> > @@ -5183,7 +5183,7 @@ reinit_after_soft_reset:
> >  	rebuild_lun_table(h, 1, 0);
> >  	cciss_engage_scsi(h);
> >  	h->busy_initializing = 0;
> > -	return 1;
> > +	return 0;
> >  
> >  clean4:
> >  	cciss_free_cmd_pool(h);
> > 
> 
> How did this ever work?

Beats me.  local_pci_probe() does

	rc = pci_drv->probe(pci_dev, ddi->id);
	if (rc) {
		pci_dev->driver = NULL;
		pm_runtime_put_sync(dev);
	}
	return rc;

shrug, maybe this ->probe somehow has a different caller which checks
for <0.


While we're there...

From: Andrew Morton <akpm@linux-foundation.org>
Subject: drivers/block/cciss.c:cciss_init_one(): use proper errnos

pci_driver.probe should return a meaningful errno, not -1.

Cc: Jens Axboe <axboe@kernel.dk>
Cc: Stephen M. Cameron <scameron@beardog.cce.hp.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/block/cciss.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff -puN drivers/block/cciss.c~drivers-block-ccissc-cciss_init_one-use-proper-errnos drivers/block/cciss.c
--- a/drivers/block/cciss.c~drivers-block-ccissc-cciss_init_one-use-proper-errnos
+++ a/drivers/block/cciss.c
@@ -5004,7 +5004,7 @@ reinit_after_soft_reset:
 
 	i = alloc_cciss_hba(pdev);
 	if (i < 0)
-		return -1;
+		return -ENOMEM;
 
 	h = hba[i];
 	h->pdev = pdev;
@@ -5205,7 +5205,7 @@ clean_no_release_regions:
 	 */
 	pci_set_drvdata(pdev, NULL);
 	free_hba(h);
-	return -1;
+	return -ENODEV;
 }
 
 static void cciss_shutdown(struct pci_dev *pdev)
_

Re: [PATCH] cciss: return 0 from driver probe function on success, not 1

[scameron]

On Thu, Oct 31, 2013 at 02:42:41PM -0700, Andrew Morton wrote:
> On Tue, 29 Oct 2013 12:58:09 -0600 Jens Axboe <axboe@kernel.dk> wrote:
> 
> > On 10/29/2013 12:41 PM, Stephen M. Cameron wrote:
> > > From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> > > 
> > > A return value of 1 is interpreted as an error
> > > 
> > > Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> > > ---
> > >  drivers/block/cciss.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
> > > index edfa251..0c004ac 100644
> > > --- a/drivers/block/cciss.c
> > > +++ b/drivers/block/cciss.c
> > > @@ -5183,7 +5183,7 @@ reinit_after_soft_reset:
> > >  	rebuild_lun_table(h, 1, 0);
> > >  	cciss_engage_scsi(h);
> > >  	h->busy_initializing = 0;
> > > -	return 1;
> > > +	return 0;
> > >  
> > >  clean4:
> > >  	cciss_free_cmd_pool(h);
> > > 
> > 
> > How did this ever work?
> 
> Beats me.  local_pci_probe() does
> 
> 	rc = pci_drv->probe(pci_dev, ddi->id);
> 	if (rc) {
> 		pci_dev->driver = NULL;
> 		pm_runtime_put_sync(dev);
> 	}
> 	return rc;
> 
> shrug, maybe this ->probe somehow has a different caller which checks
> for <0.

Older kernels (eg: http://lxr.linux.no/#linux+v2.6.32.61/drivers/pci/pci-driver.c )
had different code:

 330__pci_device_probe(struct pci_driver *drv, struct pci_dev *pci_dev)
 331{
 332        const struct pci_device_id *id;
 333        int error = 0;
 334
 335        if (!pci_dev->driver && drv->probe) {
 336                error = -ENODEV;
 337
 338                id = pci_match_device(drv, pci_dev);
 339                if (id)
 340                        error = pci_call_probe(drv, pci_dev, id);
 341                if (error >= 0) {
 342                        pci_dev->driver = drv;
 343                        error = 0;
 344                }
 345        }
 346        return error;
 347}

> 
> 
> While we're there...
> 
> From: Andrew Morton <akpm@linux-foundation.org>
> Subject: drivers/block/cciss.c:cciss_init_one(): use proper errnos
> 
> pci_driver.probe should return a meaningful errno, not -1.
> 
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  drivers/block/cciss.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff -puN drivers/block/cciss.c~drivers-block-ccissc-cciss_init_one-use-proper-errnos drivers/block/cciss.c
> --- a/drivers/block/cciss.c~drivers-block-ccissc-cciss_init_one-use-proper-errnos
> +++ a/drivers/block/cciss.c
> @@ -5004,7 +5004,7 @@ reinit_after_soft_reset:
>  
>  	i = alloc_cciss_hba(pdev);
>  	if (i < 0)
> -		return -1;
> +		return -ENOMEM;
>  
>  	h = hba[i];
>  	h->pdev = pdev;
> @@ -5205,7 +5205,7 @@ clean_no_release_regions:
>  	 */
>  	pci_set_drvdata(pdev, NULL);
>  	free_hba(h);
> -	return -1;
> +	return -ENODEV;
>  }
>  
>  static void cciss_shutdown(struct pci_dev *pdev)
> _

Thanks.

-- steve

Re: [PATCH] cciss: return 0 from driver probe function on success, not 1

[Andrew Morton]

On Thu, 31 Oct 2013 16:54:44 -0500 scameron@beardog.cce.hp.com wrote:

> > > How did this ever work?
> > 
> > Beats me.  local_pci_probe() does
> > 
> > 	rc = pci_drv->probe(pci_dev, ddi->id);
> > 	if (rc) {
> > 		pci_dev->driver = NULL;
> > 		pm_runtime_put_sync(dev);
> > 	}
> > 	return rc;
> > 
> > shrug, maybe this ->probe somehow has a different caller which checks
> > for <0.
> 
> Older kernels (eg: http://lxr.linux.no/#linux+v2.6.32.61/drivers/pci/pci-driver.c )
> had different code:
> 
>  330__pci_device_probe(struct pci_driver *drv, struct pci_dev *pci_dev)
>  331{
>  332        const struct pci_device_id *id;
>  333        int error = 0;
>  334
>  335        if (!pci_dev->driver && drv->probe) {
>  336                error = -ENODEV;
>  337
>  338                id = pci_match_device(drv, pci_dev);
>  339                if (id)
>  340                        error = pci_call_probe(drv, pci_dev, id);
>  341                if (error >= 0) {
>  342                        pci_dev->driver = drv;
>  343                        error = 0;
>  344                }
>  345        }
>  346        return error;
>  347}

So cciss is presently kompletely kaput?  If so, the kapputting code is
present in 3.9 and probably earlier, so this patch is needed in 3.12 and
-stable.  Or if not, what?

(Playing question and answer like this is a bad way of writing a
changelog btw - all this stuff should have been right there in the v1
changelog).

Re: [PATCH] cciss: return 0 from driver probe function on success, not 1

[Tomas Henzl]

On 10/31/2013 11:06 PM, Andrew Morton wrote:
> On Thu, 31 Oct 2013 16:54:44 -0500 scameron@beardog.cce.hp.com wrote:
>
>>>> How did this ever work?
>>> Beats me.  local_pci_probe() does
>>>
>>> 	rc = pci_drv->probe(pci_dev, ddi->id);
>>> 	if (rc) {
>>> 		pci_dev->driver = NULL;
>>> 		pm_runtime_put_sync(dev);
>>> 	}
>>> 	return rc;
>>>
>>> shrug, maybe this ->probe somehow has a different caller which checks
>>> for <0.
>> Older kernels (eg: http://lxr.linux.no/#linux+v2.6.32.61/drivers/pci/pci-driver.c )
>> had different code:
>>
>>  330__pci_device_probe(struct pci_driver *drv, struct pci_dev *pci_dev)
>>  331{
>>  332        const struct pci_device_id *id;
>>  333        int error = 0;
>>  334
>>  335        if (!pci_dev->driver && drv->probe) {
>>  336                error = -ENODEV;
>>  337
>>  338                id = pci_match_device(drv, pci_dev);
>>  339                if (id)
>>  340                        error = pci_call_probe(drv, pci_dev, id);
>>  341                if (error >= 0) {
>>  342                        pci_dev->driver = drv;
>>  343                        error = 0;
>>  344                }
>>  345        }
>>  346        return error;
>>  347}
> So cciss is presently kompletely kaput?  If so, the kapputting code is
> present in 3.9 and probably earlier, so this patch is needed in 3.12 and
> -stable.  Or if not, what?

Steve posted similar patch for hpsa too and I'm sure someone would notice
when the hpsa driver was kaput. If I understand it correctly the driver
works with some limitation - rmmod doesn't work for example.

The problem in kernel is that the error handling in local_pci_probe
and  in __pci_device_probe is different for ret values > 0,
so we should fix it somewhere so it is in sync.
The documentation states that the probe function should return zero on success
so what about this -

This would bring the handling to sync
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 98f7b9b..200a071 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -317,8 +317,6 @@ __pci_device_probe(struct pci_driver *drv, struct pci_dev *pci_dev)
 		id = pci_match_device(drv, pci_dev);
 		if (id)
 			error = pci_call_probe(drv, pci_dev, id);
-		if (error >= 0)
-			error = 0;
 	}
 	return error;
 }


>
> (Playing question and answer like this is a bad way of writing a
> changelog btw - all this stuff should have been right there in the v1
> changelog).
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] cciss: return 0 from driver probe function on success, not 1

[Andrew Morton]

On Fri, 01 Nov 2013 14:06:45 +0100 Tomas Henzl <thenzl@redhat.com> wrote:

> The problem in kernel is that the error handling in local_pci_probe
> and  in __pci_device_probe is different for ret values > 0,
> so we should fix it somewhere so it is in sync.
> The documentation states that the probe function should return zero on success
> so what about this -
> 
> This would bring the handling to sync
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 98f7b9b..200a071 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -317,8 +317,6 @@ __pci_device_probe(struct pci_driver *drv, struct pci_dev *pci_dev)
>  		id = pci_match_device(drv, pci_dev);
>  		if (id)
>  			error = pci_call_probe(drv, pci_dev, id);
> -		if (error >= 0)
> -			error = 0;
>  	}
>  	return error;
>  }

ah, there it is.

This change would turn semi-kaput drivers into kaput-kaput drivers.  It
would be better to add a runtime warning here so those drivers get
fixed.  Such a warning would need to reliably identify the offending
probe function so a simple WARN_ON() wouldn't be sufficient.

Re: [PATCH] cciss: return 0 from driver probe function on success, not 1

[scameron]

On Fri, Nov 01, 2013 at 06:31:10AM -0700, Andrew Morton wrote:
> On Fri, 01 Nov 2013 14:06:45 +0100 Tomas Henzl <thenzl@redhat.com> wrote:
> 
> > The problem in kernel is that the error handling in local_pci_probe
> > and  in __pci_device_probe is different for ret values > 0,
> > so we should fix it somewhere so it is in sync.
> > The documentation states that the probe function should return zero on success
> > so what about this -
> > 
> > This would bring the handling to sync
> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > index 98f7b9b..200a071 100644
> > --- a/drivers/pci/pci-driver.c
> > +++ b/drivers/pci/pci-driver.c
> > @@ -317,8 +317,6 @@ __pci_device_probe(struct pci_driver *drv, struct pci_dev *pci_dev)
> >  		id = pci_match_device(drv, pci_dev);
> >  		if (id)
> >  			error = pci_call_probe(drv, pci_dev, id);
> > -		if (error >= 0)
> > -			error = 0;
> >  	}
> >  	return error;
> >  }
> 
> ah, there it is.
> 
> This change would turn semi-kaput drivers into kaput-kaput drivers.  It
> would be better to add a runtime warning here so those drivers get
> fixed.  Such a warning would need to reliably identify the offending
> probe function so a simple WARN_ON() wouldn't be sufficient.
> 

FWIW, I just booted up with the following change:

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 98f7b9b..ef71bb5 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -264,9 +264,16 @@ static long local_pci_probe(void *_ddi)
 	pm_runtime_get_sync(dev);
 	pci_dev->driver = pci_drv;
 	rc = pci_drv->probe(pci_dev, ddi->id);
-	if (rc) {
+	if (rc < 0) {
 		pci_dev->driver = NULL;
 		pm_runtime_put_sync(dev);
+	} else {
+		if (rc > 0) {
+			dev_warn(dev,
+				"Driver probe function returned %d, greater than 0\n", rc);
+			rc = 0;
+			
+		}
 	}
 	return rc;
 }


And,

[scameron@localhost linux-3.12-rc6]$ dmesg | grep 'Driver probe'
[scameron@localhost linux-3.12-rc6]$

Not that it means all that much since I don't have hardware for
the majority of drivers, obviously.

I think the above should make drivers with probe functions
returning > 0 "work" again, but with a warning.

The question would be, are there drivers which return > 0 and
by this value intend to convey that the probe function has failed?

-- steve

Re: [PATCH] cciss: return 0 from driver probe function on success, not 1

[Tomas Henzl]

On 11/01/2013 03:08 PM, scameron@beardog.cce.hp.com wrote:
> On Fri, Nov 01, 2013 at 06:31:10AM -0700, Andrew Morton wrote:
>> On Fri, 01 Nov 2013 14:06:45 +0100 Tomas Henzl <thenzl@redhat.com> wrote:
>>
>>> The problem in kernel is that the error handling in local_pci_probe
>>> and  in __pci_device_probe is different for ret values > 0,
>>> so we should fix it somewhere so it is in sync.
>>> The documentation states that the probe function should return zero on success
>>> so what about this -
>>>
>>> This would bring the handling to sync
>>> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
>>> index 98f7b9b..200a071 100644
>>> --- a/drivers/pci/pci-driver.c
>>> +++ b/drivers/pci/pci-driver.c
>>> @@ -317,8 +317,6 @@ __pci_device_probe(struct pci_driver *drv, struct pci_dev *pci_dev)
>>>  		id = pci_match_device(drv, pci_dev);
>>>  		if (id)
>>>  			error = pci_call_probe(drv, pci_dev, id);
>>> -		if (error >= 0)
>>> -			error = 0;
>>>  	}
>>>  	return error;
>>>  }
>> ah, there it is.
>>
>> This change would turn semi-kaput drivers into kaput-kaput drivers.  It
>> would be better to add a runtime warning here so those drivers get
>> fixed.  Such a warning would need to reliably identify the offending
>> probe function so a simple WARN_ON() wouldn't be sufficient.
>>
> FWIW, I just booted up with the following change:
>
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 98f7b9b..ef71bb5 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -264,9 +264,16 @@ static long local_pci_probe(void *_ddi)
>  	pm_runtime_get_sync(dev);
>  	pci_dev->driver = pci_drv;
>  	rc = pci_drv->probe(pci_dev, ddi->id);
> -	if (rc) {
> +	if (rc < 0) {
>  		pci_dev->driver = NULL;
>  		pm_runtime_put_sync(dev);
> +	} else {
> +		if (rc > 0) {
> +			dev_warn(dev,
> +				"Driver probe function returned %d, greater than 0\n", rc);
> +			rc = 0;
> +			
> +		}
>  	}
>  	return rc;
>  }
>
>
> And,
>
> [scameron@localhost linux-3.12-rc6]$ dmesg | grep 'Driver probe'
> [scameron@localhost linux-3.12-rc6]$
>
> Not that it means all that much since I don't have hardware for
> the majority of drivers, obviously.
>
> I think the above should make drivers with probe functions
> returning > 0 "work" again, but with a warning.
>
> The question would be, are there drivers which return > 0 and
> by this value intend to convey that the probe function has failed?

I think that this is unlikely and the patch is fine, but I can't speak for the drivers..

Please repost your patch so it gets more attention than in this thread. 

>
> -- steve
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/