Re: [PATCH] usb: roles: Switch on role-switch uevent reporting

[Bryan O'Donoghue]

On 09/05/2020 04:24, Wen Yang wrote:
> å 2020/5/9 äå12:29, Bryan O'Donoghue åé:
> > Right now we don't report to user-space a role switch when doing a
> > usb_role_switch_set_role() despite having registered the uevent 
> > callbacks.
> >
> > This patch switches on the notifications allowing user-space to see
> > role-switch change notifications and subsequently determine the current
> > controller data-role.
> >
> > example:
> > PFX=/devices/platform/soc/78d9000.usb/ci_hdrc.0
> >
> > root@somebox# udevadm monitor -p
> >
> > KERNEL[49.894994] change $PFX/usb_role/ci_hdrc.0-role-switch (usb_role)
> > ACTION=change
> > DEVPATH=$PFX/usb_role/ci_hdrc.0-role-switch
> > SUBSYSTEM=usb_role
> > DEVTYPE=usb_role_switch
> > USB_ROLE_SWITCH=ci_hdrc.0-role-switch
> > SEQNUM=2432
> >
> > Cc: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Cc: Chunfeng Yun <chunfeng.yun@xxxxxxxxxxxx>
> > Cc: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> > Cc: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>
> > Cc: Wen Yang <wenyang@xxxxxxxxxxxxxxxxx>
> > Cc: chenqiwu <chenqiwu@xxxxxxxxxx>
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
> > ---
> > Â drivers/usb/roles/class.c | 4 +++-
> > Â 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> > index 5b17709821df..27d92af29635 100644
> > --- a/drivers/usb/roles/class.c
> > +++ b/drivers/usb/roles/class.c
> > @@ -49,8 +49,10 @@ int usb_role_switch_set_role(struct usb_role_switch 
> > *sw, enum usb_role role)
> > ÂÂÂÂÂ mutex_lock(&sw->lock);
> > ÂÂÂÂÂ ret = sw->set(sw, role);
> > -ÂÂÂ if (!ret)
> > +ÂÂÂ if (!ret) {
> > ÂÂÂÂÂÂÂÂÂ sw->role = role;
> > +ÂÂÂÂÂÂÂ kobject_uevent(&sw->dev.kobj, KOBJ_CHANGE);
> > +ÂÂÂ }
> > ÂÂÂÂÂ mutex_unlock(&sw->lock);
>
> Hi, we may also need to deal with the return value of kobject_uevent(). 

For an KOBJ_ADD you'd return an error.

grep -r "= kobject_uevent(" *
drivers/misc/cxl/sysfs.c:	rc = kobject_uevent(&cr->kobj, KOBJ_ADD);
drivers/uio/uio.c:		ret = kobject_uevent(&map->kobj, KOBJ_ADD);
drivers/uio/uio.c:		ret = kobject_uevent(&portio->kobj, KOBJ_ADD);
drivers/visorbus/visorchipset.c:	res = 
kobject_uevent(&chipset_dev->acpi_device->dev.kobj, KOBJ_ONLINE);
drivers/visorbus/visorchipset.c:	int res = 
kobject_uevent(&chipset_dev->acpi_device->dev.kobj,

For a KOBJ_CHANGE I guess we could print an error

if (kobject_uevent(&sw->dev.kobj, KOBJ_CHANGE)
    dev_err(&sw->dev, "failed to signal USB role-switch uevent\n");

Nobody else seems that bothered about it.

grep -r "if (kobject_uevent(" *

> Should we move it under the line mutex_unlock(&sw->lock)?

I think probably not. the mutex serializes the notification. In theory 
outside the mutex you could get an out-of-order notification.

The main reason I put it where it is, is we already test ret and should 
only notify the change, when the role-switch has suceeded.

As I say, in theory anyway, the mutex enforces the signalling, whatever 
about the reception, of the role-switch change, so IMO inside the bounds 
of the mutex is the right place to put it.

---
bod