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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 31 Oct 2013 16:54:44 -0500
From:	scameron@...rdog.cce.hp.com
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Jens Axboe <axboe@...nel.dk>, stephenmcameron@...il.com,
	mikem@...rdog.cce.hp.com, linux-kernel@...r.kernel.org,
	thenzl@...hat.com, scameron@...rdog.cce.hp.com
Subject: Re: [PATCH] cciss: return 0 from driver probe function on success, not 1

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@...nel.dk> wrote:
> 
> > On 10/29/2013 12:41 PM, Stephen M. Cameron wrote:
> > > From: Stephen M. Cameron <scameron@...rdog.cce.hp.com>
> > > 
> > > A return value of 1 is interpreted as an error
> > > 
> > > Signed-off-by: Stephen M. Cameron <scameron@...rdog.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@...ux-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@...nel.dk>
> Cc: Stephen M. Cameron <scameron@...rdog.cce.hp.com>
> Signed-off-by: Andrew Morton <akpm@...ux-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

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