[PATCH] driver core: Don't try to create links if they are not needed

[PATCH] driver core: Don't try to create links if they are not needed

[Jon Hunter]

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@nvidia.com>
---
I am not sure if there is a better way to fix, but given that the
function device_link_add() is exported, I figured we could not just
move the test. Anyway, if there is a better way to fix this, let me
know.

 drivers/base/core.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index b69b82da8837..5d6575e63e8b 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2181,6 +2181,15 @@ static int fw_devlink_create_devlink(struct device *con,
 			goto out;
 		}
 
+		/*
+		 * SYNC_STATE_ONLY links are useless once a consumer device has probed.
+		 * So, only create it if the consumer hasn't probed yet.
+		 */
+		if (flags & DL_FLAG_SYNC_STATE_ONLY &&
+		    con->links.status != DL_DEV_NO_DRIVER &&
+		    con->links.status != DL_DEV_PROBING)
+			goto out;
+
 		if (con != sup_dev && !device_link_add(con, sup_dev, flags)) {
 			dev_err(con, "Failed to create device link (0x%x) with %s\n",
 				flags, dev_name(sup_dev));
-- 
2.34.1

Re: [PATCH] driver core: Don't try to create links if they are not needed

[Greg Kroah-Hartman]

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@nvidia.com>

What commit id does this fix?

thanks,

greg k-h

Re: [PATCH] driver core: Don't try to create links if they are not needed

[Jon Hunter]

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@nvidia.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?

Jon

-- 
nvpublic

Re: [PATCH] driver core: Don't try to create links if they are not needed

[Greg Kroah-Hartman]

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@nvidia.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!

greg k-h

Re: [PATCH] driver core: Don't try to create links if they are not needed

[Jon Hunter]

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@nvidia.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.

Thanks
Jon

-- 
nvpublic

Re: [PATCH] driver core: Don't try to create links if they are not needed

[Saravana Kannan]

On Wed, Oct 2, 2024 at 11:30 AM Jon Hunter <jonathanh@nvidia.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@nvidia.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,
Saravana

Re: [PATCH] driver core: Don't try to create links if they are not needed

[Jon Hunter]

On 02/10/2024 21:38, Saravana Kannan wrote:
> On Wed, Oct 2, 2024 at 11:30 AM Jon Hunter <jonathanh@nvidia.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@nvidia.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.

Thanks
Jon

-- 
nvpublic

Re: [PATCH] driver core: Don't try to create links if they are not needed

[Nícolas F. R. A. Prado]

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@nvidia.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@nvidia.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.

Thanks,
Nícolas

Re: [PATCH] driver core: Don't try to create links if they are not needed

[Saravana Kannan]

On Thu, Oct 3, 2024 at 7:59 AM Nícolas F. R. A. Prado
<nfraprado@collabora.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@nvidia.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@nvidia.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?

-Saravana

Re: [PATCH] driver core: Don't try to create links if they are not needed

[Jon Hunter]

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@collabora.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@nvidia.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@nvidia.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@0/padctl@3520000/ports/usb3-0 Linked as a fwnode consumer to /bus@0/i2c@c240000/typec@8
[    9.898858] /bus@0/padctl@3520000/ports/usb3-0 Dropping the fwnode link to /bus@0/i2c@c240000/typec@8/connector@0
[    9.898871] /bus@0/padctl@3520000/ports/usb2-1 Linked as a fwnode consumer to /bus@0/i2c@c240000/typec@8
[    9.898881] /bus@0/padctl@3520000/ports/usb2-1 Dropping the fwnode link to /bus@0/i2c@c240000/typec@8/connector@0
[    9.898893] /bus@0/padctl@3520000/ports/usb3-1 Linked as a fwnode consumer to /bus@0/i2c@c240000/typec@8
[    9.898899] /bus@0/padctl@3520000/ports/usb3-1 Dropping the fwnode link to /bus@0/i2c@c240000/typec@8/connector@1
[    9.898905] /bus@0/padctl@3520000/ports/usb2-0 Linked as a fwnode consumer to /bus@0/i2c@c240000/typec@8
[    9.898910] /bus@0/padctl@3520000/ports/usb2-0 Dropping the fwnode link to /bus@0/i2c@c240000/typec@8/connector@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@0/padctl@3520000/ports/usb2-0 Dropping the fwnode link to /bus@0/i2c@c240000/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@0/i2c@c240000/typec@8/connector@0 Dropping the fwnode link to /bus@0/padctl@3520000
[   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@0/i2c@c240000/typec@8/connector@1 Dropping the fwnode link to /bus@0/padctl@35

Let me know if there is anything else you need.

Jon
-- 
nvpublic

Re: [PATCH] driver core: Don't try to create links if they are not needed

[Nícolas F. R. A. Prado]

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@collabora.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@nvidia.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@nvidia.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@0/padctl@3520000/ports/usb3-0 Linked as a fwnode consumer to /bus@0/i2c@c240000/typec@8
> [    9.898858] /bus@0/padctl@3520000/ports/usb3-0 Dropping the fwnode link to /bus@0/i2c@c240000/typec@8/connector@0
> [    9.898871] /bus@0/padctl@3520000/ports/usb2-1 Linked as a fwnode consumer to /bus@0/i2c@c240000/typec@8
> [    9.898881] /bus@0/padctl@3520000/ports/usb2-1 Dropping the fwnode link to /bus@0/i2c@c240000/typec@8/connector@0
> [    9.898893] /bus@0/padctl@3520000/ports/usb3-1 Linked as a fwnode consumer to /bus@0/i2c@c240000/typec@8
> [    9.898899] /bus@0/padctl@3520000/ports/usb3-1 Dropping the fwnode link to /bus@0/i2c@c240000/typec@8/connector@1
> [    9.898905] /bus@0/padctl@3520000/ports/usb2-0 Linked as a fwnode consumer to /bus@0/i2c@c240000/typec@8
> [    9.898910] /bus@0/padctl@3520000/ports/usb2-0 Dropping the fwnode link to /bus@0/i2c@c240000/typec@8/connector@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@0/padctl@3520000/ports/usb2-0 Dropping the fwnode link to /bus@0/i2c@c240000/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@0/i2c@c240000/typec@8/connector@0 Dropping the fwnode link to /bus@0/padctl@3520000
> [   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@0/i2c@c240000/typec@8/connector@1 Dropping the fwnode link to /bus@0/padctl@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)));
        port->dev.parent = padctl->dev;

        err = dev_set_name(&port->dev, "%s-%u", name, index);


As a side note, I wonder if it would be possible to detect these mistakes... But
I'm guessing there are legitimate situations where there's no fwnode.

Thanks,
Nícolas

Re: [PATCH] driver core: Don't try to create links if they are not needed

[Jon Hunter]

On 23/10/2024 14:58, Nícolas F. R. A. Prado wrote:

...

> 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)));
>          port->dev.parent = padctl->dev;
> 
>          err = dev_set_name(&port->dev, "%s-%u", name, index);
> 
> 
> As a side note, I wonder if it would be possible to detect these mistakes... But
> I'm guessing there are legitimate situations where there's no fwnode.


Yes! That does indeed fix the issue.

Saravana, let me know if you can send a patch? I would but I can't say I 
understand that actual issue.

Jon

-- 
nvpublic

Re: [PATCH] driver core: Don't try to create links if they are not needed

[Saravana Kannan]

On Wed, Oct 23, 2024 at 7:09 AM Jon Hunter <jonathanh@nvidia.com> wrote:
>
>
> On 23/10/2024 14:58, Nícolas F. R. A. Prado wrote:
>
> ...
>
> > 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)));
> >          port->dev.parent = padctl->dev;
> >
> >          err = dev_set_name(&port->dev, "%s-%u", name, index);
> >
> >
> > As a side note, I wonder if it would be possible to detect these mistakes... But
> > I'm guessing there are legitimate situations where there's no fwnode.
>
>
> Yes! That does indeed fix the issue.
>
> Saravana, let me know if you can send a patch? I would but I can't say I
> understand that actual issue.

Heh... didn't know you were hitting the exact same issue. I'll send
out a patch. Okay to add your tested by too?

-Saravana

Re: [PATCH] driver core: Don't try to create links if they are not needed

[Saravana Kannan]

Btw, Nicholas and Jon, can you give a Nack for the patches that are
removing the error logging?

-Saravana

On Wed, Oct 23, 2024 at 11:34 AM Saravana Kannan <saravanak@google.com> wrote:
>
> On Wed, Oct 23, 2024 at 7:09 AM Jon Hunter <jonathanh@nvidia.com> wrote:
> >
> >
> > On 23/10/2024 14:58, Nícolas F. R. A. Prado wrote:
> >
> > ...
> >
> > > 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)));
> > >          port->dev.parent = padctl->dev;
> > >
> > >          err = dev_set_name(&port->dev, "%s-%u", name, index);
> > >
> > >
> > > As a side note, I wonder if it would be possible to detect these mistakes... But
> > > I'm guessing there are legitimate situations where there's no fwnode.
> >
> >
> > Yes! That does indeed fix the issue.
> >
> > Saravana, let me know if you can send a patch? I would but I can't say I
> > understand that actual issue.
>
> Heh... didn't know you were hitting the exact same issue. I'll send
> out a patch. Okay to add your tested by too?
>
> -Saravana

Re: [PATCH] driver core: Don't try to create links if they are not needed

[Jon Hunter]

On 23/10/2024 19:34, Saravana Kannan wrote:
> On Wed, Oct 23, 2024 at 7:09 AM Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>>
>> On 23/10/2024 14:58, Nícolas F. R. A. Prado wrote:
>>
>> ...
>>
>>> 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)));
>>>           port->dev.parent = padctl->dev;
>>>
>>>           err = dev_set_name(&port->dev, "%s-%u", name, index);
>>>
>>>
>>> As a side note, I wonder if it would be possible to detect these mistakes... But
>>> I'm guessing there are legitimate situations where there's no fwnode.
>>
>>
>> Yes! That does indeed fix the issue.
>>
>> Saravana, let me know if you can send a patch? I would but I can't say I
>> understand that actual issue.
> 
> Heh... didn't know you were hitting the exact same issue. I'll send
> out a patch. Okay to add your tested by too?

Yes please do!

Thanks
Jon

-- 
nvpublic

Re: [PATCH] driver core: Don't try to create links if they are not needed

[Jon Hunter]

On 10/09/2024 14:00, 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@nvidia.com>
> ---
> I am not sure if there is a better way to fix, but given that the
> function device_link_add() is exported, I figured we could not just
> move the test. Anyway, if there is a better way to fix this, let me
> know.
> 
>   drivers/base/core.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index b69b82da8837..5d6575e63e8b 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2181,6 +2181,15 @@ static int fw_devlink_create_devlink(struct device *con,
>   			goto out;
>   		}
>   
> +		/*
> +		 * SYNC_STATE_ONLY links are useless once a consumer device has probed.
> +		 * So, only create it if the consumer hasn't probed yet.
> +		 */
> +		if (flags & DL_FLAG_SYNC_STATE_ONLY &&
> +		    con->links.status != DL_DEV_NO_DRIVER &&
> +		    con->links.status != DL_DEV_PROBING)
> +			goto out;
> +
>   		if (con != sup_dev && !device_link_add(con, sup_dev, flags)) {
>   			dev_err(con, "Failed to create device link (0x%x) with %s\n",
>   				flags, dev_name(sup_dev));


NACK. Turns out that there is an actual issue here and the proper fix is 
described here ...

https://lore.kernel.org/linux-tegra/CAGETcx-cgst26+2bRScx7mmJtOmrHzEfg0eVxzqHfQDTewy_yA@mail.gmail.com/T/#m5b77f8372671e5c4daec898f767370c302511949

Jon

-- 
nvpublic

Re: [PATCH] driver core: Don't try to create links if they are not needed

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 8927 bytes --]

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@collabora.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@nvidia.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@nvidia.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@0/padctl@3520000/ports/usb3-0 Linked as a fwnode consumer to /bus@0/i2c@c240000/typec@8
> > [    9.898858] /bus@0/padctl@3520000/ports/usb3-0 Dropping the fwnode link to /bus@0/i2c@c240000/typec@8/connector@0
> > [    9.898871] /bus@0/padctl@3520000/ports/usb2-1 Linked as a fwnode consumer to /bus@0/i2c@c240000/typec@8
> > [    9.898881] /bus@0/padctl@3520000/ports/usb2-1 Dropping the fwnode link to /bus@0/i2c@c240000/typec@8/connector@0
> > [    9.898893] /bus@0/padctl@3520000/ports/usb3-1 Linked as a fwnode consumer to /bus@0/i2c@c240000/typec@8
> > [    9.898899] /bus@0/padctl@3520000/ports/usb3-1 Dropping the fwnode link to /bus@0/i2c@c240000/typec@8/connector@1
> > [    9.898905] /bus@0/padctl@3520000/ports/usb2-0 Linked as a fwnode consumer to /bus@0/i2c@c240000/typec@8
> > [    9.898910] /bus@0/padctl@3520000/ports/usb2-0 Dropping the fwnode link to /bus@0/i2c@c240000/typec@8/connector@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@0/padctl@3520000/ports/usb2-0 Dropping the fwnode link to /bus@0/i2c@c240000/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@0/i2c@c240000/typec@8/connector@0 Dropping the fwnode link to /bus@0/padctl@3520000
> > [   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@0/i2c@c240000/typec@8/connector@1 Dropping the fwnode link to /bus@0/padctl@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] driver core: Don't try to create links if they are not needed

[Saravana Kannan]

On Thu, Oct 24, 2024 at 10:07 AM Thierry Reding
<thierry.reding@gmail.com> wrote:
>
> 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@collabora.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@nvidia.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@nvidia.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@0/padctl@3520000/ports/usb3-0 Linked as a fwnode consumer to /bus@0/i2c@c240000/typec@8
> > > [    9.898858] /bus@0/padctl@3520000/ports/usb3-0 Dropping the fwnode link to /bus@0/i2c@c240000/typec@8/connector@0
> > > [    9.898871] /bus@0/padctl@3520000/ports/usb2-1 Linked as a fwnode consumer to /bus@0/i2c@c240000/typec@8
> > > [    9.898881] /bus@0/padctl@3520000/ports/usb2-1 Dropping the fwnode link to /bus@0/i2c@c240000/typec@8/connector@0
> > > [    9.898893] /bus@0/padctl@3520000/ports/usb3-1 Linked as a fwnode consumer to /bus@0/i2c@c240000/typec@8
> > > [    9.898899] /bus@0/padctl@3520000/ports/usb3-1 Dropping the fwnode link to /bus@0/i2c@c240000/typec@8/connector@1
> > > [    9.898905] /bus@0/padctl@3520000/ports/usb2-0 Linked as a fwnode consumer to /bus@0/i2c@c240000/typec@8
> > > [    9.898910] /bus@0/padctl@3520000/ports/usb2-0 Dropping the fwnode link to /bus@0/i2c@c240000/typec@8/connector@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@0/padctl@3520000/ports/usb2-0 Dropping the fwnode link to /bus@0/i2c@c240000/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@0/i2c@c240000/typec@8/connector@0 Dropping the fwnode link to /bus@0/padctl@3520000
> > > [   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@0/i2c@c240000/typec@8/connector@1 Dropping the fwnode link to /bus@0/padctl@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.

The helper function sets dev.of_node too. So this is not changing any
existing references. Just adding a new one. And I didn't want to
change any reference counting in this patch.

-Saravana