* [PATCH 1/1] driver core: Remove warning on driver unbinding
@ 2023-11-10 8:02 Herve Codina
2023-11-11 1:42 ` Saravana Kannan
0 siblings, 1 reply; 3+ messages in thread
From: Herve Codina @ 2023-11-10 8:02 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rafael J. Wysocki, Saravana Kannan
Cc: linux-kernel, Allan Nielsen, Horatiu Vultur, Steen Hegelund,
Thomas Petazzoni, Herve Codina, stable
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(-)
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);
WRITE_ONCE(link->status, DL_STATE_DORMANT);
}
}
--
2.41.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH 1/1] driver core: Remove warning on driver unbinding
2023-11-10 8:02 [PATCH 1/1] driver core: Remove warning on driver unbinding Herve Codina
@ 2023-11-11 1:42 ` Saravana Kannan
2023-11-13 17:57 ` Herve Codina
0 siblings, 1 reply; 3+ messages in thread
From: Saravana Kannan @ 2023-11-11 1:42 UTC (permalink / raw)
To: Herve Codina
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel,
Allan Nielsen, Horatiu Vultur, Steen Hegelund, Thomas Petazzoni,
stable
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH 1/1] driver core: Remove warning on driver unbinding
2023-11-11 1:42 ` Saravana Kannan
@ 2023-11-13 17:57 ` Herve Codina
0 siblings, 0 replies; 3+ messages in thread
From: Herve Codina @ 2023-11-13 17:57 UTC (permalink / raw)
To: Saravana Kannan
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel,
Allan Nielsen, Horatiu Vultur, Steen Hegelund, Thomas Petazzoni,
stable
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é
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-11-13 17:57 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-10 8:02 [PATCH 1/1] driver core: Remove warning on driver unbinding Herve Codina
2023-11-11 1:42 ` Saravana Kannan
2023-11-13 17:57 ` Herve Codina
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox