Re: [PATCH 1/1] driver core: Remove warning on driver unbinding

[Herve Codina <herve.codina@bootlin.com>]

Hi Saravana,

On Fri, 10 Nov 2023 17:42:26 -0800
Saravana Kannan <saravanak@google.com> wrote:

> On Fri, Nov 10, 2023 at 12:02 AM Herve Codina <herve.codina@bootlin.com> wrote:
> >
> > During driver unbinding, __device_links_no_driver() can raise the
> > following warning:
> >    --- 8< ---
> >    WARNING: CPU: 0 PID: 166 at drivers/base/core.c:1426 __device_links_no_driver+0xac/0xb4
> >    ...
> >    Call trace:
> >    __device_links_no_driver+0xac/0xb4
> >    device_links_driver_cleanup+0xa8/0xf0
> >    device_release_driver_internal+0x204/0x240
> >    device_release_driver+0x18/0x24
> >    bus_remove_device+0xcc/0x10c
> >    device_del+0x158/0x414
> >    platform_device_del.part.0+0x1c/0x88
> >    platform_device_unregister+0x24/0x40
> >    of_platform_device_destroy+0xfc/0x10c
> >    device_for_each_child_reverse+0x64/0xb4
> >    devm_of_platform_populate_release+0x4c/0x84
> >    release_nodes+0x5c/0x90
> >    devres_release_all+0x8c/0xdc
> >    device_unbind_cleanup+0x18/0x68
> >    device_release_driver_internal+0x20c/0x240
> >    device_links_unbind_consumers+0xe0/0x108
> >    device_release_driver_internal+0xf0/0x240
> >    driver_detach+0x50/0x9c
> >    bus_remove_driver+0x6c/0xbc
> >    driver_unregister+0x30/0x60
> >    ...
> >    --- 8< ---
> >
> > This warning is raised because, during device removal, we unlink a
> > consumer while its supplier links.status is DL_DEV_UNBINDING.
> > Even if the link is not a SYNC_STATE_ONLY, the warning should not
> > appear in that case.
> >
> > Filter out this warning in case of the supplier driver is unbinding.
> >
> > Fixes: 8c3e315d4296 ("driver core: Update device link status correctly for SYNC_STATE_ONLY links")  
> 
> Wrong Fixes tag. I just added the SYNC_STATE_ONLY exception. The issue
> has been there since before.

This commit adds the check
  if (link->supplier->links.status == DL_DEV_DRIVER_BOUND)
to set the link.status to DL_STATE_CONSUMER_PROBE or DL_STATE_DORMANT.

Also this commit adds the warning on !(link->flags & DL_FLAG_SYNC_STATE_ONLY)

> 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > ---
> >  drivers/base/core.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 17f2568e0a79..f4b09691998e 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -1423,7 +1423,8 @@ static void __device_links_no_driver(struct device *dev)
> >                 if (link->supplier->links.status == DL_DEV_DRIVER_BOUND) {
> >                         WRITE_ONCE(link->status, DL_STATE_AVAILABLE);
> >                 } else {
> > -                       WARN_ON(!(link->flags & DL_FLAG_SYNC_STATE_ONLY));
> > +                       WARN_ON(!(link->flags & DL_FLAG_SYNC_STATE_ONLY) &&
> > +                               link->supplier->links.status != DL_DEV_UNBINDING);  
> 
> Don't delete the warning please. Make it better so it doesn't warn
> when it shouldn't.
> 
> This combined with the other patches you sent make me think this is
> more of an issue in the device removal ordering than an actual issue
> with the warning. I'm not fully convinced the warning is incorrect
> yet.
> 

When link->supplier->links.status == DL_DEV_UNBINDING, 
what should be the link->status set ?
DL_STATE_DORMANT seems correct in that case.

Removing or not the warning in that case depends on the answer to:
Is DL_FLAG_SYNC_STATE_ONLY should be set in link->flags on all calls to
__device_links_no_driver() with link->supplier->links.status set to
DL_DEV_UNBINDING ?

I lack the knowledge to answer perfectly to this question.
Can you help me on this point ?

Best regards,
Hervé