Re: [PATCH v1 09/18] driver core: Allow only unprobed consumers for SYNC_STATE_ONLY device links

[Saravana Kannan]

On Mon, Nov 16, 2020 at 7:57 AM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>
> On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
> >
> > SYNC_STATE_ONLY device links only affect the behavior of sync_state()
> > callbacks. Specifically, they prevent sync_state() only callbacks from
> > being called on a device if one or more of its consumers haven't probed.
> >
> > So, creating a SYNC_STATE_ONLY device link from an already probed
> > consumer is useless. So, don't allow creating such device links.
>
> I'm wondering why this needs to be part of the series?
>
> It looks like it could go in separately, couldn't it?

Right, I just wrote this as part of the series as I noticed this gap
in the error checking as I wrote this series. It can go in separately.

>
> >
> > Signed-off-by: Saravana Kannan <saravanak@xxxxxxxxxx>
> > ---
> >  drivers/base/core.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 1a1d9a55645c..4a0907574646 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -646,6 +646,17 @@ struct device_link *device_link_add(struct device *consumer,
> >                 goto out;
> >         }
> >
> > +       /*
> > +        * SYNC_STATE_ONLY links are useless once a consumer device has probed.
> > +        * So, only create it if the consumer hasn't probed yet.
> > +        */
> > +       if (flags & DL_FLAG_SYNC_STATE_ONLY &&
> > +           consumer->links.status != DL_DEV_NO_DRIVER &&
> > +           consumer->links.status != DL_DEV_PROBING) {
> > +               link = NULL;
> > +               goto out;
> > +       }
>
> Returning NULL at this point may be confusing if there is a link
> between these devices already.

But the request is for a SYNC_STATE_ONLY link that can't be created
when this condition is met. I see it similar to the error check above.

I think returning the existing non-SYNC_STATE_ONLY link gives the
wrong impression that the link was created successfully. Also, if I
find the existing link and return it, then I need to refcount it
(conditional on STATELESS?) and
the caller who shouldn't be trying to create this link should now need
to keep track of this and release it too. I think it's cleaner and
simpler to just return NULL.


-Saravana