put_device is required to release the last reference to the device.
Signed-off-by: ChengYi He <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
> > > ---
> > > 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
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 <[email protected]>
> > > > ---
> > > > 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
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 <[email protected]>
> > > > > ---
> > > > > 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
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 <[email protected]>
> > > > > > ---
> > > > > > 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
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 <[email protected]>
> > > > > > > ---
> > > > > > > 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
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...