Message-ID: <4cevcdwbghn7nisyoetcgrentfu7zvxpzu4kl67exushjexbrv@6ialgl77ivlb>
Date: Thu, 24 Oct 2024 19:07:43 +0200
From: Thierry Reding <thierry.reding@...il.com>
To: Nícolas F. R. A. Prado <nfraprado@...labora.com>
Cc: Jon Hunter <jonathanh@...dia.com>, 
	Saravana Kannan <saravanak@...gle.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	"Rafael J . Wysocki" <rafael@...nel.org>, linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org
Subject: Re: [PATCH] driver core: Don't try to create links if they are not
 needed

On Wed, Oct 23, 2024 at 09:58:35AM -0400, Nícolas F. R. A. Prado wrote:
> On Wed, Oct 23, 2024 at 02:24:52PM +0100, Jon Hunter wrote:
> > 
> > On 23/10/2024 02:00, Saravana Kannan wrote:
> > > On Thu, Oct 3, 2024 at 7:59 AM Nícolas F. R. A. Prado
> > > <nfraprado@...labora.com> wrote:
> > > > 
> > > > On Thu, Oct 03, 2024 at 11:25:22AM +0100, Jon Hunter wrote:
> > > > > 
> > > > > On 02/10/2024 21:38, Saravana Kannan wrote:
> > > > > > On Wed, Oct 2, 2024 at 11:30 AM Jon Hunter <jonathanh@...dia.com> wrote:
> > > > > > > 
> > > > > > > Hi Greg,
> > > > > > > 
> > > > > > > On 16/09/2024 18:49, Greg Kroah-Hartman wrote:
> > > > > > > > On Mon, Sep 16, 2024 at 03:50:34PM +0100, Jon Hunter wrote:
> > > > > > > > > 
> > > > > > > > > On 11/09/2024 15:32, Greg Kroah-Hartman wrote:
> > > > > > > > > > On Tue, Sep 10, 2024 at 02:00:19PM +0100, Jon Hunter wrote:
> > > > > > > > > > > The following error messages are observed on boot with the Tegra234
> > > > > > > > > > > Jetson AGX Orin board ...
> > > > > > > > > > > 
> > > > > > > > > > >      tegra-xusb-padctl 3520000.padctl: Failed to create device link (0x180)
> > > > > > > > > > >        with 1-0008
> > > > > > > > > > >      tegra-xusb-padctl 3520000.padctl: Failed to create device link (0x180)
> > > > > > > > > > >        with 1-0008
> > > > > > > > > > >      tegra-xusb-padctl 3520000.padctl: Failed to create device link (0x180)
> > > > > > > > > > >        with 1-0008
> > > > > > > > > > > 
> > > > > > > > > > > In the above case, device_link_add() intentionally returns NULL because
> > > > > > > > > > > these are SYNC_STATE_ONLY links and the device is already probed.
> > > > > > > > > > > Therefore, the above messages are not actually errors. Fix this by
> > > > > > > > > > > replicating the test from device_link_add() in the function
> > > > > > > > > > > fw_devlink_create_devlink() and don't call device_link_add() if there
> > > > > > > > > > > are no links to create.
> > > > > > > > > > > 
> > > > > > > > > > > Signed-off-by: Jon Hunter <jonathanh@...dia.com>
> > > > > > > > > > 
> > > > > > > > > > What commit id does this fix?
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Hard to say exactly. The above error message was first added with commit
> > > > > > > > > 3fb16866b51d ("driver core: fw_devlink: Make cycle detection more robust")
> > > > > > > > > but at this time we did not have the support in place for Tegra234 USB. I am
> > > > > > > > > guessing we first started seeing this when I enabled support for the type-c
> > > > > > > > > controller in commit 16744314ee57 ("arm64: tegra: Populate USB Type-C
> > > > > > > > > Controller for Jetson AGX Orin"). I can confirm if that is helpful?
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > That helps, I'll look at this after -rc1 is out, thanks!
> > > > > > > 
> > > > > > > 
> > > > > > > Let me know if there is anything else I can answer on this one.
> > > > > > 
> > > > > > Hi Jon,
> > > > > > 
> > > > > > See this.
> > > > > > https://lore.kernel.org/all/c622df86-0372-450e-b3dd-ab93cd051d6f@notapiano/
> > > > > > 
> > > > > > Ignore my point 1. My point 2 still stands. I got busy and forgot to
> > > > > > reply to Nícolas.
> > > > > > 
> > > > > > I'm fine with either one of your patches as long as we define a
> > > > > > "useless link" function and use it in all the places.
> > > > > 
> > > > > 
> > > > > Thanks! Yes I am also fine with Nicolas' fix too. I quite like the dev_dbg()
> > > > > in Nicolas' version. I was wondering if we should define a function for this
> > > > > check too.
> > > > > 
> > > > > Nicolas do you want to update your patch with a 'useless link' function? I
> > > > > will be happy to test on my side. Looks like you identified the exact patch
> > > > > that introduced this and have the appropriate fixes tag too.
> > > > 
> > > > Hi Jon,
> > > > 
> > > > I just sent a reply to that thread yesterday going a bit further down the rabbit
> > > > hole to try and answer if there's an underlying issue there that the log
> > > > messages are just exposing, but I still don't understand all the devlink details
> > > > involved so was hoping for some feedback from Saravana.
> > > > 
> > > > But if there's no feedback I can surely update the patch with the commonized
> > > > function to fix the immediate problem. I'll wait a couple days to give Saravana
> > > > (and others) some time to respond.
> > > 
> > > Finally managed to squeeze in some time for Nicolas's issue. It was a
> > > real issue. Replied to the original thread from Nicolas.
> > > 
> > > Jon, can you do an analysis similar to Nicolas? What consumer node did
> > > fw_devlink try to create a link for and what consumer device did it
> > > end up creating a device link with instead?
> > 
> > 
> > I am not 100% sure what you want, but enabling the same debug messages
> > as Nicolas I am seeing ...
> > 
> > [    9.867969] i2c 3-0008: bus: 'i2c': __driver_probe_device: matched device with driver ucsi_ccg
> > [    9.868004] i2c 3-0008: bus: 'i2c': really_probe: probing driver ucsi_ccg with device
> > [    9.898834] ucsi_ccg 3-0008: driver: 'ucsi_ccg': driver_bound: bound to device
> > [    9.898845] /bus@...adctl@...0000/ports/usb3-0 Linked as a fwnode consumer to /bus@...2c@...0000/typec@8
> > [    9.898858] /bus@...adctl@...0000/ports/usb3-0 Dropping the fwnode link to /bus@...2c@...0000/typec@...onnector@0
> > [    9.898871] /bus@...adctl@...0000/ports/usb2-1 Linked as a fwnode consumer to /bus@...2c@...0000/typec@8
> > [    9.898881] /bus@...adctl@...0000/ports/usb2-1 Dropping the fwnode link to /bus@...2c@...0000/typec@...onnector@0
> > [    9.898893] /bus@...adctl@...0000/ports/usb3-1 Linked as a fwnode consumer to /bus@...2c@...0000/typec@8
> > [    9.898899] /bus@...adctl@...0000/ports/usb3-1 Dropping the fwnode link to /bus@...2c@...0000/typec@...onnector@1
> > [    9.898905] /bus@...adctl@...0000/ports/usb2-0 Linked as a fwnode consumer to /bus@...2c@...0000/typec@8
> > [    9.898910] /bus@...adctl@...0000/ports/usb2-0 Dropping the fwnode link to /bus@...2c@...0000/typec@...onnector@1
> > [    9.898923] device: 'i2c:3-0008--usb_role:usb2-0-role-switch': device_add
> > [    9.898961] usb_role usb2-0-role-switch: Linked as a sync state only consumer to 3-0008
> > [    9.898963] /bus@...adctl@...0000/ports/usb2-0 Dropping the fwnode link to /bus@...2c@...0000/typec@8
> > [    9.898970] tegra-xusb-padctl 3520000.padctl: Failed to create device link (0x180) with 3-0008
> > [    9.907920] tegra-xusb-padctl 3520000.padctl: Failed to create device link (0x180) with 3-0008
> > [    9.916841] tegra-xusb-padctl 3520000.padctl: Failed to create device link (0x180) with 3-0008
> > [    9.925798] ucsi_ccg 3-0008: Dropping the link to usb2-0-role-switch
> > [    9.925808] device: 'usb_role:usb2-0-role-switch--i2c:3-0008': device_unregister
> > [    9.925903] ucsi_ccg 3-0008: Dropping the link to 3520000.padctl
> > [    9.925908] device: 'platform:3520000.padctl--i2c:3-0008': device_unregister
> > [    9.926001] ucsi_ccg 3-0008: bus: 'i2c': really_probe: bound device to driver ucsi_ccg
> > [    9.963266] device: 'platform:3520000.padctl--typec:port0': device_add
> > [    9.963296] typec port0: Linked as a consumer to 3520000.padctl
> > [    9.963298] /bus@...2c@...0000/typec@...onnector@0 Dropping the fwnode link to /bus@...adctl@...0000
> > [   10.015196] device: 'platform:3520000.padctl--typec:port1': device_add
> > [   10.015291] typec port1: Linked as a sync state only consumer to 3520000.padctl
> > [   10.015302] /bus@...2c@...0000/typec@...onnector@1 Dropping the fwnode link to /bus@...adctl@35
> > 
> > Let me know if there is anything else you need.
> 
> I'm guessing a similar change to what Saravana suggested for the
> of_dp_aux_populate_bus() helper is needed here:
> 
> diff --git a/drivers/phy/tegra/xusb.c b/drivers/phy/tegra/xusb.c
> index cfdb54b6070a..0a2096085971 100644
> --- a/drivers/phy/tegra/xusb.c
> +++ b/drivers/phy/tegra/xusb.c
> @@ -543,7 +543,7 @@ static int tegra_xusb_port_init(struct tegra_xusb_port *port,
> 
>         device_initialize(&port->dev);
>         port->dev.type = &tegra_xusb_port_type;
> -       port->dev.of_node = of_node_get(np);
> +       device_set_node(&port->dev, of_fwnode_handle(of_node_get(np)));

Do we need the of_node_get() in there? The intention was to grab a
reference to it before storing it in the port->dev.of_node, so that if
we put it the reference stays balanced. Looking at it again, we never
seem to do that cleanup, though probably we should. It's unlikely that
these OF nodes will ever go away completely, so this is all a bit moot.

Anyway, I guess we can leave this in. Worst case it's going to "leak"
an OF node that's not ever going to go away anyway.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists