[PATCH] driver core: Make sure device detached from driver before deleting it

[PATCH] driver core: Make sure device detached from driver before deleting it

[Jeffy Chen]

There are cases we call device_del() without detaching it from the
driver(e.g. spi core del children devices).

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

 drivers/base/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 12ebd055724c..717efc3020af 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1951,6 +1951,8 @@ void device_del(struct device *dev)
 	struct kobject *glue_dir = NULL;
 	struct class_interface *class_intf;
 
+	device_release_driver(dev);
+
 	/* Notify clients of device removal.  This call must come
 	 * before dpm_sysfs_remove().
 	 */
-- 
2.11.0

Re: [PATCH] driver core: Make sure device detached from driver before deleting it

[Greg Kroah-Hartman]

On Wed, Oct 18, 2017 at 01:49:26PM +0800, Jeffy Chen wrote:
> There are cases we call device_del() without detaching it from the
> driver(e.g. spi core del children devices).

Why would you do that?  Shouldn't that be fixed instead of this odd
work-around for a broken bus?

thanks,

greg k-h

Re: [PATCH] driver core: Make sure device detached from driver before deleting it

[jeffy]

Hi Greg,

Thanks for your reply.

On 10/18/2017 02:19 PM, Greg Kroah-Hartman wrote:
> On Wed, Oct 18, 2017 at 01:49:26PM +0800, Jeffy Chen wrote:
>> >There are cases we call device_del() without detaching it from the
>> >driver(e.g. spi core del children devices).
> Why would you do that?  Shouldn't that be fixed instead of this odd
> work-around for a broken bus?
>
i was thinking since the device_unregister() is called everywhere, maybe 
there are some other drivers missing that too?

and the driver calls device_add(), then the core attaches the device to 
the driver automatically, maybe it would make sense to let the core 
automatic detach it somehow...

but i know nothing about driver core, maybe i should fix it in spi core, 
i'll send new patch for that :)

> thanks,
>
> greg k-h
>
>
>

Re: [PATCH] driver core: Make sure device detached from driver before deleting it

[Greg Kroah-Hartman]

On Wed, Oct 18, 2017 at 03:06:54PM +0800, jeffy wrote:
> Hi Greg,
> 
> Thanks for your reply.
> 
> On 10/18/2017 02:19 PM, Greg Kroah-Hartman wrote:
> > On Wed, Oct 18, 2017 at 01:49:26PM +0800, Jeffy Chen wrote:
> > > >There are cases we call device_del() without detaching it from the
> > > >driver(e.g. spi core del children devices).
> > Why would you do that?  Shouldn't that be fixed instead of this odd
> > work-around for a broken bus?
> > 
> i was thinking since the device_unregister() is called everywhere, maybe
> there are some other drivers missing that too?
> 
> and the driver calls device_add(), then the core attaches the device to the
> driver automatically, maybe it would make sense to let the core automatic
> detach it somehow...
> 
> but i know nothing about driver core, maybe i should fix it in spi core,
> i'll send new patch for that :)

The SPI core should have some internal housekeeping to do there as well,
right?  Is this the only bus that gets this "wrong"?

thanks,

greg k-h

Re: [PATCH] driver core: Make sure device detached from driver before deleting it

[jeffy]

Hi Greg,

On 10/18/2017 03:33 PM, Greg Kroah-Hartman wrote:
> The SPI core should have some internal housekeeping to do there as well,
> right?  Is this the only bus that gets this "wrong"?
>
hmm, i checked i2c code, it has the same issue:

void i2c_unregister_device(struct i2c_client *client)
{
         if (client->dev.of_node)
                 of_node_clear_flag(client->dev.of_node, OF_POPULATED);
         if (ACPI_COMPANION(&client->dev))
                 acpi_device_clear_enumerated(ACPI_COMPANION(&client->dev));
         device_unregister(&client->dev);
}

not sure about others, maybe mfd-core too...

> thanks,
>
> greg k-h
>
>
>

Re: [PATCH] driver core: Make sure device detached from driver before deleting it

[Mark Brown]

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

On Wed, Oct 18, 2017 at 08:19:52AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Oct 18, 2017 at 01:49:26PM +0800, Jeffy Chen wrote:
> > There are cases we call device_del() without detaching it from the
> > driver(e.g. spi core del children devices).

> Why would you do that?  Shouldn't that be fixed instead of this odd
> work-around for a broken bus?

Not that I ever looked at that bit of the SPI stack before but this
feels like an interface bug in the driver core, it's really surprising
that unregistering a device doesn't clean it up.  That's what other
unregister interfaces do.  If this is buggy it looks like the platform
bus will also be buggy, it's just doing a del and a put (plus some stuff
to free resources) which is all device_unregster() does.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] driver core: Make sure device detached from driver before deleting it

[Rafael J. Wysocki]

On Wednesday, October 18, 2017 11:47:21 AM CEST Mark Brown wrote:
> 
> --3mkxuqf5z23bfztf
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> 
> On Wed, Oct 18, 2017 at 08:19:52AM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Oct 18, 2017 at 01:49:26PM +0800, Jeffy Chen wrote:
> > > There are cases we call device_del() without detaching it from the
> > > driver(e.g. spi core del children devices).
> 
> > Why would you do that?  Shouldn't that be fixed instead of this odd
> > work-around for a broken bus?
> 
> Not that I ever looked at that bit of the SPI stack before but this
> feels like an interface bug in the driver core, it's really surprising
> that unregistering a device doesn't clean it up.  That's what other
> unregister interfaces do.  If this is buggy it looks like the platform
> bus will also be buggy, it's just doing a del and a put (plus some stuff
> to free resources) which is all device_unregster() does.

device_del() calls bus_remove_device() which then calls
device_release_driver() eventually.

So there is something going wrong, but that's not a missing
device_release_driver() call. :-)

Re: [PATCH] driver core: Make sure device detached from driver before deleting it

[Rafael J. Wysocki]

On Wednesday, October 18, 2017 7:49:26 AM CEST Jeffy Chen wrote:
> There are cases we call device_del() without detaching it from the
> driver(e.g. spi core del children devices).

But device_del() itself detaches the device from its driver.

> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
> 
>  drivers/base/core.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 12ebd055724c..717efc3020af 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1951,6 +1951,8 @@ void device_del(struct device *dev)
>  	struct kobject *glue_dir = NULL;
>  	struct class_interface *class_intf;
>  
> +	device_release_driver(dev);
> +
>  	/* Notify clients of device removal.  This call must come
>  	 * before dpm_sysfs_remove().
>  	 */
> 

But device_del() calls bus_remove_device() which in turn calls
device_release_driver(), so this looks like an ordering issue to me.

What *exactly* is not working?  Or rather, what symptoms do you see?

Thanks,
Rafael

Re: [PATCH] driver core: Make sure device detached from driver before deleting it

[jeffy]

Hi Rafael,

Thanks for your reply.

On 10/18/2017 06:04 PM, Rafael J. Wysocki wrote:
> But device_del() calls bus_remove_device() which in turn calls
> device_release_driver(), so this looks like an ordering issue to me.
>
uh, right, didn't notice that, it indeed to be a ordering issue...

it turns out in device_del() we are calling device_links_purge() before 
bus_remove_device()...

> What*exactly*  is not working?  Or rather, what symptoms do you see?
>
my board has these devices:
spi master device->spi child device->spi based pwm->pwm_bl

and i add a device link to the pwm and pwm_bl, and got a warning about 
the pwm not unbound:

static void device_links_purge(struct device *dev)
{
...
         list_for_each_entry_safe_reverse(link, ln, 
&dev->links.consumers, s_node) {
                 WARN_ON(link->status != DL_STATE_DORMANT &&
                         link->status != DL_STATE_NONE); <-- warning here!
                 __device_link_del(link);
         }


and i was confused by:
static void mac80211_hwsim_del_radio(struct mac80211_hwsim_data *data,
                                      const char *hwname,
                                      struct genl_info *info)
{
...
         device_release_driver(data->dev);
         device_unregister(data->dev);

and adding that device_release_driver hide that issue(oops...)


so we should move device_links_purge() after bus_remove_device() in 
device_del() right? maybe after device_remove_properties(), and that did 
solve the issue :)


> Thanks,
> Rafael

Re: [PATCH] driver core: Make sure device detached from driver before deleting it

[jeffy]

Hi Rafael,

On 10/18/2017 07:11 PM, jeffy wrote:
>>
> my board has these devices:
> spi master device->spi child device->spi based pwm->pwm_bl
>
> and i add a device link to the pwm and pwm_bl, and got a warning about
> the pwm not unbound:
sorry, it happens when i try to unbind the spi child device, and it's 
warning about the consumer(pwm_bl) not unbound.
>
> static void device_links_purge(struct device *dev)
> {
> ...
>          list_for_each_entry_safe_reverse(link, ln,
> &dev->links.consumers, s_node) {
>                  WARN_ON(link->status != DL_STATE_DORMANT &&
>                          link->status != DL_STATE_NONE); <-- warning here!
>                  __device_link_del(link);
>          }

and i've send a new patch to reorder the device_links_purge() and 
bus_remove_device, thanks again :)

Re: [PATCH] driver core: Make sure device detached from driver before deleting it

[Rafael J. Wysocki]

On Wednesday, October 18, 2017 1:32:15 PM CEST jeffy wrote:
> Hi Rafael,
> 
> On 10/18/2017 07:11 PM, jeffy wrote:
> >>
> > my board has these devices:
> > spi master device->spi child device->spi based pwm->pwm_bl
> >
> > and i add a device link to the pwm and pwm_bl, and got a warning about
> > the pwm not unbound:
> sorry, it happens when i try to unbind the spi child device, and it's 
> warning about the consumer(pwm_bl) not unbound.
> >
> > static void device_links_purge(struct device *dev)
> > {
> > ...
> >          list_for_each_entry_safe_reverse(link, ln,
> > &dev->links.consumers, s_node) {
> >                  WARN_ON(link->status != DL_STATE_DORMANT &&
> >                          link->status != DL_STATE_NONE); <-- warning here!
> >                  __device_link_del(link);
> >          }
> 
> and i've send a new patch to reorder the device_links_purge() and 
> bus_remove_device, thanks again :)

Which I'm not sure is the right approach just yet.

I'll follow up in the patch thread in any case.

Thanks,
Rafael

Re: [PATCH] driver core: Make sure device detached from driver before deleting it

[Rafael J. Wysocki]

On Wednesday, October 18, 2017 1:32:15 PM CEST jeffy wrote:
> Hi Rafael,
> 
> On 10/18/2017 07:11 PM, jeffy wrote:
> >>
> > my board has these devices:
> > spi master device->spi child device->spi based pwm->pwm_bl
> >
> > and i add a device link to the pwm and pwm_bl, and got a warning about
> > the pwm not unbound:
> sorry, it happens when i try to unbind the spi child device, and it's 
> warning about the consumer(pwm_bl) not unbound.
> >
> > static void device_links_purge(struct device *dev)
> > {
> > ...
> >          list_for_each_entry_safe_reverse(link, ln,
> > &dev->links.consumers, s_node) {
> >                  WARN_ON(link->status != DL_STATE_DORMANT &&
> >                          link->status != DL_STATE_NONE); <-- warning here!
> >                  __device_link_del(link);
> >          }
> 
> and i've send a new patch to reorder the device_links_purge() and 
> bus_remove_device, thanks again :)

Which I'm not sure is the right approach just yet.

I'll follow up in the patch thread in any case.

Thanks,
Rafael