Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210107034904.4112029-1-pmalani@chromium.org> <X/bRstJuBYaLz4PK@kroah.com>
In-Reply-To: <X/bRstJuBYaLz4PK@kroah.com>
From:   Prashant Malani <pmalani@chromium.org>
Date:   Thu, 7 Jan 2021 01:50:53 -0800
Message-ID: <CACeCKaediXs81OUTogTWrqoZViP5rLqodO6nngeY2PLnWw=t+w@mail.gmail.com>
Subject: Re: [PATCH] usb: typec: Send uevent for num_altmodes update
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     "open list:USB NETWORKING DRIVERS" <linux-usb@vger.kernel.org>,
        Heikki Krogerus <heikki.krogerus@linux.intel.com>,
        Benson Leung <bleung@chromium.org>,
        open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Greg,

Thanks for taking a look at the patch.

On Thu, Jan 7, 2021 at 1:16 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jan 06, 2021 at 07:49:04PM -0800, Prashant Malani wrote:
> > Generate a change uevent when the "number_of_alternate_modes" sysfs file
> > for partners and plugs is updated by a port driver.
> >
> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Cc: Benson Leung <bleung@chromium.org>
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> >  drivers/usb/typec/class.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > index ebfd3113a9a8..8f77669f9cf4 100644
> > --- a/drivers/usb/typec/class.c
> > +++ b/drivers/usb/typec/class.c
> > @@ -766,6 +766,7 @@ int typec_partner_set_num_altmodes(struct typec_partner *partner, int num_altmod
> >               return ret;
> >
> >       sysfs_notify(&partner->dev.kobj, NULL, "number_of_alternate_modes");
> > +     kobject_uevent(&partner->dev.kobj, KOBJ_CHANGE);
>
> Shouldn't the sysfs_notify() handle the "something has changed" logic
> good enough for userspace, as obviously someone is polling on the thing
> (otherwise we wouldn't be calling sysfs_notify...)
>
> The kobject itself hasn't "changed", but rather an individual attribute
> has changed.  We don't want to create uevents for every individual sysfs
> attribute changing values, do we?

Fair point. I noticed other attributes in this source file use a
similar approach (sysfs_notify + kobject_uevent)
and took guidance from there in an attempt to remain consistent
(though, of course, your point still stands).

I'm guessing it is for processes that rely on udev events
(subsystem=typec) rather than polling.

>
> What is preventing a normal "monitor the sysfs file" logic from working
> here for anyone who wants to know that the alternate modes have changed?

One limitation I can think of is that this sysfs file is hidden till
it has a valid value (i.e >= 0), so a user-space process might not
be able to poll on the file till it is visible (I suppose even then
one could poll on the parent).

Kindly disregard the patch if you reckon it is unnecessary.

Best regards,

-Prashant