[PATCH] usb: ulpi: call put_device if device_register fails

[PATCH] usb: ulpi: call put_device if device_register fails

[ChengYi He]

put_device is required to release the last reference to the device.

Signed-off-by: ChengYi He <chengyihetaipei@gmail.com>
---
 drivers/usb/common/ulpi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
index 0e6f968..bd25bdb 100644
--- a/drivers/usb/common/ulpi.c
+++ b/drivers/usb/common/ulpi.c
@@ -184,8 +184,10 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi)
 	request_module("ulpi:v%04xp%04x", ulpi->id.vendor, ulpi->id.product);
 
 	ret = device_register(&ulpi->dev);
-	if (ret)
+	if (ret) {
+		put_device(&ulpi->dev);
 		return ret;
+	}
 
 	dev_dbg(&ulpi->dev, "registered ULPI PHY: vendor %04x, product %04x\n",
 		ulpi->id.vendor, ulpi->id.product);
-- 
1.9.1

Re: [PATCH] usb: ulpi: call put_device if device_register fails

[Heikki Krogerus]

On Fri, Jun 19, 2015 at 01:12:36AM +0800, ChengYi He wrote:
> put_device is required to release the last reference to the device.
> 
> Signed-off-by: ChengYi He <chengyihetaipei@gmail.com>
> ---
>  drivers/usb/common/ulpi.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> index 0e6f968..bd25bdb 100644
> --- a/drivers/usb/common/ulpi.c
> +++ b/drivers/usb/common/ulpi.c
> @@ -184,8 +184,10 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi)
>  	request_module("ulpi:v%04xp%04x", ulpi->id.vendor, ulpi->id.product);
>  
>  	ret = device_register(&ulpi->dev);
> -	if (ret)
> +	if (ret) {
> +		put_device(&ulpi->dev);

If device_register returns failure, put_device has already been
called. Check device_add in drivers/base/core.c.


Thanks,

-- 
heikki

Re: [PATCH] usb: ulpi: call put_device if device_register fails

[Greg Kroah-Hartman]

On Tue, Jun 23, 2015 at 01:57:38PM +0300, Heikki Krogerus wrote:
> On Fri, Jun 19, 2015 at 01:12:36AM +0800, ChengYi He wrote:
> > put_device is required to release the last reference to the device.
> > 
> > Signed-off-by: ChengYi He <chengyihetaipei@gmail.com>
> > ---
> >  drivers/usb/common/ulpi.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> > index 0e6f968..bd25bdb 100644
> > --- a/drivers/usb/common/ulpi.c
> > +++ b/drivers/usb/common/ulpi.c
> > @@ -184,8 +184,10 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi)
> >  	request_module("ulpi:v%04xp%04x", ulpi->id.vendor, ulpi->id.product);
> >  
> >  	ret = device_register(&ulpi->dev);
> > -	if (ret)
> > +	if (ret) {
> > +		put_device(&ulpi->dev);
> 
> If device_register returns failure, put_device has already been
> called. Check device_add in drivers/base/core.c.

Yes, please read the function, which says:
 * NOTE: _Never_ directly free @dev after calling this function, even
 * if it returned an error! Always use put_device() to give up your
 * reference instead.

But, the problem is that the ulpi core doesn't "own" that struct device.
It comes from elsewhere.  It comes from somewhere deep down in the dw3
core, which is where I lost the path.  Something needs to be fixed in
dwc3_probe() to properly clean up the device if it fails, which is not
happening right now.

So this patch would actually cause much bigger problems than fixing
anything, so it's wrong, but for a different reason than you are talking
about here.

And ugh, the ulpi and dwc code binding together, what a mess, horrid...

greg k-h

Re: [PATCH] usb: ulpi: call put_device if device_register fails

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2004 bytes --]

On Wed, Jul 22, 2015 at 02:39:34PM -0700, Greg Kroah-Hartman wrote:
> On Tue, Jun 23, 2015 at 01:57:38PM +0300, Heikki Krogerus wrote:
> > On Fri, Jun 19, 2015 at 01:12:36AM +0800, ChengYi He wrote:
> > > put_device is required to release the last reference to the device.
> > > 
> > > Signed-off-by: ChengYi He <chengyihetaipei@gmail.com>
> > > ---
> > >  drivers/usb/common/ulpi.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> > > index 0e6f968..bd25bdb 100644
> > > --- a/drivers/usb/common/ulpi.c
> > > +++ b/drivers/usb/common/ulpi.c
> > > @@ -184,8 +184,10 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi)
> > >  	request_module("ulpi:v%04xp%04x", ulpi->id.vendor, ulpi->id.product);
> > >  
> > >  	ret = device_register(&ulpi->dev);
> > > -	if (ret)
> > > +	if (ret) {
> > > +		put_device(&ulpi->dev);
> > 
> > If device_register returns failure, put_device has already been
> > called. Check device_add in drivers/base/core.c.
> 
> Yes, please read the function, which says:
>  * NOTE: _Never_ directly free @dev after calling this function, even
>  * if it returned an error! Always use put_device() to give up your
>  * reference instead.
> 
> But, the problem is that the ulpi core doesn't "own" that struct device.
> It comes from elsewhere.  It comes from somewhere deep down in the dw3
> core, which is where I lost the path.  Something needs to be fixed in
> dwc3_probe() to properly clean up the device if it fails, which is not
> happening right now.
> 
> So this patch would actually cause much bigger problems than fixing
> anything, so it's wrong, but for a different reason than you are talking
> about here.
> 
> And ugh, the ulpi and dwc code binding together, what a mess, horrid...

any suggestions ? DWC *is* the one implementing the bus. If there's a
better way, we can certainly shuffle code around.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] usb: ulpi: call put_device if device_register fails

[Greg Kroah-Hartman]

On Wed, Jul 22, 2015 at 09:04:40PM -0500, Felipe Balbi wrote:
> On Wed, Jul 22, 2015 at 02:39:34PM -0700, Greg Kroah-Hartman wrote:
> > On Tue, Jun 23, 2015 at 01:57:38PM +0300, Heikki Krogerus wrote:
> > > On Fri, Jun 19, 2015 at 01:12:36AM +0800, ChengYi He wrote:
> > > > put_device is required to release the last reference to the device.
> > > > 
> > > > Signed-off-by: ChengYi He <chengyihetaipei@gmail.com>
> > > > ---
> > > >  drivers/usb/common/ulpi.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> > > > index 0e6f968..bd25bdb 100644
> > > > --- a/drivers/usb/common/ulpi.c
> > > > +++ b/drivers/usb/common/ulpi.c
> > > > @@ -184,8 +184,10 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi)
> > > >  	request_module("ulpi:v%04xp%04x", ulpi->id.vendor, ulpi->id.product);
> > > >  
> > > >  	ret = device_register(&ulpi->dev);
> > > > -	if (ret)
> > > > +	if (ret) {
> > > > +		put_device(&ulpi->dev);
> > > 
> > > If device_register returns failure, put_device has already been
> > > called. Check device_add in drivers/base/core.c.
> > 
> > Yes, please read the function, which says:
> >  * NOTE: _Never_ directly free @dev after calling this function, even
> >  * if it returned an error! Always use put_device() to give up your
> >  * reference instead.
> > 
> > But, the problem is that the ulpi core doesn't "own" that struct device.
> > It comes from elsewhere.  It comes from somewhere deep down in the dw3
> > core, which is where I lost the path.  Something needs to be fixed in
> > dwc3_probe() to properly clean up the device if it fails, which is not
> > happening right now.
> > 
> > So this patch would actually cause much bigger problems than fixing
> > anything, so it's wrong, but for a different reason than you are talking
> > about here.
> > 
> > And ugh, the ulpi and dwc code binding together, what a mess, horrid...
> 
> any suggestions ? DWC *is* the one implementing the bus. If there's a
> better way, we can certainly shuffle code around.

As dwc is the only thing using the bus, why is it drivers/usb/core/ ?

And the error path here is broken, the bus should be creating the device
(i.e. no subsystem should ever be registering a device it did not
create), so that it can properly clean things up when stuff goes wrong.

The whole subsys_init() is also a bad feeling that it's not architected
correctly, that shouldn't be needed, which is why I never took that
patch.  Just noticed it came in through yours, I wanted it "broken" so
it would be fixed "properly" and not papered over like this.

thanks,

greg k-h

Re: [PATCH] usb: ulpi: call put_device if device_register fails

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3140 bytes --]

Hi,

On Wed, Jul 22, 2015 at 08:14:46PM -0700, Greg Kroah-Hartman wrote:
> On Wed, Jul 22, 2015 at 09:04:40PM -0500, Felipe Balbi wrote:
> > On Wed, Jul 22, 2015 at 02:39:34PM -0700, Greg Kroah-Hartman wrote:
> > > On Tue, Jun 23, 2015 at 01:57:38PM +0300, Heikki Krogerus wrote:
> > > > On Fri, Jun 19, 2015 at 01:12:36AM +0800, ChengYi He wrote:
> > > > > put_device is required to release the last reference to the device.
> > > > > 
> > > > > Signed-off-by: ChengYi He <chengyihetaipei@gmail.com>
> > > > > ---
> > > > >  drivers/usb/common/ulpi.c | 4 +++-
> > > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> > > > > index 0e6f968..bd25bdb 100644
> > > > > --- a/drivers/usb/common/ulpi.c
> > > > > +++ b/drivers/usb/common/ulpi.c
> > > > > @@ -184,8 +184,10 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi)
> > > > >  	request_module("ulpi:v%04xp%04x", ulpi->id.vendor, ulpi->id.product);
> > > > >  
> > > > >  	ret = device_register(&ulpi->dev);
> > > > > -	if (ret)
> > > > > +	if (ret) {
> > > > > +		put_device(&ulpi->dev);
> > > > 
> > > > If device_register returns failure, put_device has already been
> > > > called. Check device_add in drivers/base/core.c.
> > > 
> > > Yes, please read the function, which says:
> > >  * NOTE: _Never_ directly free @dev after calling this function, even
> > >  * if it returned an error! Always use put_device() to give up your
> > >  * reference instead.
> > > 
> > > But, the problem is that the ulpi core doesn't "own" that struct device.
> > > It comes from elsewhere.  It comes from somewhere deep down in the dw3
> > > core, which is where I lost the path.  Something needs to be fixed in
> > > dwc3_probe() to properly clean up the device if it fails, which is not
> > > happening right now.
> > > 
> > > So this patch would actually cause much bigger problems than fixing
> > > anything, so it's wrong, but for a different reason than you are talking
> > > about here.
> > > 
> > > And ugh, the ulpi and dwc code binding together, what a mess, horrid...
> > 
> > any suggestions ? DWC *is* the one implementing the bus. If there's a
> > better way, we can certainly shuffle code around.
> 
> As dwc is the only thing using the bus, why is it drivers/usb/core/ ?

musb also has a SW-accessible ULPI bus. And, IIRC, so does DWC2 ;-)

> And the error path here is broken, the bus should be creating the device
> (i.e. no subsystem should ever be registering a device it did not
> create), so that it can properly clean things up when stuff goes wrong.
> 
> The whole subsys_init() is also a bad feeling that it's not architected
> correctly, that shouldn't be needed, which is why I never took that
> patch.  Just noticed it came in through yours, I wanted it "broken" so
> it would be fixed "properly" and not papered over like this.

I just felt it would be better to 'fix' it for the -rc until it can be
fixed *properly*. A follow up fix should incur no visible changes to
drivers anyway.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] usb: ulpi: call put_device if device_register fails

[Greg Kroah-Hartman]

On Thu, Jul 23, 2015 at 12:02:40AM -0500, Felipe Balbi wrote:
> Hi,
> 
> On Wed, Jul 22, 2015 at 08:14:46PM -0700, Greg Kroah-Hartman wrote:
> > On Wed, Jul 22, 2015 at 09:04:40PM -0500, Felipe Balbi wrote:
> > > On Wed, Jul 22, 2015 at 02:39:34PM -0700, Greg Kroah-Hartman wrote:
> > > > On Tue, Jun 23, 2015 at 01:57:38PM +0300, Heikki Krogerus wrote:
> > > > > On Fri, Jun 19, 2015 at 01:12:36AM +0800, ChengYi He wrote:
> > > > > > put_device is required to release the last reference to the device.
> > > > > > 
> > > > > > Signed-off-by: ChengYi He <chengyihetaipei@gmail.com>
> > > > > > ---
> > > > > >  drivers/usb/common/ulpi.c | 4 +++-
> > > > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> > > > > > index 0e6f968..bd25bdb 100644
> > > > > > --- a/drivers/usb/common/ulpi.c
> > > > > > +++ b/drivers/usb/common/ulpi.c
> > > > > > @@ -184,8 +184,10 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi)
> > > > > >  	request_module("ulpi:v%04xp%04x", ulpi->id.vendor, ulpi->id.product);
> > > > > >  
> > > > > >  	ret = device_register(&ulpi->dev);
> > > > > > -	if (ret)
> > > > > > +	if (ret) {
> > > > > > +		put_device(&ulpi->dev);
> > > > > 
> > > > > If device_register returns failure, put_device has already been
> > > > > called. Check device_add in drivers/base/core.c.
> > > > 
> > > > Yes, please read the function, which says:
> > > >  * NOTE: _Never_ directly free @dev after calling this function, even
> > > >  * if it returned an error! Always use put_device() to give up your
> > > >  * reference instead.
> > > > 
> > > > But, the problem is that the ulpi core doesn't "own" that struct device.
> > > > It comes from elsewhere.  It comes from somewhere deep down in the dw3
> > > > core, which is where I lost the path.  Something needs to be fixed in
> > > > dwc3_probe() to properly clean up the device if it fails, which is not
> > > > happening right now.
> > > > 
> > > > So this patch would actually cause much bigger problems than fixing
> > > > anything, so it's wrong, but for a different reason than you are talking
> > > > about here.
> > > > 
> > > > And ugh, the ulpi and dwc code binding together, what a mess, horrid...
> > > 
> > > any suggestions ? DWC *is* the one implementing the bus. If there's a
> > > better way, we can certainly shuffle code around.
> > 
> > As dwc is the only thing using the bus, why is it drivers/usb/core/ ?
> 
> musb also has a SW-accessible ULPI bus. And, IIRC, so does DWC2 ;-)

But they aren't calling ulpi_register(), so how can they be using this
code?

> > And the error path here is broken, the bus should be creating the device
> > (i.e. no subsystem should ever be registering a device it did not
> > create), so that it can properly clean things up when stuff goes wrong.
> > 
> > The whole subsys_init() is also a bad feeling that it's not architected
> > correctly, that shouldn't be needed, which is why I never took that
> > patch.  Just noticed it came in through yours, I wanted it "broken" so
> > it would be fixed "properly" and not papered over like this.
> 
> I just felt it would be better to 'fix' it for the -rc until it can be
> fixed *properly*. A follow up fix should incur no visible changes to
> drivers anyway.

I don't like fixes like this because no one now has any pressure to fix
it "properly".  Are you doing that work?  If not, who is?

thanks,

greg k-h

Re: [PATCH] usb: ulpi: call put_device if device_register fails

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3941 bytes --]

Hi,

On Thu, Jul 23, 2015 at 11:00:29AM -0700, Greg Kroah-Hartman wrote:
> On Thu, Jul 23, 2015 at 12:02:40AM -0500, Felipe Balbi wrote:
> > Hi,
> > 
> > On Wed, Jul 22, 2015 at 08:14:46PM -0700, Greg Kroah-Hartman wrote:
> > > On Wed, Jul 22, 2015 at 09:04:40PM -0500, Felipe Balbi wrote:
> > > > On Wed, Jul 22, 2015 at 02:39:34PM -0700, Greg Kroah-Hartman wrote:
> > > > > On Tue, Jun 23, 2015 at 01:57:38PM +0300, Heikki Krogerus wrote:
> > > > > > On Fri, Jun 19, 2015 at 01:12:36AM +0800, ChengYi He wrote:
> > > > > > > put_device is required to release the last reference to the device.
> > > > > > > 
> > > > > > > Signed-off-by: ChengYi He <chengyihetaipei@gmail.com>
> > > > > > > ---
> > > > > > >  drivers/usb/common/ulpi.c | 4 +++-
> > > > > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> > > > > > > index 0e6f968..bd25bdb 100644
> > > > > > > --- a/drivers/usb/common/ulpi.c
> > > > > > > +++ b/drivers/usb/common/ulpi.c
> > > > > > > @@ -184,8 +184,10 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi)
> > > > > > >  	request_module("ulpi:v%04xp%04x", ulpi->id.vendor, ulpi->id.product);
> > > > > > >  
> > > > > > >  	ret = device_register(&ulpi->dev);
> > > > > > > -	if (ret)
> > > > > > > +	if (ret) {
> > > > > > > +		put_device(&ulpi->dev);
> > > > > > 
> > > > > > If device_register returns failure, put_device has already been
> > > > > > called. Check device_add in drivers/base/core.c.
> > > > > 
> > > > > Yes, please read the function, which says:
> > > > >  * NOTE: _Never_ directly free @dev after calling this function, even
> > > > >  * if it returned an error! Always use put_device() to give up your
> > > > >  * reference instead.
> > > > > 
> > > > > But, the problem is that the ulpi core doesn't "own" that struct device.
> > > > > It comes from elsewhere.  It comes from somewhere deep down in the dw3
> > > > > core, which is where I lost the path.  Something needs to be fixed in
> > > > > dwc3_probe() to properly clean up the device if it fails, which is not
> > > > > happening right now.
> > > > > 
> > > > > So this patch would actually cause much bigger problems than fixing
> > > > > anything, so it's wrong, but for a different reason than you are talking
> > > > > about here.
> > > > > 
> > > > > And ugh, the ulpi and dwc code binding together, what a mess, horrid...
> > > > 
> > > > any suggestions ? DWC *is* the one implementing the bus. If there's a
> > > > better way, we can certainly shuffle code around.
> > > 
> > > As dwc is the only thing using the bus, why is it drivers/usb/core/ ?
> > 
> > musb also has a SW-accessible ULPI bus. And, IIRC, so does DWC2 ;-)
> 
> But they aren't calling ulpi_register(), so how can they be using this
> code?

the thing was just added :-) It didn't exist before.

> > > And the error path here is broken, the bus should be creating the device
> > > (i.e. no subsystem should ever be registering a device it did not
> > > create), so that it can properly clean things up when stuff goes wrong.
> > > 
> > > The whole subsys_init() is also a bad feeling that it's not architected
> > > correctly, that shouldn't be needed, which is why I never took that
> > > patch.  Just noticed it came in through yours, I wanted it "broken" so
> > > it would be fixed "properly" and not papered over like this.
> > 
> > I just felt it would be better to 'fix' it for the -rc until it can be
> > fixed *properly*. A follow up fix should incur no visible changes to
> > drivers anyway.
> 
> I don't like fixes like this because no one now has any pressure to fix
> it "properly".  Are you doing that work?  If not, who is?

Heikki is author, I'd expect him to fix it up. We can also revert the
fix if you prefer, I'm totally fine with that.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] usb: ulpi: call put_device if device_register fails

[Greg Kroah-Hartman]

On Thu, Jul 23, 2015 at 03:08:08PM -0500, Felipe Balbi wrote:
> > I don't like fixes like this because no one now has any pressure to fix
> > it "properly".  Are you doing that work?  If not, who is?
> 
> Heikki is author, I'd expect him to fix it up. We can also revert the
> fix if you prefer, I'm totally fine with that.

Let's leave it alone for now and see what happens...