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

On Sun, Jul 8, 2018 at 4:25 PM Rafael J. Wysocki [off-list ref] wrote:

On Sat, Jul 7, 2018 at 6:24 AM, Pingfan Liu [off-list ref] wrote:

quoted

On Fri, Jul 6, 2018 at 9:55 PM Pingfan Liu [off-list ref] wrote:

quoted

On Fri, Jul 6, 2018 at 4:47 PM Rafael J. Wysocki [off-list ref] wrote:

quoted

On Fri, Jul 6, 2018 at 10:36 AM, Lukas Wunner [off-list ref] wrote:

quoted

[cc += Kishon Vijay Abraham]

On Thu, Jul 05, 2018 at 11:18:28AM +0200, Rafael J. Wysocki wrote:

quoted

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".

quoted

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.

quoted

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