Re: [PATCH] pinctrl: Enforce device links

[Ulf Hansson <ulf.hansson@xxxxxxxxxx>]

On Thu, 12 Dec 2019 at 14:19, Benjamin GAIGNARD
<benjamin.gaignard@xxxxxx> wrote:
>
>
> On 12/12/19 11:56 AM, Ulf Hansson wrote:
> > + Benjamin
> >
> > On Thu, 12 Dec 2019 at 11:11, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> >> Instead of opt-in device links, enforce it across the board.
> >> Everyone probably needs this anyway, lest runtime[_pm] suspend
> >> order will be haphazard.
> >>
> >> Cc: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> >> Cc: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> >> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> >> ---
> >> As there is no progress on opting in drivers for this we can just
> >> enforce it.
> >>
> >> I am a bit concerned that with every pin control state change the
> >> link reference count will just go up, but does it really matter?
> >> ---
> >>   drivers/pinctrl/core.c                | 25 ++++++++++++++-----------
> >>   drivers/pinctrl/pinctrl-stmfx.c       |  1 -
> >>   drivers/pinctrl/stm32/pinctrl-stm32.c |  1 -
> >>   include/linux/pinctrl/pinctrl.h       |  5 -----
> >>   4 files changed, 14 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> >> index 2bbd8ee93507..1d2cdeebb316 100644
> >> --- a/drivers/pinctrl/core.c
> >> +++ b/drivers/pinctrl/core.c
> >> @@ -1220,15 +1220,6 @@ struct pinctrl_state *pinctrl_lookup_state(struct pinctrl *p,
> >>   }
> >>   EXPORT_SYMBOL_GPL(pinctrl_lookup_state);
> >>
> >> -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);
> >> -}
> >> -
> >>   /**
> >>    * pinctrl_commit_state() - select/activate/program a pinctrl state to HW
> >>    * @p: the pinctrl handle for the device that requests configuration
> >> @@ -1276,8 +1267,20 @@ static int pinctrl_commit_state(struct pinctrl *p, struct pinctrl_state *state)
> >>                  }
> >>
> >>                  /* Do not link hogs (circular dependency) */
> >> -               if (p != setting->pctldev->p)
> >> -                       pinctrl_link_add(setting->pctldev, p->dev);
> >> +               if (p != setting->pctldev->p) {
> >> +                       /*
> >> +                        * Create a device link to the consumer such that
> >> +                        * it will enforce that runtime PM suspend/resume
> >> +                        * is done first on consumers before we get to
> >> +                        * the pin controller itself. As some devices get
> >> +                        * their pin control state even before probe() it is
> >> +                        * important to use DL_FLAG_AUTOREMOVE_CONSUMER.
> >> +                        */
> >> +                       device_link_add(p->dev,
> >> +                                       setting->pctldev->dev,
> >> +                                       DL_FLAG_PM_RUNTIME |
> >> +                                       DL_FLAG_AUTOREMOVE_CONSUMER);
> > I understand DL_FLAG_PM_RUNTIME is used even prior $subject patch, but
> > could you please explain some of the reasons behind that?

Can you please clarify on this part as well?

> >
> > In regards to adding a new link every time you commit/select a new
> > state, that sounds wrong to me. Why are we doing this?
>
> If a link already exists it will not add new one so it safe for me.

Right, but it seems silly to walk the list of links to find out that
it already exist and then bail out.

The point is, it adds unnecessary overhead, every time there is a new
state being selected. Don't you think?

>
> >
> > Instead, don't you want to add a link when the consumer looks up the
> > pinctrl cookie/handle (devm_pinctrl_get()), thus when create_pinctrl()
> > is called?
> Because it was the only place I add found where I get a the same
> time pointers on the consumer and the producer devices.

At least create_pinctrl() have the consumer device, but I am pretty
sure we can lookup the producer device from there, in some way. Linus?

>
> >
> > Additionally, I didn't find any place where the link is removed. I
> > looks like that should happen when the consumer drops the reference
> > for the pinctrl cookie, no?
>
> The link is automatically removed when the consumer device is deleted
>
> thanks to DL_FLAG_AUTOREMOVE_CONSUMER flag.

Ahh, I see. That should be fine, I guess.

[...]

Kind regards
Uffe