[PATCH] usb: udc: add gadget state kobject uevent

[PATCH] usb: udc: add gadget state kobject uevent

[Rong Wang]

    usb: udc: add gadget state kobject uevent

    Add USB_UDC_STATE environment variable in udc uevent
    callback and trigger kobject_uevent in usb_gadget_set_state
    to inform the user-space the state of the gadget.

    Signed-off-by: Rong Wang <Rong.Wang@csr.com>

diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
index ffd8fa5..05715d1 100644
--- a/drivers/usb/gadget/udc-core.c
+++ b/drivers/usb/gadget/udc-core.c
@@ -101,11 +101,32 @@ EXPORT_SYMBOL_GPL(usb_gadget_unmap_request);

 /* ------------------------------------------------------------------------- */

+/**
+ * usb_gadget_set_state - set usb gadget state
+ * @gadget: gadget device
+ * @state: state defined in USB specification ch9
+ * Context: !in_interrupt()
+ */
 void usb_gadget_set_state(struct usb_gadget *gadget,
                enum usb_device_state state)
 {
+       struct usb_udc          *udc = NULL;
+
        gadget->state = state;
        sysfs_notify(&gadget->dev.kobj, NULL, "status");
+
+       mutex_lock(&udc_lock);
+       list_for_each_entry(udc, &udc_list, list)
+               if (udc->gadget == gadget)
+                       goto found;
+
+       dev_err(gadget->dev.parent, "gadget not registered.\n");
+       mutex_unlock(&udc_lock);
+       return;
+
+found:
+       mutex_unlock(&udc_lock);
+       kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
 }
 EXPORT_SYMBOL_GPL(usb_gadget_set_state);

@@ -538,6 +559,15 @@ static int usb_udc_uevent(struct device *dev,
struct kobj_uevent_env *env)
                }
        }

+       if (udc->gadget) {
+               ret = add_uevent_var(env, "USB_UDC_STATE=%s",
+                               usb_state_string(udc->gadget->state));
+               if (ret) {
+                       dev_err(dev, "failed to add uevent USB_UDC_STATE\n");
+                       return ret;
+               }
+       }
+
        return 0;
 }

Re: [PATCH] usb: udc: add gadget state kobject uevent

[Greg KH]

On Mon, Jul 15, 2013 at 11:57:24AM +0800, Rong Wang wrote:
>     usb: udc: add gadget state kobject uevent
> 
>     Add USB_UDC_STATE environment variable in udc uevent
>     callback and trigger kobject_uevent in usb_gadget_set_state
>     to inform the user-space the state of the gadget.

Why?

And what's with the indentation?

>     Signed-off-by: Rong Wang <Rong.Wang@csr.com>
> 
> diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
> index ffd8fa5..05715d1 100644
> --- a/drivers/usb/gadget/udc-core.c
> +++ b/drivers/usb/gadget/udc-core.c
> @@ -101,11 +101,32 @@ EXPORT_SYMBOL_GPL(usb_gadget_unmap_request);
> 
>  /* ------------------------------------------------------------------------- */
> 
> +/**
> + * usb_gadget_set_state - set usb gadget state
> + * @gadget: gadget device
> + * @state: state defined in USB specification ch9
> + * Context: !in_interrupt()
> + */
>  void usb_gadget_set_state(struct usb_gadget *gadget,
>                 enum usb_device_state state)
>  {
> +       struct usb_udc          *udc = NULL;
> +
>         gadget->state = state;
>         sysfs_notify(&gadget->dev.kobj, NULL, "status");
> +
> +       mutex_lock(&udc_lock);
> +       list_for_each_entry(udc, &udc_list, list)
> +               if (udc->gadget == gadget)
> +                       goto found;
> +
> +       dev_err(gadget->dev.parent, "gadget not registered.\n");
> +       mutex_unlock(&udc_lock);
> +       return;
> +
> +found:
> +       mutex_unlock(&udc_lock);
> +       kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);

I really don't like adding kobject change events, as they usually never
get listened to, and there are other ways to get this information from
userspace, right?

Why do you need/want this?  What is it going to be used for?  What tools
will use it?  How will they use it?

And why can't the existing notification events for gadget devices work
for you?

thanks,

greg k-h

Re: [PATCH] usb: udc: add gadget state kobject uevent

[Rong Wang]

Hi Greg,

The USB on our platform can change roles between HOST and GADGET, but
it is not capable of OTG.
When the USB changes between roles the udev will run some scripts
automatically according to the udev rules.
The default role is GADGET, and we bind the g_mass_storage to the USB
GADGET role.

We should secure the back end file storage between the device and the
host PC connecting to our device.
We need to know when the GADGET is really connect to a host PC, then
we can umount the file on the device
and export it to the g_mass_storage.

The question is since we default GADGET, so the g_mass_storage.ko is
installed early but connecting to a host PC
is randomly, But the udev has no idea when a host PC connects our device.

So we consider it's reasonable to let the udev know the GADGET device state.
Is there any alternative to our question?

Thanks!

On Tue, Jul 16, 2013 at 12:52 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Mon, Jul 15, 2013 at 11:57:24AM +0800, Rong Wang wrote:
>>     usb: udc: add gadget state kobject uevent
>>
>>     Add USB_UDC_STATE environment variable in udc uevent
>>     callback and trigger kobject_uevent in usb_gadget_set_state
>>     to inform the user-space the state of the gadget.
>
> Why?
>
> And what's with the indentation?
>
>>     Signed-off-by: Rong Wang <Rong.Wang@csr.com>
>>
>> diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
>> index ffd8fa5..05715d1 100644
>> --- a/drivers/usb/gadget/udc-core.c
>> +++ b/drivers/usb/gadget/udc-core.c
>> @@ -101,11 +101,32 @@ EXPORT_SYMBOL_GPL(usb_gadget_unmap_request);
>>
>>  /* ------------------------------------------------------------------------- */
>>
>> +/**
>> + * usb_gadget_set_state - set usb gadget state
>> + * @gadget: gadget device
>> + * @state: state defined in USB specification ch9
>> + * Context: !in_interrupt()
>> + */
>>  void usb_gadget_set_state(struct usb_gadget *gadget,
>>                 enum usb_device_state state)
>>  {
>> +       struct usb_udc          *udc = NULL;
>> +
>>         gadget->state = state;
>>         sysfs_notify(&gadget->dev.kobj, NULL, "status");
>> +
>> +       mutex_lock(&udc_lock);
>> +       list_for_each_entry(udc, &udc_list, list)
>> +               if (udc->gadget == gadget)
>> +                       goto found;
>> +
>> +       dev_err(gadget->dev.parent, "gadget not registered.\n");
>> +       mutex_unlock(&udc_lock);
>> +       return;
>> +
>> +found:
>> +       mutex_unlock(&udc_lock);
>> +       kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
>
> I really don't like adding kobject change events, as they usually never
> get listened to, and there are other ways to get this information from
> userspace, right?
>
> Why do you need/want this?  What is it going to be used for?  What tools
> will use it?  How will they use it?
>
> And why can't the existing notification events for gadget devices work
> for you?
>
> thanks,
>
> greg k-h

Re: [PATCH] usb: udc: add gadget state kobject uevent

[Greg KH]

On Tue, Jul 16, 2013 at 11:49:07AM +0800, Rong Wang wrote:
> Hi Greg,
> 
> The USB on our platform can change roles between HOST and GADGET, but
> it is not capable of OTG.

That kind of sounds like the definition of OTG :)

> When the USB changes between roles the udev will run some scripts
> automatically according to the udev rules.

What exactly does udev/userspace see when the roles change?

And what can trigger the change in roles?

> The default role is GADGET, and we bind the g_mass_storage to the USB
> GADGET role.
> 
> We should secure the back end file storage between the device and the
> host PC connecting to our device.
> We need to know when the GADGET is really connect to a host PC, then
> we can umount the file on the device
> and export it to the g_mass_storage.

I thought you already get an event for this, otherwise no one would be
able to properly deal with this.

> The question is since we default GADGET, so the g_mass_storage.ko is
> installed early but connecting to a host PC
> is randomly, But the udev has no idea when a host PC connects our device.
> 
> So we consider it's reasonable to let the udev know the GADGET device state.
> Is there any alternative to our question?

I thought we already export events for gadget device states, have you
looked for them?  I can't dig through the code at the moment, but this
seems like a pretty common issue...

Felipe, any ideas?

greg k-h

Re: [PATCH] usb: udc: add gadget state kobject uevent

[Peter Chen]

On Mon, Jul 15, 2013 at 11:31:17PM -0700, Greg KH wrote:
> On Tue, Jul 16, 2013 at 11:49:07AM +0800, Rong Wang wrote:
> > Hi Greg,
> > 
> > The USB on our platform can change roles between HOST and GADGET, but
> > it is not capable of OTG.
> 
> That kind of sounds like the definition of OTG :)
> 
> > When the USB changes between roles the udev will run some scripts
> > automatically according to the udev rules.
> 
> What exactly does udev/userspace see when the roles change?
> 
> And what can trigger the change in roles?
> 
> > The default role is GADGET, and we bind the g_mass_storage to the USB
> > GADGET role.
> > 
> > We should secure the back end file storage between the device and the
> > host PC connecting to our device.
> > We need to know when the GADGET is really connect to a host PC, then
> > we can umount the file on the device
> > and export it to the g_mass_storage.
> 
> I thought you already get an event for this, otherwise no one would be
> able to properly deal with this.
> 
> > The question is since we default GADGET, so the g_mass_storage.ko is
> > installed early but connecting to a host PC
> > is randomly, But the udev has no idea when a host PC connects our device.
> > 
> > So we consider it's reasonable to let the udev know the GADGET device state.
> > Is there any alternative to our question?
> 
> I thought we already export events for gadget device states, have you
> looked for them?  I can't dig through the code at the moment, but this
> seems like a pretty common issue...
> 

If I understand correctly,  what Rong wants is udev can be notified the
udc state changes, like connect/disconnect event. Currently, we only
export it to /sys.

-- 

Best Regards,
Peter Chen

Re: [PATCH] usb: udc: add gadget state kobject uevent

[Rong Wang]

Hi Peter,


On Wed, Jul 17, 2013 at 10:36 AM, Peter Chen <peter.chen@freescale.com> wrote:
> On Mon, Jul 15, 2013 at 11:31:17PM -0700, Greg KH wrote:
>> On Tue, Jul 16, 2013 at 11:49:07AM +0800, Rong Wang wrote:
>> > Hi Greg,
>> >
>> > The USB on our platform can change roles between HOST and GADGET, but
>> > it is not capable of OTG.
>>
>> That kind of sounds like the definition of OTG :)
>>
>> > When the USB changes between roles the udev will run some scripts
>> > automatically according to the udev rules.
>>
>> What exactly does udev/userspace see when the roles change?
>>
>> And what can trigger the change in roles?
>>
>> > The default role is GADGET, and we bind the g_mass_storage to the USB
>> > GADGET role.
>> >
>> > We should secure the back end file storage between the device and the
>> > host PC connecting to our device.
>> > We need to know when the GADGET is really connect to a host PC, then
>> > we can umount the file on the device
>> > and export it to the g_mass_storage.
>>
>> I thought you already get an event for this, otherwise no one would be
>> able to properly deal with this.
>>
>> > The question is since we default GADGET, so the g_mass_storage.ko is
>> > installed early but connecting to a host PC
>> > is randomly, But the udev has no idea when a host PC connects our device.
>> >
>> > So we consider it's reasonable to let the udev know the GADGET device state.
>> > Is there any alternative to our question?
>>
>> I thought we already export events for gadget device states, have you
>> looked for them?  I can't dig through the code at the moment, but this
>> seems like a pretty common issue...
>>
>
> If I understand correctly,  what Rong wants is udev can be notified the
> udc state changes, like connect/disconnect event. Currently, we only
> export it to /sys.

OK. Thanks for your share.

And you develop a new utility rather than udev to monitor that file?
And you probably create a work queue in your udc driver to do this work?

>
> --
>
> Best Regards,
> Peter Chen
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] usb: udc: add gadget state kobject uevent

[Chen Peter-B29397]

 
> 
> On Wed, Jul 17, 2013 at 10:36 AM, Peter Chen <peter.chen@freescale.com>
> wrote:
> > On Mon, Jul 15, 2013 at 11:31:17PM -0700, Greg KH wrote:
> >> On Tue, Jul 16, 2013 at 11:49:07AM +0800, Rong Wang wrote:
> >> > Hi Greg,
> >> >
> >> > The USB on our platform can change roles between HOST and GADGET,
> but
> >> > it is not capable of OTG.
> >>
> >> That kind of sounds like the definition of OTG :)
> >>
> >> > When the USB changes between roles the udev will run some scripts
> >> > automatically according to the udev rules.
> >>
> >> What exactly does udev/userspace see when the roles change?
> >>
> >> And what can trigger the change in roles?
> >>
> >> > The default role is GADGET, and we bind the g_mass_storage to the
> USB
> >> > GADGET role.
> >> >
> >> > We should secure the back end file storage between the device and
> the
> >> > host PC connecting to our device.
> >> > We need to know when the GADGET is really connect to a host PC, then
> >> > we can umount the file on the device
> >> > and export it to the g_mass_storage.
> >>
> >> I thought you already get an event for this, otherwise no one would be
> >> able to properly deal with this.
> >>
> >> > The question is since we default GADGET, so the g_mass_storage.ko is
> >> > installed early but connecting to a host PC
> >> > is randomly, But the udev has no idea when a host PC connects our
> device.
> >> >
> >> > So we consider it's reasonable to let the udev know the GADGET
> device state.
> >> > Is there any alternative to our question?
> >>
> >> I thought we already export events for gadget device states, have you
> >> looked for them?  I can't dig through the code at the moment, but this
> >> seems like a pretty common issue...
> >>
> >
> > If I understand correctly,  what Rong wants is udev can be notified the
> > udc state changes, like connect/disconnect event. Currently, we only
> > export it to /sys.
> 
> OK. Thanks for your share.
> 
> And you develop a new utility rather than udev to monitor that file?
> And you probably create a work queue in your udc driver to do this work?
> 
 
Not yet, no one complains it, so I haven't added it.
But, it is a useful user interface, Android has implemented similar functions
at its gadget driver.

Best regards,
Peter

Re: [PATCH] usb: udc: add gadget state kobject uevent

[Rong Wang]

Hi Greg,

On Tue, Jul 16, 2013 at 2:31 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Tue, Jul 16, 2013 at 11:49:07AM +0800, Rong Wang wrote:
>> Hi Greg,
>>
>> The USB on our platform can change roles between HOST and GADGET, but
>> it is not capable of OTG.
>
> That kind of sounds like the definition of OTG :)

Yes. But it just initiates its role according to the ID pin status.

>
>> When the USB changes between roles the udev will run some scripts
>> automatically according to the udev rules.
>
> What exactly does udev/userspace see when the roles change?
>

Take HOST -> GADGET for example, the driver removes the USB hcd first,

ACTION=remove
DEVPATH=/devices/axi.0/uus-iobg.13/b8010000.usbcontroller/ci_hdrc.0/usb1/1-0:1.0
SUBSYSTEM=usb
DEVTYPE=usb_interface
PRODUCT=1d6b/2/310
TYPE=9/0/1
INTERFACE=9/0/0
MODALIAS=usb:v1D6Bp0002d0310dc09dsc00dp01ic09isc00ip00in00
SEQNUM=1843

Then it initiates the GADGET role which will call usb_add_gadget_udc

ACTION=add
DEVPATH=/devices/axi.0/uus-iobg.13/b8010000.usbcontroller/ci_hdrc.0/udc/ci_hdrc.0
SUBSYSTEM=udc
USB_UDC_NAME=ci13xxx_sirf
SEQNUM=1845

At this time, the udev finds this matches the rule and it will install
g_mass_storage.ko. But we are actually not connecting to a PC now, so
we do not umount the back-end file storage on our device.

Currently the udc framework introduces an API usb_gadget_set_state
to report to user-space the USB device state. But it's not compatible with
udev. It needs user-space utility polling the "status" file under /sys.
And it cannot be called in interrupt context but drivers do the state change
in interrupt service routine.

What's more, I grep the USB driver and find few driver apply this API except
the udc core. But it just initiated its state as "NOT ATTACHED" when registering
a new udc.

In my opinion, the USB device state change is a common operation and it
should be implemented by driver framework. The best place to do this is
when composite framework handling standard USB requests. But it's not
implemented yet.

So we are not informed the USB state until the role changes again.
We do not know when to secure the back-end file between our device and the
host PC.

> And what can trigger the change in roles?

The role changes according to the ID pin status.
When we plug in a mini-A (ID pin connects to ground) it will cause a
ID pin interrupt
and we will change to HOST role.
If the ID pin connects to nothing we will act as GADGET.

In a sum, role change takes place when we plug in different USB connectors.

>
>> The default role is GADGET, and we bind the g_mass_storage to the USB
>> GADGET role.
>>
>> We should secure the back end file storage between the device and the
>> host PC connecting to our device.
>> We need to know when the GADGET is really connect to a host PC, then
>> we can umount the file on the device
>> and export it to the g_mass_storage.
>
> I thought you already get an event for this, otherwise no one would be
> able to properly deal with this.

Yes. We install file storage module on this event but we do not get
further notice
as mentioned above.

>
>> The question is since we default GADGET, so the g_mass_storage.ko is
>> installed early but connecting to a host PC
>> is randomly, But the udev has no idea when a host PC connects our device.
>>
>> So we consider it's reasonable to let the udev know the GADGET device state.
>> Is there any alternative to our question?
>
> I thought we already export events for gadget device states, have you
> looked for them?  I can't dig through the code at the moment, but this
> seems like a pretty common issue...
>
> Felipe, any ideas?
>
> greg k-h

Re: [PATCH] usb: udc: add gadget state kobject uevent

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

Hi,

On Mon, Jul 15, 2013 at 11:31:17PM -0700, Greg KH wrote:
> > The question is since we default GADGET, so the g_mass_storage.ko is
> > installed early but connecting to a host PC
> > is randomly, But the udev has no idea when a host PC connects our device.
> > 
> > So we consider it's reasonable to let the udev know the GADGET device state.
> > Is there any alternative to our question?
> 
> I thought we already export events for gadget device states, have you
> looked for them?  I can't dig through the code at the moment, but this
> seems like a pretty common issue...
> 
> Felipe, any ideas?

we already expose that in sysfs. IIRC udev can act on sysfs changes,
no ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] usb: udc: add gadget state kobject uevent

[Rong Wang]

Hi Felipe,

On Wed, Jul 17, 2013 at 3:57 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Mon, Jul 15, 2013 at 11:31:17PM -0700, Greg KH wrote:
>> > The question is since we default GADGET, so the g_mass_storage.ko is
>> > installed early but connecting to a host PC
>> > is randomly, But the udev has no idea when a host PC connects our device.
>> >
>> > So we consider it's reasonable to let the udev know the GADGET device state.
>> > Is there any alternative to our question?
>>
>> I thought we already export events for gadget device states, have you
>> looked for them?  I can't dig through the code at the moment, but this
>> seems like a pretty common issue...
>>
>> Felipe, any ideas?
>
> we already expose that in sysfs. IIRC udev can act on sysfs changes,
> no ?

I do not know if udev can polling sysfs file content change. I'll study this.

But the change is triggered by calling usb_gadget_set_state, and I find
composite framework do not call this. Then we should do this common work
in every udc driver?

>
> --
> balbi


===============================================

Hi Greg,

On Tue, Jul 16, 2013 at 2:31 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Tue, Jul 16, 2013 at 11:49:07AM +0800, Rong Wang wrote:
>> Hi Greg,
>>
>> The USB on our platform can change roles between HOST and GADGET, but
>> it is not capable of OTG.
>
> That kind of sounds like the definition of OTG :)

Yes. But it just initiates its role according to the ID pin status.

>
>> When the USB changes between roles the udev will run some scripts
>> automatically according to the udev rules.
>
> What exactly does udev/userspace see when the roles change?
>

Take HOST -> GADGET for example, the driver removes the USB hcd first,

ACTION=remove
DEVPATH=/devices/axi.0/uus-
iobg.13/b8010000.usbcontroller/ci_hdrc.0/usb1/1-0:1.0
SUBSYSTEM=usb
DEVTYPE=usb_interface
PRODUCT=1d6b/2/310
TYPE=9/0/1
INTERFACE=9/0/0
MODALIAS=usb:v1D6Bp0002d0310dc09dsc00dp01ic09isc00ip00in00
SEQNUM=1843

Then it initiates the GADGET role which will call usb_add_gadget_udc

ACTION=add
DEVPATH=/devices/axi.0/uus-iobg.13/b8010000.usbcontroller/ci_hdrc.0/udc/ci_hdrc.0
SUBSYSTEM=udc
USB_UDC_NAME=ci13xxx_sirf
SEQNUM=1845

At this time, the udev finds this matches the rule and it will install
g_mass_storage.ko. But we are actually not connecting to a PC now, so
we do not umount the back-end file storage on our device.

Currently the udc framework introduces an API usb_gadget_set_state
to report to user-space the USB device state. But it's not compatible with
udev. It needs user-space utility polling the "status" file under /sys.
And it cannot be called in interrupt context but drivers do the state change
in interrupt service routine.

What's more, I grep the USB driver and find few driver apply this API except
the udc core. But it just initiated its state as "NOT ATTACHED" when registering
a new udc.

In my opinion, the USB device state change is a common operation and it
should be implemented by driver framework. The best place to do this is
when composite framework handling standard USB requests. But it's not
implemented yet.

So we are not informed the USB state until the role changes again.
We do not know when to secure the back-end file between our device and the
host PC.

> And what can trigger the change in roles?

The role changes according to the ID pin status.
When we plug in a mini-A (ID pin connects to ground) it will cause a
ID pin interrupt
and we will change to HOST role.
If the ID pin connects to nothing we will act as GADGET.

In a sum, role change takes place when we plug in different USB connectors.

>
>> The default role is GADGET, and we bind the g_mass_storage to the USB
>> GADGET role.
>>
>> We should secure the back end file storage between the device and the
>> host PC connecting to our device.
>> We need to know when the GADGET is really connect to a host PC, then
>> we can umount the file on the device
>> and export it to the g_mass_storage.
>
> I thought you already get an event for this, otherwise no one would be
> able to properly deal with this.

Yes. We install file storage module on this event but we do not get
further notice
as mentioned above.

>
>> The question is since we default GADGET, so the g_mass_storage.ko is
>> installed early but connecting to a host PC
>> is randomly, But the udev has no idea when a host PC connects our device.
>>
>> So we consider it's reasonable to let the udev know the GADGET device state.
>> Is there any alternative to our question?
>
> I thought we already export events for gadget device states, have you
> looked for them?  I can't dig through the code at the moment, but this
> seems like a pretty common issue...
>
> Felipe, any ideas?
>
> greg k-h

Re: [PATCH] usb: udc: add gadget state kobject uevent

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1263 bytes --]

On Wed, Jul 17, 2013 at 09:04:54PM +0800, Rong Wang wrote:
> Hi Felipe,
> 
> On Wed, Jul 17, 2013 at 3:57 PM, Felipe Balbi <balbi@ti.com> wrote:
> > Hi,
> >
> > On Mon, Jul 15, 2013 at 11:31:17PM -0700, Greg KH wrote:
> >> > The question is since we default GADGET, so the g_mass_storage.ko is
> >> > installed early but connecting to a host PC
> >> > is randomly, But the udev has no idea when a host PC connects our device.
> >> >
> >> > So we consider it's reasonable to let the udev know the GADGET device state.
> >> > Is there any alternative to our question?
> >>
> >> I thought we already export events for gadget device states, have you
> >> looked for them?  I can't dig through the code at the moment, but this
> >> seems like a pretty common issue...
> >>
> >> Felipe, any ideas?
> >
> > we already expose that in sysfs. IIRC udev can act on sysfs changes,
> > no ?
> 
> I do not know if udev can polling sysfs file content change. I'll study this.
> 
> But the change is triggered by calling usb_gadget_set_state, and I find
> composite framework do not call this. Then we should do this common work
> in every udc driver?

yes. Only the UDC driver knows when the controller is moving among those
states.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] usb: udc: add gadget state kobject uevent

[Alan Stern]

On Wed, 17 Jul 2013, Felipe Balbi wrote:

> On Wed, Jul 17, 2013 at 09:04:54PM +0800, Rong Wang wrote:
> > Hi Felipe,
> > 
> > On Wed, Jul 17, 2013 at 3:57 PM, Felipe Balbi <balbi@ti.com> wrote:
> > > Hi,
> > >
> > > On Mon, Jul 15, 2013 at 11:31:17PM -0700, Greg KH wrote:
> > >> > The question is since we default GADGET, so the g_mass_storage.ko is
> > >> > installed early but connecting to a host PC
> > >> > is randomly, But the udev has no idea when a host PC connects our device.
> > >> >
> > >> > So we consider it's reasonable to let the udev know the GADGET device state.
> > >> > Is there any alternative to our question?
> > >>
> > >> I thought we already export events for gadget device states, have you
> > >> looked for them?  I can't dig through the code at the moment, but this
> > >> seems like a pretty common issue...
> > >>
> > >> Felipe, any ideas?
> > >
> > > we already expose that in sysfs. IIRC udev can act on sysfs changes,
> > > no ?
> > 
> > I do not know if udev can polling sysfs file content change. I'll study this.
> > 
> > But the change is triggered by calling usb_gadget_set_state, and I find
> > composite framework do not call this. Then we should do this common work
> > in every udc driver?
> 
> yes. Only the UDC driver knows when the controller is moving among those
> states.

Not quite.  Only the gadget driver knows when the transition between 
ADDRESS and CONFIGURED occurs.  This should be added to composite.c.

Alan Stern

Re: [PATCH] usb: udc: add gadget state kobject uevent

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3271 bytes --]

Hi,

On Wed, Jul 17, 2013 at 11:37:35AM -0400, Alan Stern wrote:
> On Wed, 17 Jul 2013, Felipe Balbi wrote:
> 
> > On Wed, Jul 17, 2013 at 09:04:54PM +0800, Rong Wang wrote:
> > > Hi Felipe,
> > > 
> > > On Wed, Jul 17, 2013 at 3:57 PM, Felipe Balbi <balbi@ti.com> wrote:
> > > > Hi,
> > > >
> > > > On Mon, Jul 15, 2013 at 11:31:17PM -0700, Greg KH wrote:
> > > >> > The question is since we default GADGET, so the g_mass_storage.ko is
> > > >> > installed early but connecting to a host PC
> > > >> > is randomly, But the udev has no idea when a host PC connects our device.
> > > >> >
> > > >> > So we consider it's reasonable to let the udev know the GADGET device state.
> > > >> > Is there any alternative to our question?
> > > >>
> > > >> I thought we already export events for gadget device states, have you
> > > >> looked for them?  I can't dig through the code at the moment, but this
> > > >> seems like a pretty common issue...
> > > >>
> > > >> Felipe, any ideas?
> > > >
> > > > we already expose that in sysfs. IIRC udev can act on sysfs changes,
> > > > no ?
> > > 
> > > I do not know if udev can polling sysfs file content change. I'll study this.
> > > 
> > > But the change is triggered by calling usb_gadget_set_state, and I find
> > > composite framework do not call this. Then we should do this common work
> > > in every udc driver?
> > 
> > yes. Only the UDC driver knows when the controller is moving among those
> > states.
> 
> Not quite.  Only the gadget driver knows when the transition between 
> ADDRESS and CONFIGURED occurs.  This should be added to composite.c.

that's not entirely true :-) See how we handle that in dwc3:

| static int dwc3_ep0_set_config(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl)
| {
| 	enum usb_device_state state = dwc->gadget.state;
| 	u32 cfg;
| 	int ret;
| 	u32 reg;
| 
| 	dwc->start_config_issued = false;
| 	cfg = le16_to_cpu(ctrl->wValue);
| 
| 	switch (state) {
| 	case USB_STATE_DEFAULT:
| 		return -EINVAL;
| 		break;
| 
| 	case USB_STATE_ADDRESS:
| 		ret = dwc3_ep0_delegate_req(dwc, ctrl);
| 		/* if the cfg matches and the cfg is non zero */
| 		if (cfg && (!ret || (ret == USB_GADGET_DELAYED_STATUS))) {
| 			usb_gadget_set_state(&dwc->gadget,
| 					USB_STATE_CONFIGURED);
| 
| 			/*
| 			 * Enable transition to U1/U2 state when
| 			 * nothing is pending from application.
| 			 */
| 			reg = dwc3_readl(dwc->regs, DWC3_DCTL);
| 			reg |= (DWC3_DCTL_ACCEPTU1ENA | DWC3_DCTL_ACCEPTU2ENA);
| 			dwc3_writel(dwc->regs, DWC3_DCTL, reg);
| 
| 			dwc->resize_fifos = true;
| 			dev_dbg(dwc->dev, "resize fifos flag SET\n");
| 		}
| 		break;
| 
| 	case USB_STATE_CONFIGURED:
| 		ret = dwc3_ep0_delegate_req(dwc, ctrl);
| 		if (!cfg)
| 			usb_gadget_set_state(&dwc->gadget,
| 					USB_STATE_ADDRESS);
| 		break;
| 	default:
| 		ret = -EINVAL;
| 	}
| 	return ret;
| }

Also, until other gadget drivers add notifications to the other cases, I
don't think it's wise to add a transition from NOTATTACHED to
CONFIGURED.

But I have one change I'll send for the gadget notifications, I'm just
trying to get a new OMAP5 board to test because the FTDI chip on mine
died and I have no console :-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] usb: udc: add gadget state kobject uevent

[Alan Stern]

On Thu, 18 Jul 2013, Felipe Balbi wrote:

> > > yes. Only the UDC driver knows when the controller is moving among those
> > > states.
> > 
> > Not quite.  Only the gadget driver knows when the transition between 
> > ADDRESS and CONFIGURED occurs.  This should be added to composite.c.
> 
> that's not entirely true :-) See how we handle that in dwc3:
> 
> | static int dwc3_ep0_set_config(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl)
> | {
> | 	enum usb_device_state state = dwc->gadget.state;
> | 	u32 cfg;
> | 	int ret;
> | 	u32 reg;
> | 
> | 	dwc->start_config_issued = false;
> | 	cfg = le16_to_cpu(ctrl->wValue);
> | 
> | 	switch (state) {
> | 	case USB_STATE_DEFAULT:
> | 		return -EINVAL;
> | 		break;
> | 
> | 	case USB_STATE_ADDRESS:
> | 		ret = dwc3_ep0_delegate_req(dwc, ctrl);
> | 		/* if the cfg matches and the cfg is non zero */
> | 		if (cfg && (!ret || (ret == USB_GADGET_DELAYED_STATUS))) {
> | 			usb_gadget_set_state(&dwc->gadget,
> | 					USB_STATE_CONFIGURED);

In theory, this should not occur until the gadget driver has finished 
the transition to the CONFIGURED state, which doesn't occur until later 
in the case of USB_GADGET_DELAYED_STATUS.

> | 
> | 			/*
> | 			 * Enable transition to U1/U2 state when
> | 			 * nothing is pending from application.
> | 			 */
> | 			reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> | 			reg |= (DWC3_DCTL_ACCEPTU1ENA | DWC3_DCTL_ACCEPTU2ENA);
> | 			dwc3_writel(dwc->regs, DWC3_DCTL, reg);
> | 
> | 			dwc->resize_fifos = true;
> | 			dev_dbg(dwc->dev, "resize fifos flag SET\n");
> | 		}
> | 		break;
> | 
> | 	case USB_STATE_CONFIGURED:
> | 		ret = dwc3_ep0_delegate_req(dwc, ctrl);
> | 		if (!cfg)
> | 			usb_gadget_set_state(&dwc->gadget,
> | 					USB_STATE_ADDRESS);

No check on the value of ret?  What if the request was rejected?

> | 		break;
> | 	default:
> | 		ret = -EINVAL;
> | 	}
> | 	return ret;
> | }

This illustrates the folly of exposing your code to public review.  :-)
Nevertheless, I take your point.

> Also, until other gadget drivers add notifications to the other cases, I
> don't think it's wise to add a transition from NOTATTACHED to
> CONFIGURED.

Another good point.

Alan Stern

Re: [PATCH] usb: udc: add gadget state kobject uevent

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2406 bytes --]

Hi,

On Thu, Jul 18, 2013 at 09:50:37AM -0400, Alan Stern wrote:
> On Thu, 18 Jul 2013, Felipe Balbi wrote:
> 
> > > > yes. Only the UDC driver knows when the controller is moving among those
> > > > states.
> > > 
> > > Not quite.  Only the gadget driver knows when the transition between 
> > > ADDRESS and CONFIGURED occurs.  This should be added to composite.c.
> > 
> > that's not entirely true :-) See how we handle that in dwc3:
> > 
> > | static int dwc3_ep0_set_config(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl)
> > | {
> > | 	enum usb_device_state state = dwc->gadget.state;
> > | 	u32 cfg;
> > | 	int ret;
> > | 	u32 reg;
> > | 
> > | 	dwc->start_config_issued = false;
> > | 	cfg = le16_to_cpu(ctrl->wValue);
> > | 
> > | 	switch (state) {
> > | 	case USB_STATE_DEFAULT:
> > | 		return -EINVAL;
> > | 		break;
> > | 
> > | 	case USB_STATE_ADDRESS:
> > | 		ret = dwc3_ep0_delegate_req(dwc, ctrl);
> > | 		/* if the cfg matches and the cfg is non zero */
> > | 		if (cfg && (!ret || (ret == USB_GADGET_DELAYED_STATUS))) {
> > | 			usb_gadget_set_state(&dwc->gadget,
> > | 					USB_STATE_CONFIGURED);
> 
> In theory, this should not occur until the gadget driver has finished 
> the transition to the CONFIGURED state, which doesn't occur until later 
> in the case of USB_GADGET_DELAYED_STATUS.
> 
> > | 
> > | 			/*
> > | 			 * Enable transition to U1/U2 state when
> > | 			 * nothing is pending from application.
> > | 			 */
> > | 			reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> > | 			reg |= (DWC3_DCTL_ACCEPTU1ENA | DWC3_DCTL_ACCEPTU2ENA);
> > | 			dwc3_writel(dwc->regs, DWC3_DCTL, reg);
> > | 
> > | 			dwc->resize_fifos = true;
> > | 			dev_dbg(dwc->dev, "resize fifos flag SET\n");
> > | 		}
> > | 		break;
> > | 
> > | 	case USB_STATE_CONFIGURED:
> > | 		ret = dwc3_ep0_delegate_req(dwc, ctrl);
> > | 		if (!cfg)
> > | 			usb_gadget_set_state(&dwc->gadget,
> > | 					USB_STATE_ADDRESS);
> 
> No check on the value of ret?  What if the request was rejected?
> 
> > | 		break;
> > | 	default:
> > | 		ret = -EINVAL;
> > | 	}
> > | 	return ret;
> > | }
> 
> This illustrates the folly of exposing your code to public review.  :-)

it's always good to have extra eyes looking at the code, I'll patch
those up, but since it has always been like that and nobody complained,
I won't tag it for stable.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] usb: udc: add gadget state kobject uevent

[Rong Wang]

On Wed, Jul 17, 2013 at 9:27 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Wed, Jul 17, 2013 at 09:04:54PM +0800, Rong Wang wrote:
>> Hi Felipe,
>>
>> On Wed, Jul 17, 2013 at 3:57 PM, Felipe Balbi <balbi@ti.com> wrote:
>> > Hi,
>> >
>> > On Mon, Jul 15, 2013 at 11:31:17PM -0700, Greg KH wrote:
>> >> > The question is since we default GADGET, so the g_mass_storage.ko is
>> >> > installed early but connecting to a host PC
>> >> > is randomly, But the udev has no idea when a host PC connects our device.
>> >> >
>> >> > So we consider it's reasonable to let the udev know the GADGET device state.
>> >> > Is there any alternative to our question?
>> >>
>> >> I thought we already export events for gadget device states, have you
>> >> looked for them?  I can't dig through the code at the moment, but this
>> >> seems like a pretty common issue...
>> >>
>> >> Felipe, any ideas?
>> >
>> > we already expose that in sysfs. IIRC udev can act on sysfs changes,
>> > no ?
>>
>> I do not know if udev can polling sysfs file content change. I'll study this.
>>
>> But the change is triggered by calling usb_gadget_set_state, and I find
>> composite framework do not call this. Then we should do this common work
>> in every udc driver?
>
> yes. Only the UDC driver knows when the controller is moving among those
> states.

OK. I got that.

But I think it may be more reasonable for the udc driver to maintain a
work queue
to handle the state change since this operation mostly happen in ISR ?


>
> --
> balbi

Re: [PATCH] usb: udc: add gadget state kobject uevent

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 5189 bytes --]

Hi,

On Thu, Jul 18, 2013 at 04:33:43PM +0800, Rong Wang wrote:
> On Wed, Jul 17, 2013 at 9:27 PM, Felipe Balbi <balbi@ti.com> wrote:
> > On Wed, Jul 17, 2013 at 09:04:54PM +0800, Rong Wang wrote:
> >> Hi Felipe,
> >>
> >> On Wed, Jul 17, 2013 at 3:57 PM, Felipe Balbi <balbi@ti.com> wrote:
> >> > Hi,
> >> >
> >> > On Mon, Jul 15, 2013 at 11:31:17PM -0700, Greg KH wrote:
> >> >> > The question is since we default GADGET, so the g_mass_storage.ko is
> >> >> > installed early but connecting to a host PC
> >> >> > is randomly, But the udev has no idea when a host PC connects our device.
> >> >> >
> >> >> > So we consider it's reasonable to let the udev know the GADGET device state.
> >> >> > Is there any alternative to our question?
> >> >>
> >> >> I thought we already export events for gadget device states, have you
> >> >> looked for them?  I can't dig through the code at the moment, but this
> >> >> seems like a pretty common issue...
> >> >>
> >> >> Felipe, any ideas?
> >> >
> >> > we already expose that in sysfs. IIRC udev can act on sysfs changes,
> >> > no ?
> >>
> >> I do not know if udev can polling sysfs file content change. I'll study this.
> >>
> >> But the change is triggered by calling usb_gadget_set_state, and I find
> >> composite framework do not call this. Then we should do this common work
> >> in every udc driver?
> >
> > yes. Only the UDC driver knows when the controller is moving among those
> > states.
> 
> OK. I got that.
> 
> But I think it may be more reasonable for the udc driver to maintain a
> work queue
> to handle the state change since this operation mostly happen in ISR ?

And that's the patch I want to test. Adding a workqueue to every single
UDC will be too much, so I tried to hide it in udc-core.c. I agree with
you we need to pass the sysfs_notification to a separate workqueue
though. If you can test the patch below, that would be great.

commit d32521bd775d48b600e67f23d363d70f71597ffd
Author: Felipe Balbi <balbi@ti.com>
Date:   Wed Jul 17 11:09:49 2013 +0300

    usb: gadget: udc-core: move sysfs_notify() to a workqueue
    
    usb_gadget_set_state() will call sysfs_notify()
    which might sleep. Some users might want to call
    usb_gadget_set_state() from the very IRQ handler
    which actually changes the gadget state.
    
    Instead of having every UDC driver add their own
    workqueue for such a simple notification, we're
    adding it generically to our struct usb_gadget,
    so the details are hidden from all UDC drivers.
    
    Signed-off-by: Felipe Balbi <balbi@ti.com>

diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
index ffd8fa5..b0d91b1 100644
--- a/drivers/usb/gadget/udc-core.c
+++ b/drivers/usb/gadget/udc-core.c
@@ -23,6 +23,7 @@
 #include <linux/list.h>
 #include <linux/err.h>
 #include <linux/dma-mapping.h>
+#include <linux/workqueue.h>
 
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
@@ -101,11 +102,18 @@ EXPORT_SYMBOL_GPL(usb_gadget_unmap_request);
 
 /* ------------------------------------------------------------------------- */
 
+static void usb_gadget_state_work(struct work_struct *work)
+{
+	struct usb_gadget	*gadget = work_to_gadget(work);
+
+	sysfs_notify(&gadget->dev.kobj, NULL, "status");
+}
+
 void usb_gadget_set_state(struct usb_gadget *gadget,
 		enum usb_device_state state)
 {
 	gadget->state = state;
-	sysfs_notify(&gadget->dev.kobj, NULL, "status");
+	schedule_work(&gadget->work);
 }
 EXPORT_SYMBOL_GPL(usb_gadget_set_state);
 
@@ -192,6 +200,7 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
 		goto err1;
 
 	dev_set_name(&gadget->dev, "gadget");
+	INIT_WORK(&gadget->work, usb_gadget_state_work);
 	gadget->dev.parent = parent;
 
 	dma_set_coherent_mask(&gadget->dev, parent->coherent_dma_mask);
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index f1b0dca..942ef5e 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -22,6 +22,7 @@
 #include <linux/slab.h>
 #include <linux/scatterlist.h>
 #include <linux/types.h>
+#include <linux/workqueue.h>
 #include <linux/usb/ch9.h>
 
 struct usb_ep;
@@ -475,6 +476,7 @@ struct usb_gadget_ops {
 
 /**
  * struct usb_gadget - represents a usb slave device
+ * @work: (internal use) Workqueue to be used for sysfs_notify()
  * @ops: Function pointers used to access hardware-specific operations.
  * @ep0: Endpoint zero, used when reading or writing responses to
  *	driver setup() requests
@@ -520,6 +522,7 @@ struct usb_gadget_ops {
  * device is acting as a B-Peripheral (so is_a_peripheral is false).
  */
 struct usb_gadget {
+	struct work_struct		work;
 	/* readonly to gadget driver */
 	const struct usb_gadget_ops	*ops;
 	struct usb_ep			*ep0;
@@ -538,6 +541,7 @@ struct usb_gadget {
 	unsigned			out_epnum;
 	unsigned			in_epnum;
 };
+#define work_to_gadget(w)	(container_of((w), struct usb_gadget, work))
 
 static inline void set_gadget_data(struct usb_gadget *gadget, void *data)
 	{ dev_set_drvdata(&gadget->dev, data); }

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] usb: udc: add gadget state kobject uevent

[Rong Wang]

Hi Felipe,

Thanks, I'll test the patch.

But sysfs_notify(&gadget->dev.kobj, NULL, "status"), status or state ?
I notice that DEVICE_ATTR(state, S_IRUGO, usb_gadget_state_show, NULL)

On Thu, Jul 18, 2013 at 4:40 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Thu, Jul 18, 2013 at 04:33:43PM +0800, Rong Wang wrote:
>> On Wed, Jul 17, 2013 at 9:27 PM, Felipe Balbi <balbi@ti.com> wrote:
>> > On Wed, Jul 17, 2013 at 09:04:54PM +0800, Rong Wang wrote:
>> >> Hi Felipe,
>> >>
>> >> On Wed, Jul 17, 2013 at 3:57 PM, Felipe Balbi <balbi@ti.com> wrote:
>> >> > Hi,
>> >> >
>> >> > On Mon, Jul 15, 2013 at 11:31:17PM -0700, Greg KH wrote:
>> >> >> > The question is since we default GADGET, so the g_mass_storage.ko is
>> >> >> > installed early but connecting to a host PC
>> >> >> > is randomly, But the udev has no idea when a host PC connects our device.
>> >> >> >
>> >> >> > So we consider it's reasonable to let the udev know the GADGET device state.
>> >> >> > Is there any alternative to our question?
>> >> >>
>> >> >> I thought we already export events for gadget device states, have you
>> >> >> looked for them?  I can't dig through the code at the moment, but this
>> >> >> seems like a pretty common issue...
>> >> >>
>> >> >> Felipe, any ideas?
>> >> >
>> >> > we already expose that in sysfs. IIRC udev can act on sysfs changes,
>> >> > no ?
>> >>
>> >> I do not know if udev can polling sysfs file content change. I'll study this.
>> >>
>> >> But the change is triggered by calling usb_gadget_set_state, and I find
>> >> composite framework do not call this. Then we should do this common work
>> >> in every udc driver?
>> >
>> > yes. Only the UDC driver knows when the controller is moving among those
>> > states.
>>
>> OK. I got that.
>>
>> But I think it may be more reasonable for the udc driver to maintain a
>> work queue
>> to handle the state change since this operation mostly happen in ISR ?
>
> And that's the patch I want to test. Adding a workqueue to every single
> UDC will be too much, so I tried to hide it in udc-core.c. I agree with
> you we need to pass the sysfs_notification to a separate workqueue
> though. If you can test the patch below, that would be great.
>
> commit d32521bd775d48b600e67f23d363d70f71597ffd
> Author: Felipe Balbi <balbi@ti.com>
> Date:   Wed Jul 17 11:09:49 2013 +0300
>
>     usb: gadget: udc-core: move sysfs_notify() to a workqueue
>
>     usb_gadget_set_state() will call sysfs_notify()
>     which might sleep. Some users might want to call
>     usb_gadget_set_state() from the very IRQ handler
>     which actually changes the gadget state.
>
>     Instead of having every UDC driver add their own
>     workqueue for such a simple notification, we're
>     adding it generically to our struct usb_gadget,
>     so the details are hidden from all UDC drivers.
>
>     Signed-off-by: Felipe Balbi <balbi@ti.com>
>
> diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
> index ffd8fa5..b0d91b1 100644
> --- a/drivers/usb/gadget/udc-core.c
> +++ b/drivers/usb/gadget/udc-core.c
> @@ -23,6 +23,7 @@
>  #include <linux/list.h>
>  #include <linux/err.h>
>  #include <linux/dma-mapping.h>
> +#include <linux/workqueue.h>
>
>  #include <linux/usb/ch9.h>
>  #include <linux/usb/gadget.h>
> @@ -101,11 +102,18 @@ EXPORT_SYMBOL_GPL(usb_gadget_unmap_request);
>
>  /* ------------------------------------------------------------------------- */
>
> +static void usb_gadget_state_work(struct work_struct *work)
> +{
> +       struct usb_gadget       *gadget = work_to_gadget(work);
> +
> +       sysfs_notify(&gadget->dev.kobj, NULL, "status");
> +}
> +
>  void usb_gadget_set_state(struct usb_gadget *gadget,
>                 enum usb_device_state state)
>  {
>         gadget->state = state;
> -       sysfs_notify(&gadget->dev.kobj, NULL, "status");
> +       schedule_work(&gadget->work);
>  }
>  EXPORT_SYMBOL_GPL(usb_gadget_set_state);
>
> @@ -192,6 +200,7 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
>                 goto err1;
>
>         dev_set_name(&gadget->dev, "gadget");
> +       INIT_WORK(&gadget->work, usb_gadget_state_work);
>         gadget->dev.parent = parent;
>
>         dma_set_coherent_mask(&gadget->dev, parent->coherent_dma_mask);
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index f1b0dca..942ef5e 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -22,6 +22,7 @@
>  #include <linux/slab.h>
>  #include <linux/scatterlist.h>
>  #include <linux/types.h>
> +#include <linux/workqueue.h>
>  #include <linux/usb/ch9.h>
>
>  struct usb_ep;
> @@ -475,6 +476,7 @@ struct usb_gadget_ops {
>
>  /**
>   * struct usb_gadget - represents a usb slave device
> + * @work: (internal use) Workqueue to be used for sysfs_notify()
>   * @ops: Function pointers used to access hardware-specific operations.
>   * @ep0: Endpoint zero, used when reading or writing responses to
>   *     driver setup() requests
> @@ -520,6 +522,7 @@ struct usb_gadget_ops {
>   * device is acting as a B-Peripheral (so is_a_peripheral is false).
>   */
>  struct usb_gadget {
> +       struct work_struct              work;
>         /* readonly to gadget driver */
>         const struct usb_gadget_ops     *ops;
>         struct usb_ep                   *ep0;
> @@ -538,6 +541,7 @@ struct usb_gadget {
>         unsigned                        out_epnum;
>         unsigned                        in_epnum;
>  };
> +#define work_to_gadget(w)      (container_of((w), struct usb_gadget, work))
>
>  static inline void set_gadget_data(struct usb_gadget *gadget, void *data)
>         { dev_set_drvdata(&gadget->dev, data); }
>
> --
> balbi

Re: [PATCH] usb: udc: add gadget state kobject uevent

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 432 bytes --]

On Thu, Jul 18, 2013 at 05:28:19PM +0800, Rong Wang wrote:
> Hi Felipe,
> 
> Thanks, I'll test the patch.
> 
> But sysfs_notify(&gadget->dev.kobj, NULL, "status"), status or state ?
> I notice that DEVICE_ATTR(state, S_IRUGO, usb_gadget_state_show, NULL)

good eyes, please send a patch which I'll queue on this -rc and Cc:
<stable@vger.kernel.org>.

I'll rewrite my patch on top of the patched -rc later.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] usb: udc: add gadget state kobject uevent

[Rong Wang]

Hi Felipe,

Here's the patch. If you are OK with it, I'll send it to the list formally.
Thanks.

-------------------------------------------------------------------------------------------------

usb: gadget: udc-core: make udc state attribute name consistent

The name of udc state attribute file under sysfs is
registered as "state", while usb_gadget_set_state
take it as "status" when it's going to update.
Here it is made consistent as "state".

Signed-off-by: Rong Wang <Rong.Wang@csr.com>

diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
index ffd8fa5..5514822 100644
--- a/drivers/usb/gadget/udc-core.c
+++ b/drivers/usb/gadget/udc-core.c
@@ -105,7 +105,7 @@ void usb_gadget_set_state(struct usb_gadget *gadget,
                enum usb_device_state state)
 {
        gadget->state = state;
-       sysfs_notify(&gadget->dev.kobj, NULL, "status");
+      sysfs_notify(&gadget->dev.kobj, NULL, "state");
 }
 EXPORT_SYMBOL_GPL(usb_gadget_set_state);

On Thu, Jul 18, 2013 at 6:10 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Thu, Jul 18, 2013 at 05:28:19PM +0800, Rong Wang wrote:
>> Hi Felipe,
>>
>> Thanks, I'll test the patch.
>>
>> But sysfs_notify(&gadget->dev.kobj, NULL, "status"), status or state ?
>> I notice that DEVICE_ATTR(state, S_IRUGO, usb_gadget_state_show, NULL)
>
> good eyes, please send a patch which I'll queue on this -rc and Cc:
> <stable@vger.kernel.org>.
>
> I'll rewrite my patch on top of the patched -rc later.
>
> --
> balbi

Re: [PATCH] usb: udc: add gadget state kobject uevent

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1194 bytes --]

On Thu, Jul 18, 2013 at 06:54:38PM +0800, Rong Wang wrote:
> Hi Felipe,
> 
> Here's the patch. If you are OK with it, I'll send it to the list formally.
> Thanks.
> 
> -------------------------------------------------------------------------------------------------
> 
> usb: gadget: udc-core: make udc state attribute name consistent
> 
> The name of udc state attribute file under sysfs is
> registered as "state", while usb_gadget_set_state
> take it as "status" when it's going to update.
> Here it is made consistent as "state".
> 
> Signed-off-by: Rong Wang <Rong.Wang@csr.com>
> 
> diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
> index ffd8fa5..5514822 100644
> --- a/drivers/usb/gadget/udc-core.c
> +++ b/drivers/usb/gadget/udc-core.c
> @@ -105,7 +105,7 @@ void usb_gadget_set_state(struct usb_gadget *gadget,
>                 enum usb_device_state state)
>  {
>         gadget->state = state;
> -       sysfs_notify(&gadget->dev.kobj, NULL, "status");
> +      sysfs_notify(&gadget->dev.kobj, NULL, "state");

just fix the indentation and prevent gmail from mangling the tabs into
spaces, you're good to go :-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] usb: udc: add gadget state kobject uevent

[Barry Song]

2013/7/18 Felipe Balbi <balbi@ti.com>:
> On Thu, Jul 18, 2013 at 05:28:19PM +0800, Rong Wang wrote:
>> Hi Felipe,
>>
>> Thanks, I'll test the patch.
>>
>> But sysfs_notify(&gadget->dev.kobj, NULL, "status"), status or state ?
>> I notice that DEVICE_ATTR(state, S_IRUGO, usb_gadget_state_show, NULL)
>
> good eyes, please send a patch which I'll queue on this -rc and Cc:
> <stable@vger.kernel.org>.

ok. i will handle that for rong. and pls consider to merge rong's
patch about sending uevent.

>
> I'll rewrite my patch on top of the patched -rc later.
>
> --
> balbi

-barry

Re: [PATCH] usb: udc: add gadget state kobject uevent

[Greg KH]

On Fri, Jul 26, 2013 at 01:56:19PM +0800, Barry Song wrote:
> 2013/7/18 Felipe Balbi <balbi@ti.com>:
> > On Thu, Jul 18, 2013 at 05:28:19PM +0800, Rong Wang wrote:
> >> Hi Felipe,
> >>
> >> Thanks, I'll test the patch.
> >>
> >> But sysfs_notify(&gadget->dev.kobj, NULL, "status"), status or state ?
> >> I notice that DEVICE_ATTR(state, S_IRUGO, usb_gadget_state_show, NULL)
> >
> > good eyes, please send a patch which I'll queue on this -rc and Cc:
> > <stable@vger.kernel.org>.
> 
> ok. i will handle that for rong. and pls consider to merge rong's
> patch about sending uevent.

It needs to be fixed based on the comments I had for it, before _anyone_
can merge it.

thanks,

greg k-h

Re: [PATCH] usb: udc: add gadget state kobject uevent

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 5232 bytes --]

Hi,

On Thu, Jul 18, 2013 at 11:40:41AM +0300, Felipe Balbi wrote:
> > >> >> > The question is since we default GADGET, so the g_mass_storage.ko is
> > >> >> > installed early but connecting to a host PC
> > >> >> > is randomly, But the udev has no idea when a host PC connects our device.
> > >> >> >
> > >> >> > So we consider it's reasonable to let the udev know the GADGET device state.
> > >> >> > Is there any alternative to our question?
> > >> >>
> > >> >> I thought we already export events for gadget device states, have you
> > >> >> looked for them?  I can't dig through the code at the moment, but this
> > >> >> seems like a pretty common issue...
> > >> >>
> > >> >> Felipe, any ideas?
> > >> >
> > >> > we already expose that in sysfs. IIRC udev can act on sysfs changes,
> > >> > no ?
> > >>
> > >> I do not know if udev can polling sysfs file content change. I'll study this.
> > >>
> > >> But the change is triggered by calling usb_gadget_set_state, and I find
> > >> composite framework do not call this. Then we should do this common work
> > >> in every udc driver?
> > >
> > > yes. Only the UDC driver knows when the controller is moving among those
> > > states.
> > 
> > OK. I got that.
> > 
> > But I think it may be more reasonable for the udc driver to maintain a
> > work queue
> > to handle the state change since this operation mostly happen in ISR ?
> 
> And that's the patch I want to test. Adding a workqueue to every single
> UDC will be too much, so I tried to hide it in udc-core.c. I agree with
> you we need to pass the sysfs_notification to a separate workqueue
> though. If you can test the patch below, that would be great.
> 
> commit d32521bd775d48b600e67f23d363d70f71597ffd
> Author: Felipe Balbi <balbi@ti.com>
> Date:   Wed Jul 17 11:09:49 2013 +0300
> 
>     usb: gadget: udc-core: move sysfs_notify() to a workqueue
>     
>     usb_gadget_set_state() will call sysfs_notify()
>     which might sleep. Some users might want to call
>     usb_gadget_set_state() from the very IRQ handler
>     which actually changes the gadget state.
>     
>     Instead of having every UDC driver add their own
>     workqueue for such a simple notification, we're
>     adding it generically to our struct usb_gadget,
>     so the details are hidden from all UDC drivers.
>     
>     Signed-off-by: Felipe Balbi <balbi@ti.com>
> 
> diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
> index ffd8fa5..b0d91b1 100644
> --- a/drivers/usb/gadget/udc-core.c
> +++ b/drivers/usb/gadget/udc-core.c
> @@ -23,6 +23,7 @@
>  #include <linux/list.h>
>  #include <linux/err.h>
>  #include <linux/dma-mapping.h>
> +#include <linux/workqueue.h>
>  
>  #include <linux/usb/ch9.h>
>  #include <linux/usb/gadget.h>
> @@ -101,11 +102,18 @@ EXPORT_SYMBOL_GPL(usb_gadget_unmap_request);
>  
>  /* ------------------------------------------------------------------------- */
>  
> +static void usb_gadget_state_work(struct work_struct *work)
> +{
> +	struct usb_gadget	*gadget = work_to_gadget(work);
> +
> +	sysfs_notify(&gadget->dev.kobj, NULL, "status");
> +}
> +
>  void usb_gadget_set_state(struct usb_gadget *gadget,
>  		enum usb_device_state state)
>  {
>  	gadget->state = state;
> -	sysfs_notify(&gadget->dev.kobj, NULL, "status");
> +	schedule_work(&gadget->work);
>  }
>  EXPORT_SYMBOL_GPL(usb_gadget_set_state);
>  
> @@ -192,6 +200,7 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
>  		goto err1;
>  
>  	dev_set_name(&gadget->dev, "gadget");
> +	INIT_WORK(&gadget->work, usb_gadget_state_work);
>  	gadget->dev.parent = parent;
>  
>  	dma_set_coherent_mask(&gadget->dev, parent->coherent_dma_mask);
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index f1b0dca..942ef5e 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -22,6 +22,7 @@
>  #include <linux/slab.h>
>  #include <linux/scatterlist.h>
>  #include <linux/types.h>
> +#include <linux/workqueue.h>
>  #include <linux/usb/ch9.h>
>  
>  struct usb_ep;
> @@ -475,6 +476,7 @@ struct usb_gadget_ops {
>  
>  /**
>   * struct usb_gadget - represents a usb slave device
> + * @work: (internal use) Workqueue to be used for sysfs_notify()
>   * @ops: Function pointers used to access hardware-specific operations.
>   * @ep0: Endpoint zero, used when reading or writing responses to
>   *	driver setup() requests
> @@ -520,6 +522,7 @@ struct usb_gadget_ops {
>   * device is acting as a B-Peripheral (so is_a_peripheral is false).
>   */
>  struct usb_gadget {
> +	struct work_struct		work;
>  	/* readonly to gadget driver */
>  	const struct usb_gadget_ops	*ops;
>  	struct usb_ep			*ep0;
> @@ -538,6 +541,7 @@ struct usb_gadget {
>  	unsigned			out_epnum;
>  	unsigned			in_epnum;
>  };
> +#define work_to_gadget(w)	(container_of((w), struct usb_gadget, work))
>  
>  static inline void set_gadget_data(struct usb_gadget *gadget, void *data)
>  	{ dev_set_drvdata(&gadget->dev, data); }

nevermind, colleague borrowed me his omap5 board. It's tested, will send
a proper patch now.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] usb: udc: add gadget state kobject uevent

[Greg KH]

On Wed, Jul 17, 2013 at 10:57:06AM +0300, Felipe Balbi wrote:
> Hi,
> 
> On Mon, Jul 15, 2013 at 11:31:17PM -0700, Greg KH wrote:
> > > The question is since we default GADGET, so the g_mass_storage.ko is
> > > installed early but connecting to a host PC
> > > is randomly, But the udev has no idea when a host PC connects our device.
> > > 
> > > So we consider it's reasonable to let the udev know the GADGET device state.
> > > Is there any alternative to our question?
> > 
> > I thought we already export events for gadget device states, have you
> > looked for them?  I can't dig through the code at the moment, but this
> > seems like a pretty common issue...
> > 
> > Felipe, any ideas?
> 
> we already expose that in sysfs. IIRC udev can act on sysfs changes,
> no ?

If something is polling the sysfs file, yes.  If not, a change event
should be sent to notify people that they need to go re-read it as
something major happened (laptop docked, disk got removed, device
changed state, etc.)

thanks,

greg k-h

Re: [PATCH] usb: udc: add gadget state kobject uevent

[Barry Song]

2013/7/18 Greg KH <gregkh@linuxfoundation.org>:
> On Wed, Jul 17, 2013 at 10:57:06AM +0300, Felipe Balbi wrote:
>> Hi,
>>
>> On Mon, Jul 15, 2013 at 11:31:17PM -0700, Greg KH wrote:
>> > > The question is since we default GADGET, so the g_mass_storage.ko is
>> > > installed early but connecting to a host PC
>> > > is randomly, But the udev has no idea when a host PC connects our device.
>> > >
>> > > So we consider it's reasonable to let the udev know the GADGET device state.
>> > > Is there any alternative to our question?
>> >
>> > I thought we already export events for gadget device states, have you
>> > looked for them?  I can't dig through the code at the moment, but this
>> > seems like a pretty common issue...
>> >
>> > Felipe, any ideas?
>>
>> we already expose that in sysfs. IIRC udev can act on sysfs changes,
>> no ?
>
> If something is polling the sysfs file, yes.  If not, a change event
> should be sent to notify people that they need to go re-read it as
> something major happened (laptop docked, disk got removed, device
> changed state, etc.)

sysfs_notify() should be a mechinism to permit userspace to
poll()/select() the sysfs file if users want to . for example, if an
user cares about sysfs changes, he can do:

res = select(sysfd+1,
             NULL,               // readfds - not needed
             NULL,               // writefds - not needed
             &exceptfds,
             NULL);              // timeout (never)

if (res > 0 && FD_ISSET(sysfd, &exceptfds))
{

}

i didn't see udev can poll sysfs node which will change value
dynamically. it depends on 3rd applications to do that. and i also
didn't find we can define a udev rule to monitor sysfs change.

so the benefit of this patch here is that if we send uevent, we can
easily define a rule in udev to switch the file_storage partition to
target board or PC dynamically. this is a very popular user scenerios
actually, considering a phone, the SD card is mounted to mobilephone
at first, then after we connect the phone to PC, the SD is given to
PC, after we disconnect, the partition will be given back to phone.

we did see Android have done similar patch in its tree. so i would
suggest we continue to work on rong's patch and make it visible in
mainline.

>
> thanks,
>
> greg k-h

-barry

[PATCH] usb: udc: add gadget state kobject uevent

[Rong Wang]

    usb: udc: add gadget state kobject uevent

    Add USB_UDC_STATE environment variable in udc uevent
    callback and trigger kobject_uevent in usb_gadget_set_state
    to inform the user-space the state of the gadget.

    Signed-off-by: Rong Wang <Rong.Wang@csr.com>

diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
index ffd8fa5..05715d1 100644
--- a/drivers/usb/gadget/udc-core.c
+++ b/drivers/usb/gadget/udc-core.c
@@ -101,11 +101,32 @@ EXPORT_SYMBOL_GPL(usb_gadget_unmap_request);

 /* ------------------------------------------------------------------------- */

+/**
+ * usb_gadget_set_state - set usb gadget state
+ * @gadget: gadget device
+ * @state: state defined in USB specification ch9
+ * Context: !in_interrupt()
+ */
 void usb_gadget_set_state(struct usb_gadget *gadget,
                enum usb_device_state state)
 {
+       struct usb_udc          *udc = NULL;
+
        gadget->state = state;
        sysfs_notify(&gadget->dev.kobj, NULL, "status");
+
+       mutex_lock(&udc_lock);
+       list_for_each_entry(udc, &udc_list, list)
+               if (udc->gadget == gadget)
+                       goto found;
+
+       dev_err(gadget->dev.parent, "gadget not registered.\n");
+       mutex_unlock(&udc_lock);
+       return;
+
+found:
+       mutex_unlock(&udc_lock);
+       kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
 }
 EXPORT_SYMBOL_GPL(usb_gadget_set_state);

@@ -538,6 +559,15 @@ static int usb_udc_uevent(struct device *dev,
struct kobj_uevent_env *env)
                }
        }

+       if (udc->gadget) {
+               ret = add_uevent_var(env, "USB_UDC_STATE=%s",
+                               usb_state_string(udc->gadget->state));
+               if (ret) {
+                       dev_err(dev, "failed to add uevent USB_UDC_STATE\n");
+                       return ret;
+               }
+       }
+
        return 0;
 }