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

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

[Jeff Garzik]

Linux Kernel Mailing List wrote:
> diff -Nru a/drivers/misc/ibmasm/module.c b/drivers/misc/ibmasm/module.c
> --- a/drivers/misc/ibmasm/module.c	2004-08-24 15:25:05 -07:00
> +++ b/drivers/misc/ibmasm/module.c	2004-08-24 15:25:05 -07: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;
> +	}


Another [minor] bug in this series of changes:  you should propagate the 
return value of pci_enable_device(), not make up your own.

	Jeff

[PATCH] ibmasm: add missing pci_enable_device()

[Bjorn Helgaas]

I don't have this hardware, so this has not been tested.


Add pci_enable_device()/pci_disable_device().  In the past, drivers
often worked without this, but it is now required in order to route
PCI interrupts correctly.

Signed-off-by: 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;
+	}
+
 	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;
 	}
 	memset(sp, 0, sizeof(struct service_processor));
 
@@ -148,6 +155,8 @@
 	ibmasm_event_buffer_exit(sp);
 error_eventbuffer:
 	kfree(sp);
+error_kmalloc:
+	pci_disable_device(pdev);
 
 	return result;
 }
@@ -166,6 +175,7 @@
 	iounmap(sp->base_address);
 	ibmasm_event_buffer_exit(sp);
 	kfree(sp);
+	pci_disable_device(pdev);
 }
 
 static struct pci_device_id ibmasm_pci_table[] =

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

[Max Asbock]

I tested this on the hardware. It works fine.
Is there any reason we shouldn't use dev_err() for the pci_enable error
message like in the other messages?

regards,
max

On Wed, 2004-08-04 at 14:32, Bjorn Helgaas wrote:
> I don't have this hardware, so this has not been tested.
> 
> 
> Add pci_enable_device()/pci_disable_device().  In the past, drivers
> often worked without this, but it is now required in order to route
> PCI interrupts correctly.
> 
> Signed-off-by: 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;
> +	}
> +
>  	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;
>  	}
>  	memset(sp, 0, sizeof(struct service_processor));
>  
> @@ -148,6 +155,8 @@
>  	ibmasm_event_buffer_exit(sp);
>  error_eventbuffer:
>  	kfree(sp);
> +error_kmalloc:
> +	pci_disable_device(pdev);
>  
>  	return result;
>  }
> @@ -166,6 +175,7 @@
>  	iounmap(sp->base_address);
>  	ibmasm_event_buffer_exit(sp);
>  	kfree(sp);
> +	pci_disable_device(pdev);
>  }
>  
>  static struct pci_device_id ibmasm_pci_table[] =
> -
> 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] ibmasm: add missing pci_enable_device()

[Bjorn Helgaas]

On Monday 09 August 2004 11:18 am, Max Asbock wrote:
> I tested this on the hardware. It works fine.
> Is there any reason we shouldn't use dev_err() for the pci_enable error
> message like in the other messages?

That would probably be fine, too.  ISTR looking at one case where the
printk wrapper wouldn't work quite right if pci_enable_device() failed.
But dev_err() looks like it should work fine.

> regards,
> max
> 
> On Wed, 2004-08-04 at 14:32, Bjorn Helgaas wrote:
> > I don't have this hardware, so this has not been tested.
> > 
> > 
> > Add pci_enable_device()/pci_disable_device().  In the past, drivers
> > often worked without this, but it is now required in order to route
> > PCI interrupts correctly.
> > 
> > Signed-off-by: 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;
> > +	}
> > +
> >  	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;
> >  	}
> >  	memset(sp, 0, sizeof(struct service_processor));
> >  
> > @@ -148,6 +155,8 @@
> >  	ibmasm_event_buffer_exit(sp);
> >  error_eventbuffer:
> >  	kfree(sp);
> > +error_kmalloc:
> > +	pci_disable_device(pdev);
> >  
> >  	return result;
> >  }
> > @@ -166,6 +175,7 @@
> >  	iounmap(sp->base_address);
> >  	ibmasm_event_buffer_exit(sp);
> >  	kfree(sp);
> > +	pci_disable_device(pdev);
> >  }
> >  
> >  static struct pci_device_id ibmasm_pci_table[] =
> > -
> > 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] ibmasm: add missing pci_enable_device()

[Francois Romieu]

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