[PATCH] usb: gadget: goku_udc: Fix error path

[PATCH] usb: gadget: goku_udc: Fix error path

[Rahul Ruikar]

call put_device() when device_register() fails.

Signed-off-by: Rahul Ruikar <rahul.ruikar@gmail.com>
---
 drivers/usb/gadget/goku_udc.c |    7 ++++++-
 drivers/usb/gadget/goku_udc.h |    3 ++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/goku_udc.c b/drivers/usb/gadget/goku_udc.c
index 1088d08..080779b 100644
--- a/drivers/usb/gadget/goku_udc.c
+++ b/drivers/usb/gadget/goku_udc.c
@@ -1847,8 +1847,13 @@ static int goku_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	/* done */
 	the_controller = dev;
 	retval = device_register(&dev->gadget.dev);
-	if (retval == 0)
+	if (retval != 0) {
+		dev_reg_status = 2;
+		goto done;
+	} else {
+		dev_reg_status = 1;
 		return 0;
+	}
 
 done:
 	if (dev)
diff --git a/drivers/usb/gadget/goku_udc.h b/drivers/usb/gadget/goku_udc.h
index 566cb23..c27b9e2 100644
--- a/drivers/usb/gadget/goku_udc.h
+++ b/drivers/usb/gadget/goku_udc.h
@@ -251,7 +251,8 @@ struct goku_udc {
 					got_region:1,
 					req_config:1,
 					configured:1,
-					enabled:1;
+					enabled:1,
+					dev_reg_status:2;
 
 	/* pci state used to access those endpoints */
 	struct pci_dev			*pdev;
-- 
1.7.2.3

Re: [PATCH] usb: gadget: goku_udc: Fix error path

[Dan Carpenter]

On Sun, Oct 03, 2010 at 07:40:16PM +0530, Rahul Ruikar wrote:
> call put_device() when device_register() fails.
> 

Huh?  I don't see any calls to put_device() in your patch.

regards,
dan carpenter


> Signed-off-by: Rahul Ruikar <rahul.ruikar@gmail.com>
> ---
>  drivers/usb/gadget/goku_udc.c |    7 ++++++-
>  drivers/usb/gadget/goku_udc.h |    3 ++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/goku_udc.c b/drivers/usb/gadget/goku_udc.c
> index 1088d08..080779b 100644
> --- a/drivers/usb/gadget/goku_udc.c
> +++ b/drivers/usb/gadget/goku_udc.c
> @@ -1847,8 +1847,13 @@ static int goku_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	/* done */
>  	the_controller = dev;
>  	retval = device_register(&dev->gadget.dev);
> -	if (retval == 0)
> +	if (retval != 0) {
> +		dev_reg_status = 2;
> +		goto done;
> +	} else {
> +		dev_reg_status = 1;
>  		return 0;
> +	}
>  
>  done:
>  	if (dev)
> diff --git a/drivers/usb/gadget/goku_udc.h b/drivers/usb/gadget/goku_udc.h
> index 566cb23..c27b9e2 100644
> --- a/drivers/usb/gadget/goku_udc.h
> +++ b/drivers/usb/gadget/goku_udc.h
> @@ -251,7 +251,8 @@ struct goku_udc {
>  					got_region:1,
>  					req_config:1,
>  					configured:1,
> -					enabled:1;
> +					enabled:1,
> +					dev_reg_status:2;
>  
>  	/* pci state used to access those endpoints */
>  	struct pci_dev			*pdev;
> -- 
> 1.7.2.3

Re: [PATCH] usb: gadget: goku_udc: Fix error path

[Sergei Shtylyov]

Hello.

On 03-10-2010 18:10, Rahul Ruikar wrote:

> call put_device() when device_register() fails.

> Signed-off-by: Rahul Ruikar<rahul.ruikar@gmail.com>
[...]
> diff --git a/drivers/usb/gadget/goku_udc.c b/drivers/usb/gadget/goku_udc.c
> index 1088d08..080779b 100644
> --- a/drivers/usb/gadget/goku_udc.c
> +++ b/drivers/usb/gadget/goku_udc.c
> @@ -1847,8 +1847,13 @@ static int goku_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   	/* done */
>   	the_controller = dev;
>   	retval = device_register(&dev->gadget.dev);
> -	if (retval == 0)
> +	if (retval != 0) {
> +		dev_reg_status = 2;
> +		goto done;
> +	} else {
> +		dev_reg_status = 1;

    Did you mean 'dev->dev_reg_status'?

>   		return 0;
> +	}
>
>   done:
>   	if (dev)
> diff --git a/drivers/usb/gadget/goku_udc.h b/drivers/usb/gadget/goku_udc.h
> index 566cb23..c27b9e2 100644
> --- a/drivers/usb/gadget/goku_udc.h
> +++ b/drivers/usb/gadget/goku_udc.h
> @@ -251,7 +251,8 @@ struct goku_udc {
>   					got_region:1,
>   					req_config:1,
>   					configured:1,
> -					enabled:1;
> +					enabled:1,
> +					dev_reg_status:2;

    I don't see where this new field is read in your patch...

WBR, Sergei

Re: [PATCH] usb: gadget: goku_udc: Fix error path

[Rahul Ruikar]

Sergei,

Please ignore this particular patch.. I resubmitted this in another mail

Thanks,
- Rahul Ruikar





On 4 October 2010 16:16, Sergei Shtylyov <sshtylyov@mvista.com> wrote:
> Hello.
>
> On 03-10-2010 18:10, Rahul Ruikar wrote:
>
>> call put_device() when device_register() fails.
>
>> Signed-off-by: Rahul Ruikar<rahul.ruikar@gmail.com>
>
> [...]
>>
>> diff --git a/drivers/usb/gadget/goku_udc.c b/drivers/usb/gadget/goku_udc.c
>> index 1088d08..080779b 100644
>> --- a/drivers/usb/gadget/goku_udc.c
>> +++ b/drivers/usb/gadget/goku_udc.c
>> @@ -1847,8 +1847,13 @@ static int goku_probe(struct pci_dev *pdev, const
>> struct pci_device_id *id)
>>        /* done */
>>        the_controller = dev;
>>        retval = device_register(&dev->gadget.dev);
>> -       if (retval == 0)
>> +       if (retval != 0) {
>> +               dev_reg_status = 2;
>> +               goto done;
>> +       } else {
>> +               dev_reg_status = 1;
>
>   Did you mean 'dev->dev_reg_status'?
>
>>                return 0;
>> +       }
>>
>>  done:
>>        if (dev)
>> diff --git a/drivers/usb/gadget/goku_udc.h b/drivers/usb/gadget/goku_udc.h
>> index 566cb23..c27b9e2 100644
>> --- a/drivers/usb/gadget/goku_udc.h
>> +++ b/drivers/usb/gadget/goku_udc.h
>> @@ -251,7 +251,8 @@ struct goku_udc {
>>                                        got_region:1,
>>                                        req_config:1,
>>                                        configured:1,
>> -                                       enabled:1;
>> +                                       enabled:1,
>> +                                       dev_reg_status:2;
>
>   I don't see where this new field is read in your patch...
>
> WBR, Sergei
>

[RESEND/PATCH] usb: gadget: goku_udc: Fix error path

[Rahul Ruikar]

call put_device() when device_register() fails.

Signed-off-by: Rahul Ruikar <rahul.ruikar@gmail.com>
---
 drivers/usb/gadget/goku_udc.c |   12 ++++++++++--
 drivers/usb/gadget/goku_udc.h |    3 ++-
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/goku_udc.c b/drivers/usb/gadget/goku_udc.c
index 1088d08..bb70125 100644
--- a/drivers/usb/gadget/goku_udc.c
+++ b/drivers/usb/gadget/goku_udc.c
@@ -1744,7 +1744,10 @@ static void goku_remove(struct pci_dev *pdev)
 				pci_resource_len (pdev, 0));
 	if (dev->enabled)
 		pci_disable_device(pdev);
-	device_unregister(&dev->gadget.dev);
+	if (dev->reg_status == 2)
+		device_unregister(&dev->gadget.dev);
+	else if (dev->reg_status == 1)
+		put_device(&dev->gadget.dev);
 
 	pci_set_drvdata(pdev, NULL);
 	dev->regs = NULL;
@@ -1847,8 +1850,13 @@ static int goku_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	/* done */
 	the_controller = dev;
 	retval = device_register(&dev->gadget.dev);
-	if (retval == 0)
+	if (retval != 0) {
+		dev->reg_status = 2;
+		goto done;
+	} else {
+		dev->reg_status = 1;
 		return 0;
+	}
 
 done:
 	if (dev)
diff --git a/drivers/usb/gadget/goku_udc.h b/drivers/usb/gadget/goku_udc.h
index 566cb23..64f2bfb 100644
--- a/drivers/usb/gadget/goku_udc.h
+++ b/drivers/usb/gadget/goku_udc.h
@@ -251,7 +251,8 @@ struct goku_udc {
 					got_region:1,
 					req_config:1,
 					configured:1,
-					enabled:1;
+					enabled:1,
+					reg_status:2;
 
 	/* pci state used to access those endpoints */
 	struct pci_dev			*pdev;
-- 
1.7.2.3

Re: [RESEND/PATCH] usb: gadget: goku_udc: Fix error path

[Dan Carpenter]

On Mon, Oct 04, 2010 at 01:29:00AM +0530, Rahul Ruikar wrote:
> call put_device() when device_register() fails.
> 

Sorry I didn't realize what you were trying to do here.  This is not
correct at all.

The right thing is to fix device_register() to call put_device() itself.
It's a bit involved, because all the callers will need to be audited but
someone is working on this I think.

regards,
dan carpenter

Re: [RESEND/PATCH] usb: gadget: goku_udc: Fix error path

[Greg KH]

On Mon, Oct 04, 2010 at 02:22:14PM +0200, Dan Carpenter wrote:
> On Mon, Oct 04, 2010 at 01:29:00AM +0530, Rahul Ruikar wrote:
> > call put_device() when device_register() fails.
> > 
> 
> Sorry I didn't realize what you were trying to do here.  This is not
> correct at all.
> 
> The right thing is to fix device_register() to call put_device() itself.
> It's a bit involved, because all the callers will need to be audited but
> someone is working on this I think.

No, no one is working on this, and no, it's not possible to make this
type of change to the driver core as discussed in the past due to the
way struct device can be embedded within another structure.

thanks,

greg k-h

[PATCH] usb: gadget: goku_udc: Fix error path

[Dan Carpenter]

This is based on an initial patch by Rahul Ruikar.

The goku_remove() function can be called before device_register() so it
can call device_unregister() improperly.  Also if the call to 
device_register() fails we need to call put_device().

As I was changing the error handling in goku_probe(), I noticed that
the label was "done" but actually if the function succeeds we return
earlier.  I renamed the error path to "err" instead of "done."

Reported-by: Rahul Ruikar <rahul.ruikar@gmail.com>
Signed-off-by: Dan Carpenter <error27@gmail.com>

diff --git a/drivers/usb/gadget/goku_udc.c b/drivers/usb/gadget/goku_udc.c
index 1088d08..7232018 100644
--- a/drivers/usb/gadget/goku_udc.c
+++ b/drivers/usb/gadget/goku_udc.c
@@ -1744,7 +1744,8 @@ static void goku_remove(struct pci_dev *pdev)
 				pci_resource_len (pdev, 0));
 	if (dev->enabled)
 		pci_disable_device(pdev);
-	device_unregister(&dev->gadget.dev);
+	if (dev->registered)
+		device_unregister(&dev->gadget.dev);
 
 	pci_set_drvdata(pdev, NULL);
 	dev->regs = NULL;
@@ -1774,7 +1775,7 @@ static int goku_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (!pdev->irq) {
 		printk(KERN_ERR "Check PCI %s IRQ setup!\n", pci_name(pdev));
 		retval = -ENODEV;
-		goto done;
+		goto err;
 	}
 
 	/* alloc, and start init */
@@ -1782,7 +1783,7 @@ static int goku_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (dev == NULL){
 		pr_debug("enomem %s\n", pci_name(pdev));
 		retval = -ENOMEM;
-		goto done;
+		goto err;
 	}
 
 	spin_lock_init(&dev->lock);
@@ -1800,7 +1801,7 @@ static int goku_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	retval = pci_enable_device(pdev);
 	if (retval < 0) {
 		DBG(dev, "can't enable, %d\n", retval);
-		goto done;
+		goto err;
 	}
 	dev->enabled = 1;
 
@@ -1809,7 +1810,7 @@ static int goku_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (!request_mem_region(resource, len, driver_name)) {
 		DBG(dev, "controller already in use\n");
 		retval = -EBUSY;
-		goto done;
+		goto err;
 	}
 	dev->got_region = 1;
 
@@ -1817,7 +1818,7 @@ static int goku_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (base == NULL) {
 		DBG(dev, "can't map memory\n");
 		retval = -EFAULT;
-		goto done;
+		goto err;
 	}
 	dev->regs = (struct goku_udc_regs __iomem *) base;
 
@@ -1833,7 +1834,7 @@ static int goku_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 			driver_name, dev) != 0) {
 		DBG(dev, "request interrupt %d failed\n", pdev->irq);
 		retval = -EBUSY;
-		goto done;
+		goto err;
 	}
 	dev->got_irq = 1;
 	if (use_dma)
@@ -1844,13 +1845,16 @@ static int goku_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	create_proc_read_entry(proc_node_name, 0, NULL, udc_proc_read, dev);
 #endif
 
-	/* done */
 	the_controller = dev;
 	retval = device_register(&dev->gadget.dev);
-	if (retval == 0)
-		return 0;
+	if (retval) {
+		put_device(&dev->gadget.dev);
+		goto err;
+	}
+	dev->registered = 1;
+	return 0;
 
-done:
+err:
 	if (dev)
 		goku_remove (pdev);
 	return retval;