Commit 52cdbdd49853 ("driver core: correct device's shutdown
order") allowed for proper sequencing of the gpio-keys device
shutdown callbacks by moving the device to the end of the
devices_kset list at probe which was delayed by child
dependencies.
However, commit 722e5f2b1eec ("driver core: Partially revert
"driver core: correct device's shutdown order"") removed this
portion of that commit causing a reversion in the gpio-keys
behavior which can prevent waking from shutdown.
This RFC is an attempt to find a better solution for properly
creating gpio-keys device links to ensure its suspend/resume and
shutdown services are invoked before those of its suppliers.
The first patch here is pretty brute force but simple and has
the advantage that it should be easily backportable to the
versions where the regression first occurred.
The second patch is perhaps better in spirit though still a bit
inelegant, but it can only be backported to versions of the
kernel that contain the commit in its 'Fixes:' tag. That isn't
really a valid 'Fixes:' tag since that commit did not cause the
regression, but it does represent how far the patch could be
backported.
Both commits shouldn't really exist in the same kernel so the
third patch reverts the first in an attempt to make that clear
(though it may be a source of confusion for some).
Hopefully someone with a better understanding of device links
will see a less intrusive way to automatically capture these
dependencies for parent device drivers that implement the
functions of child node devices.
Doug Berger (3):
input: gpio-keys - use device_pm_move_to_tail
input: gpio-keys - add device links of children
Revert "input: gpio-keys - use device_pm_move_to_tail"
drivers/input/keyboard/gpio_keys.c | 7 +++++++
1 file changed, 7 insertions(+)
--
2.34.1
Since the child nodes of gpio-keys are implemented by the
gpio-keys device driver, that driver should explicitly create
the appropriate device links to support proper device power
management and shutdown sequencing.
Fixes: f9aa460672c9 ("driver core: Refactor fw_devlink feature")
Signed-off-by: Doug Berger <[email protected]>
---
drivers/input/keyboard/gpio_keys.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 0516c6279d8a..7a0dcfeb02dc 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -865,6 +865,7 @@ static int gpio_keys_probe(struct platform_device *pdev)
for (i = 0; i < pdata->nbuttons; i++) {
const struct gpio_keys_button *button = &pdata->buttons[i];
+ struct fwnode_link *link;
if (!dev_get_platdata(dev)) {
child = device_get_next_child_node(dev, child);
@@ -882,6 +883,12 @@ static int gpio_keys_probe(struct platform_device *pdev)
fwnode_handle_put(child);
return error;
}
+ if (child) {
+ list_for_each_entry(link, &child->suppliers, c_hook) {
+ device_link_add(dev, link->supplier->dev,
+ DL_FLAG_AUTOREMOVE_CONSUMER);
+ }
+ }
if (button->wakeup)
wakeup = 1;
--
2.34.1
The gpio-keys device driver implements the functionality of its
child nodes which do not receive dedicated drivers. This means
it should inherit the dependencies of these child nodes and
their effects on suspend/resume and shutdown order.
This commit exposes the device_pm_move_to_tail function to
allow the driver to move itself to the end of the lists upon a
successful probe to allow proper sequencing when other methods
are not available.
Fixes: 722e5f2b1eec ("driver core: Partially revert "driver core: correct device's shutdown order"")
Signed-off-by: Doug Berger <[email protected]>
---
drivers/base/core.c | 1 +
drivers/input/keyboard/gpio_keys.c | 2 ++
include/linux/device.h | 1 +
3 files changed, 4 insertions(+)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 6878dfcbf0d6..8385df4d9677 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -428,6 +428,7 @@ void device_pm_move_to_tail(struct device *dev)
device_pm_unlock();
device_links_read_unlock(idx);
}
+EXPORT_SYMBOL_GPL(device_pm_move_to_tail);
#define to_devlink(dev) container_of((dev), struct device_link, link_dev)
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index c42f86ad0766..0516c6279d8a 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -810,6 +810,8 @@ static int gpio_keys_probe(struct platform_device *pdev)
int i, error;
int wakeup = 0;
+ device_pm_move_to_tail(dev);
+
if (!pdata) {
pdata = gpio_keys_get_devtree_pdata(dev);
if (IS_ERR(pdata))
diff --git a/include/linux/device.h b/include/linux/device.h
index 1508e637bb26..dad40bd45509 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1082,6 +1082,7 @@ void device_link_del(struct device_link *link);
void device_link_remove(void *consumer, struct device *supplier);
void device_links_supplier_sync_state_pause(void);
void device_links_supplier_sync_state_resume(void);
+void device_pm_move_to_tail(struct device *dev);
extern __printf(3, 4)
int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
--
2.34.1
This reverts commit 2569873096f7eb1acf63624e9772d82b23923bf4.
Signed-off-by: Doug Berger <[email protected]>
---
drivers/base/core.c | 1 -
drivers/input/keyboard/gpio_keys.c | 2 --
include/linux/device.h | 1 -
3 files changed, 4 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 8385df4d9677..6878dfcbf0d6 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -428,7 +428,6 @@ void device_pm_move_to_tail(struct device *dev)
device_pm_unlock();
device_links_read_unlock(idx);
}
-EXPORT_SYMBOL_GPL(device_pm_move_to_tail);
#define to_devlink(dev) container_of((dev), struct device_link, link_dev)
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 7a0dcfeb02dc..e5af0a254a3f 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -810,8 +810,6 @@ static int gpio_keys_probe(struct platform_device *pdev)
int i, error;
int wakeup = 0;
- device_pm_move_to_tail(dev);
-
if (!pdata) {
pdata = gpio_keys_get_devtree_pdata(dev);
if (IS_ERR(pdata))
diff --git a/include/linux/device.h b/include/linux/device.h
index dad40bd45509..1508e637bb26 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1082,7 +1082,6 @@ void device_link_del(struct device_link *link);
void device_link_remove(void *consumer, struct device *supplier);
void device_links_supplier_sync_state_pause(void);
void device_links_supplier_sync_state_resume(void);
-void device_pm_move_to_tail(struct device *dev);
extern __printf(3, 4)
int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
--
2.34.1
On Thu, Apr 27, 2023 at 03:16:25PM -0700, Doug Berger wrote:
> This reverts commit 2569873096f7eb1acf63624e9772d82b23923bf4.
You have to give a reason why you are reverting it please...
thanks,
greg k-h
On Fri, Apr 28, 2023 at 6:47 AM Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Thu, Apr 27, 2023 at 03:16:25PM -0700, Doug Berger wrote:
> > This reverts commit 2569873096f7eb1acf63624e9772d82b23923bf4.
>
> You have to give a reason why you are reverting it please...
Also, the commit ID above doesn't match any commits in the mainline.
On 4/28/2023 4:40 AM, Rafael J. Wysocki wrote:
> On Fri, Apr 28, 2023 at 6:47 AM Greg Kroah-Hartman
> <[email protected]> wrote:
>>
>> On Thu, Apr 27, 2023 at 03:16:25PM -0700, Doug Berger wrote:
>>> This reverts commit 2569873096f7eb1acf63624e9772d82b23923bf4.
>>
>> You have to give a reason why you are reverting it please...
>
> Also, the commit ID above doesn't match any commits in the mainline.
Apologies. I attempted to explain this in the cover letter, but as
anticipated it caused confusion. The relevant text was:
Both commits shouldn't really exist in the same kernel so the
third patch reverts the first in an attempt to make that clear
(though it may be a source of confusion for some).
This commit ID is the ID of the first patch of this set in my tree. It
slipped my mind that of course that wouldn't be conveyed through
send-mail :). D'oh!
To be clear, this is really a "straw man" request for comment with hope
of finding a more elegant solution to the issue.
Thanks,
Doug
On Thu, Apr 27, 2023 at 3:18 PM Doug Berger <[email protected]> wrote:
>
> Commit 52cdbdd49853 ("driver core: correct device's shutdown
> order") allowed for proper sequencing of the gpio-keys device
> shutdown callbacks by moving the device to the end of the
> devices_kset list at probe which was delayed by child
> dependencies.
>
> However, commit 722e5f2b1eec ("driver core: Partially revert
> "driver core: correct device's shutdown order"") removed this
> portion of that commit causing a reversion in the gpio-keys
> behavior which can prevent waking from shutdown.
>
> This RFC is an attempt to find a better solution for properly
> creating gpio-keys device links to ensure its suspend/resume and
> shutdown services are invoked before those of its suppliers.
>
> The first patch here is pretty brute force but simple and has
> the advantage that it should be easily backportable to the
> versions where the regression first occurred.
We really shouldn't be calling device_pm_move_to_tail() in drivers
because device link uses device_pm_move_to_tail() for ordering too.
And it becomes a "race" between device links and when the driver calls
device_pm_move_to_tail() and I'm not sure we'll get the same ordering
every time.
>
> The second patch is perhaps better in spirit though still a bit
> inelegant, but it can only be backported to versions of the
> kernel that contain the commit in its 'Fixes:' tag. That isn't
> really a valid 'Fixes:' tag since that commit did not cause the
> regression, but it does represent how far the patch could be
> backported.
>
> Both commits shouldn't really exist in the same kernel so the
> third patch reverts the first in an attempt to make that clear
> (though it may be a source of confusion for some).
>
> Hopefully someone with a better understanding of device links
> will see a less intrusive way to automatically capture these
> dependencies for parent device drivers that implement the
> functions of child node devices.
Can you give a summary of the issue on a real system? I took a look at
the two commits you've referenced above and it's not clear what's
still broken in the 6.3+
But I'd think that just teaching fw_devlink about some property should
be sufficient. If you are facing a real issue, have you made sure you
have fw_devlink=on (this is the default unless you turned it off in
the commandline when it had issues in the past).
-Saravana
>
> Doug Berger (3):
> input: gpio-keys - use device_pm_move_to_tail
> input: gpio-keys - add device links of children
> Revert "input: gpio-keys - use device_pm_move_to_tail"
>
> drivers/input/keyboard/gpio_keys.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> --
> 2.34.1
>
On Mon, May 1, 2023 at 1:40 PM Saravana Kannan <[email protected]> wrote:
>
> On Thu, Apr 27, 2023 at 3:18 PM Doug Berger <[email protected]> wrote:
> >
> > Commit 52cdbdd49853 ("driver core: correct device's shutdown
> > order") allowed for proper sequencing of the gpio-keys device
> > shutdown callbacks by moving the device to the end of the
> > devices_kset list at probe which was delayed by child
> > dependencies.
> >
> > However, commit 722e5f2b1eec ("driver core: Partially revert
> > "driver core: correct device's shutdown order"") removed this
> > portion of that commit causing a reversion in the gpio-keys
> > behavior which can prevent waking from shutdown.
> >
> > This RFC is an attempt to find a better solution for properly
> > creating gpio-keys device links to ensure its suspend/resume and
> > shutdown services are invoked before those of its suppliers.
> >
> > The first patch here is pretty brute force but simple and has
> > the advantage that it should be easily backportable to the
> > versions where the regression first occurred.
>
> We really shouldn't be calling device_pm_move_to_tail() in drivers
> because device link uses device_pm_move_to_tail() for ordering too.
> And it becomes a "race" between device links and when the driver calls
> device_pm_move_to_tail() and I'm not sure we'll get the same ordering
> every time.
>
> >
> > The second patch is perhaps better in spirit though still a bit
> > inelegant, but it can only be backported to versions of the
> > kernel that contain the commit in its 'Fixes:' tag. That isn't
> > really a valid 'Fixes:' tag since that commit did not cause the
> > regression, but it does represent how far the patch could be
> > backported.
> >
> > Both commits shouldn't really exist in the same kernel so the
> > third patch reverts the first in an attempt to make that clear
> > (though it may be a source of confusion for some).
> >
> > Hopefully someone with a better understanding of device links
> > will see a less intrusive way to automatically capture these
> > dependencies for parent device drivers that implement the
> > functions of child node devices.
>
> Can you give a summary of the issue on a real system? I took a look at
> the two commits you've referenced above and it's not clear what's
> still broken in the 6.3+
>
> But I'd think that just teaching fw_devlink about some property should
> be sufficient. If you are facing a real issue, have you made sure you
> have fw_devlink=on (this is the default unless you turned it off in
> the commandline when it had issues in the past).
>
I took a closer look at how gpio-keys work and I can see why
fw_devlink doesn't pick up the GPIO dependencies. It's because the
gpio dependencies are listed under child "key-x" device nodes under
the main "gpio-keys" device tree node. fw_devlink doesn't consider
dependencies under child nodes as mandatory dependencies of the parent
node.
The main reason for this was because of how fw_devlink used to work.
But I might be able to change fw_devlink to pick this up
automatically. I need to think a bit more about this because in some
cases, ignoring those dependencies is the right thing to do. Give me a
few weeks to think through and experiment with this on my end.
-Saravana
On 5/2/2023 7:18 PM, Saravana Kannan wrote:
> On Mon, May 1, 2023 at 1:40 PM Saravana Kannan <[email protected]> wrote:
>>
>> On Thu, Apr 27, 2023 at 3:18 PM Doug Berger <[email protected]> wrote:
>>>
>>> Commit 52cdbdd49853 ("driver core: correct device's shutdown
>>> order") allowed for proper sequencing of the gpio-keys device
>>> shutdown callbacks by moving the device to the end of the
>>> devices_kset list at probe which was delayed by child
>>> dependencies.
>>>
>>> However, commit 722e5f2b1eec ("driver core: Partially revert
>>> "driver core: correct device's shutdown order"") removed this
>>> portion of that commit causing a reversion in the gpio-keys
>>> behavior which can prevent waking from shutdown.
>>>
>>> This RFC is an attempt to find a better solution for properly
>>> creating gpio-keys device links to ensure its suspend/resume and
>>> shutdown services are invoked before those of its suppliers.
>>>
>>> The first patch here is pretty brute force but simple and has
>>> the advantage that it should be easily backportable to the
>>> versions where the regression first occurred.
>>
>> We really shouldn't be calling device_pm_move_to_tail() in drivers
>> because device link uses device_pm_move_to_tail() for ordering too.
>> And it becomes a "race" between device links and when the driver calls
>> device_pm_move_to_tail() and I'm not sure we'll get the same ordering
>> every time.
>>
>>>
>>> The second patch is perhaps better in spirit though still a bit
>>> inelegant, but it can only be backported to versions of the
>>> kernel that contain the commit in its 'Fixes:' tag. That isn't
>>> really a valid 'Fixes:' tag since that commit did not cause the
>>> regression, but it does represent how far the patch could be
>>> backported.
>>>
>>> Both commits shouldn't really exist in the same kernel so the
>>> third patch reverts the first in an attempt to make that clear
>>> (though it may be a source of confusion for some).
>>>
>>> Hopefully someone with a better understanding of device links
>>> will see a less intrusive way to automatically capture these
>>> dependencies for parent device drivers that implement the
>>> functions of child node devices.
>>
>> Can you give a summary of the issue on a real system? I took a look at
>> the two commits you've referenced above and it's not clear what's
>> still broken in the 6.3+
>>
>> But I'd think that just teaching fw_devlink about some property should
>> be sufficient. If you are facing a real issue, have you made sure you
>> have fw_devlink=on (this is the default unless you turned it off in
>> the commandline when it had issues in the past).
>>
>
> I took a closer look at how gpio-keys work and I can see why
> fw_devlink doesn't pick up the GPIO dependencies. It's because the
> gpio dependencies are listed under child "key-x" device nodes under
> the main "gpio-keys" device tree node. fw_devlink doesn't consider
> dependencies under child nodes as mandatory dependencies of the parent
> node.
>
> The main reason for this was because of how fw_devlink used to work.
> But I might be able to change fw_devlink to pick this up
> automatically. I need to think a bit more about this because in some
> cases, ignoring those dependencies is the right thing to do. Give me a
> few weeks to think through and experiment with this on my end.
Thank you for taking a deeper look at gpio-keys, and for getting through
the gobblety-gook description in my cover-letter ;).
Yes, the dependencies of children are not automatically inherited by
their parents and it is not clear to me whether or not that should
change, but it definitely creates a problem for the sequencing of
gpio-keys device callbacks.
I initially prepared the second patch as a way to explicitly create
device links specifically for the gpio-keys device from these child
dependencies as a work around for the fw_devlinks being dropped. I don't
really consider this a viable patch which is why I made it an RFC, but I
hoped it would highlight the issue.
However, the regression actually occurs in v4.18 and fw_devlink isn't
introduced until v5.7 so it is desirable to think about solutions that
could be backported to older versions. This is why I provided the first
patch for discussion. Again, it is not a desirable solution just an
illustration what could be easily backported to restore the gpio-keys
behavior prior to commit 722e5f2b1eec ("driver core: Partially revert
"driver core: correct device's shutdown order"") without affecting other
devices.
Thanks again for your willingness to take the time to think this through,
Doug
>
> -Saravana
On Wed, May 3, 2023 at 3:21 PM Doug Berger <[email protected]> wrote:
>
> On 5/2/2023 7:18 PM, Saravana Kannan wrote:
> > On Mon, May 1, 2023 at 1:40 PM Saravana Kannan <[email protected]> wrote:
> >>
> >> On Thu, Apr 27, 2023 at 3:18 PM Doug Berger <[email protected]> wrote:
> >>>
> >>> Commit 52cdbdd49853 ("driver core: correct device's shutdown
> >>> order") allowed for proper sequencing of the gpio-keys device
> >>> shutdown callbacks by moving the device to the end of the
> >>> devices_kset list at probe which was delayed by child
> >>> dependencies.
> >>>
> >>> However, commit 722e5f2b1eec ("driver core: Partially revert
> >>> "driver core: correct device's shutdown order"") removed this
> >>> portion of that commit causing a reversion in the gpio-keys
> >>> behavior which can prevent waking from shutdown.
> >>>
> >>> This RFC is an attempt to find a better solution for properly
> >>> creating gpio-keys device links to ensure its suspend/resume and
> >>> shutdown services are invoked before those of its suppliers.
> >>>
> >>> The first patch here is pretty brute force but simple and has
> >>> the advantage that it should be easily backportable to the
> >>> versions where the regression first occurred.
> >>
> >> We really shouldn't be calling device_pm_move_to_tail() in drivers
> >> because device link uses device_pm_move_to_tail() for ordering too.
> >> And it becomes a "race" between device links and when the driver calls
> >> device_pm_move_to_tail() and I'm not sure we'll get the same ordering
> >> every time.
> >>
> >>>
> >>> The second patch is perhaps better in spirit though still a bit
> >>> inelegant, but it can only be backported to versions of the
> >>> kernel that contain the commit in its 'Fixes:' tag. That isn't
> >>> really a valid 'Fixes:' tag since that commit did not cause the
> >>> regression, but it does represent how far the patch could be
> >>> backported.
> >>>
> >>> Both commits shouldn't really exist in the same kernel so the
> >>> third patch reverts the first in an attempt to make that clear
> >>> (though it may be a source of confusion for some).
> >>>
> >>> Hopefully someone with a better understanding of device links
> >>> will see a less intrusive way to automatically capture these
> >>> dependencies for parent device drivers that implement the
> >>> functions of child node devices.
> >>
> >> Can you give a summary of the issue on a real system? I took a look at
> >> the two commits you've referenced above and it's not clear what's
> >> still broken in the 6.3+
> >>
> >> But I'd think that just teaching fw_devlink about some property should
> >> be sufficient. If you are facing a real issue, have you made sure you
> >> have fw_devlink=on (this is the default unless you turned it off in
> >> the commandline when it had issues in the past).
> >>
> >
> > I took a closer look at how gpio-keys work and I can see why
> > fw_devlink doesn't pick up the GPIO dependencies. It's because the
> > gpio dependencies are listed under child "key-x" device nodes under
> > the main "gpio-keys" device tree node. fw_devlink doesn't consider
> > dependencies under child nodes as mandatory dependencies of the parent
> > node.
> >
> > The main reason for this was because of how fw_devlink used to work.
> > But I might be able to change fw_devlink to pick this up
> > automatically. I need to think a bit more about this because in some
> > cases, ignoring those dependencies is the right thing to do. Give me a
> > few weeks to think through and experiment with this on my end.
> Thank you for taking a deeper look at gpio-keys, and for getting through
> the gobblety-gook description in my cover-letter ;).
>
> Yes, the dependencies of children are not automatically inherited by
> their parents and it is not clear to me whether or not that should
> change, but it definitely creates a problem for the sequencing of
> gpio-keys device callbacks.
>
> I initially prepared the second patch as a way to explicitly create
> device links specifically for the gpio-keys device from these child
> dependencies as a work around for the fw_devlinks being dropped. I don't
> really consider this a viable patch which is why I made it an RFC, but I
> hoped it would highlight the issue.
Thanks. It definitely made it easier for me to get to the root of the
problem/omission.
> However, the regression actually occurs in v4.18 and fw_devlink isn't
> introduced until v5.7 so it is desirable to think about solutions that
> could be backported to older versions.
For older versions, if they have device link support, I'd recommend
creating device links and letting that take care of it.
Also, if you use the right GPIO APIs, at least on newer kernels Linus
W was looking into creating device links automatically from the GPIO
framework level.
So maybe you can backport some variant of that into the older kernels
and leave it to fw_devlink on the newer ones.
-Saravana
> This is why I provided the first
> patch for discussion. Again, it is not a desirable solution just an
> illustration what could be easily backported to restore the gpio-keys
> behavior prior to commit 722e5f2b1eec ("driver core: Partially revert
> "driver core: correct device's shutdown order"") without affecting other
> devices.
>
> Thanks again for your willingness to take the time to think this through,
> Doug
>
> >
> > -Saravana
>