Re: [PATCH] ibmasm: add missing pci_enable_device()

[Francois Romieu <romieu@fr.zoreil.com>]

Bjorn Helgaas <bjorn.helgaas@hp.com> :
[...]
> > > ===== drivers/misc/ibmasm/module.c 1.2 vs edited =====
> > > --- 1.2/drivers/misc/ibmasm/module.c	2004-05-14 06:00:50 -06:00
> > > +++ edited/drivers/misc/ibmasm/module.c	2004-08-04 13:15:46 -06:00
> > > @@ -62,10 +62,17 @@
> > >  	int result = -ENOMEM;
> > >  	struct service_processor *sp;
> > >  
> > > +	if (pci_enable_device(pdev)) {
> > > +		printk(KERN_ERR "%s: can't enable PCI device at %s\n",
> > > +			DRIVER_NAME, pci_name(pdev));
> > > +		return -ENODEV;

Btw, pci_enable_device returns a perfectly fine status code. There is no
reason to override it ('result = pci_enable_device(...' and you can even 
remove the previous ENOMEM initialization).

[...]
> > >  	sp = kmalloc(sizeof(struct service_processor), GFP_KERNEL);
> > >  	if (sp == NULL) {
> > >  		dev_err(&pdev->dev, "Failed to allocate memory\n");
> > > -		return result;
> > > +		result = -ENOMEM;
> > > +		goto error_kmalloc;

You may consider calling it err_pci_disable (or such). This way:
- one can check that the kmalloc is preceeded by a pci_enable;
- one can locally check that the unroll path does its job
  (error_pci_disable is followed by a pci_disable_xxx -> makes sense).

--
Ueimor