Re: [PATCHv3 3/4] drivers/base: clean up the usage of devices_kset_move_last()

Re: [PATCHv3 3/4] drivers/base: clean up the usage of devices_kset_move_last()

[Rafael J. Wysocki]

On Wednesday, July 4, 2018 6:40:09 AM CEST Pingfan Liu wrote:
> On Tue, Jul 3, 2018 at 10:28 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > On Tuesday, July 3, 2018 8:50:41 AM CEST Pingfan Liu wrote:
> > > Clean up the referring to the code in commit 52cdbdd49853 ("driver core:
> > > correct device's shutdown order"). So later we can revert it safely.
> > >
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > Cc: Grygorii Strashko <grygorii.strashko@ti.com>
> > > Cc: Christoph Hellwig <hch@infradead.org>
> > > Cc: Bjorn Helgaas <helgaas@kernel.org>
> > > Cc: Dave Young <dyoung@redhat.com>
> > > Cc: linux-pci@vger.kernel.org
> > > Cc: linuxppc-dev@lists.ozlabs.org
> > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > > ---
> > >  drivers/base/core.c | 7 -------
> > >  1 file changed, 7 deletions(-)
> > >
> > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > index 684b994..db3deb8 100644
> > > --- a/drivers/base/core.c
> > > +++ b/drivers/base/core.c
> > > @@ -127,13 +127,6 @@ static int device_reorder_to_tail(struct device *dev, void *not_used)
> > >  {
> > >       struct device_link *link;
> > >
> > > -     /*
> > > -      * Devices that have not been registered yet will be put to the ends
> > > -      * of the lists during the registration, so skip them here.
> > > -      */
> > > -     if (device_is_registered(dev))
> > > -             devices_kset_move_last(dev);
> > > -
> > >       if (device_pm_initialized(dev))
> > >               device_pm_move_last(dev);
> >
> > You can't do this.
> >
> > If you do it, that will break power management in some situations.
> >
> Could you shed light on it? I had a quick browsing of pm code, but it
> is a big function, and I got lost in it.
> If the above code causes failure, then does it imply that the seq in
> devices_kset should be the same as dpm_list?

Generally, yes it should.

> But in device_shutdown(), it only intersect with pm by
> pm_runtime_get_noresume(dev) and pm_runtime_barrier(dev). How do these
> function affect the seq in dpm_list?

They are not related to dpm_list directly.

However, if you shut down a supplier device before its consumer and that
involves power management, then the consumer shutdown may fail and lock up
the system

I asked you elsewhere to clearly describe the problem you are trying to
address.  Please do that in the first place.

Thanks,
Rafael

Re: [PATCHv3 3/4] drivers/base: clean up the usage of devices_kset_move_last()

[Pingfan Liu]

On Wed, Jul 4, 2018 at 6:18 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Wednesday, July 4, 2018 6:40:09 AM CEST Pingfan Liu wrote:
> > On Tue, Jul 3, 2018 at 10:28 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > >
> > > On Tuesday, July 3, 2018 8:50:41 AM CEST Pingfan Liu wrote:
> > > > Clean up the referring to the code in commit 52cdbdd49853 ("driver core:
> > > > correct device's shutdown order"). So later we can revert it safely.
> > > >
> > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > Cc: Grygorii Strashko <grygorii.strashko@ti.com>
> > > > Cc: Christoph Hellwig <hch@infradead.org>
> > > > Cc: Bjorn Helgaas <helgaas@kernel.org>
> > > > Cc: Dave Young <dyoung@redhat.com>
> > > > Cc: linux-pci@vger.kernel.org
> > > > Cc: linuxppc-dev@lists.ozlabs.org
> > > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > > > ---
> > > >  drivers/base/core.c | 7 -------
> > > >  1 file changed, 7 deletions(-)
> > > >
> > > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > > index 684b994..db3deb8 100644
> > > > --- a/drivers/base/core.c
> > > > +++ b/drivers/base/core.c
> > > > @@ -127,13 +127,6 @@ static int device_reorder_to_tail(struct device *dev, void *not_used)
> > > >  {
> > > >       struct device_link *link;
> > > >
> > > > -     /*
> > > > -      * Devices that have not been registered yet will be put to the ends
> > > > -      * of the lists during the registration, so skip them here.
> > > > -      */
> > > > -     if (device_is_registered(dev))
> > > > -             devices_kset_move_last(dev);
> > > > -
> > > >       if (device_pm_initialized(dev))
> > > >               device_pm_move_last(dev);
> > >
> > > You can't do this.
> > >
> > > If you do it, that will break power management in some situations.
> > >
> > Could you shed light on it? I had a quick browsing of pm code, but it
> > is a big function, and I got lost in it.
> > If the above code causes failure, then does it imply that the seq in
> > devices_kset should be the same as dpm_list?
>
> Generally, yes it should.
>
> > But in device_shutdown(), it only intersect with pm by
> > pm_runtime_get_noresume(dev) and pm_runtime_barrier(dev). How do these
> > function affect the seq in dpm_list?
>
> They are not related to dpm_list directly.
>
> However, if you shut down a supplier device before its consumer and that
> involves power management, then the consumer shutdown may fail and lock up
> the system
>
Ah, get your point. The patch in this series "[PATCHv3 2/4]
drivers/base: utilize device tree info to shutdown devices" still obey
the shutdown order "parent<-child" and "supplier<-consumer". It just
utilizes device-tree info to achieve this, since it turns out not easy
to maintain such order in devices_kset. As I described in the commit
log of [2/4], it needs two nested recursion, and should consider the
breakage of devices_kset's spinlock.

> I asked you elsewhere to clearly describe the problem you are trying to
> address.  Please do that in the first place.
>
OK, I will reply your question in [0/4]

Thanks,
Pingfan

Re: [PATCHv3 0/4] drivers/base: bugfix for supplier<-consumer ordering in device_kset

[Rafael J. Wysocki]

On Wednesday, July 4, 2018 4:47:07 AM CEST Pingfan Liu wrote:
> On Tue, Jul 3, 2018 at 10:36 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > On Tuesday, July 3, 2018 8:50:38 AM CEST Pingfan Liu wrote:
> > > commit 52cdbdd49853 ("driver core: correct device's shutdown order")
> > > places an assumption of supplier<-consumer order on the process of probe.
> > > But it turns out to break down the parent <- child order in some scene.
> > > E.g in pci, a bridge is enabled by pci core, and behind it, the devices
> > > have been probed. Then comes the bridge's module, which enables extra
> > > feature(such as hotplug) on this bridge.
> >
> > So what *exactly* does happen in that case?
> >
> I saw the  shpc_probe() is called on the bridge, although the probing
> failed on that bare-metal. But if it success, then it will enable the
> hotplug feature on the bridge.

I don't understand what you are saying here, sorry.

device_reorder_to_tail() walks the entire device hierarchy below the target
and moves all of the children in there *after* their parents.

How can it break "the parent <- child order" then?

Thanks,
Rafael

Re: [PATCHv3 0/4] drivers/base: bugfix for supplier<-consumer ordering in device_kset

[Pingfan Liu]

On Wed, Jul 4, 2018 at 6:23 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Wednesday, July 4, 2018 4:47:07 AM CEST Pingfan Liu wrote:
> > On Tue, Jul 3, 2018 at 10:36 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > >
> > > On Tuesday, July 3, 2018 8:50:38 AM CEST Pingfan Liu wrote:
> > > > commit 52cdbdd49853 ("driver core: correct device's shutdown order")
> > > > places an assumption of supplier<-consumer order on the process of probe.
> > > > But it turns out to break down the parent <- child order in some scene.
> > > > E.g in pci, a bridge is enabled by pci core, and behind it, the devices
> > > > have been probed. Then comes the bridge's module, which enables extra
> > > > feature(such as hotplug) on this bridge.
> > >
> > > So what *exactly* does happen in that case?
> > >
> > I saw the  shpc_probe() is called on the bridge, although the probing
> > failed on that bare-metal. But if it success, then it will enable the
> > hotplug feature on the bridge.
>
> I don't understand what you are saying here, sorry.
>
On the system, I observe the following:
[    2.114986] devices_kset: Moving 0004:00:00.0 to end of list
<---pcie port drive's probe, but it failed
[    2.115192] devices_kset: Moving 0004:01:00.0 to end of list
[    2.115591] devices_kset: Moving 0004:02:02.0 to end of list
[    2.115923] devices_kset: Moving 0004:02:0a.0 to end of list
[    2.116141] devices_kset: Moving 0004:02:0b.0 to end of list
[    2.116358] devices_kset: Moving 0004:02:0c.0 to end of list
[    3.181860] devices_kset: Moving 0004:03:00.0 to end of list
<---the ata disk controller which sits behind the bridge
[   10.267081] devices_kset: Moving 0004:00:00.0 to end of list
 <---shpc_probe() on this bridge, failed too.

As you can the the parent device "0004:00:00.0" is moved twice, and
finally, it is after the "0004:03:00.0", this will break the
"parent<-child" order in devices_kset. This is caused by the code
really_probe()->devices_kset_move_last(). Apparently, it makes
assumption that child device's probing comes after its parent's. But
it does not stand up in the case.

> device_reorder_to_tail() walks the entire device hierarchy below the target
> and moves all of the children in there *after* their parents.
>
As described, the bug is not related with device_reorder_to_tail(), it
is related with really_probe()->devices_kset_move_last(). So [2/4]
uses different method to achieve the "parent<-child" and
"supplier<-consumer" order. The [3/4] clean up some code in
device_reorder_to_tail(), since I need to revert the commit.

> How can it break "the parent <- child order" then?
>
As described, it does not, just not be in use any longer.

Thanks and regards,
Pingfan

Re: [PATCHv3 0/4] drivers/base: bugfix for supplier<-consumer ordering in device_kset

[Rafael J. Wysocki]

On Thu, Jul 5, 2018 at 4:44 AM, Pingfan Liu <kernelfans@gmail.com> wrote:
> On Wed, Jul 4, 2018 at 6:23 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>
>> On Wednesday, July 4, 2018 4:47:07 AM CEST Pingfan Liu wrote:
>> > On Tue, Jul 3, 2018 at 10:36 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > >
>> > > On Tuesday, July 3, 2018 8:50:38 AM CEST Pingfan Liu wrote:
>> > > > commit 52cdbdd49853 ("driver core: correct device's shutdown order")
>> > > > places an assumption of supplier<-consumer order on the process of probe.
>> > > > But it turns out to break down the parent <- child order in some scene.
>> > > > E.g in pci, a bridge is enabled by pci core, and behind it, the devices
>> > > > have been probed. Then comes the bridge's module, which enables extra
>> > > > feature(such as hotplug) on this bridge.
>> > >
>> > > So what *exactly* does happen in that case?
>> > >
>> > I saw the  shpc_probe() is called on the bridge, although the probing
>> > failed on that bare-metal. But if it success, then it will enable the
>> > hotplug feature on the bridge.
>>
>> I don't understand what you are saying here, sorry.
>>
> On the system, I observe the following:
> [    2.114986] devices_kset: Moving 0004:00:00.0 to end of list
> <---pcie port drive's probe, but it failed
> [    2.115192] devices_kset: Moving 0004:01:00.0 to end of list
> [    2.115591] devices_kset: Moving 0004:02:02.0 to end of list
> [    2.115923] devices_kset: Moving 0004:02:0a.0 to end of list
> [    2.116141] devices_kset: Moving 0004:02:0b.0 to end of list
> [    2.116358] devices_kset: Moving 0004:02:0c.0 to end of list
> [    3.181860] devices_kset: Moving 0004:03:00.0 to end of list
> <---the ata disk controller which sits behind the bridge
> [   10.267081] devices_kset: Moving 0004:00:00.0 to end of list
>  <---shpc_probe() on this bridge, failed too.
>
> As you can the the parent device "0004:00:00.0" is moved twice, and
> finally, it is after the "0004:03:00.0", this will break the
> "parent<-child" order in devices_kset. This is caused by the code
> really_probe()->devices_kset_move_last(). Apparently, it makes
> assumption that child device's probing comes after its parent's. But
> it does not stand up in the case.
>
>> device_reorder_to_tail() walks the entire device hierarchy below the target
>> and moves all of the children in there *after* their parents.
>>
> As described, the bug is not related with device_reorder_to_tail(), it
> is related with really_probe()->devices_kset_move_last().

OK, so calling devices_kset_move_last() from really_probe() clearly is
a mistake.

I'm not really sure what the intention of it was as the changelog of
commit 52cdbdd49853d doesn't really explain that (why would it be
insufficient without that change?) and I'm quite sure that in the
majority of cases it is unnecessary.

I *think* that it attempted to cover a use case similar to the device
links one, but it should have moved children along with the parent
every time like device_link_add() does.

> So [2/4] uses different method to achieve the "parent<-child" and
> "supplier<-consumer" order. The [3/4] clean up some code in
> device_reorder_to_tail(), since I need to revert the commit.

OK, let me look at that one.

Thanks,
Rafael

Re: [PATCHv3 0/4] drivers/base: bugfix for supplier<-consumer ordering in device_kset

[Lukas Wunner]

[cc += Kishon Vijay Abraham]

On Thu, Jul 05, 2018 at 11:18:28AM +0200, Rafael J. Wysocki wrote:
> OK, so calling devices_kset_move_last() from really_probe() clearly is
> a mistake.
> 
> I'm not really sure what the intention of it was as the changelog of
> commit 52cdbdd49853d doesn't really explain that (why would it be
> insufficient without that change?)

It seems 52cdbdd49853d fixed an issue with boards which have an MMC
whose reset pin needs to be driven high on shutdown, lest the MMC
won't be found on the next boot.

The boards' devicetrees use a kludge wherein the reset pin is modelled
as a regulator.  The regulator is enabled when the MMC probes and
disabled on driver unbind and shutdown.  As a result, the pin is driven
low on shutdown and the MMC is not found on the next boot.

To fix this, another kludge was invented wherein the GPIO expander
driving the reset pin unconditionally drives all its pins high on
shutdown, see pcf857x_shutdown() in drivers/gpio/gpio-pcf857x.c
(commit adc284755055, "gpio: pcf857x: restore the initial line state
of all pcf lines").

For this kludge to work, the GPIO expander's ->shutdown hook needs to
be executed after the MMC expander's ->shutdown hook.

Commit 52cdbdd49853d achieved that by reordering devices_kset according
to the probe order.  Apparently the MMC probes after the GPIO expander,
possibly because it returns -EPROBE_DEFER if the vmmc regulator isn't
available yet, see mmc_regulator_get_supply().

Note, I'm just piecing the information together from git history,
I'm not responsible for these kludges.  (I'm innocent!)

@Pingfan Liu, if you just remove the call to devices_kset_move_last()
from really_probe(), does the issue go away?

If so, it might be best to remove that call and model the dependency
with a call to device_link_add() in mmc_regulator_get_supply().
Another idea would be to automatically add a device_link in the
driver core if -EPROBE_DEFER is returned.

Thanks,

Lukas

Re: [PATCHv3 0/4] drivers/base: bugfix for supplier<-consumer ordering in device_kset

[Rafael J. Wysocki]

On Fri, Jul 6, 2018 at 10:36 AM, Lukas Wunner <lukas@wunner.de> wrote:
> [cc += Kishon Vijay Abraham]
>
> On Thu, Jul 05, 2018 at 11:18:28AM +0200, Rafael J. Wysocki wrote:
>> OK, so calling devices_kset_move_last() from really_probe() clearly is
>> a mistake.
>>
>> I'm not really sure what the intention of it was as the changelog of
>> commit 52cdbdd49853d doesn't really explain that (why would it be
>> insufficient without that change?)
>
> It seems 52cdbdd49853d fixed an issue with boards which have an MMC
> whose reset pin needs to be driven high on shutdown, lest the MMC
> won't be found on the next boot.
>
> The boards' devicetrees use a kludge wherein the reset pin is modelled
> as a regulator.  The regulator is enabled when the MMC probes and
> disabled on driver unbind and shutdown.  As a result, the pin is driven
> low on shutdown and the MMC is not found on the next boot.
>
> To fix this, another kludge was invented wherein the GPIO expander
> driving the reset pin unconditionally drives all its pins high on
> shutdown, see pcf857x_shutdown() in drivers/gpio/gpio-pcf857x.c
> (commit adc284755055, "gpio: pcf857x: restore the initial line state
> of all pcf lines").
>
> For this kludge to work, the GPIO expander's ->shutdown hook needs to
> be executed after the MMC expander's ->shutdown hook.
>
> Commit 52cdbdd49853d achieved that by reordering devices_kset according
> to the probe order.  Apparently the MMC probes after the GPIO expander,
> possibly because it returns -EPROBE_DEFER if the vmmc regulator isn't
> available yet, see mmc_regulator_get_supply().
>
> Note, I'm just piecing the information together from git history,
> I'm not responsible for these kludges.  (I'm innocent!)

Sure enough. :-)

In any case, calling devices_kset_move_last() in really_probe() is
plain broken and if its only purpose was to address a single, arguably
kludgy, use case, let's just get rid of it in the first place IMO.

> @Pingfan Liu, if you just remove the call to devices_kset_move_last()
> from really_probe(), does the issue go away?

I would think so from the description of the problem (elsewhere in this thread).

Thanks,
Rafael

Re: [PATCHv3 0/4] drivers/base: bugfix for supplier<-consumer ordering in device_kset

[Pingfan Liu]

On Fri, Jul 6, 2018 at 4:47 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Fri, Jul 6, 2018 at 10:36 AM, Lukas Wunner <lukas@wunner.de> wrote:
> > [cc += Kishon Vijay Abraham]
> >
> > On Thu, Jul 05, 2018 at 11:18:28AM +0200, Rafael J. Wysocki wrote:
> >> OK, so calling devices_kset_move_last() from really_probe() clearly is
> >> a mistake.
> >>
> >> I'm not really sure what the intention of it was as the changelog of
> >> commit 52cdbdd49853d doesn't really explain that (why would it be
> >> insufficient without that change?)
> >
> > It seems 52cdbdd49853d fixed an issue with boards which have an MMC
> > whose reset pin needs to be driven high on shutdown, lest the MMC
> > won't be found on the next boot.
> >
> > The boards' devicetrees use a kludge wherein the reset pin is modelled
> > as a regulator.  The regulator is enabled when the MMC probes and
> > disabled on driver unbind and shutdown.  As a result, the pin is driven
> > low on shutdown and the MMC is not found on the next boot.
> >
> > To fix this, another kludge was invented wherein the GPIO expander
> > driving the reset pin unconditionally drives all its pins high on
> > shutdown, see pcf857x_shutdown() in drivers/gpio/gpio-pcf857x.c
> > (commit adc284755055, "gpio: pcf857x: restore the initial line state
> > of all pcf lines").
> >
> > For this kludge to work, the GPIO expander's ->shutdown hook needs to
> > be executed after the MMC expander's ->shutdown hook.
> >
> > Commit 52cdbdd49853d achieved that by reordering devices_kset according
> > to the probe order.  Apparently the MMC probes after the GPIO expander,
> > possibly because it returns -EPROBE_DEFER if the vmmc regulator isn't
> > available yet, see mmc_regulator_get_supply().
> >
> > Note, I'm just piecing the information together from git history,
> > I'm not responsible for these kludges.  (I'm innocent!)
>
> Sure enough. :-)
>
> In any case, calling devices_kset_move_last() in really_probe() is
> plain broken and if its only purpose was to address a single, arguably
> kludgy, use case, let's just get rid of it in the first place IMO.
>
Yes, if it is only used for a single use case.

> > @Pingfan Liu, if you just remove the call to devices_kset_move_last()
> > from really_probe(), does the issue go away?
>
> I would think so from the description of the problem (elsewhere in this thread).
>
Yes.

Thanks,
Pingfan

Re: [PATCHv3 0/4] drivers/base: bugfix for supplier<-consumer ordering in device_kset

[Pingfan Liu]

On Fri, Jul 6, 2018 at 9:55 PM Pingfan Liu <kernelfans@gmail.com> wrote:
>
> On Fri, Jul 6, 2018 at 4:47 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Fri, Jul 6, 2018 at 10:36 AM, Lukas Wunner <lukas@wunner.de> wrote:
> > > [cc += Kishon Vijay Abraham]
> > >
> > > On Thu, Jul 05, 2018 at 11:18:28AM +0200, Rafael J. Wysocki wrote:
> > >> OK, so calling devices_kset_move_last() from really_probe() clearly is
> > >> a mistake.
> > >>
> > >> I'm not really sure what the intention of it was as the changelog of
> > >> commit 52cdbdd49853d doesn't really explain that (why would it be
> > >> insufficient without that change?)
> > >
> > > It seems 52cdbdd49853d fixed an issue with boards which have an MMC
> > > whose reset pin needs to be driven high on shutdown, lest the MMC
> > > won't be found on the next boot.
> > >
> > > The boards' devicetrees use a kludge wherein the reset pin is modelled
> > > as a regulator.  The regulator is enabled when the MMC probes and
> > > disabled on driver unbind and shutdown.  As a result, the pin is driven
> > > low on shutdown and the MMC is not found on the next boot.
> > >
> > > To fix this, another kludge was invented wherein the GPIO expander
> > > driving the reset pin unconditionally drives all its pins high on
> > > shutdown, see pcf857x_shutdown() in drivers/gpio/gpio-pcf857x.c
> > > (commit adc284755055, "gpio: pcf857x: restore the initial line state
> > > of all pcf lines").
> > >
> > > For this kludge to work, the GPIO expander's ->shutdown hook needs to
> > > be executed after the MMC expander's ->shutdown hook.
> > >
> > > Commit 52cdbdd49853d achieved that by reordering devices_kset according
> > > to the probe order.  Apparently the MMC probes after the GPIO expander,
> > > possibly because it returns -EPROBE_DEFER if the vmmc regulator isn't
> > > available yet, see mmc_regulator_get_supply().
> > >
> > > Note, I'm just piecing the information together from git history,
> > > I'm not responsible for these kludges.  (I'm innocent!)
> >
> > Sure enough. :-)
> >
> > In any case, calling devices_kset_move_last() in really_probe() is
> > plain broken and if its only purpose was to address a single, arguably
> > kludgy, use case, let's just get rid of it in the first place IMO.
> >
> Yes, if it is only used for a single use case.
>
Think it again, I saw other potential issue with the current code.
device_link_add->device_reorder_to_tail() can break the
"supplier<-consumer" order. During moving children after parent's
supplier, it ignores the order of child's consumer.  Beside this,
essentially both devices_kset_move_after/_before() and
device_pm_move_after/_before() expose  the shutdown order to the
indirect caller,  and we can not expect that the caller can not handle
it correctly. It should be a job of drivers core.  It is hard to
extract high dimension info and pack them into one dimension
linked-list. And in theory, it is warranted that the shutdown seq is
correct by using device tree info. More important, it is cheap with
the data structure in hand. So I think it is time to resolve the issue
once for all.

Thanks and regards,
Pingfan

Re: [PATCHv3 0/4] drivers/base: bugfix for supplier<-consumer ordering in device_kset

[Rafael J. Wysocki]

On Sat, Jul 7, 2018 at 6:24 AM, Pingfan Liu <kernelfans@gmail.com> wrote:
> On Fri, Jul 6, 2018 at 9:55 PM Pingfan Liu <kernelfans@gmail.com> wrote:
>>
>> On Fri, Jul 6, 2018 at 4:47 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>> >
>> > On Fri, Jul 6, 2018 at 10:36 AM, Lukas Wunner <lukas@wunner.de> wrote:
>> > > [cc += Kishon Vijay Abraham]
>> > >
>> > > On Thu, Jul 05, 2018 at 11:18:28AM +0200, Rafael J. Wysocki wrote:
>> > >> OK, so calling devices_kset_move_last() from really_probe() clearly is
>> > >> a mistake.
>> > >>
>> > >> I'm not really sure what the intention of it was as the changelog of
>> > >> commit 52cdbdd49853d doesn't really explain that (why would it be
>> > >> insufficient without that change?)
>> > >
>> > > It seems 52cdbdd49853d fixed an issue with boards which have an MMC
>> > > whose reset pin needs to be driven high on shutdown, lest the MMC
>> > > won't be found on the next boot.
>> > >
>> > > The boards' devicetrees use a kludge wherein the reset pin is modelled
>> > > as a regulator.  The regulator is enabled when the MMC probes and
>> > > disabled on driver unbind and shutdown.  As a result, the pin is driven
>> > > low on shutdown and the MMC is not found on the next boot.
>> > >
>> > > To fix this, another kludge was invented wherein the GPIO expander
>> > > driving the reset pin unconditionally drives all its pins high on
>> > > shutdown, see pcf857x_shutdown() in drivers/gpio/gpio-pcf857x.c
>> > > (commit adc284755055, "gpio: pcf857x: restore the initial line state
>> > > of all pcf lines").
>> > >
>> > > For this kludge to work, the GPIO expander's ->shutdown hook needs to
>> > > be executed after the MMC expander's ->shutdown hook.
>> > >
>> > > Commit 52cdbdd49853d achieved that by reordering devices_kset according
>> > > to the probe order.  Apparently the MMC probes after the GPIO expander,
>> > > possibly because it returns -EPROBE_DEFER if the vmmc regulator isn't
>> > > available yet, see mmc_regulator_get_supply().
>> > >
>> > > Note, I'm just piecing the information together from git history,
>> > > I'm not responsible for these kludges.  (I'm innocent!)
>> >
>> > Sure enough. :-)
>> >
>> > In any case, calling devices_kset_move_last() in really_probe() is
>> > plain broken and if its only purpose was to address a single, arguably
>> > kludgy, use case, let's just get rid of it in the first place IMO.
>> >
>> Yes, if it is only used for a single use case.
>>
> Think it again, I saw other potential issue with the current code.
> device_link_add->device_reorder_to_tail() can break the
> "supplier<-consumer" order. During moving children after parent's
> supplier, it ignores the order of child's consumer.

What do you mean?

> Beside this, essentially both devices_kset_move_after/_before() and
> device_pm_move_after/_before() expose  the shutdown order to the
> indirect caller,  and we can not expect that the caller can not handle
> it correctly. It should be a job of drivers core.

Arguably so, but that's how those functions were designed and the
callers should be aware of the limitation.

If they aren't, there is a bug in the caller.

> It is hard to extract high dimension info and pack them into one dimension
> linked-list.

Well, yes and no.

We know it for a fact that there is a linear ordering that will work.
It is inefficient to figure it out every time during system suspend
and resume, for one and that's why we have dpm_list.

Now, if we have it for suspend and resume, it can also be used for shutdown.

> And in theory, it is warranted that the shutdown seq is
> correct by using device tree info. More important, it is cheap with
> the data structure in hand. So I think it is time to resolve the issue
> once for all.

Not the way you want to do that, though.

Thanks,
Rafael

Re: [PATCHv3 0/4] drivers/base: bugfix for supplier<-consumer ordering in device_kset

[Pingfan Liu]

On Sun, Jul 8, 2018 at 4:25 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Sat, Jul 7, 2018 at 6:24 AM, Pingfan Liu <kernelfans@gmail.com> wrote:
> > On Fri, Jul 6, 2018 at 9:55 PM Pingfan Liu <kernelfans@gmail.com> wrote:
> >>
> >> On Fri, Jul 6, 2018 at 4:47 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >> >
> >> > On Fri, Jul 6, 2018 at 10:36 AM, Lukas Wunner <lukas@wunner.de> wrote:
> >> > > [cc += Kishon Vijay Abraham]
> >> > >
> >> > > On Thu, Jul 05, 2018 at 11:18:28AM +0200, Rafael J. Wysocki wrote:
> >> > >> OK, so calling devices_kset_move_last() from really_probe() clearly is
> >> > >> a mistake.
> >> > >>
> >> > >> I'm not really sure what the intention of it was as the changelog of
> >> > >> commit 52cdbdd49853d doesn't really explain that (why would it be
> >> > >> insufficient without that change?)
> >> > >
> >> > > It seems 52cdbdd49853d fixed an issue with boards which have an MMC
> >> > > whose reset pin needs to be driven high on shutdown, lest the MMC
> >> > > won't be found on the next boot.
> >> > >
> >> > > The boards' devicetrees use a kludge wherein the reset pin is modelled
> >> > > as a regulator.  The regulator is enabled when the MMC probes and
> >> > > disabled on driver unbind and shutdown.  As a result, the pin is driven
> >> > > low on shutdown and the MMC is not found on the next boot.
> >> > >
> >> > > To fix this, another kludge was invented wherein the GPIO expander
> >> > > driving the reset pin unconditionally drives all its pins high on
> >> > > shutdown, see pcf857x_shutdown() in drivers/gpio/gpio-pcf857x.c
> >> > > (commit adc284755055, "gpio: pcf857x: restore the initial line state
> >> > > of all pcf lines").
> >> > >
> >> > > For this kludge to work, the GPIO expander's ->shutdown hook needs to
> >> > > be executed after the MMC expander's ->shutdown hook.
> >> > >
> >> > > Commit 52cdbdd49853d achieved that by reordering devices_kset according
> >> > > to the probe order.  Apparently the MMC probes after the GPIO expander,
> >> > > possibly because it returns -EPROBE_DEFER if the vmmc regulator isn't
> >> > > available yet, see mmc_regulator_get_supply().
> >> > >
> >> > > Note, I'm just piecing the information together from git history,
> >> > > I'm not responsible for these kludges.  (I'm innocent!)
> >> >
> >> > Sure enough. :-)
> >> >
> >> > In any case, calling devices_kset_move_last() in really_probe() is
> >> > plain broken and if its only purpose was to address a single, arguably
> >> > kludgy, use case, let's just get rid of it in the first place IMO.
> >> >
> >> Yes, if it is only used for a single use case.
> >>
> > Think it again, I saw other potential issue with the current code.
> > device_link_add->device_reorder_to_tail() can break the
> > "supplier<-consumer" order. During moving children after parent's
> > supplier, it ignores the order of child's consumer.
>
> What do you mean?
>
The drivers use device_link_add() to build "supplier<-consumer" order
without knowing each other. Hence there is the following potential
odds: (consumerX, child_a, ...) (consumer_a,..) (supplierX), where
consumer_a consumes child_a. When
device_link_add()->device_reorder_to_tail() moves all descendant of
consumerX to the tail, it breaks the "supplier<-consumer" order by
"consumer_a <- child_a". And we need recrusion to resolve the item in
(consumer_a,..), each time when moving a consumer behind its supplier,
we may break "parent<-child".

> > Beside this, essentially both devices_kset_move_after/_before() and
> > device_pm_move_after/_before() expose  the shutdown order to the
> > indirect caller,  and we can not expect that the caller can not handle
> > it correctly. It should be a job of drivers core.
>
> Arguably so, but that's how those functions were designed and the
> callers should be aware of the limitation.
>
> If they aren't, there is a bug in the caller.
>
If we consider device_move()-> device_pm_move_after/_before() more
carefully like the above description, then we can hide the detail from
caller. And keep the info of the pm order inside the core.

> > It is hard to extract high dimension info and pack them into one dimension
> > linked-list.
>
> Well, yes and no.
>
For "hard", I means that we need two interleaved recursion to make the
order correct. Otherwise, I think it is a bug or limitation.

> We know it for a fact that there is a linear ordering that will work.
> It is inefficient to figure it out every time during system suspend
> and resume, for one and that's why we have dpm_list.
>
Yeah, I agree that iterating over device tree may hurt performance. I
guess the iterating will not cost the majority of the suspend time,
comparing to the device_suspend(), which causes hardware's sync. But
data is more persuasive. Besides the performance, do you have other
concern till now?

> Now, if we have it for suspend and resume, it can also be used for shutdown.
>
Yes, I do think so.

Thanks and regards,
Pingfan

Re: [PATCHv3 0/4] drivers/base: bugfix for supplier<-consumer ordering in device_kset

[Rafael J. Wysocki]

On Mon, Jul 9, 2018 at 8:48 AM, Pingfan Liu <kernelfans@gmail.com> wrote:
> On Sun, Jul 8, 2018 at 4:25 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>
>> On Sat, Jul 7, 2018 at 6:24 AM, Pingfan Liu <kernelfans@gmail.com> wrote:
>> > On Fri, Jul 6, 2018 at 9:55 PM Pingfan Liu <kernelfans@gmail.com> wrote:
>> >>
>> >> On Fri, Jul 6, 2018 at 4:47 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>> >> >
>> >> > On Fri, Jul 6, 2018 at 10:36 AM, Lukas Wunner <lukas@wunner.de> wrote:
>> >> > > [cc += Kishon Vijay Abraham]
>> >> > >
>> >> > > On Thu, Jul 05, 2018 at 11:18:28AM +0200, Rafael J. Wysocki wrote:
>> >> > >> OK, so calling devices_kset_move_last() from really_probe() clearly is
>> >> > >> a mistake.
>> >> > >>
>> >> > >> I'm not really sure what the intention of it was as the changelog of
>> >> > >> commit 52cdbdd49853d doesn't really explain that (why would it be
>> >> > >> insufficient without that change?)
>> >> > >
>> >> > > It seems 52cdbdd49853d fixed an issue with boards which have an MMC
>> >> > > whose reset pin needs to be driven high on shutdown, lest the MMC
>> >> > > won't be found on the next boot.
>> >> > >
>> >> > > The boards' devicetrees use a kludge wherein the reset pin is modelled
>> >> > > as a regulator.  The regulator is enabled when the MMC probes and
>> >> > > disabled on driver unbind and shutdown.  As a result, the pin is driven
>> >> > > low on shutdown and the MMC is not found on the next boot.
>> >> > >
>> >> > > To fix this, another kludge was invented wherein the GPIO expander
>> >> > > driving the reset pin unconditionally drives all its pins high on
>> >> > > shutdown, see pcf857x_shutdown() in drivers/gpio/gpio-pcf857x.c
>> >> > > (commit adc284755055, "gpio: pcf857x: restore the initial line state
>> >> > > of all pcf lines").
>> >> > >
>> >> > > For this kludge to work, the GPIO expander's ->shutdown hook needs to
>> >> > > be executed after the MMC expander's ->shutdown hook.
>> >> > >
>> >> > > Commit 52cdbdd49853d achieved that by reordering devices_kset according
>> >> > > to the probe order.  Apparently the MMC probes after the GPIO expander,
>> >> > > possibly because it returns -EPROBE_DEFER if the vmmc regulator isn't
>> >> > > available yet, see mmc_regulator_get_supply().
>> >> > >
>> >> > > Note, I'm just piecing the information together from git history,
>> >> > > I'm not responsible for these kludges.  (I'm innocent!)
>> >> >
>> >> > Sure enough. :-)
>> >> >
>> >> > In any case, calling devices_kset_move_last() in really_probe() is
>> >> > plain broken and if its only purpose was to address a single, arguably
>> >> > kludgy, use case, let's just get rid of it in the first place IMO.
>> >> >
>> >> Yes, if it is only used for a single use case.
>> >>
>> > Think it again, I saw other potential issue with the current code.
>> > device_link_add->device_reorder_to_tail() can break the
>> > "supplier<-consumer" order. During moving children after parent's
>> > supplier, it ignores the order of child's consumer.
>>
>> What do you mean?
>>
> The drivers use device_link_add() to build "supplier<-consumer" order
> without knowing each other. Hence there is the following potential
> odds: (consumerX, child_a, ...) (consumer_a,..) (supplierX), where
> consumer_a consumes child_a.

Well, what's the initial state of the list?

> When device_link_add()->device_reorder_to_tail() moves all descendant of
> consumerX to the tail, it breaks the "supplier<-consumer" order by
> "consumer_a <- child_a".

That depends on what the initial ordering of the list is and please
note that circular dependencies are explicitly assumed to be not
present.

The assumption is that the initial ordering of the list reflects the
correct suspend (or shutdown) order without the new link.  Therefore
initially all children are located after their parents and all known
consumers are located after their suppliers.

If a new link is added, the new consumer goes to the end of the list
and all of its children and all of its consumers go after it.
device_reorder_to_tail() is recursive, so for each of the devices that
went to the end of the list, all of its children and all of its
consumers go after it and so on.

Now, that operation doesn't change the order of any of the
parent<-child or supplier<-consumer pairs that get moved and since all
of the devices that depend on any device that get moved go to the end
of list after it, the only devices that don't go to the end of list
are guaranteed to not depend on any of them (they may be parents or
suppliers of the devices that go to the end of the list, but not their
children or suppliers).

> And we need recrusion to resolve the item in
> (consumer_a,..), each time when moving a consumer behind its supplier,
> we may break "parent<-child".

I don't see this as per the above.

Say, device_reorder_to_tail() moves a parent after its child.  This
means that device_reorder_to_tail() was not called for the child after
it had been called for the parent, but that is not true, because it is
called for all of the children of each device that gets moved *after*
moving that device.

>> > Beside this, essentially both devices_kset_move_after/_before() and
>> > device_pm_move_after/_before() expose  the shutdown order to the
>> > indirect caller,  and we can not expect that the caller can not handle
>> > it correctly. It should be a job of drivers core.
>>
>> Arguably so, but that's how those functions were designed and the
>> callers should be aware of the limitation.
>>
>> If they aren't, there is a bug in the caller.
>>
> If we consider device_move()-> device_pm_move_after/_before() more
> carefully like the above description, then we can hide the detail from
> caller. And keep the info of the pm order inside the core.

Yes, we can.

My point is that we have not been doing that so far and the current
callers of those routines are expected to know that.

We can do that to make the life of *future* callers easier (and maybe
to simplify the current ones), but currently the caller is expected to
do the right thing.

>> > It is hard to extract high dimension info and pack them into one dimension
>> > linked-list.
>>
>> Well, yes and no.
>>
> For "hard", I means that we need two interleaved recursion to make the
> order correct. Otherwise, I think it is a bug or limitation.

So the limitation is that circular dependencies may not exist, because
if they did, there would be no suitable suspend/shutdown ordering
between devices.

>> We know it for a fact that there is a linear ordering that will work.
>> It is inefficient to figure it out every time during system suspend
>> and resume, for one and that's why we have dpm_list.
>>
> Yeah, I agree that iterating over device tree may hurt performance. I
> guess the iterating will not cost the majority of the suspend time,
> comparing to the device_suspend(), which causes hardware's sync. But
> data is more persuasive. Besides the performance, do you have other
> concern till now?

I simply think that there should be one way to iterate over devices
for both system-wide PM and shutdown.

The reason why it is not like that today is because of the development
history, but if it doesn't work and we want to fix it, let's just
consolidate all of that.

Now, system-wide suspend resume sometimes iterates the list in the
reverse order which would be hard without having a list, wouldn't it?

>> Now, if we have it for suspend and resume, it can also be used for shutdown.
>>
> Yes, I do think so.

OK

Thanks,
Rafael

Re: [PATCHv3 0/4] drivers/base: bugfix for supplier<-consumer ordering in device_kset

[Pingfan Liu]

On Mon, Jul 9, 2018 at 3:48 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Jul 9, 2018 at 8:48 AM, Pingfan Liu <kernelfans@gmail.com> wrote:
> > On Sun, Jul 8, 2018 at 4:25 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >>
> >> On Sat, Jul 7, 2018 at 6:24 AM, Pingfan Liu <kernelfans@gmail.com> wrote:
> >> > On Fri, Jul 6, 2018 at 9:55 PM Pingfan Liu <kernelfans@gmail.com> wrote:
> >> >>
> >> >> On Fri, Jul 6, 2018 at 4:47 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >> >> >
> >> >> > On Fri, Jul 6, 2018 at 10:36 AM, Lukas Wunner <lukas@wunner.de> wrote:
> >> >> > > [cc += Kishon Vijay Abraham]
> >> >> > >
> >> >> > > On Thu, Jul 05, 2018 at 11:18:28AM +0200, Rafael J. Wysocki wrote:
> >> >> > >> OK, so calling devices_kset_move_last() from really_probe() clearly is
> >> >> > >> a mistake.
> >> >> > >>
> >> >> > >> I'm not really sure what the intention of it was as the changelog of
> >> >> > >> commit 52cdbdd49853d doesn't really explain that (why would it be
> >> >> > >> insufficient without that change?)
> >> >> > >
> >> >> > > It seems 52cdbdd49853d fixed an issue with boards which have an MMC
> >> >> > > whose reset pin needs to be driven high on shutdown, lest the MMC
> >> >> > > won't be found on the next boot.
> >> >> > >
> >> >> > > The boards' devicetrees use a kludge wherein the reset pin is modelled
> >> >> > > as a regulator.  The regulator is enabled when the MMC probes and
> >> >> > > disabled on driver unbind and shutdown.  As a result, the pin is driven
> >> >> > > low on shutdown and the MMC is not found on the next boot.
> >> >> > >
> >> >> > > To fix this, another kludge was invented wherein the GPIO expander
> >> >> > > driving the reset pin unconditionally drives all its pins high on
> >> >> > > shutdown, see pcf857x_shutdown() in drivers/gpio/gpio-pcf857x.c
> >> >> > > (commit adc284755055, "gpio: pcf857x: restore the initial line state
> >> >> > > of all pcf lines").
> >> >> > >
> >> >> > > For this kludge to work, the GPIO expander's ->shutdown hook needs to
> >> >> > > be executed after the MMC expander's ->shutdown hook.
> >> >> > >
> >> >> > > Commit 52cdbdd49853d achieved that by reordering devices_kset according
> >> >> > > to the probe order.  Apparently the MMC probes after the GPIO expander,
> >> >> > > possibly because it returns -EPROBE_DEFER if the vmmc regulator isn't
> >> >> > > available yet, see mmc_regulator_get_supply().
> >> >> > >
> >> >> > > Note, I'm just piecing the information together from git history,
> >> >> > > I'm not responsible for these kludges.  (I'm innocent!)
> >> >> >
> >> >> > Sure enough. :-)
> >> >> >
> >> >> > In any case, calling devices_kset_move_last() in really_probe() is
> >> >> > plain broken and if its only purpose was to address a single, arguably
> >> >> > kludgy, use case, let's just get rid of it in the first place IMO.
> >> >> >
> >> >> Yes, if it is only used for a single use case.
> >> >>
> >> > Think it again, I saw other potential issue with the current code.
> >> > device_link_add->device_reorder_to_tail() can break the
> >> > "supplier<-consumer" order. During moving children after parent's
> >> > supplier, it ignores the order of child's consumer.
> >>
> >> What do you mean?
> >>
> > The drivers use device_link_add() to build "supplier<-consumer" order
> > without knowing each other. Hence there is the following potential
> > odds: (consumerX, child_a, ...) (consumer_a,..) (supplierX), where
> > consumer_a consumes child_a.
>
> Well, what's the initial state of the list?
>
> > When device_link_add()->device_reorder_to_tail() moves all descendant of
> > consumerX to the tail, it breaks the "supplier<-consumer" order by
> > "consumer_a <- child_a".
>
> That depends on what the initial ordering of the list is and please
> note that circular dependencies are explicitly assumed to be not
> present.
>
> The assumption is that the initial ordering of the list reflects the
> correct suspend (or shutdown) order without the new link.  Therefore
> initially all children are located after their parents and all known
> consumers are located after their suppliers.
>
> If a new link is added, the new consumer goes to the end of the list
> and all of its children and all of its consumers go after it.
> device_reorder_to_tail() is recursive, so for each of the devices that
> went to the end of the list, all of its children and all of its
> consumers go after it and so on.
>
> Now, that operation doesn't change the order of any of the
> parent<-child or supplier<-consumer pairs that get moved and since all
> of the devices that depend on any device that get moved go to the end
> of list after it, the only devices that don't go to the end of list
> are guaranteed to not depend on any of them (they may be parents or
> suppliers of the devices that go to the end of the list, but not their
> children or suppliers).
>
Thanks for the detailed explain. It is clear now, and you are right.

> > And we need recrusion to resolve the item in
> > (consumer_a,..), each time when moving a consumer behind its supplier,
> > we may break "parent<-child".
>
> I don't see this as per the above.
>
> Say, device_reorder_to_tail() moves a parent after its child.  This
> means that device_reorder_to_tail() was not called for the child after
> it had been called for the parent, but that is not true, because it is
> called for all of the children of each device that gets moved *after*
> moving that device.
>
Yes, you are right.

> >> > Beside this, essentially both devices_kset_move_after/_before() and
> >> > device_pm_move_after/_before() expose  the shutdown order to the
> >> > indirect caller,  and we can not expect that the caller can not handle
> >> > it correctly. It should be a job of drivers core.
> >>
> >> Arguably so, but that's how those functions were designed and the
> >> callers should be aware of the limitation.
> >>
> >> If they aren't, there is a bug in the caller.
> >>
> > If we consider device_move()-> device_pm_move_after/_before() more
> > carefully like the above description, then we can hide the detail from
> > caller. And keep the info of the pm order inside the core.
>
> Yes, we can.
>
> My point is that we have not been doing that so far and the current
> callers of those routines are expected to know that.
>
> We can do that to make the life of *future* callers easier (and maybe
> to simplify the current ones), but currently the caller is expected to
> do the right thing.
>
OK, I get your point.

> >> > It is hard to extract high dimension info and pack them into one dimension
> >> > linked-list.
> >>
> >> Well, yes and no.
> >>
> > For "hard", I means that we need two interleaved recursion to make the
> > order correct. Otherwise, I think it is a bug or limitation.
>
> So the limitation is that circular dependencies may not exist, because
> if they did, there would be no suitable suspend/shutdown ordering
> between devices.
>
Yes.

> >> We know it for a fact that there is a linear ordering that will work.
> >> It is inefficient to figure it out every time during system suspend
> >> and resume, for one and that's why we have dpm_list.
> >>
> > Yeah, I agree that iterating over device tree may hurt performance. I
> > guess the iterating will not cost the majority of the suspend time,
> > comparing to the device_suspend(), which causes hardware's sync. But
> > data is more persuasive. Besides the performance, do you have other
> > concern till now?
>
> I simply think that there should be one way to iterate over devices
> for both system-wide PM and shutdown.
>
> The reason why it is not like that today is because of the development
> history, but if it doesn't work and we want to fix it, let's just
> consolidate all of that.
>
> Now, system-wide suspend resume sometimes iterates the list in the
> reverse order which would be hard without having a list, wouldn't it?
>
Yes, it would be hard without having a list. I just thought to use
device tree info to build up a shadowed list, and rebuild the list
until there is new device_link_add() operation. For
device_add/_remove(), it can modify the shadowed list directly.

Thanks,
Pingfan

Re: [PATCHv3 0/4] drivers/base: bugfix for supplier<-consumer ordering in device_kset

[Rafael J. Wysocki]

On Mon, Jul 9, 2018 at 10:40 AM, Pingfan Liu <kernelfans@gmail.com> wrote:
> On Mon, Jul 9, 2018 at 3:48 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>
>> On Mon, Jul 9, 2018 at 8:48 AM, Pingfan Liu <kernelfans@gmail.com> wrote:
>> > On Sun, Jul 8, 2018 at 4:25 PM Rafael J. Wysocki <rafael@kernel.org> wrote:

[cut]

>>
>> I simply think that there should be one way to iterate over devices
>> for both system-wide PM and shutdown.
>>
>> The reason why it is not like that today is because of the development
>> history, but if it doesn't work and we want to fix it, let's just
>> consolidate all of that.
>>
>> Now, system-wide suspend resume sometimes iterates the list in the
>> reverse order which would be hard without having a list, wouldn't it?
>>
> Yes, it would be hard without having a list. I just thought to use
> device tree info to build up a shadowed list, and rebuild the list
> until there is new device_link_add() operation. For
> device_add/_remove(), it can modify the shadowed list directly.

Right, and that's the idea of dpm_list, generally speaking: It
represents one of the (possibly many) orders in which devices can be
suspended (or shut down) based on the information coming from the
device hierarchy and device links.

So it appears straightforward (even though it may be complicated
because of the build-time dependencies) to start using dpm_list for
shutdown too - and to ensure that it is properly maintained
everywhere.

Thanks,
Rafael

Re: [PATCHv3 0/4] drivers/base: bugfix for supplier<-consumer ordering in device_kset

[Kishon Vijay Abraham I]

+Grygorii, linux-omap

On Friday 06 July 2018 02:06 PM, Lukas Wunner wrote:
> [cc += Kishon Vijay Abraham]
> 
> On Thu, Jul 05, 2018 at 11:18:28AM +0200, Rafael J. Wysocki wrote:
>> OK, so calling devices_kset_move_last() from really_probe() clearly is
>> a mistake.
>>
>> I'm not really sure what the intention of it was as the changelog of
>> commit 52cdbdd49853d doesn't really explain that (why would it be
>> insufficient without that change?)
> 
> It seems 52cdbdd49853d fixed an issue with boards which have an MMC
> whose reset pin needs to be driven high on shutdown, lest the MMC
> won't be found on the next boot.
> 
> The boards' devicetrees use a kludge wherein the reset pin is modelled
> as a regulator.  The regulator is enabled when the MMC probes and
> disabled on driver unbind and shutdown.  As a result, the pin is driven
> low on shutdown and the MMC is not found on the next boot.
> 
> To fix this, another kludge was invented wherein the GPIO expander
> driving the reset pin unconditionally drives all its pins high on
> shutdown, see pcf857x_shutdown() in drivers/gpio/gpio-pcf857x.c
> (commit adc284755055, "gpio: pcf857x: restore the initial line state
> of all pcf lines").
> 
> For this kludge to work, the GPIO expander's ->shutdown hook needs to
> be executed after the MMC expander's ->shutdown hook.
> 
> Commit 52cdbdd49853d achieved that by reordering devices_kset according
> to the probe order.  Apparently the MMC probes after the GPIO expander,
> possibly because it returns -EPROBE_DEFER if the vmmc regulator isn't
> available yet, see mmc_regulator_get_supply().
> 
> Note, I'm just piecing the information together from git history,
> I'm not responsible for these kludges.  (I'm innocent!)
> 
> @Pingfan Liu, if you just remove the call to devices_kset_move_last()
> from really_probe(), does the issue go away?
> 
> If so, it might be best to remove that call and model the dependency
> with a call to device_link_add() in mmc_regulator_get_supply().

hmm.. had a quick look on this and looks like struct regulator is a regulator
frameworks internal data structure, so its members are not accessible outside.
struct regulator's device pointer is required for device_link_add().

Thanks
Kishon

Re: [PATCHv3 0/4] drivers/base: bugfix for supplier<-consumer ordering in device_kset

[Pingfan Liu]

On Fri, Jul 6, 2018 at 4:36 PM Lukas Wunner <lukas@wunner.de> wrote:
>
> [cc += Kishon Vijay Abraham]
>
> On Thu, Jul 05, 2018 at 11:18:28AM +0200, Rafael J. Wysocki wrote:
> > OK, so calling devices_kset_move_last() from really_probe() clearly is
> > a mistake.
> >
> > I'm not really sure what the intention of it was as the changelog of
> > commit 52cdbdd49853d doesn't really explain that (why would it be
> > insufficient without that change?)
>
> It seems 52cdbdd49853d fixed an issue with boards which have an MMC
> whose reset pin needs to be driven high on shutdown, lest the MMC
> won't be found on the next boot.
>
> The boards' devicetrees use a kludge wherein the reset pin is modelled
> as a regulator.  The regulator is enabled when the MMC probes and
> disabled on driver unbind and shutdown.  As a result, the pin is driven
> low on shutdown and the MMC is not found on the next boot.
>
> To fix this, another kludge was invented wherein the GPIO expander
> driving the reset pin unconditionally drives all its pins high on
> shutdown, see pcf857x_shutdown() in drivers/gpio/gpio-pcf857x.c
> (commit adc284755055, "gpio: pcf857x: restore the initial line state
> of all pcf lines").
>
> For this kludge to work, the GPIO expander's ->shutdown hook needs to
> be executed after the MMC expander's ->shutdown hook.
>
> Commit 52cdbdd49853d achieved that by reordering devices_kset according
> to the probe order.  Apparently the MMC probes after the GPIO expander,
> possibly because it returns -EPROBE_DEFER if the vmmc regulator isn't
> available yet, see mmc_regulator_get_supply().
>
> Note, I'm just piecing the information together from git history,
> I'm not responsible for these kludges.  (I'm innocent!)
>
Thanks for your exploration, very clearly. I had tried, but failed
since these commits are contributed with different authors. I am not
familiar with ARM and dts, so had thought
really_probe()->devices_kset_move_last() is used to address a very
popular "supplier<-consumer" order issue in smart phone, based on the
configuration hard-coded in "bios(or counterpart in ARM).

> @Pingfan Liu, if you just remove the call to devices_kset_move_last()
> from really_probe(), does the issue go away?
>
Yes, it goes away.

> If so, it might be best to remove that call and model the dependency
> with a call to device_link_add() in mmc_regulator_get_supply().
> Another idea would be to automatically add a device_link in the
> driver core if -EPROBE_DEFER is returned.
>
Maybe the first one is better, as it is already used by other drivers.

Thanks,
Pingfan

[PATCH] driver core: Drop devices_kset_move_last() call from really_probe()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The devices_kset_move_last() call in really_probe() is a mistake
as it may cause parents to follow children in the devices_kset list
which then causes system shutdown to fail.  Namely, if a device has
children before really_probe() is called for it (which is not
uncommon), that call will cause it to be reordered after the children
in the devices_kset list and the ordering of that list will not
reflect the correct device shutdown order.

Also it causes the devices_kset list to be constantly reordered
until all drivers have been probed which is totally pointless
overhead in the majority of cases.

For that reason, revert the really_probe() modifications made by
commit 52cdbdd49853.

Fixes: 52cdbdd49853 (driver core: correct device's shutdown order)
Link: https://lore.kernel.org/lkml/CAFgQCTt7VfqM=UyCnvNFxrSw8Z6cUtAi3HUwR4_xPAc03SgHjQ@mail.gmail.com/
Reported-by: Pingfan Liu <kernelfans@gmail.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/dd.c |    8 --------
 1 file changed, 8 deletions(-)

Index: linux-pm/drivers/base/dd.c
===================================================================
--- linux-pm.orig/drivers/base/dd.c
+++ linux-pm/drivers/base/dd.c
@@ -434,14 +434,6 @@ re_probe:
 			goto probe_failed;
 	}
 
-	/*
-	 * Ensure devices are listed in devices_kset in correct order
-	 * It's important to move Dev to the end of devices_kset before
-	 * calling .probe, because it could be recursive and parent Dev
-	 * should always go first
-	 */
-	devices_kset_move_last(dev);
-
 	if (dev->bus->probe) {
 		ret = dev->bus->probe(dev);
 		if (ret)

Re: [PATCH] driver core: Drop devices_kset_move_last() call from really_probe()

[Bjorn Helgaas]

On Fri, Jul 6, 2018 at 5:01 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> The devices_kset_move_last() call in really_probe() is a mistake
> as it may cause parents to follow children in the devices_kset list
> which then causes system shutdown to fail.  Namely, if a device has
> children before really_probe() is called for it (which is not
> uncommon), that call will cause it to be reordered after the children
> in the devices_kset list and the ordering of that list will not
> reflect the correct device shutdown order.
>
> Also it causes the devices_kset list to be constantly reordered
> until all drivers have been probed which is totally pointless
> overhead in the majority of cases.
>
> For that reason, revert the really_probe() modifications made by
> commit 52cdbdd49853.

I'm sure you've considered this, but I can't figure out whether this
patch will reintroduce the problem that was solved by 52cdbdd49853.
That patch updated two places: (1) really_probe(), the change you're
reverting here, and (2) device_move().

device_move() is only called from 4-5 places, none of which look
related to the problem fixed by 52cdbdd49853, so it seems like that
problem was probably resolved by the hunk you're reverting.

> Fixes: 52cdbdd49853 (driver core: correct device's shutdown order)
> Link: https://lore.kernel.org/lkml/CAFgQCTt7VfqM=UyCnvNFxrSw8Z6cUtAi3HUwR4_xPAc03SgHjQ@mail.gmail.com/
> Reported-by: Pingfan Liu <kernelfans@gmail.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/base/dd.c |    8 --------
>  1 file changed, 8 deletions(-)
>
> Index: linux-pm/drivers/base/dd.c
> ===================================================================
> --- linux-pm.orig/drivers/base/dd.c
> +++ linux-pm/drivers/base/dd.c
> @@ -434,14 +434,6 @@ re_probe:
>                         goto probe_failed;
>         }
>
> -       /*
> -        * Ensure devices are listed in devices_kset in correct order
> -        * It's important to move Dev to the end of devices_kset before
> -        * calling .probe, because it could be recursive and parent Dev
> -        * should always go first
> -        */
> -       devices_kset_move_last(dev);
> -
>         if (dev->bus->probe) {
>                 ret = dev->bus->probe(dev);
>                 if (ret)
>

Re: [PATCH] driver core: Drop devices_kset_move_last() call from really_probe()

[Rafael J. Wysocki]

On Mon, Jul 9, 2018 at 3:57 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Fri, Jul 6, 2018 at 5:01 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> The devices_kset_move_last() call in really_probe() is a mistake
>> as it may cause parents to follow children in the devices_kset list
>> which then causes system shutdown to fail.  Namely, if a device has
>> children before really_probe() is called for it (which is not
>> uncommon), that call will cause it to be reordered after the children
>> in the devices_kset list and the ordering of that list will not
>> reflect the correct device shutdown order.
>>
>> Also it causes the devices_kset list to be constantly reordered
>> until all drivers have been probed which is totally pointless
>> overhead in the majority of cases.
>>
>> For that reason, revert the really_probe() modifications made by
>> commit 52cdbdd49853.
>
> I'm sure you've considered this, but I can't figure out whether this
> patch will reintroduce the problem that was solved by 52cdbdd49853.
> That patch updated two places: (1) really_probe(), the change you're
> reverting here, and (2) device_move().
>
> device_move() is only called from 4-5 places, none of which look
> related to the problem fixed by 52cdbdd49853, so it seems like that
> problem was probably resolved by the hunk you're reverting.

That's right, but I don't want to revert all of it.  The other parts
of it are kind of useful as they make the handling of the devices_kset
list be consistent with the handling of dpm_list.

The hunk I'm reverting, however, is completely off.  It not only is
incorrect (as per the above), but it also causes the devices_kset list
and dpm_list to be handled differently.

It had attempted to fix something, but it failed miserably at that.

>> Fixes: 52cdbdd49853 (driver core: correct device's shutdown order)
>> Link: https://lore.kernel.org/lkml/CAFgQCTt7VfqM=UyCnvNFxrSw8Z6cUtAi3HUwR4_xPAc03SgHjQ@mail.gmail.com/
>> Reported-by: Pingfan Liu <kernelfans@gmail.com>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> ---
>>  drivers/base/dd.c |    8 --------
>>  1 file changed, 8 deletions(-)
>>
>> Index: linux-pm/drivers/base/dd.c
>> ===================================================================
>> --- linux-pm.orig/drivers/base/dd.c
>> +++ linux-pm/drivers/base/dd.c
>> @@ -434,14 +434,6 @@ re_probe:
>>                         goto probe_failed;
>>         }
>>
>> -       /*
>> -        * Ensure devices are listed in devices_kset in correct order
>> -        * It's important to move Dev to the end of devices_kset before
>> -        * calling .probe, because it could be recursive and parent Dev
>> -        * should always go first
>> -        */
>> -       devices_kset_move_last(dev);
>> -
>>         if (dev->bus->probe) {
>>                 ret = dev->bus->probe(dev);
>>                 if (ret)
>>

Re: [PATCH] driver core: Drop devices_kset_move_last() call from really_probe()

[Bjorn Helgaas]

[+cc Kishon]

On Mon, Jul 9, 2018 at 4:35 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Jul 9, 2018 at 3:57 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > On Fri, Jul 6, 2018 at 5:01 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >>
> >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>
> >> The devices_kset_move_last() call in really_probe() is a mistake
> >> as it may cause parents to follow children in the devices_kset list
> >> which then causes system shutdown to fail.  Namely, if a device has
> >> children before really_probe() is called for it (which is not
> >> uncommon), that call will cause it to be reordered after the children
> >> in the devices_kset list and the ordering of that list will not
> >> reflect the correct device shutdown order.
> >>
> >> Also it causes the devices_kset list to be constantly reordered
> >> until all drivers have been probed which is totally pointless
> >> overhead in the majority of cases.
> >>
> >> For that reason, revert the really_probe() modifications made by
> >> commit 52cdbdd49853.
> >
> > I'm sure you've considered this, but I can't figure out whether this
> > patch will reintroduce the problem that was solved by 52cdbdd49853.
> > That patch updated two places: (1) really_probe(), the change you're
> > reverting here, and (2) device_move().
> >
> > device_move() is only called from 4-5 places, none of which look
> > related to the problem fixed by 52cdbdd49853, so it seems like that
> > problem was probably resolved by the hunk you're reverting.
>
> That's right, but I don't want to revert all of it.  The other parts
> of it are kind of useful as they make the handling of the devices_kset
> list be consistent with the handling of dpm_list.
>
> The hunk I'm reverting, however, is completely off.  It not only is
> incorrect (as per the above), but it also causes the devices_kset list
> and dpm_list to be handled differently.

If I understand correctly, you are saying:

  - the 52cdbdd49853 really_probe() hunk fixed a problem, but
  - that hunk was the wrong fix for it, and
  - this patch removes the wrong fix (and probably reintroduces the problem)

If devices_kset is supposed to be ordered so children follow parents,
I agree the really_probe() hunk doesn't make much sense because the
parent/child relation is determined by the circuit design, not by the
probe order.

It just seems like it's worth being clear that we're reintroducing the
problem fixed by 52cdbdd49853, so it needs to be solved a different
way.  Ideally that would be done before this patch so there's not a
regression, and this changelog could mention what's happening.

> It had attempted to fix something, but it failed miserably at that.

If you're saying that 52cdbdd49853 *tried* to fix a DRA7XX_evm reboot
problem, but in fact, it did not fix that problem, then I guess there
should be no issue with reverting that hunk.

> >> Fixes: 52cdbdd49853 (driver core: correct device's shutdown order)
> >> Link: https://lore.kernel.org/lkml/CAFgQCTt7VfqM=UyCnvNFxrSw8Z6cUtAi3HUwR4_xPAc03SgHjQ@mail.gmail.com/
> >> Reported-by: Pingfan Liu <kernelfans@gmail.com>
> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> ---
> >>  drivers/base/dd.c |    8 --------
> >>  1 file changed, 8 deletions(-)
> >>
> >> Index: linux-pm/drivers/base/dd.c
> >> ===================================================================
> >> --- linux-pm.orig/drivers/base/dd.c
> >> +++ linux-pm/drivers/base/dd.c
> >> @@ -434,14 +434,6 @@ re_probe:
> >>                         goto probe_failed;
> >>         }
> >>
> >> -       /*
> >> -        * Ensure devices are listed in devices_kset in correct order
> >> -        * It's important to move Dev to the end of devices_kset before
> >> -        * calling .probe, because it could be recursive and parent Dev
> >> -        * should always go first
> >> -        */
> >> -       devices_kset_move_last(dev);
> >> -
> >>         if (dev->bus->probe) {
> >>                 ret = dev->bus->probe(dev);
> >>                 if (ret)
> >>

Re: [PATCH] driver core: Drop devices_kset_move_last() call from really_probe()

[Kishon Vijay Abraham I]

+Mark, Liam

Hi,

On Tuesday 10 July 2018 03:36 AM, Bjorn Helgaas wrote:
> [+cc Kishon]
> 
> On Mon, Jul 9, 2018 at 4:35 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>
>> On Mon, Jul 9, 2018 at 3:57 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>> On Fri, Jul 6, 2018 at 5:01 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>>
>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>
>>>> The devices_kset_move_last() call in really_probe() is a mistake
>>>> as it may cause parents to follow children in the devices_kset list
>>>> which then causes system shutdown to fail.  Namely, if a device has
>>>> children before really_probe() is called for it (which is not
>>>> uncommon), that call will cause it to be reordered after the children
>>>> in the devices_kset list and the ordering of that list will not
>>>> reflect the correct device shutdown order.
>>>>
>>>> Also it causes the devices_kset list to be constantly reordered
>>>> until all drivers have been probed which is totally pointless
>>>> overhead in the majority of cases.
>>>>
>>>> For that reason, revert the really_probe() modifications made by
>>>> commit 52cdbdd49853.
>>>
>>> I'm sure you've considered this, but I can't figure out whether this
>>> patch will reintroduce the problem that was solved by 52cdbdd49853.
>>> That patch updated two places: (1) really_probe(), the change you're
>>> reverting here, and (2) device_move().
>>>
>>> device_move() is only called from 4-5 places, none of which look
>>> related to the problem fixed by 52cdbdd49853, so it seems like that
>>> problem was probably resolved by the hunk you're reverting.
>>
>> That's right, but I don't want to revert all of it.  The other parts
>> of it are kind of useful as they make the handling of the devices_kset
>> list be consistent with the handling of dpm_list.
>>
>> The hunk I'm reverting, however, is completely off.  It not only is
>> incorrect (as per the above), but it also causes the devices_kset list
>> and dpm_list to be handled differently.
> 
> If I understand correctly, you are saying:
> 
>   - the 52cdbdd49853 really_probe() hunk fixed a problem, but
>   - that hunk was the wrong fix for it, and
>   - this patch removes the wrong fix (and probably reintroduces the problem)
> 
> If devices_kset is supposed to be ordered so children follow parents,
> I agree the really_probe() hunk doesn't make much sense because the
> parent/child relation is determined by the circuit design, not by the
> probe order.
> 
> It just seems like it's worth being clear that we're reintroducing the
> problem fixed by 52cdbdd49853, so it needs to be solved a different
> way.  Ideally that would be done before this patch so there's not a
> regression, and this changelog could mention what's happening.
> 
>> It had attempted to fix something, but it failed miserably at that.
> 
> If you're saying that 52cdbdd49853 *tried* to fix a DRA7XX_evm reboot
> problem, but in fact, it did not fix that problem, then I guess there
> should be no issue with reverting that hunk.

It did fix a problem making sure the regulator's shutdown is invoked after the
mmc shutdown. And reverting 52cdbdd49853 reintroduces the problem.

I tried adding device_link_add in the _regulator_get, something like below and
it seems to fix the problem again. But I guess we have to maintain a list of
device_link's in regulator_dev since there can be many consumers for a single
regulator and we also have to invoke device_link_del in _regulator_put. In
general this might have to be extended to other producers like PHY, pinctrl etc..

If this looks okay, I can post a patch after adding a list and invoking
device_link_del() in regulator core.

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 6ed568b96c0e..24a25700128a 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1740,6 +1740,7 @@ struct regulator *_regulator_get(struct device *dev,
const char *id,
                        rdev->use_count = 0;
        }

+       device_link_add(dev, &rdev->dev, DL_FLAG_STATELESS);
        return regulator;
 }

Thanks
Kishon

Re: [PATCH] driver core: Drop devices_kset_move_last() call from really_probe()

[Rafael J. Wysocki]

On Tue, Jul 10, 2018 at 8:19 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
> +Mark, Liam
>
> Hi,
>
> On Tuesday 10 July 2018 03:36 AM, Bjorn Helgaas wrote:
>> [+cc Kishon]
>>
>> On Mon, Jul 9, 2018 at 4:35 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>>
>>> On Mon, Jul 9, 2018 at 3:57 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>>> On Fri, Jul 6, 2018 at 5:01 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>>>
>>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>>
>>>>> The devices_kset_move_last() call in really_probe() is a mistake
>>>>> as it may cause parents to follow children in the devices_kset list
>>>>> which then causes system shutdown to fail.  Namely, if a device has
>>>>> children before really_probe() is called for it (which is not
>>>>> uncommon), that call will cause it to be reordered after the children
>>>>> in the devices_kset list and the ordering of that list will not
>>>>> reflect the correct device shutdown order.
>>>>>
>>>>> Also it causes the devices_kset list to be constantly reordered
>>>>> until all drivers have been probed which is totally pointless
>>>>> overhead in the majority of cases.
>>>>>
>>>>> For that reason, revert the really_probe() modifications made by
>>>>> commit 52cdbdd49853.
>>>>
>>>> I'm sure you've considered this, but I can't figure out whether this
>>>> patch will reintroduce the problem that was solved by 52cdbdd49853.
>>>> That patch updated two places: (1) really_probe(), the change you're
>>>> reverting here, and (2) device_move().
>>>>
>>>> device_move() is only called from 4-5 places, none of which look
>>>> related to the problem fixed by 52cdbdd49853, so it seems like that
>>>> problem was probably resolved by the hunk you're reverting.
>>>
>>> That's right, but I don't want to revert all of it.  The other parts
>>> of it are kind of useful as they make the handling of the devices_kset
>>> list be consistent with the handling of dpm_list.
>>>
>>> The hunk I'm reverting, however, is completely off.  It not only is
>>> incorrect (as per the above), but it also causes the devices_kset list
>>> and dpm_list to be handled differently.
>>
>> If I understand correctly, you are saying:
>>
>>   - the 52cdbdd49853 really_probe() hunk fixed a problem, but
>>   - that hunk was the wrong fix for it, and
>>   - this patch removes the wrong fix (and probably reintroduces the problem)
>>
>> If devices_kset is supposed to be ordered so children follow parents,
>> I agree the really_probe() hunk doesn't make much sense because the
>> parent/child relation is determined by the circuit design, not by the
>> probe order.
>>
>> It just seems like it's worth being clear that we're reintroducing the
>> problem fixed by 52cdbdd49853, so it needs to be solved a different
>> way.  Ideally that would be done before this patch so there's not a
>> regression, and this changelog could mention what's happening.
>>
>>> It had attempted to fix something, but it failed miserably at that.
>>
>> If you're saying that 52cdbdd49853 *tried* to fix a DRA7XX_evm reboot
>> problem, but in fact, it did not fix that problem, then I guess there
>> should be no issue with reverting that hunk.
>
> It did fix a problem making sure the regulator's shutdown is invoked after the
> mmc shutdown. And reverting 52cdbdd49853 reintroduces the problem.

But, of course, it didn't prevent regulator suspend from being run
before mmc suspend, so it really addressed part of the problem only
and while doing that it introduced a regression.

This piece of really_probe() is incorrect and it has to go away.

Thanks,
Rafael

Re: [PATCH] driver core: Drop devices_kset_move_last() call from really_probe()

[Rafael J. Wysocki]

On Tue, Jul 10, 2018 at 12:06 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc Kishon]
>
> On Mon, Jul 9, 2018 at 4:35 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>
>> On Mon, Jul 9, 2018 at 3:57 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> > On Fri, Jul 6, 2018 at 5:01 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> >>
>> >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> >>
>> >> The devices_kset_move_last() call in really_probe() is a mistake
>> >> as it may cause parents to follow children in the devices_kset list
>> >> which then causes system shutdown to fail.  Namely, if a device has
>> >> children before really_probe() is called for it (which is not
>> >> uncommon), that call will cause it to be reordered after the children
>> >> in the devices_kset list and the ordering of that list will not
>> >> reflect the correct device shutdown order.
>> >>
>> >> Also it causes the devices_kset list to be constantly reordered
>> >> until all drivers have been probed which is totally pointless
>> >> overhead in the majority of cases.
>> >>
>> >> For that reason, revert the really_probe() modifications made by
>> >> commit 52cdbdd49853.
>> >
>> > I'm sure you've considered this, but I can't figure out whether this
>> > patch will reintroduce the problem that was solved by 52cdbdd49853.
>> > That patch updated two places: (1) really_probe(), the change you're
>> > reverting here, and (2) device_move().
>> >
>> > device_move() is only called from 4-5 places, none of which look
>> > related to the problem fixed by 52cdbdd49853, so it seems like that
>> > problem was probably resolved by the hunk you're reverting.
>>
>> That's right, but I don't want to revert all of it.  The other parts
>> of it are kind of useful as they make the handling of the devices_kset
>> list be consistent with the handling of dpm_list.
>>
>> The hunk I'm reverting, however, is completely off.  It not only is
>> incorrect (as per the above), but it also causes the devices_kset list
>> and dpm_list to be handled differently.
>
> If I understand correctly, you are saying:
>
>   - the 52cdbdd49853 really_probe() hunk fixed a problem, but

It papered over a shutdown failure.  Calling it a "fix" is an overstatement IMO.

>   - that hunk was the wrong fix for it, and
>   - this patch removes the wrong fix (and probably reintroduces the problem)
>
> If devices_kset is supposed to be ordered so children follow parents,
> I agree the really_probe() hunk doesn't make much sense because the
> parent/child relation is determined by the circuit design, not by the
> probe order.

Exactly.

> It just seems like it's worth being clear that we're reintroducing the
> problem fixed by 52cdbdd49853, so it needs to be solved a different
> way.

OK

> Ideally that would be done before this patch so there's not a
> regression, and this changelog could mention what's happening.

Well, commit 52cdbdd49853 introduced a regression by itself, but that
regression has only been reported recently.

I don't really want to go into a discussion on which of the two
regressions is more painful, but then IMO going back to the state from
before commit 52cdbdd49853 is fair enough.  Hence the patch.

>> It had attempted to fix something, but it failed miserably at that.
>
> If you're saying that 52cdbdd49853 *tried* to fix a DRA7XX_evm reboot
> problem, but in fact, it did not fix that problem, then I guess there
> should be no issue with reverting that hunk.

Again, it hid the reboot problem by changing the core in a way that
led to a shutdown regression elsewhere.

Also it looks like the platform(s) having that reboot issue do(es)n't
really do system-wide suspend/resume, because that "fix" obviously
doesn't help there.

>> >> Fixes: 52cdbdd49853 (driver core: correct device's shutdown order)
>> >> Link: https://lore.kernel.org/lkml/CAFgQCTt7VfqM=UyCnvNFxrSw8Z6cUtAi3HUwR4_xPAc03SgHjQ@mail.gmail.com/
>> >> Reported-by: Pingfan Liu <kernelfans@gmail.com>
>> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> >> ---
>> >>  drivers/base/dd.c |    8 --------
>> >>  1 file changed, 8 deletions(-)
>> >>
>> >> Index: linux-pm/drivers/base/dd.c
>> >> ===================================================================
>> >> --- linux-pm.orig/drivers/base/dd.c
>> >> +++ linux-pm/drivers/base/dd.c
>> >> @@ -434,14 +434,6 @@ re_probe:
>> >>                         goto probe_failed;
>> >>         }
>> >>
>> >> -       /*
>> >> -        * Ensure devices are listed in devices_kset in correct order
>> >> -        * It's important to move Dev to the end of devices_kset before
>> >> -        * calling .probe, because it could be recursive and parent Dev
>> >> -        * should always go first
>> >> -        */
>> >> -       devices_kset_move_last(dev);
>> >> -
>> >>         if (dev->bus->probe) {
>> >>                 ret = dev->bus->probe(dev);
>> >>                 if (ret)
>> >>

Re: [PATCH] driver core: Drop devices_kset_move_last() call from really_probe()

[Pingfan Liu]

On Fri, Jul 6, 2018 at 6:01 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> The devices_kset_move_last() call in really_probe() is a mistake
> as it may cause parents to follow children in the devices_kset list
> which then causes system shutdown to fail.  Namely, if a device has
> children before really_probe() is called for it (which is not
> uncommon), that call will cause it to be reordered after the children
> in the devices_kset list and the ordering of that list will not
> reflect the correct device shutdown order.
>
> Also it causes the devices_kset list to be constantly reordered
> until all drivers have been probed which is totally pointless
> overhead in the majority of cases.
>
> For that reason, revert the really_probe() modifications made by
> commit 52cdbdd49853.
>
> Fixes: 52cdbdd49853 (driver core: correct device's shutdown order)
> Link: https://lore.kernel.org/lkml/CAFgQCTt7VfqM=UyCnvNFxrSw8Z6cUtAi3HUwR4_xPAc03SgHjQ@mail.gmail.com/
> Reported-by: Pingfan Liu <kernelfans@gmail.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/base/dd.c |    8 --------
>  1 file changed, 8 deletions(-)
>
> Index: linux-pm/drivers/base/dd.c
> ===================================================================
> --- linux-pm.orig/drivers/base/dd.c
> +++ linux-pm/drivers/base/dd.c
> @@ -434,14 +434,6 @@ re_probe:
>                         goto probe_failed;
>         }
>
> -       /*
> -        * Ensure devices are listed in devices_kset in correct order
> -        * It's important to move Dev to the end of devices_kset before
> -        * calling .probe, because it could be recursive and parent Dev
> -        * should always go first
> -        */
> -       devices_kset_move_last(dev);
> -
>         if (dev->bus->probe) {
>                 ret = dev->bus->probe(dev);
>                 if (ret)
>
Tested-by: Pingfan Liu <kernelfans@gmail.com>

[PATCH] driver core: Partially revert "driver core: correct device's shutdown order"

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Commit 52cdbdd49853 (driver core: correct device's shutdown order)
introduced a regression by breaking device shutdown on some systems.

Namely, the devices_kset_move_last() call in really_probe() added by
that commit is a mistake as it may cause parents to follow children
in the devices_kset list which then causes shutdown to fail.  For
example, if a device has children before really_probe() is called
for it (which is not uncommon), that call will cause it to be
reordered after the children in the devices_kset list and the
ordering of that list will not reflect the correct device shutdown
order any more.

Also it causes the devices_kset list to be constantly reordered
until all drivers have been probed which is totally pointless
overhead in the majority of cases and it only covers an issue
with system shutdown, while system-wide suspend/resume potentially
has the same issue on the affected platforms (which is not covered).

For that reason, revert the really_probe() modifications made by
commit 52cdbdd49853 which unfortunately will expose the shutdown
issue the problematic commit attempted to fix (and which will have
to be addressed differently and correctly in the future).

The other code changes made by commit 52cdbdd49853 are useful and
they need not be reverted.

Fixes: 52cdbdd49853 (driver core: correct device's shutdown order)
Link: https://lore.kernel.org/lkml/CAFgQCTt7VfqM=UyCnvNFxrSw8Z6cUtAi3HUwR4_xPAc03SgHjQ@mail.gmail.com/
Reported-by: Pingfan Liu <kernelfans@gmail.com>
Tested-by: Pingfan Liu <kernelfans@gmail.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

This is a v2 of https://patchwork.kernel.org/patch/10511241/ under
a different subject.

The patch itself hasn't changed, but I rewrote the changelog (as per
the Bjorn's request) and changed the subject accordingly.

---
 drivers/base/dd.c |    8 --------
 1 file changed, 8 deletions(-)

Index: linux-pm/drivers/base/dd.c
===================================================================
--- linux-pm.orig/drivers/base/dd.c
+++ linux-pm/drivers/base/dd.c
@@ -434,14 +434,6 @@ re_probe:
 			goto probe_failed;
 	}
 
-	/*
-	 * Ensure devices are listed in devices_kset in correct order
-	 * It's important to move Dev to the end of devices_kset before
-	 * calling .probe, because it could be recursive and parent Dev
-	 * should always go first
-	 */
-	devices_kset_move_last(dev);
-
 	if (dev->bus->probe) {
 		ret = dev->bus->probe(dev);
 		if (ret)

Re: [PATCH] driver core: Partially revert "driver core: correct device's shutdown order"

[Kishon Vijay Abraham I]

Hi,

On Tuesday 10 July 2018 05:05 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Commit 52cdbdd49853 (driver core: correct device's shutdown order)
> introduced a regression by breaking device shutdown on some systems.
> 
> Namely, the devices_kset_move_last() call in really_probe() added by
> that commit is a mistake as it may cause parents to follow children
> in the devices_kset list which then causes shutdown to fail.  For
> example, if a device has children before really_probe() is called
> for it (which is not uncommon), that call will cause it to be
> reordered after the children in the devices_kset list and the
> ordering of that list will not reflect the correct device shutdown
> order any more.
> 
> Also it causes the devices_kset list to be constantly reordered
> until all drivers have been probed which is totally pointless
> overhead in the majority of cases and it only covers an issue
> with system shutdown, while system-wide suspend/resume potentially
> has the same issue on the affected platforms (which is not covered).
> 
> For that reason, revert the really_probe() modifications made by
> commit 52cdbdd49853 which unfortunately will expose the shutdown
> issue the problematic commit attempted to fix (and which will have
> to be addressed differently and correctly in the future).
> 
> The other code changes made by commit 52cdbdd49853 are useful and
> they need not be reverted.
> 
> Fixes: 52cdbdd49853 (driver core: correct device's shutdown order)
> Link: https://lore.kernel.org/lkml/CAFgQCTt7VfqM=UyCnvNFxrSw8Z6cUtAi3HUwR4_xPAc03SgHjQ@mail.gmail.com/
> Reported-by: Pingfan Liu <kernelfans@gmail.com>
> Tested-by: Pingfan Liu <kernelfans@gmail.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---

This issue because of which 52cdbdd49853 (driver core: correct device's
shutdown order) was added is not present from 4.18, since dra7 started using
sdhci-omap.c driver which doesn't disable regulator during shutdown. (The
original issue was present in omap_hsmmc driver).

When sdhci-omap driver is modified to disable regulator during shutdown,
something like device_link_add() can be added in _regulator_get().

Since this doesn't reintroduce the problem that was solved by 52cdbdd49853,
this can be safely merged.

Reviewed-by: Kishon Vijay Abraham I <kishon@ti.com>

Thanks
Kishon

Re: [PATCH] driver core: Partially revert "driver core: correct device's shutdown order"

[Rafael J. Wysocki]

Hi,

On Tue, Jul 10, 2018 at 2:22 PM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
> Hi,
>
> On Tuesday 10 July 2018 05:05 PM, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> Commit 52cdbdd49853 (driver core: correct device's shutdown order)
>> introduced a regression by breaking device shutdown on some systems.
>>
>> Namely, the devices_kset_move_last() call in really_probe() added by
>> that commit is a mistake as it may cause parents to follow children
>> in the devices_kset list which then causes shutdown to fail.  For
>> example, if a device has children before really_probe() is called
>> for it (which is not uncommon), that call will cause it to be
>> reordered after the children in the devices_kset list and the
>> ordering of that list will not reflect the correct device shutdown
>> order any more.
>>
>> Also it causes the devices_kset list to be constantly reordered
>> until all drivers have been probed which is totally pointless
>> overhead in the majority of cases and it only covers an issue
>> with system shutdown, while system-wide suspend/resume potentially
>> has the same issue on the affected platforms (which is not covered).
>>
>> For that reason, revert the really_probe() modifications made by
>> commit 52cdbdd49853 which unfortunately will expose the shutdown
>> issue the problematic commit attempted to fix (and which will have
>> to be addressed differently and correctly in the future).
>>
>> The other code changes made by commit 52cdbdd49853 are useful and
>> they need not be reverted.
>>
>> Fixes: 52cdbdd49853 (driver core: correct device's shutdown order)
>> Link: https://lore.kernel.org/lkml/CAFgQCTt7VfqM=UyCnvNFxrSw8Z6cUtAi3HUwR4_xPAc03SgHjQ@mail.gmail.com/
>> Reported-by: Pingfan Liu <kernelfans@gmail.com>
>> Tested-by: Pingfan Liu <kernelfans@gmail.com>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> ---
>
> This issue because of which 52cdbdd49853 (driver core: correct device's
> shutdown order) was added is not present from 4.18, since dra7 started using
> sdhci-omap.c driver which doesn't disable regulator during shutdown. (The
> original issue was present in omap_hsmmc driver).
>
> When sdhci-omap driver is modified to disable regulator during shutdown,
> something like device_link_add() can be added in _regulator_get().
>
> Since this doesn't reintroduce the problem that was solved by 52cdbdd49853,
> this can be safely merged.

This is very useful information, let me add it to the patch changelog.

> Reviewed-by: Kishon Vijay Abraham I <kishon@ti.com>

Thank you!

[PATCH v2] driver core: Partially revert "driver core: correct device's shutdown order"

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Commit 52cdbdd49853 (driver core: correct device's shutdown order)
introduced a regression by breaking device shutdown on some systems.

Namely, the devices_kset_move_last() call in really_probe() added by
that commit is a mistake as it may cause parents to follow children
in the devices_kset list which then causes shutdown to fail.  For
example, if a device has children before really_probe() is called
for it (which is not uncommon), that call will cause it to be
reordered after the children in the devices_kset list and the
ordering of that list will not reflect the correct device shutdown
order any more.

Also it causes the devices_kset list to be constantly reordered
until all drivers have been probed which is totally pointless
overhead in the majority of cases and it only covered an issue
with system shutdown, while system-wide suspend/resume potentially
had the same issue on the affected platforms (which was not covered).

Moreover, the shutdown issue originally addressed by the change in
really_probe() made by commit 52cdbdd49853 is not present in 4.18-rc
any more, since dra7 started to use the sdhci-omap driver which
doesn't disable any regulators during shutdown, so the really_probe()
part of commit 52cdbdd49853 can be safely reverted.  [The original
issue was related to the omap_hsmmc driver used by dra7 previously.]

For the above reasons, revert the really_probe() modifications made
by commit 52cdbdd49853.

The other code changes made by commit 52cdbdd49853 are useful and
they need not be reverted.

Fixes: 52cdbdd49853 (driver core: correct device's shutdown order)
Link: https://lore.kernel.org/lkml/CAFgQCTt7VfqM=UyCnvNFxrSw8Z6cUtAi3HUwR4_xPAc03SgHjQ@mail.gmail.com/
Reported-by: Pingfan Liu <kernelfans@gmail.com>
Tested-by: Pingfan Liu <kernelfans@gmail.com>
Reviewed-by: Kishon Vijay Abraham I <kishon@ti.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

-> v2: Added information from Kishon on the fact that it should be safe
       to revert the really_probe() modifications added by the
       problematic commit.  Also added the Reviewed-by tag from Kishon.

---
 drivers/base/dd.c |    8 --------
 1 file changed, 8 deletions(-)

Index: linux-pm/drivers/base/dd.c
===================================================================
--- linux-pm.orig/drivers/base/dd.c
+++ linux-pm/drivers/base/dd.c
@@ -434,14 +434,6 @@ re_probe:
 			goto probe_failed;
 	}
 
-	/*
-	 * Ensure devices are listed in devices_kset in correct order
-	 * It's important to move Dev to the end of devices_kset before
-	 * calling .probe, because it could be recursive and parent Dev
-	 * should always go first
-	 */
-	devices_kset_move_last(dev);
-
 	if (dev->bus->probe) {
 		ret = dev->bus->probe(dev);
 		if (ret)

Re: [PATCH v2] driver core: Partially revert "driver core: correct device's shutdown order"

[Greg Kroah-Hartman]

On Tue, Jul 10, 2018 at 02:51:33PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Commit 52cdbdd49853 (driver core: correct device's shutdown order)
> introduced a regression by breaking device shutdown on some systems.
> 
> Namely, the devices_kset_move_last() call in really_probe() added by
> that commit is a mistake as it may cause parents to follow children
> in the devices_kset list which then causes shutdown to fail.  For
> example, if a device has children before really_probe() is called
> for it (which is not uncommon), that call will cause it to be
> reordered after the children in the devices_kset list and the
> ordering of that list will not reflect the correct device shutdown
> order any more.
> 
> Also it causes the devices_kset list to be constantly reordered
> until all drivers have been probed which is totally pointless
> overhead in the majority of cases and it only covered an issue
> with system shutdown, while system-wide suspend/resume potentially
> had the same issue on the affected platforms (which was not covered).
> 
> Moreover, the shutdown issue originally addressed by the change in
> really_probe() made by commit 52cdbdd49853 is not present in 4.18-rc
> any more, since dra7 started to use the sdhci-omap driver which
> doesn't disable any regulators during shutdown, so the really_probe()
> part of commit 52cdbdd49853 can be safely reverted.  [The original
> issue was related to the omap_hsmmc driver used by dra7 previously.]
> 
> For the above reasons, revert the really_probe() modifications made
> by commit 52cdbdd49853.
> 
> The other code changes made by commit 52cdbdd49853 are useful and
> they need not be reverted.
> 
> Fixes: 52cdbdd49853 (driver core: correct device's shutdown order)
> Link: https://lore.kernel.org/lkml/CAFgQCTt7VfqM=UyCnvNFxrSw8Z6cUtAi3HUwR4_xPAc03SgHjQ@mail.gmail.com/
> Reported-by: Pingfan Liu <kernelfans@gmail.com>
> Tested-by: Pingfan Liu <kernelfans@gmail.com>
> Reviewed-by: Kishon Vijay Abraham I <kishon@ti.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> -> v2: Added information from Kishon on the fact that it should be safe
>        to revert the really_probe() modifications added by the
>        problematic commit.  Also added the Reviewed-by tag from Kishon.

Looks good to me, want me to queue it up in my tree, or are you going to
send it on to Linus?

And shouldn't this have a stable tag as well?

thanks,

greg k-h

Re: [PATCH v2] driver core: Partially revert "driver core: correct device's shutdown order"

[Rafael J. Wysocki]

On Tue, Jul 10, 2018 at 2:59 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, Jul 10, 2018 at 02:51:33PM +0200, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> Commit 52cdbdd49853 (driver core: correct device's shutdown order)
>> introduced a regression by breaking device shutdown on some systems.
>>
>> Namely, the devices_kset_move_last() call in really_probe() added by
>> that commit is a mistake as it may cause parents to follow children
>> in the devices_kset list which then causes shutdown to fail.  For
>> example, if a device has children before really_probe() is called
>> for it (which is not uncommon), that call will cause it to be
>> reordered after the children in the devices_kset list and the
>> ordering of that list will not reflect the correct device shutdown
>> order any more.
>>
>> Also it causes the devices_kset list to be constantly reordered
>> until all drivers have been probed which is totally pointless
>> overhead in the majority of cases and it only covered an issue
>> with system shutdown, while system-wide suspend/resume potentially
>> had the same issue on the affected platforms (which was not covered).
>>
>> Moreover, the shutdown issue originally addressed by the change in
>> really_probe() made by commit 52cdbdd49853 is not present in 4.18-rc
>> any more, since dra7 started to use the sdhci-omap driver which
>> doesn't disable any regulators during shutdown, so the really_probe()
>> part of commit 52cdbdd49853 can be safely reverted.  [The original
>> issue was related to the omap_hsmmc driver used by dra7 previously.]
>>
>> For the above reasons, revert the really_probe() modifications made
>> by commit 52cdbdd49853.
>>
>> The other code changes made by commit 52cdbdd49853 are useful and
>> they need not be reverted.
>>
>> Fixes: 52cdbdd49853 (driver core: correct device's shutdown order)
>> Link: https://lore.kernel.org/lkml/CAFgQCTt7VfqM=UyCnvNFxrSw8Z6cUtAi3HUwR4_xPAc03SgHjQ@mail.gmail.com/
>> Reported-by: Pingfan Liu <kernelfans@gmail.com>
>> Tested-by: Pingfan Liu <kernelfans@gmail.com>
>> Reviewed-by: Kishon Vijay Abraham I <kishon@ti.com>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> ---
>>
>> -> v2: Added information from Kishon on the fact that it should be safe
>>        to revert the really_probe() modifications added by the
>>        problematic commit.  Also added the Reviewed-by tag from Kishon.
>
> Looks good to me, want me to queue it up in my tree, or are you going to
> send it on to Linus?

Please queue it up.

> And shouldn't this have a stable tag as well?

That is sort of a gray area, because I think it may expose the
shutdown issue on dra7 in -stable, but technically it still fixes a
regression in the driver core.  So your call I suppose. :-)

FWIW, commit 52cdbdd49853 went in during the 4.3 cycle AFAICS.

Cheers,
Rafael

Re: [PATCH v2] driver core: Partially revert "driver core: correct device's shutdown order"

[Greg Kroah-Hartman]

On Tue, Jul 10, 2018 at 05:40:21PM +0200, Rafael J. Wysocki wrote:
> On Tue, Jul 10, 2018 at 2:59 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Tue, Jul 10, 2018 at 02:51:33PM +0200, Rafael J. Wysocki wrote:
> >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>
> >> Commit 52cdbdd49853 (driver core: correct device's shutdown order)
> >> introduced a regression by breaking device shutdown on some systems.
> >>
> >> Namely, the devices_kset_move_last() call in really_probe() added by
> >> that commit is a mistake as it may cause parents to follow children
> >> in the devices_kset list which then causes shutdown to fail.  For
> >> example, if a device has children before really_probe() is called
> >> for it (which is not uncommon), that call will cause it to be
> >> reordered after the children in the devices_kset list and the
> >> ordering of that list will not reflect the correct device shutdown
> >> order any more.
> >>
> >> Also it causes the devices_kset list to be constantly reordered
> >> until all drivers have been probed which is totally pointless
> >> overhead in the majority of cases and it only covered an issue
> >> with system shutdown, while system-wide suspend/resume potentially
> >> had the same issue on the affected platforms (which was not covered).
> >>
> >> Moreover, the shutdown issue originally addressed by the change in
> >> really_probe() made by commit 52cdbdd49853 is not present in 4.18-rc
> >> any more, since dra7 started to use the sdhci-omap driver which
> >> doesn't disable any regulators during shutdown, so the really_probe()
> >> part of commit 52cdbdd49853 can be safely reverted.  [The original
> >> issue was related to the omap_hsmmc driver used by dra7 previously.]
> >>
> >> For the above reasons, revert the really_probe() modifications made
> >> by commit 52cdbdd49853.
> >>
> >> The other code changes made by commit 52cdbdd49853 are useful and
> >> they need not be reverted.
> >>
> >> Fixes: 52cdbdd49853 (driver core: correct device's shutdown order)
> >> Link: https://lore.kernel.org/lkml/CAFgQCTt7VfqM=UyCnvNFxrSw8Z6cUtAi3HUwR4_xPAc03SgHjQ@mail.gmail.com/
> >> Reported-by: Pingfan Liu <kernelfans@gmail.com>
> >> Tested-by: Pingfan Liu <kernelfans@gmail.com>
> >> Reviewed-by: Kishon Vijay Abraham I <kishon@ti.com>
> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> ---
> >>
> >> -> v2: Added information from Kishon on the fact that it should be safe
> >>        to revert the really_probe() modifications added by the
> >>        problematic commit.  Also added the Reviewed-by tag from Kishon.
> >
> > Looks good to me, want me to queue it up in my tree, or are you going to
> > send it on to Linus?
> 
> Please queue it up.
> 
> > And shouldn't this have a stable tag as well?
> 
> That is sort of a gray area, because I think it may expose the
> shutdown issue on dra7 in -stable, but technically it still fixes a
> regression in the driver core.  So your call I suppose. :-)

Being bug compatible is key :)

I'll add a stable tag.

thanks,

greg k-h

Re: [PATCH v2] driver core: Partially revert "driver core: correct device's shutdown order"

[Kishon Vijay Abraham I]

Hi,

On Tuesday 10 July 2018 09:17 PM, Greg Kroah-Hartman wrote:
> On Tue, Jul 10, 2018 at 05:40:21PM +0200, Rafael J. Wysocki wrote:
>> On Tue, Jul 10, 2018 at 2:59 PM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>>> On Tue, Jul 10, 2018 at 02:51:33PM +0200, Rafael J. Wysocki wrote:
>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>
>>>> Commit 52cdbdd49853 (driver core: correct device's shutdown order)
>>>> introduced a regression by breaking device shutdown on some systems.
>>>>
>>>> Namely, the devices_kset_move_last() call in really_probe() added by
>>>> that commit is a mistake as it may cause parents to follow children
>>>> in the devices_kset list which then causes shutdown to fail.  For
>>>> example, if a device has children before really_probe() is called
>>>> for it (which is not uncommon), that call will cause it to be
>>>> reordered after the children in the devices_kset list and the
>>>> ordering of that list will not reflect the correct device shutdown
>>>> order any more.
>>>>
>>>> Also it causes the devices_kset list to be constantly reordered
>>>> until all drivers have been probed which is totally pointless
>>>> overhead in the majority of cases and it only covered an issue
>>>> with system shutdown, while system-wide suspend/resume potentially
>>>> had the same issue on the affected platforms (which was not covered).
>>>>
>>>> Moreover, the shutdown issue originally addressed by the change in
>>>> really_probe() made by commit 52cdbdd49853 is not present in 4.18-rc
>>>> any more, since dra7 started to use the sdhci-omap driver which
>>>> doesn't disable any regulators during shutdown, so the really_probe()
>>>> part of commit 52cdbdd49853 can be safely reverted.  [The original
>>>> issue was related to the omap_hsmmc driver used by dra7 previously.]
>>>>
>>>> For the above reasons, revert the really_probe() modifications made
>>>> by commit 52cdbdd49853.
>>>>
>>>> The other code changes made by commit 52cdbdd49853 are useful and
>>>> they need not be reverted.
>>>>
>>>> Fixes: 52cdbdd49853 (driver core: correct device's shutdown order)
>>>> Link: https://lore.kernel.org/lkml/CAFgQCTt7VfqM=UyCnvNFxrSw8Z6cUtAi3HUwR4_xPAc03SgHjQ@mail.gmail.com/
>>>> Reported-by: Pingfan Liu <kernelfans@gmail.com>
>>>> Tested-by: Pingfan Liu <kernelfans@gmail.com>
>>>> Reviewed-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>> ---
>>>>
>>>> -> v2: Added information from Kishon on the fact that it should be safe
>>>>        to revert the really_probe() modifications added by the
>>>>        problematic commit.  Also added the Reviewed-by tag from Kishon.
>>>
>>> Looks good to me, want me to queue it up in my tree, or are you going to
>>> send it on to Linus?
>>
>> Please queue it up.
>>
>>> And shouldn't this have a stable tag as well?
>>
>> That is sort of a gray area, because I think it may expose the
>> shutdown issue on dra7 in -stable, but technically it still fixes a
>> regression in the driver core.  So your call I suppose. :-)
> 
> Being bug compatible is key :)
> 
> I'll add a stable tag.

sure. If I see the issue in dra7, I'll send a stable fix in omap_hsmmmc driver.

Thanks
Kishon