Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Badhri Jagan Sridharan <badhri@google.com>]

Hi,

At present I am using the uevent in the userspace to infer
the Presence of a port on the remote end through the
appearance of usbc*-partner.

Userspace uses this info to decide on when to show a USB
notification on the screen and what should be the options
provided in the dialog.

I was assuming that this is not something that would be dropped.

Coding using events was relatively easier to program from userspace ..

Is it possible to use POLL for identifying the appearance of port partner ?
I did not notice sysfs_notify call in typec_connect/typec_disconnect.

It would also be nice to have uevent notifications when the contents
of current_data_role or current_power_role changes.

Is that too costly to have ?

Thanks,
Badhri.


On Tue, Nov 15, 2016 at 1:25 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 11/14/2016 11:07 PM, Greg KH wrote:
>>
>> On Mon, Nov 14, 2016 at 12:46:50PM -0800, Guenter Roeck wrote:
>>>
>>> On Mon, Nov 14, 2016 at 02:32:35PM +0200, Heikki Krogerus wrote:
>>>>
>>>> Hi Greg,
>>>>
>>>> On Mon, Nov 14, 2016 at 10:51:48AM +0100, Greg KH wrote:
>>>>>
>>>>> On Mon, Sep 19, 2016 at 02:16:56PM +0300, Heikki Krogerus wrote:
>>>>>>
>>>>>> The purpose of USB Type-C connector class is to provide
>>>>>> unified interface for the user space to get the status and
>>>>>> basic information about USB Type-C connectors on a system,
>>>>>> control over data role swapping, and when the port supports
>>>>>> USB Power Delivery, also control over power role swapping
>>>>>> and Alternate Modes.
>>>>>>
>>>>>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>>>>>> Tested-by: Guenter Roeck <linux@roeck-us.net>
>>>>>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>>>>> ---
>>>>>>  Documentation/ABI/testing/sysfs-class-typec |  218 ++++++
>>>>>>  Documentation/usb/typec.txt                 |  103 +++
>>>>>>  MAINTAINERS                                 |    9 +
>>>>>>  drivers/usb/Kconfig                         |    2 +
>>>>>>  drivers/usb/Makefile                        |    2 +
>>>>>>  drivers/usb/typec/Kconfig                   |    7 +
>>>>>>  drivers/usb/typec/Makefile                  |    1 +
>>>>>>  drivers/usb/typec/typec.c                   | 1075
>>>>>> +++++++++++++++++++++++++++
>>>>>>  include/linux/usb/typec.h                   |  252 +++++++
>>>>>>  9 files changed, 1669 insertions(+)
>>>>>>  create mode 100644 Documentation/ABI/testing/sysfs-class-typec
>>>>>>  create mode 100644 Documentation/usb/typec.txt
>>>>>>  create mode 100644 drivers/usb/typec/Kconfig
>>>>>>  create mode 100644 drivers/usb/typec/Makefile
>>>>>>  create mode 100644 drivers/usb/typec/typec.c
>>>>>>  create mode 100644 include/linux/usb/typec.h
>>>>>
>>>>>
>>> [ ... ]
>>>
>>>>>> +
>>>>>> +int typec_connect(struct typec_port *port, struct typec_connection
>>>>>> *con)
>>>>>> +{
>>>>>> +       int ret;
>>>>>> +
>>>>>> +       if (!con->partner && !con->cable)
>>>>>> +               return -EINVAL;
>>>>>> +
>>>>>> +       port->connected = 1;
>>>>>> +       port->data_role = con->data_role;
>>>>>> +       port->pwr_role = con->pwr_role;
>>>>>> +       port->vconn_role = con->vconn_role;
>>>>>> +       port->pwr_opmode = con->pwr_opmode;
>>>>>> +
>>>>>> +       kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);
>>>>>
>>>>>
>>>>> This worries me.  Who is listening for it?  What will you do with it?
>>>>> Shouldn't you just poll on an attribute file instead?
>>>>
>>>>
>>>> Oliver! Did you need this or can we remove it?
>>>>
>>>> I remember I removed the "connected" attribute because you did not see
>>>> any use for it at one point. I don't remember the reason exactly why?
>>>>
>>>
>>> The Android team tells me that they are currently using the udev events
>>> to track port role changes, and to detect presence of port partner.
>>>
>>> Also, there are plans to track changes on usbc*cable to differentiate
>>> between cable attach vs. device being attached on the remote end.
>>>
>>> What is the problem with using kobject_uevent() and thus presumably
>>> udev events ?
>>
>>
>> It's not a "normal" thing to do and is pretty "heavy" to do.  What does
>> userspace do with that change event?  Does it read specific attributes?
>> What causes the event to happen in the kernel, is it really just a
>> change in the specific object, or do new ones get added/removed?
>>
>> In short, document the heck out of this please so people know how to use
>> it, and what is happening when the event happens.
>>
>
> Badhri,
>
> can you clarify which events you are using in detail, and what for ?
>
> Thanks,
> Guenter
>