Re: [PATCH v2] usb: typec: Add a sysfs node to manage port type

[Guenter Roeck]

On Thu, May 25, 2017 at 11:24:20AM -0700, Badhri Jagan Sridharan wrote:
> On Thu, May 25, 2017 at 12:48 AM, Guenter Roeck  wrote:
> > On 05/24/2017 08:10 PM, Badhri Jagan Sridharan wrote:
> >>
> >> Thanks comments inline.
> >>
> >> On Tue, May 23, 2017 at 7:38 PM, Guenter Roeck  wrote:
> >>>
> >>> On 05/23/2017 06:28 PM, Badhri Jagan Sridharan wrote:
> 
> 
>  User space applications in some cases have the need to enforce a
>  specific port type(DFP/UFP/DRP). This change allows userspace to
>  attempt setting the desired port type. Low level drivers can
>  however reject the request if the specific port type is not supported.
> 
>  Signed-off-by: Badhri Jagan Sridharan 
>  ---
>  Changelog since v1:
>  - introduced a new variable port_type in struct typec_port to track
>  the current port type instead of changing type member in
>  typec_capability to address Heikki Krogerus comments.
>  - changed the output format and strings that would be displayed as
>  suggested by Heikki Krogerus.
> >
> >Documentation/ABI/testing/sysfs-class-typec | 13 ++
> 
> drivers/usb/typec/typec.c   | 66
>  +
> include/linux/usb/typec.h   |  4 ++
> 3 files changed, 83 insertions(+)
> 
>  diff --git a/Documentation/ABI/testing/sysfs-class-typec
>  b/Documentation/ABI/testing/sysfs-class-typec
>  index d4a3d23eb09c..1f224c2e391f 100644
>  --- a/Documentation/ABI/testing/sysfs-class-typec
>  +++ b/Documentation/ABI/testing/sysfs-class-typec
>  @@ -73,6 +73,19 @@ Description:
>   Valid values: source, sink, none (to remove preference)
> +What:   /sys/class/typec//port_type
>  +Date:   May 2017
>  +Description:
>  +   Indicates the type of the port. This attribute can be
>  used
>  for
>  +   requesting a change in the port type. Port type change
>  is
>  +   supported as a synchronous operation, so write(2) to the
>  +   attribute will not return until the operation has
>  finished.
>  +
>  +   Valid values:
>  +   - source
>  +   - sink
>  +   - dual
>  +
> What: /sys/class/typec//supported_accessory_modes
> Date: April 2017
> Contact:  Heikki Krogerus 
>  diff --git a/drivers/usb/typec/typec.c b/drivers/usb/typec/typec.c
>  index 89e540bb7ff3..5063d6e0f8c7 100644
>  --- a/drivers/usb/typec/typec.c
>  +++ b/drivers/usb/typec/typec.c
>  @@ -69,6 +69,7 @@ struct typec_port {
>   enum typec_role pwr_role;
>   enum typec_role vconn_role;
>   enum typec_pwr_opmode   pwr_opmode;
>  +   enum typec_port_typeport_type;
> >>>
> >>>
> >>>
> >>> I am missing where this variable is initialized (when the port is
> >>> registered
> >>> ?).
> >>
> >>
> >> Yes.. I missed it while cleaning up the patch. Will add it to my next
> >> patch.
> >>
> >>>
>   const struct typec_capability   *cap;
> };
>  @@ -789,6 +790,12 @@ static const char * const typec_data_roles[] = {
>   [TYPEC_HOST]= "host",
> };
> +static const char * const typec_port_types[] = {
>  +   [TYPEC_PORT_DFP] = "source",
>  +   [TYPEC_PORT_UFP] = "sink",
>  +   [TYPEC_PORT_DRP] = "dual",
>  +};
>  +
> static ssize_t
> preferred_role_store(struct device *dev, struct device_attribute
>  *attr,
>    const char *buf, size_t size)
>  @@ -856,6 +863,12 @@ static ssize_t data_role_store(struct device *dev,
>   return -EOPNOTSUPP;
>   }
> + if (port->port_type != TYPEC_PORT_DRP) {
>  +   dev_dbg(dev, "port type fixed at \"%s\"",
>  +typec_port_types[port->port_type]);
>  +   return -EIO;
> >>>
> >>>
> >>>
> >>> ?? This is already there, or am I missing something ?
> >>
> >>
> >> I am checking against the current value of port_type variable.
> >> Dont we want to reject role swaps if the port_type is not
> >> TYPEC_PORT_DRP ? My understanding is that if the port type
> >> is fixed at say PORT_TYPE_DFP by userspace, then unless
> >> the userspace sets port_type back to PORT_TYPE_DRP,
> >> role swap requests have to rejected. Is my understanding not
> >> correct ?
> >>
> >
> > Ah yes, the existing check is for port->cap->type. But why not just
> > replace that check with port->port_type ? Checking both seems overkill.
> 
> Thanks. Sure will stick to just checking port->port_type.
> 
> >
> >>>
>  +   }
> 

Re: [PATCH v2] usb: typec: Add a sysfs node to manage port type

[Badhri Jagan Sridharan]

On Thu, May 25, 2017 at 12:48 AM, Guenter Roeck  wrote:
> On 05/24/2017 08:10 PM, Badhri Jagan Sridharan wrote:
>>
>> Thanks comments inline.
>>
>> On Tue, May 23, 2017 at 7:38 PM, Guenter Roeck  wrote:
>>>
>>> On 05/23/2017 06:28 PM, Badhri Jagan Sridharan wrote:


 User space applications in some cases have the need to enforce a
 specific port type(DFP/UFP/DRP). This change allows userspace to
 attempt setting the desired port type. Low level drivers can
 however reject the request if the specific port type is not supported.

 Signed-off-by: Badhri Jagan Sridharan 
 ---
 Changelog since v1:
 - introduced a new variable port_type in struct typec_port to track
 the current port type instead of changing type member in
 typec_capability to address Heikki Krogerus comments.
 - changed the output format and strings that would be displayed as
 suggested by Heikki Krogerus.
>
>Documentation/ABI/testing/sysfs-class-typec | 13 ++

drivers/usb/typec/typec.c   | 66
 +
include/linux/usb/typec.h   |  4 ++
3 files changed, 83 insertions(+)

 diff --git a/Documentation/ABI/testing/sysfs-class-typec
 b/Documentation/ABI/testing/sysfs-class-typec
 index d4a3d23eb09c..1f224c2e391f 100644
 --- a/Documentation/ABI/testing/sysfs-class-typec
 +++ b/Documentation/ABI/testing/sysfs-class-typec
 @@ -73,6 +73,19 @@ Description:
  Valid values: source, sink, none (to remove preference)
+What:   /sys/class/typec//port_type
 +Date:   May 2017
 +Description:
 +   Indicates the type of the port. This attribute can be
 used
 for
 +   requesting a change in the port type. Port type change
 is
 +   supported as a synchronous operation, so write(2) to the
 +   attribute will not return until the operation has
 finished.
 +
 +   Valid values:
 +   - source
 +   - sink
 +   - dual
 +
What: /sys/class/typec//supported_accessory_modes
Date: April 2017
Contact:  Heikki Krogerus 
 diff --git a/drivers/usb/typec/typec.c b/drivers/usb/typec/typec.c
 index 89e540bb7ff3..5063d6e0f8c7 100644
 --- a/drivers/usb/typec/typec.c
 +++ b/drivers/usb/typec/typec.c
 @@ -69,6 +69,7 @@ struct typec_port {
  enum typec_role pwr_role;
  enum typec_role vconn_role;
  enum typec_pwr_opmode   pwr_opmode;
 +   enum typec_port_typeport_type;
>>>
>>>
>>>
>>> I am missing where this variable is initialized (when the port is
>>> registered
>>> ?).
>>
>>
>> Yes.. I missed it while cleaning up the patch. Will add it to my next
>> patch.
>>
>>>
  const struct typec_capability   *cap;
};
 @@ -789,6 +790,12 @@ static const char * const typec_data_roles[] = {
  [TYPEC_HOST]= "host",
};
+static const char * const typec_port_types[] = {
 +   [TYPEC_PORT_DFP] = "source",
 +   [TYPEC_PORT_UFP] = "sink",
 +   [TYPEC_PORT_DRP] = "dual",
 +};
 +
static ssize_t
preferred_role_store(struct device *dev, struct device_attribute
 *attr,
   const char *buf, size_t size)
 @@ -856,6 +863,12 @@ static ssize_t data_role_store(struct device *dev,
  return -EOPNOTSUPP;
  }
+ if (port->port_type != TYPEC_PORT_DRP) {
 +   dev_dbg(dev, "port type fixed at \"%s\"",
 +typec_port_types[port->port_type]);
 +   return -EIO;
>>>
>>>
>>>
>>> ?? This is already there, or am I missing something ?
>>
>>
>> I am checking against the current value of port_type variable.
>> Dont we want to reject role swaps if the port_type is not
>> TYPEC_PORT_DRP ? My understanding is that if the port type
>> is fixed at say PORT_TYPE_DFP by userspace, then unless
>> the userspace sets port_type back to PORT_TYPE_DRP,
>> role swap requests have to rejected. Is my understanding not
>> correct ?
>>
>
> Ah yes, the existing check is for port->cap->type. But why not just
> replace that check with port->port_type ? Checking both seems overkill.

Thanks. Sure will stick to just checking port->port_type.

>
>>>
 +   }
 +
  ret = sysfs_match_string(typec_data_roles, buf);
  if (ret < 0)
  return ret;
 @@ -897,6 +910,12 @@ static ssize_t power_role_store(struct device *dev,
  return -EOPNOTSUPP;
  }
+ if (port->port_type != TYPEC_PORT_DRP) {

Re: [PATCH v2] usb: typec: Add a sysfs node to manage port type

[Guenter Roeck]

On 05/24/2017 08:10 PM, Badhri Jagan Sridharan wrote:

Thanks comments inline.

On Tue, May 23, 2017 at 7:38 PM, Guenter Roeck  wrote:

On 05/23/2017 06:28 PM, Badhri Jagan Sridharan wrote:


User space applications in some cases have the need to enforce a
specific port type(DFP/UFP/DRP). This change allows userspace to
attempt setting the desired port type. Low level drivers can
however reject the request if the specific port type is not supported.

Signed-off-by: Badhri Jagan Sridharan 
---
Changelog since v1:
- introduced a new variable port_type in struct typec_port to track
the current port type instead of changing type member in
typec_capability to address Heikki Krogerus comments.
- changed the output format and strings that would be displayed as
suggested by Heikki Krogerus.

   Documentation/ABI/testing/sysfs-class-typec | 13 ++

   drivers/usb/typec/typec.c   | 66
+
   include/linux/usb/typec.h   |  4 ++
   3 files changed, 83 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-typec
b/Documentation/ABI/testing/sysfs-class-typec
index d4a3d23eb09c..1f224c2e391f 100644
--- a/Documentation/ABI/testing/sysfs-class-typec
+++ b/Documentation/ABI/testing/sysfs-class-typec
@@ -73,6 +73,19 @@ Description:
 Valid values: source, sink, none (to remove preference)
   +What:   /sys/class/typec//port_type
+Date:   May 2017
+Description:
+   Indicates the type of the port. This attribute can be used
for
+   requesting a change in the port type. Port type change is
+   supported as a synchronous operation, so write(2) to the
+   attribute will not return until the operation has
finished.
+
+   Valid values:
+   - source
+   - sink
+   - dual
+
   What: /sys/class/typec//supported_accessory_modes
   Date: April 2017
   Contact:  Heikki Krogerus 
diff --git a/drivers/usb/typec/typec.c b/drivers/usb/typec/typec.c
index 89e540bb7ff3..5063d6e0f8c7 100644
--- a/drivers/usb/typec/typec.c
+++ b/drivers/usb/typec/typec.c
@@ -69,6 +69,7 @@ struct typec_port {
 enum typec_role pwr_role;
 enum typec_role vconn_role;
 enum typec_pwr_opmode   pwr_opmode;
+   enum typec_port_typeport_type;



I am missing where this variable is initialized (when the port is registered
?).


Yes.. I missed it while cleaning up the patch. Will add it to my next patch.




 const struct typec_capability   *cap;
   };
@@ -789,6 +790,12 @@ static const char * const typec_data_roles[] = {
 [TYPEC_HOST]= "host",
   };
   +static const char * const typec_port_types[] = {
+   [TYPEC_PORT_DFP] = "source",
+   [TYPEC_PORT_UFP] = "sink",
+   [TYPEC_PORT_DRP] = "dual",
+};
+
   static ssize_t
   preferred_role_store(struct device *dev, struct device_attribute *attr,
  const char *buf, size_t size)
@@ -856,6 +863,12 @@ static ssize_t data_role_store(struct device *dev,
 return -EOPNOTSUPP;
 }
   + if (port->port_type != TYPEC_PORT_DRP) {
+   dev_dbg(dev, "port type fixed at \"%s\"",
+typec_port_types[port->port_type]);
+   return -EIO;



?? This is already there, or am I missing something ?


I am checking against the current value of port_type variable.
Dont we want to reject role swaps if the port_type is not
TYPEC_PORT_DRP ? My understanding is that if the port type
is fixed at say PORT_TYPE_DFP by userspace, then unless
the userspace sets port_type back to PORT_TYPE_DRP,
role swap requests have to rejected. Is my understanding not
correct ?



Ah yes, the existing check is for port->cap->type. But why not just
replace that check with port->port_type ? Checking both seems overkill.




+   }
+
 ret = sysfs_match_string(typec_data_roles, buf);
 if (ret < 0)
 return ret;
@@ -897,6 +910,12 @@ static ssize_t power_role_store(struct device *dev,
 return -EOPNOTSUPP;
 }
   + if (port->port_type != TYPEC_PORT_DRP) {
+   dev_dbg(dev, "port type fixed at \"%s\"",
+typec_port_types[port->port_type]);
+   return -EIO;



Unrelated change; should be in a separate patch. Also, it should
probably return -EOPNOTSUPP.


similar to what I am doing for data_role_store function.



Not sure here. This is currently treated differently. A host-only
or device-only port was still considered supportable if it supports
power delivery.




+   }
+
 if (port->pwr_opmode != TYPEC_PWR_MODE_PD) {
 dev_dbg(dev, "partner unable to swap power role\n");
 return -EIO;
@@ -926,6 +945,52 @@ static ssize_t 

Re: [PATCH v2] usb: typec: Add a sysfs node to manage port type

[Badhri Jagan Sridharan]

Thanks comments inline.

On Tue, May 23, 2017 at 7:38 PM, Guenter Roeck  wrote:
> On 05/23/2017 06:28 PM, Badhri Jagan Sridharan wrote:
>>
>> User space applications in some cases have the need to enforce a
>> specific port type(DFP/UFP/DRP). This change allows userspace to
>> attempt setting the desired port type. Low level drivers can
>> however reject the request if the specific port type is not supported.
>>
>> Signed-off-by: Badhri Jagan Sridharan 
>> ---
>> Changelog since v1:
>> - introduced a new variable port_type in struct typec_port to track
>>the current port type instead of changing type member in
>>typec_capability to address Heikki Krogerus comments.
>> - changed the output format and strings that would be displayed as
>>suggested by Heikki Krogerus.
>> >   Documentation/ABI/testing/sysfs-class-typec | 13 ++
>>   drivers/usb/typec/typec.c   | 66
>> +
>>   include/linux/usb/typec.h   |  4 ++
>>   3 files changed, 83 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-typec
>> b/Documentation/ABI/testing/sysfs-class-typec
>> index d4a3d23eb09c..1f224c2e391f 100644
>> --- a/Documentation/ABI/testing/sysfs-class-typec
>> +++ b/Documentation/ABI/testing/sysfs-class-typec
>> @@ -73,6 +73,19 @@ Description:
>> Valid values: source, sink, none (to remove preference)
>>   +What:   /sys/class/typec//port_type
>> +Date:   May 2017
>> +Description:
>> +   Indicates the type of the port. This attribute can be used
>> for
>> +   requesting a change in the port type. Port type change is
>> +   supported as a synchronous operation, so write(2) to the
>> +   attribute will not return until the operation has
>> finished.
>> +
>> +   Valid values:
>> +   - source
>> +   - sink
>> +   - dual
>> +
>>   What: /sys/class/typec//supported_accessory_modes
>>   Date: April 2017
>>   Contact:  Heikki Krogerus 
>> diff --git a/drivers/usb/typec/typec.c b/drivers/usb/typec/typec.c
>> index 89e540bb7ff3..5063d6e0f8c7 100644
>> --- a/drivers/usb/typec/typec.c
>> +++ b/drivers/usb/typec/typec.c
>> @@ -69,6 +69,7 @@ struct typec_port {
>> enum typec_role pwr_role;
>> enum typec_role vconn_role;
>> enum typec_pwr_opmode   pwr_opmode;
>> +   enum typec_port_typeport_type;
>
>
> I am missing where this variable is initialized (when the port is registered
> ?).

Yes.. I missed it while cleaning up the patch. Will add it to my next patch.

>
>> const struct typec_capability   *cap;
>>   };
>> @@ -789,6 +790,12 @@ static const char * const typec_data_roles[] = {
>> [TYPEC_HOST]= "host",
>>   };
>>   +static const char * const typec_port_types[] = {
>> +   [TYPEC_PORT_DFP] = "source",
>> +   [TYPEC_PORT_UFP] = "sink",
>> +   [TYPEC_PORT_DRP] = "dual",
>> +};
>> +
>>   static ssize_t
>>   preferred_role_store(struct device *dev, struct device_attribute *attr,
>>  const char *buf, size_t size)
>> @@ -856,6 +863,12 @@ static ssize_t data_role_store(struct device *dev,
>> return -EOPNOTSUPP;
>> }
>>   + if (port->port_type != TYPEC_PORT_DRP) {
>> +   dev_dbg(dev, "port type fixed at \"%s\"",
>> +typec_port_types[port->port_type]);
>> +   return -EIO;
>
>
> ?? This is already there, or am I missing something ?

I am checking against the current value of port_type variable.
Dont we want to reject role swaps if the port_type is not
TYPEC_PORT_DRP ? My understanding is that if the port type
is fixed at say PORT_TYPE_DFP by userspace, then unless
the userspace sets port_type back to PORT_TYPE_DRP,
role swap requests have to rejected. Is my understanding not
correct ?

>
>> +   }
>> +
>> ret = sysfs_match_string(typec_data_roles, buf);
>> if (ret < 0)
>> return ret;
>> @@ -897,6 +910,12 @@ static ssize_t power_role_store(struct device *dev,
>> return -EOPNOTSUPP;
>> }
>>   + if (port->port_type != TYPEC_PORT_DRP) {
>> +   dev_dbg(dev, "port type fixed at \"%s\"",
>> +typec_port_types[port->port_type]);
>> +   return -EIO;
>
>
> Unrelated change; should be in a separate patch. Also, it should
> probably return -EOPNOTSUPP.

similar to what I am doing for data_role_store function.

>
>> +   }
>> +
>> if (port->pwr_opmode != TYPEC_PWR_MODE_PD) {
>> dev_dbg(dev, "partner unable to swap power role\n");
>> return -EIO;
>> @@ -926,6 +945,52 @@ static ssize_t power_role_show(struct device *dev,
>>   }
>>   static DEVICE_ATTR_RW(power_role);
>>   +static ssize_t
>> 

Re: [PATCH v2] usb: typec: Add a sysfs node to manage port type

[Guenter Roeck]

On 05/23/2017 06:28 PM, Badhri Jagan Sridharan wrote:

User space applications in some cases have the need to enforce a
specific port type(DFP/UFP/DRP). This change allows userspace to
attempt setting the desired port type. Low level drivers can
however reject the request if the specific port type is not supported.

Signed-off-by: Badhri Jagan Sridharan 
---
Changelog since v1:
- introduced a new variable port_type in struct typec_port to track
   the current port type instead of changing type member in
   typec_capability to address Heikki Krogerus comments.
- changed the output format and strings that would be displayed as
   suggested by Heikki Krogerus.
>   Documentation/ABI/testing/sysfs-class-typec | 13 ++
  drivers/usb/typec/typec.c   | 66 +
  include/linux/usb/typec.h   |  4 ++
  3 files changed, 83 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-typec 
b/Documentation/ABI/testing/sysfs-class-typec
index d4a3d23eb09c..1f224c2e391f 100644
--- a/Documentation/ABI/testing/sysfs-class-typec
+++ b/Documentation/ABI/testing/sysfs-class-typec
@@ -73,6 +73,19 @@ Description:
  
  		Valid values: source, sink, none (to remove preference)
  
+What:   /sys/class/typec//port_type

+Date:   May 2017
+Description:
+   Indicates the type of the port. This attribute can be used for
+   requesting a change in the port type. Port type change is
+   supported as a synchronous operation, so write(2) to the
+   attribute will not return until the operation has finished.
+
+   Valid values:
+   - source
+   - sink
+   - dual
+
  What: /sys/class/typec//supported_accessory_modes
  Date: April 2017
  Contact:  Heikki Krogerus 
diff --git a/drivers/usb/typec/typec.c b/drivers/usb/typec/typec.c
index 89e540bb7ff3..5063d6e0f8c7 100644
--- a/drivers/usb/typec/typec.c
+++ b/drivers/usb/typec/typec.c
@@ -69,6 +69,7 @@ struct typec_port {
enum typec_role pwr_role;
enum typec_role vconn_role;
enum typec_pwr_opmode   pwr_opmode;
+   enum typec_port_typeport_type;


I am missing where this variable is initialized (when the port is registered ?).

  
  	const struct typec_capability	*cap;

  };
@@ -789,6 +790,12 @@ static const char * const typec_data_roles[] = {
[TYPEC_HOST]= "host",
  };
  
+static const char * const typec_port_types[] = {

+   [TYPEC_PORT_DFP] = "source",
+   [TYPEC_PORT_UFP] = "sink",
+   [TYPEC_PORT_DRP] = "dual",
+};
+
  static ssize_t
  preferred_role_store(struct device *dev, struct device_attribute *attr,
 const char *buf, size_t size)
@@ -856,6 +863,12 @@ static ssize_t data_role_store(struct device *dev,
return -EOPNOTSUPP;
}
  
+	if (port->port_type != TYPEC_PORT_DRP) {

+   dev_dbg(dev, "port type fixed at \"%s\"",
+typec_port_types[port->port_type]);
+   return -EIO;


?? This is already there, or am I missing something ?


+   }
+
ret = sysfs_match_string(typec_data_roles, buf);
if (ret < 0)
return ret;
@@ -897,6 +910,12 @@ static ssize_t power_role_store(struct device *dev,
return -EOPNOTSUPP;
}
  
+	if (port->port_type != TYPEC_PORT_DRP) {

+   dev_dbg(dev, "port type fixed at \"%s\"",
+typec_port_types[port->port_type]);
+   return -EIO;


Unrelated change; should be in a separate patch. Also, it should
probably return -EOPNOTSUPP.


+   }
+
if (port->pwr_opmode != TYPEC_PWR_MODE_PD) {
dev_dbg(dev, "partner unable to swap power role\n");
return -EIO;
@@ -926,6 +945,52 @@ static ssize_t power_role_show(struct device *dev,
  }
  static DEVICE_ATTR_RW(power_role);
  
+static ssize_t

+port_type_store(struct device *dev, struct device_attribute *attr,
+   const char *buf, size_t size)
+{
+   struct typec_port *port = to_typec_port(dev);
+   int ret, type;
+


I think 'type' should be 'enum typec_port_type'.


+   if (!port->cap->port_type_set || port->cap->type != TYPEC_PORT_DRP) {
+   dev_dbg(dev, "changing port type not supported\n");
+   return -EOPNOTSUPP;
+   }
+
+   ret = sysfs_match_string(typec_port_types, buf);
+   if (ret < 0)
+   return ret;
+

If the new type matches the current type, you could return immediately here.


+   type = ret;
+
+   ret = port->cap->port_type_set(port->cap, type);
+   if (ret)
+   return ret;
+
+   port->port_type = type;


We have no locking here, meaning a second request could be processed in 
parallel.
There is no guarantee that the resulting