Re: [PATCH] driver core: Don't log intentional skip of device link creation as error

[Saravana Kannan]

On Mon, Jul 29, 2024 at 2:25 PM Nícolas F. R. A. Prado
<nfraprado@xxxxxxxxxxxxx> wrote:
>
> On Tue, Jun 25, 2024 at 09:56:07AM -0400, Nícolas F. R. A. Prado wrote:
> > On Mon, Jun 24, 2024 at 04:53:30PM -0700, Saravana Kannan wrote:
> > > On Mon, Jun 24, 2024 at 8:21 AM Nícolas F. R. A. Prado
> > > <nfraprado@xxxxxxxxxxxxx> wrote:
> > > >
> > > > Commit ac66c5bbb437 ("driver core: Allow only unprobed consumers for
> > > > SYNC_STATE_ONLY device links") introduced an early return in
> > > > device_link_add() to prevent useless links from being created. However
> > > > the calling function fw_devlink_create_devlink() unconditionally prints
> > > > an error if device_link_add() didn't create a link, even in this case
> > > > where it is intentionally skipping the link creation.
> > > >
> > > > Add a check to detect if the link wasn't created intentionally and in
> > > > that case don't log an error.
> > >
> > > Your point is somewhat valid, and I might Ack this. But this really
> > > shouldn't be happening a lot. Can you give more context on how you are
> > > hitting this?
> >
> > Of course. I'm seeing this on the mt8195-cherry-tomato-r2 platform.
> >
> > The following error is printed during boot:
> >
> >   mediatek-drm-dp 1c500000.edp-tx: Failed to create device link (0x180) with backlight-lcd0
> >
> > It doesn't happen with the upstream defconfig, but with the following config
> > change it does:
> >
> >   -CONFIG_PWM_MTK_DISP=m
> >   +CONFIG_PWM_MTK_DISP=y
> >
> > That probably changes the order in which the MTK DP and the backlight drivers
> > probe, resulting in the error.
> >
> > One peculiarity that comes to mind is that the DP driver calls
> > devm_of_dp_aux_populate_bus() to run a callback once the panel has finished
> > probing. I'm not sure if this could have something to do with the error.
> >
> > Full log at https://lava.collabora.dev/scheduler/job/14573149
>
> Hi Saravana,
>
> With the given context for where this issue is happening, what do you think
> about this patch?

Ah sorry, missed your earlier email.

Couple of points:
1. It looks like the link in question is "SYNC_STATE_ONLY" because
it's part of a cycle. Correct me if I'm wrong. You might want to use
the new "post-init-providers" property to help fw_devlink break the
cycle and enforce the right dependency between the edp-tx and your
backlight. And then this error should go away and your device ordering
is enforced a bit better.

2. If we take this patch, it might be better to make a "useless device
links" helper function and use the same on in device_link_add() and in
the "if" condition that's used to decide if a device link should be
created. The con == sup is another "useless device link" check. But
I'm not really sure if we should mask this error as it help with stuff
like (1). con == sup is a bit more clear cut example of a "useless
device link" in the context of fw_devlink.

-Saravana