Re: Question on device links

[Saravana Kannan <saravanak@xxxxxxxxxx>]

On Wed, Feb 22, 2023 at 1:34 AM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>
> Hi Saravana,
>
> please help me to look at this device link stuff in the pinctrl
> subsystem!

Hi Linus,

Thanks for reaching out!

> I started at one point to add device links for pin control and GPIO
> based on something that was done inside a pin control driver,
> resulting in these patches:
>
> Enforcing links in some select drivers:
> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/log/?h=consumer-links
>
> Enforcing it over all drivers:
> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/commit/?h=consumer-link-enforce
>
> I am under the impression that your generic dependency work
> now made these patches obsolete, so I can drop these branches
> and forget about it, is that correct?

Short answer is, yes, it'll be superseded by my work in driver core in
the cases you have in mind when asking these questions.

Now for the long answer that makes it less clear:

fw_devlink currently supports "off" and "permissive" that are less
stringent than the default "on". Not sure how long we want to support
those option and whether we care about the device links in that case.
The reason I haven't removed "off" is because 1) it was a good debug
option during the early days if fw_devlink to see if it's breaking
things. 2) early on during fw_devlink work, there were some debates
about memory usage and one could argue that there might be low memory
devices that want to save memory by avoiding the creation of device
links (not sure how valid this argument is). fw_devlink seems a lot
more stable and less "breaky" these days -- so (1) might not be a real
concern anymore.

Similarly the "permissive" option was useful in the early days where
you still wanted to make sure sync_state() functionality worked
correctly, but didn't want fw_devlink to order probing and break
things. So, if anything, we can remove this even before we remove
"off".

The current default option for fw_devlink is "on" and doesn't set the
DL_FLAG_PM_RUNTIME flag. fw_devlink=rpm does set that flag. But it
hasn't been enabled by default so I'm not sure if there are any hidden
bugs there -- unlikely when probe ordering is working, but still a
possibility. The ideal goal is to make "rpm" the default and delete
all the other options.

And then, there are cycles. When there are cyclic dependencies in DT,
fw_devlink doesn't enforce any ordering between the devices in the
cycle because it doesn't know what the real probe dependencies are
(and by extension suspend/resume). See [1] for additional context. So,
if the cycle is A -> B -> C -> A, between A, B and C it only creates
SYNC_STATE_ONLY device links (ensures correct sync_state() calls, but
no other enforcement). An idea I have floating in my head as a TODO is
for fw_devlink to learn the real dependency based on what it sees the
drivers attempt to do. For example, if driver for B tries to create a
device link for B -> C, then fw_devlink can start enforcing that going
forward even if B defers probe for some other reason. The usefulness
of this is very limited if B is already creating a B -> C device link
-- it just avoids a few hypothetical deferred probes. Also, Rob was
okay with adding DT properties to break cycles at fw_devlink level,
but I haven't thought through what that would look like. So, at least
for now, when there are cycles, there's some use to the frameworks
creating device links.

Another point is, what about old ARM32 boards that don't use DT and
use a machine specific .c file to create all the devices. fw_devlink
won't do anything for them -- or at least, I haven't bothered to try
to see if there's any way those can fake a "firmware" from fw_devlink
perspective.

Btw, all the comments above are only true since series[2] that landed
in linux-next. Before that series, fw_devlink uses to create device
links between the consumer and the parent of the pinctrl that was
registering all the GPIOs (typically a platform or i2c device). But
with [2], fw_devlink is smarter (see cover letter for details) and
will create device links to the actual gpio devices (or whatever
resource).

Oh, and even if you do continue creating these device links from the
pinctrl framework, it doesn't cause creation of separate device links
from fw_devlink. The device links API just bumps up the ref counts
appropriately. So, the real benefit is about reduced code in the
framework and not any memory savings.

So, TL;DR is that if you don't care about all of these:
- old ARM32 boards
- cases where someone might turn off fw_devlink
- the corner cases where there are cycles (we could push the
responsibility of creating device links to the drivers in that case)
then you can drop all the code you are pointing to.

Hope this helps?

> What about these two drivers we already have:
>
> $ git grep link_consumers drivers/pinctrl/
> drivers/pinctrl/core.c: if (pctldev->desc->link_consumers)
> drivers/pinctrl/pinctrl-stmfx.c:        pctl->pctl_desc.link_consumers = true;
> drivers/pinctrl/stm32/pinctrl-stm32.c:  pctl->pctl_desc.link_consumers = true;
>
> The effect will be to enforce links for each handle from a consumer
> of a pinctrl handle:
>
> static void pinctrl_link_add(struct pinctrl_dev *pctldev,
>                              struct device *consumer)
> {
>         if (pctldev->desc->link_consumers)
>                 device_link_add(consumer, pctldev->dev,
>                                 DL_FLAG_PM_RUNTIME |
>                                 DL_FLAG_AUTOREMOVE_CONSUMER);
> }
>
> Is this also in effect superceded with core dependency tracking
> so this code should simply be deleted?

My comments above apply to these too. I think if we are going to
create device links outside of fw_devlink, we should do it at
framework level as much as possible.

Thanks,
Saravana
[1] - https://lore.kernel.org/lkml/20230207014207.1678715-9-saravanak@xxxxxxxxxx/
[2] - https://lore.kernel.org/lkml/20230207014207.1678715-1-saravanak@xxxxxxxxxx/