* Calling device_init_wakeup() on driver removal @ 2017-01-15 4:46 Guenter Roeck 2017-01-15 14:49 ` Rafael J. Wysocki 0 siblings, 1 reply; 5+ messages in thread From: Guenter Roeck @ 2017-01-15 4:46 UTC (permalink / raw) To: Rafael J. Wysocki, Len Brown, Pavel Machek Cc: Greg Kroah-Hartman, linux-pm, linux-kernel Hi folks, while looking through driver initialization and removal functions, I noticed that many drivers call device_init_wakeup(dev, false) in the removal function. Given that the driver is about to be removed, that doesn't make much sense to me, especially since device_wakeup_disable() is called from device_pm_remove() anyway. Is it safe to assume that all those calls can be removed, or is there a possible reason for keeping them around ? Thanks, Guenter ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Calling device_init_wakeup() on driver removal 2017-01-15 4:46 Calling device_init_wakeup() on driver removal Guenter Roeck @ 2017-01-15 14:49 ` Rafael J. Wysocki 2017-01-15 15:23 ` Guenter Roeck 0 siblings, 1 reply; 5+ messages in thread From: Rafael J. Wysocki @ 2017-01-15 14:49 UTC (permalink / raw) To: Guenter Roeck Cc: Len Brown, Pavel Machek, Greg Kroah-Hartman, linux-pm, linux-kernel On Saturday, January 14, 2017 08:46:05 PM Guenter Roeck wrote: > Hi folks, Hi, > while looking through driver initialization and removal functions, I noticed that many drivers > call device_init_wakeup(dev, false) in the removal function. Given that the driver is about > to be removed, that doesn't make much sense to me, especially since device_wakeup_disable() > is called from device_pm_remove() anyway. > > Is it safe to assume that all those calls can be removed, or is there a possible reason for > keeping them around ? Removing them automatically might break things, because device_init_wakeup(dev, false) also clears the power.can_wakeup flag and removes the "wakeup" attribute from sysfs. I guess they could be removed safely in the majority of cases, though. Thanks, Rafael ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Calling device_init_wakeup() on driver removal 2017-01-15 14:49 ` Rafael J. Wysocki @ 2017-01-15 15:23 ` Guenter Roeck 2017-01-16 21:50 ` Rafael J. Wysocki 0 siblings, 1 reply; 5+ messages in thread From: Guenter Roeck @ 2017-01-15 15:23 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Len Brown, Pavel Machek, Greg Kroah-Hartman, linux-pm, linux-kernel On 01/15/2017 06:49 AM, Rafael J. Wysocki wrote: > On Saturday, January 14, 2017 08:46:05 PM Guenter Roeck wrote: >> Hi folks, > > Hi, > >> while looking through driver initialization and removal functions, I noticed that many drivers >> call device_init_wakeup(dev, false) in the removal function. Given that the driver is about >> to be removed, that doesn't make much sense to me, especially since device_wakeup_disable() >> is called from device_pm_remove() anyway. >> >> Is it safe to assume that all those calls can be removed, or is there a possible reason for >> keeping them around ? > > Removing them automatically might break things, because device_init_wakeup(dev, false) > also clears the power.can_wakeup flag and removes the "wakeup" attribute from sysfs. > I had the same concern, but I concluded that the wakeup attribute should be removed automatically, since it is added with sysfs_merge_group(), and the matching unmerge call is also made in dpm_sysfs_remove(). power.can_wakeup is part of the device structure, which is in the process of being removed, so I am not sure I understand how that can be problematic. > I guess they could be removed safely in the majority of cases, though. How would one decide if it is needed ? I see some drivers call it on remove, but others don't. I don't see a clear pattern; unless I am missing something, it seems to be more or less random. Thanks, Guenter ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Calling device_init_wakeup() on driver removal 2017-01-15 15:23 ` Guenter Roeck @ 2017-01-16 21:50 ` Rafael J. Wysocki 2017-01-17 4:37 ` Guenter Roeck 0 siblings, 1 reply; 5+ messages in thread From: Rafael J. Wysocki @ 2017-01-16 21:50 UTC (permalink / raw) To: Guenter Roeck Cc: Rafael J. Wysocki, Len Brown, Pavel Machek, Greg Kroah-Hartman, linux-pm, linux-kernel On Sun, Jan 15, 2017 at 4:23 PM, Guenter Roeck <linux@roeck-us.net> wrote: > On 01/15/2017 06:49 AM, Rafael J. Wysocki wrote: >> >> On Saturday, January 14, 2017 08:46:05 PM Guenter Roeck wrote: >>> >>> Hi folks, >> >> >> Hi, >> >>> while looking through driver initialization and removal functions, I >>> noticed that many drivers >>> call device_init_wakeup(dev, false) in the removal function. Given that >>> the driver is about >>> to be removed, that doesn't make much sense to me, especially since >>> device_wakeup_disable() >>> is called from device_pm_remove() anyway. >>> >>> Is it safe to assume that all those calls can be removed, or is there a >>> possible reason for >>> keeping them around ? >> >> >> Removing them automatically might break things, because >> device_init_wakeup(dev, false) >> also clears the power.can_wakeup flag and removes the "wakeup" attribute >> from sysfs. >> > > I had the same concern, but I concluded that the wakeup attribute should be > removed > automatically, since it is added with sysfs_merge_group(), and the matching > unmerge call > is also made in dpm_sysfs_remove(). power.can_wakeup is part of the device > structure, > which is in the process of being removed, so I am not sure I understand how > that can be > problematic. That's when the device goes away, but ->remove() is invoked when the driver goes away too and that's the more common case (by far IMO). >> I guess they could be removed safely in the majority of cases, though. > > > How would one decide if it is needed ? I see some drivers call it on remove, > but others > don't. I don't see a clear pattern; unless I am missing something, it seems > to be > more or less random. My guess would be that drivers calling it don't want the wakeup capability to be exposed after they have gone away. Most likely these are the ones that expose the wakeup attribute on probe. IMO the rule should be that whoever exposes the wakeup attribute one init, should also hide it on cleanup. Thanks, Rafael ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Calling device_init_wakeup() on driver removal 2017-01-16 21:50 ` Rafael J. Wysocki @ 2017-01-17 4:37 ` Guenter Roeck 0 siblings, 0 replies; 5+ messages in thread From: Guenter Roeck @ 2017-01-17 4:37 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Rafael J. Wysocki, Len Brown, Pavel Machek, Greg Kroah-Hartman, linux-pm, linux-kernel On 01/16/2017 01:50 PM, Rafael J. Wysocki wrote: > On Sun, Jan 15, 2017 at 4:23 PM, Guenter Roeck <linux@roeck-us.net> wrote: >> On 01/15/2017 06:49 AM, Rafael J. Wysocki wrote: >>> >>> On Saturday, January 14, 2017 08:46:05 PM Guenter Roeck wrote: >>>> >>>> Hi folks, >>> >>> >>> Hi, >>> >>>> while looking through driver initialization and removal functions, I >>>> noticed that many drivers >>>> call device_init_wakeup(dev, false) in the removal function. Given that >>>> the driver is about >>>> to be removed, that doesn't make much sense to me, especially since >>>> device_wakeup_disable() >>>> is called from device_pm_remove() anyway. >>>> >>>> Is it safe to assume that all those calls can be removed, or is there a >>>> possible reason for >>>> keeping them around ? >>> >>> >>> Removing them automatically might break things, because >>> device_init_wakeup(dev, false) >>> also clears the power.can_wakeup flag and removes the "wakeup" attribute >>> from sysfs. >>> >> >> I had the same concern, but I concluded that the wakeup attribute should be >> removed >> automatically, since it is added with sysfs_merge_group(), and the matching >> unmerge call >> is also made in dpm_sysfs_remove(). power.can_wakeup is part of the device >> structure, >> which is in the process of being removed, so I am not sure I understand how >> that can be >> problematic. > > That's when the device goes away, but ->remove() is invoked when the > driver goes away too and that's the more common case (by far IMO). > Confused ... ah, no, I am using sloppy terminology again. Sorry for that. When I talked about "driver remove function, I meant the remove function defined in 'struct platform_driver' or 'struct i2c_driver', which is the device remove function, not the driver remove function. I am not talking about functions like platform_driver_unregister() or i2c_del_driver(). >>> I guess they could be removed safely in the majority of cases, though. >> >> >> How would one decide if it is needed ? I see some drivers call it on remove, >> but others >> don't. I don't see a clear pattern; unless I am missing something, it seems >> to be >> more or less random. > > My guess would be that drivers calling it don't want the wakeup > capability to be exposed after they have gone away. Most likely these > are the ones that expose the wakeup attribute on probe. > > IMO the rule should be that whoever exposes the wakeup attribute one > init, should also hide it on cleanup. > The probe function is a device probe function. Are you saying that calling device_init_wakeup(dev, false) in the device remove function is necessary if the probe function called device_init_wakeup(dev, true) ? That is exactly what I am wondering. Because, if so, several drivers violate that. Also, I don't really see the point because the device structure is in the process of being removed (and I don't see anything in the code that isn't already cleaned up). Thanks, Guenter ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-01-17 4:37 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-15 4:46 Calling device_init_wakeup() on driver removal Guenter Roeck 2017-01-15 14:49 ` Rafael J. Wysocki 2017-01-15 15:23 ` Guenter Roeck 2017-01-16 21:50 ` Rafael J. Wysocki 2017-01-17 4:37 ` Guenter Roeck
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).