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

  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")
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(-)
  

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.

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

-Saravana

>                         WRITE_ONCE(link->status, DL_STATE_DORMANT);
>                 }
>         }
> --
> 2.41.0
>
  

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é