Calling device_init_wakeup() on driver removal

Calling device_init_wakeup() on driver removal

[Guenter Roeck]

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

Re: Calling device_init_wakeup() on driver removal

[Rafael J. Wysocki]

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

Re: Calling device_init_wakeup() on driver removal

[Guenter Roeck]

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

Re: Calling device_init_wakeup() on driver removal

[Rafael J. Wysocki]

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

Re: Calling device_init_wakeup() on driver removal

[Guenter Roeck]

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