Re: [PATCH v1 1/4] driver core: Rely on supplier driver binding at link creation

["Lucero Palau, Alejandro" <alejandro.lucero-palau@amd.com>]

On 23/09/2026 10:58, Lucero Palau, Alejandro wrote:
>
> On 23/09/2026 09:49, Lucero Palau, Alejandro wrote:
>>
>> On 22/09/2026 22:39, Maxime Chevallier wrote:
>>> Hi,
>>>
>>> On 9/21/26 21:12, alucerop@amd.com wrote:
>>>> From: Alejandro Lucero <alucerop@amd.com>
>>>>
>>>> Differentiate between driver binding from PM dependency when a device
>>>> link is created. Rely on the device being bound to a driver for 
>>>> validating
>>>> the supplier as some device drivers could not support PM.
>>>>
>>>> Check for supplier's PM state only if consumer specifies PM_RUNTIME 
>>>> flag.
>>
>>
>> Hi Maxime,
>>
>>
>>> Hmmm this patch seems to have broken pretty much all the boards I'm 
>>> running that
>>> boot with DT. I'm getting logs such as :
>>>
>>> platform 16100000.serial: Failed to create device link (0x124) with 
>>> supplier
>>>    16000400.clock-controller
>>>
>>> Some are just hanging at "Starting kernel..." from u-boot.
>>>
>>> Then boards go silent as the uart dies, and I'm using that uart to 
>>> access the
>>> device's console :(
>>>
>>> Found with a WIP stmmac runner for netdev CI.
>>>
>>> Unfortunately I don't have much logs to share, as this just prevents 
>>> the boards
>>> from booting :( With this patch reverted, all boards boot fine
>>
>>
>> It is obvious I did not understand well the implications ...
>>
>>
>> I think the supplier device lock could be the problem behind the 
>> hanging and how the supplier initialization is checked now based on 
>> the driver bound behind the console issue ...
>>
>>
>> I have a couple of embedded boards to play with so I will try to 
>> reproduce the problem for getting more info. FWIW, all was fine with 
>> the server I tested this, no device link errors at all.
>>
>
> I have to amend my words ... the server had not exactly the same code 
> :-)  I have to copy things in and out due to security measures and 
> sometimes is hard to have all synced.
>
>
> I think the sashiko reports could be a good start to fixing this.
>

FWIW, the problem seems to be the scoped guard. I had a plain 
device_lock for using device_is_bound as required then device_unlock 
after it, but moved to scoped_guard blindly ... .


I think for my impending multipf support I only need something like this:


--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -761,7 +761,7 @@ struct device_link *device_link_add(struct device 
*consumer,
          * SYNC_STATE_ONLY link, we don't check for reverse dependencies
          * because it only affects sync_state() callbacks.
          */
-       if (!device_pm_initialized(supplier)
+       if ((!device_pm_not_required(supplier) && 
!device_pm_initialized(supplier))
             || (!(flags & DL_FLAG_SYNC_STATE_ONLY) &&
                   device_is_dependent(consumer, supplier))) {
                 link = NULL;

but when I studied the code I thought checking for "the supplier has not 
been fully registered yet" was not being achieved if the registration 
meant driver binding, what honestly confuses me because checking for 
device_pm_initialized implies looking at the dev->power.in_dpm_list what 
seems to only happen at device_add time ... so I wonder how the 
device_link_add could use a supplier device without device_add completed.


Anyway, I will go with the simpler change posted above in v2, and keep 
studying the other potential problem not directly connected to my 
multipf support.


>
> Apologies for the any inconvenient.
>
> Thanks,
>
> Alejandro.
>
>
>>
>> Thank you for testing it!
>>
>>
>>
>>>
>>> Maxime
>>>
>>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>>>> ---
>>>>   drivers/base/core.c | 27 +++++++++++++++++++--------
>>>>   1 file changed, 19 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>>>> index 4c0c373998a1..eb6d87e35d76 100644
>>>> --- a/drivers/base/core.c
>>>> +++ b/drivers/base/core.c
>>>> @@ -834,15 +834,26 @@ struct device_link *device_link_add(struct 
>>>> device *consumer,
>>>>       device_pm_lock();
>>>>         /*
>>>> -     * If the supplier has not been fully registered yet or there 
>>>> is a
>>>> -     * reverse (non-SYNC_STATE_ONLY) dependency between the 
>>>> consumer and
>>>> -     * the supplier already in the graph, return NULL. If the link 
>>>> is a
>>>> -     * SYNC_STATE_ONLY link, we don't check for reverse dependencies
>>>> -     * because it only affects sync_state() callbacks.
>>>> +     * If the supplier has not been fully registered yet with a 
>>>> driver
>>>> +     * return NULL.
>>>>        */
>>>> -    if (!device_pm_initialized(supplier)
>>>> -        || (!(flags & DL_FLAG_SYNC_STATE_ONLY) &&
>>>> -          device_is_dependent(consumer, supplier))) {
>>>> +    scoped_guard(device, supplier) {
>>>> +        if (!device_is_bound(supplier)) {
>>>> +            link = NULL;
>>>> +            goto out;
>>>> +        }
>>>> +    }
>>>> +    /*
>>>> +     * If consumer asks for PM to use the link and the supplier 
>>>> has not
>>>> +     * PM initialized, or if there is a reverse (non-SYNC_STATE_ONLY)
>>>> +     * dependency between the consumer and the supplier already in 
>>>> the
>>>> +     * graph, return NULL. If the link is a SYNC_STATE_ONLY link, we
>>>> +     * don't check for reverse dependencies because it only affects
>>>> +     * sync_state() callbacks.
>>>> +     */
>>>> +    if (((flags & DL_FLAG_PM_RUNTIME) && 
>>>> !device_pm_initialized(supplier)) ||
>>>> +        (!(flags & DL_FLAG_SYNC_STATE_ONLY) &&
>>>> +         device_is_dependent(consumer, supplier))) {
>>>>           link = NULL;
>>>>           goto out;
>>>>       }