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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <c622df86-0372-450e-b3dd-ab93cd051d6f@notapiano>
Date: Fri, 9 Aug 2024 12:13:25 -0400
From: Nícolas F. R. A. Prado <nfraprado@...labora.com>
To: Saravana Kannan <saravanak@...gle.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>, kernel@...labora.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] driver core: Don't log intentional skip of device link
 creation as error

On Mon, Jul 29, 2024 at 05:08:48PM -0700, Saravana Kannan wrote:
> On Mon, Jul 29, 2024 at 2:25 PM Nícolas F. R. A. Prado
> <nfraprado@...labora.com> 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@...labora.com> 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.

I don't see any cycle there. edp-tx points to backlight, but backlight doesn't
point back (from mt8195-cherry.dtsi): 

  &edp_tx {
  	...
  	aux-bus {
  		panel {
  			compatible = "edp-panel";
  			power-supply = <&pp3300_disp_x>;
  			backlight = <&backlight_lcd0>;

  
  backlight_lcd0: backlight-lcd0 {
  	compatible = "pwm-backlight";
  	brightness-levels = <0 1023>;
  	default-brightness-level = <576>;
  	enable-gpios = <&pio 82 GPIO_ACTIVE_HIGH>;
  	num-interpolated-steps = <1023>;
  	pwms = <&disp_pwm0 0 500000>;
  	power-supply = <&ppvar_sys>;
  };

And DL_FLAG_CYCLE is not set in the flags in the error log: 0x180. (Let me know
if there's something else that I should be looking at to detect a cycle)

Thanks,
Nícolas

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