[PATCH v1] driver core: Reject pointless SYNC_STATE_ONLY device links

[PATCH v1] driver core: Reject pointless SYNC_STATE_ONLY device links

[Saravana Kannan]

SYNC_STATE_ONLY device links intentionally allow cycles because cyclic
sync_state() dependencies are valid and necessary.

However a SYNC_STATE_ONLY device link where the consumer and the supplier
are the same device is pointless because the device link would be deleted
as soon as the device probes (because it's also the consumer) and won't
affect when the sync_state() callback is called. It's a waste of CPU cycles
and memory to create this device link. So reject any attempts to create
such a device link.

Fixes: 05ef983e0d65 ("driver core: Add device link support for SYNC_STATE_ONLY flag")
Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/base/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 15986cc2fe5e..eed27933ac4d 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -716,7 +716,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_initialized(supplier) || consumer == supplier
 	    || (!(flags & DL_FLAG_SYNC_STATE_ONLY) &&
 		  device_is_dependent(consumer, supplier))) {
 		link = NULL;
-- 
2.33.0.685.g46640cef36-goog

Re: [PATCH v1] driver core: Reject pointless SYNC_STATE_ONLY device links

[Rafael J. Wysocki]

On Wed, Sep 29, 2021 at 2:51 AM Saravana Kannan <saravanak@google.com> wrote:
>
> SYNC_STATE_ONLY device links intentionally allow cycles because cyclic
> sync_state() dependencies are valid and necessary.
>
> However a SYNC_STATE_ONLY device link where the consumer and the supplier
> are the same device is pointless because the device link would be deleted
> as soon as the device probes (because it's also the consumer) and won't
> affect when the sync_state() callback is called. It's a waste of CPU cycles
> and memory to create this device link. So reject any attempts to create
> such a device link.
>
> Fixes: 05ef983e0d65 ("driver core: Add device link support for SYNC_STATE_ONLY flag")
> Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/base/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 15986cc2fe5e..eed27933ac4d 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -716,7 +716,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_initialized(supplier) || consumer == supplier

Why do we need to get all the way down to here in order to return NULL
in the consumer == supplier case?

IMO this should be checked at the beginning along with !consumer and !supplier.

>             || (!(flags & DL_FLAG_SYNC_STATE_ONLY) &&
>                   device_is_dependent(consumer, supplier))) {
>                 link = NULL;
> --
> 2.33.0.685.g46640cef36-goog
>

Re: [PATCH v1] driver core: Reject pointless SYNC_STATE_ONLY device links

[Saravana Kannan]

On Wed, Sep 29, 2021 at 5:12 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Sep 29, 2021 at 2:51 AM Saravana Kannan <saravanak@google.com> wrote:
> >
> > SYNC_STATE_ONLY device links intentionally allow cycles because cyclic
> > sync_state() dependencies are valid and necessary.
> >
> > However a SYNC_STATE_ONLY device link where the consumer and the supplier
> > are the same device is pointless because the device link would be deleted
> > as soon as the device probes (because it's also the consumer) and won't
> > affect when the sync_state() callback is called. It's a waste of CPU cycles
> > and memory to create this device link. So reject any attempts to create
> > such a device link.
> >
> > Fixes: 05ef983e0d65 ("driver core: Add device link support for SYNC_STATE_ONLY flag")
> > Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  drivers/base/core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 15986cc2fe5e..eed27933ac4d 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -716,7 +716,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_initialized(supplier) || consumer == supplier
>
> Why do we need to get all the way down to here in order to return NULL
> in the consumer == supplier case?

This is where it used to be checked before the "Fixes commit" so I
added it back here. But sure, I can move it up there.

-Saravana

>
> IMO this should be checked at the beginning along with !consumer and !supplier.
>
> >             || (!(flags & DL_FLAG_SYNC_STATE_ONLY) &&
> >                   device_is_dependent(consumer, supplier))) {
> >                 link = NULL;
> > --
> > 2.33.0.685.g46640cef36-goog
> >