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

[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

[Rahul Ruikar]

Dan,

Following things I tried to do with this change.
there can be 3 cases where one need to handle device_register()
1) reaching error path but device_register() is never called..( ie,
error occurred before calling device_register() function call, in this
case "dev->reg_status" will have value "0" and in error handler
device_unregister() or put_device() will not be called.

2) error occurred at device_register() ( ie. it fails and calls error
handler, this way "dev->reg_status" will have value "2" and in error
handler, based on this value "put_device() will be called.

3) error occured after device_register() success. ( ie, error occured
due to some other function, ) in this case "dev->reg_status" will have
value "1" and in error handler, based on this value
"device_unregister" will be called.

but anyways someone has now proposed to change it to new usb
interface, so this patch is of no use now.

Thanks
Rahul Ruikar





On 4 October 2010 17:52, Dan Carpenter <error27@gmail.com> 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.
>
> regards,
> dan carpenter
>
>

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

[Dan Carpenter]

On Mon, Oct 04, 2010 at 06:54:00PM +0530, Rahul Ruikar wrote:
> Dan,
> 
> Following things I tried to do with this change.
> there can be 3 cases where one need to handle device_register()
> 1) reaching error path but device_register() is never called..( ie,
> error occurred before calling device_register() function call, in this
> case "dev->reg_status" will have value "0" and in error handler
> device_unregister() or put_device() will not be called.
> 

Good point.  This is a bug in the original code.

> 2) error occurred at device_register() ( ie. it fails and calls error
> handler, this way "dev->reg_status" will have value "2" and in error
> handler, based on this value "put_device() will be called.
> 

Yeah.  But device_register() sucks.  It shouldn't require such byzantine
error handling.  Someone is working on this.

regards,
dan carpenter

> 3) error occured after device_register() success. ( ie, error occured
> due to some other function, ) in this case "dev->reg_status" will have
> value "1" and in error handler, based on this value
> "device_unregister" will be called.
> 
> but anyways someone has now proposed to change it to new usb
> interface, so this patch is of no use now.
> 
> Thanks
> Rahul Ruikar
> 

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

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

[Dan Carpenter]

On Tue, Oct 05, 2010 at 06:20:05AM -0700, Greg KH wrote:
> 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.

Sorry I guess I lost track of the discussion.  We could write a helper
macro which added the extra put device on failure?  Isn't that the more
common case anyway?  To be honest, I'm still not clear on when the extra
put_device() is appropriate and when it's not.  :/  I'll look through
the archive.

Anyway, I would prefer a slightly different patch.  The logic is easier
to follow if we keep the put_device() right next to the
device_register().  Also if we keep it in one function and we add a
helper macro later then coccinelle can rewrite it automatically.

I'll send my patch.

regards,
dan carpenter

[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;

[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
>