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

[Badhri Jagan Sridharan]

On Fri, May 26, 2017 at 11:31 AM, Greg Kroah-Hartman
 wrote:
> On Fri, May 26, 2017 at 10:45:59AM -0700, 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 
>> Change-Id: Ia76877558c47271a34d912a54eea3459655dda3c
>
> Why is this line here?  :)
Oops default commit hook for internal change tracking.
Removing in my next patch :)

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

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

[Badhri Jagan Sridharan]

On Fri, May 26, 2017 at 11:00 AM, Guenter Roeck  wrote:
> On Fri, May 26, 2017 at 10:45:59AM -0700, 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 
>> Change-Id: Ia76877558c47271a34d912a54eea3459655dda3c
>> ---
>> 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.
>>
>> Changelog since v2:
>> - introduced a new mutex lock to protect port_type for addressing
>>   the race conditions identified by Geunter Roeck
>> - added typec_port_types_drp for printing port_type when cap->type
>>   is TYPE_PORT_DRP as suggested by Geunter Roeck
>> - Power role swap and data role swaps would be rejected unless
>>   port port_type == TYPE_PORT_DRP
>> - port_type_store would return immediately if the current port_type
>>   is same as the new port_type as suggested by Geunter Roeck
>>
>>  drivers/usb/typec/typec.c | 127 
>> --
>>  include/linux/usb/typec.h |   4 ++
>>  2 files changed, 116 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/usb/typec/typec.c b/drivers/usb/typec/typec.c
>> index 89e540bb7ff3..a0d8887ad560 100644
>> --- a/drivers/usb/typec/typec.c
>> +++ b/drivers/usb/typec/typec.c
>> @@ -11,9 +11,11 @@
>>
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>  #include 
>>
>> +
>
> Single empty lines only, please.
>
>>  struct typec_mode {
>>   int index;
>>   u32 vdo;
>> @@ -69,6 +71,8 @@ struct typec_port {
>>   enum typec_role pwr_role;
>>   enum typec_role vconn_role;
>>   enum typec_pwr_opmode   pwr_opmode;
>> + enum typec_port_typeport_type;
>> + struct mutexport_type_lock;
>>
>>   const struct typec_capability   *cap;
>>  };
>> @@ -789,6 +793,19 @@ 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 const char * const typec_port_types_drp[] = {
>> + [TYPEC_PORT_DFP] = "dual [source] sink",
>> + [TYPEC_PORT_UFP] = "dual source [sink]",
>> + [TYPEC_PORT_DRP] = "[dual] source sink",
>> +};
>> +
>> +
> Single empty line.
>
>>  static ssize_t
>>  preferred_role_store(struct device *dev, struct device_attribute *attr,
>>const char *buf, size_t size)
>> @@ -846,25 +863,34 @@ static ssize_t data_role_store(struct device *dev,
>>   struct typec_port *port = to_typec_port(dev);
>>   int ret;
>>
>> - if (port->cap->type != TYPEC_PORT_DRP) {
>> - dev_dbg(dev, "data role swap only supported with DRP ports\n");
>> - return -EOPNOTSUPP;
>> - }
>> + mutex_lock(>port_type_lock);
>>
>>   if (!port->cap->dr_set) {
>>   dev_dbg(dev, "data role swapping not supported\n");
>> - return -EOPNOTSUPP;
>> + ret = -EOPNOTSUPP;
>> + goto unlock_and_ret;
>> + }
>
> I think you can check this outside the lock; capabilities won't change.
>
>> +
>> + if (port->port_type != TYPEC_PORT_DRP) {
>> + dev_dbg(dev, "port type fixed at \"%s\"",
>> +  typec_port_types[port->port_type]);
>> + ret = -EOPNOTSUPP;
>> + goto unlock_and_ret;
>>   }
>>
>>   ret = sysfs_match_string(typec_data_roles, buf);
>>   if (ret < 0)
>> - return ret;
>> + goto unlock_and_ret;
>
> Move outside the lock ?
>
>>
>>   ret = port->cap->dr_set(port->cap, ret);
>>   if (ret)
>> - return ret;
>> + goto unlock_and_ret;
>>
>> - return size;
>> + ret = size;
>> +
>> +unlock_and_ret:
>> + mutex_unlock(>port_type_lock);
>> + return ret;
>>  }
>>
>>  static ssize_t data_role_show(struct device *dev,
>> @@ -885,32 +911,47 @@ static ssize_t power_role_store(struct device *dev,
>>   const char *buf, size_t size)
>>  {
>>   struct typec_port *port = to_typec_port(dev);
>> - int ret = size;
>> + int ret;
>>
>> + mutex_lock(>port_type_lock);
>>   if (!port->cap->pd_revision) {
>>   dev_dbg(dev, "USB Power Delivery not supported\n");
>> - return -EOPNOTSUPP;
>> + ret = -EOPNOTSUPP;
>> + 

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

[Greg Kroah-Hartman]

On Fri, May 26, 2017 at 10:45:59AM -0700, 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 
> Change-Id: Ia76877558c47271a34d912a54eea3459655dda3c

Why is this line here?  :)

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

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

[Guenter Roeck]

On Fri, May 26, 2017 at 10:45:59AM -0700, 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 
> Change-Id: Ia76877558c47271a34d912a54eea3459655dda3c
> ---
> 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.
> 
> Changelog since v2:
> - introduced a new mutex lock to protect port_type for addressing
>   the race conditions identified by Geunter Roeck
> - added typec_port_types_drp for printing port_type when cap->type
>   is TYPE_PORT_DRP as suggested by Geunter Roeck
> - Power role swap and data role swaps would be rejected unless
>   port port_type == TYPE_PORT_DRP
> - port_type_store would return immediately if the current port_type
>   is same as the new port_type as suggested by Geunter Roeck
> 
>  drivers/usb/typec/typec.c | 127 
> --
>  include/linux/usb/typec.h |   4 ++
>  2 files changed, 116 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/usb/typec/typec.c b/drivers/usb/typec/typec.c
> index 89e540bb7ff3..a0d8887ad560 100644
> --- a/drivers/usb/typec/typec.c
> +++ b/drivers/usb/typec/typec.c
> @@ -11,9 +11,11 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> +

Single empty lines only, please.

>  struct typec_mode {
>   int index;
>   u32 vdo;
> @@ -69,6 +71,8 @@ struct typec_port {
>   enum typec_role pwr_role;
>   enum typec_role vconn_role;
>   enum typec_pwr_opmode   pwr_opmode;
> + enum typec_port_typeport_type;
> + struct mutexport_type_lock;
>  
>   const struct typec_capability   *cap;
>  };
> @@ -789,6 +793,19 @@ 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 const char * const typec_port_types_drp[] = {
> + [TYPEC_PORT_DFP] = "dual [source] sink",
> + [TYPEC_PORT_UFP] = "dual source [sink]",
> + [TYPEC_PORT_DRP] = "[dual] source sink",
> +};
> +
> +
Single empty line.

>  static ssize_t
>  preferred_role_store(struct device *dev, struct device_attribute *attr,
>const char *buf, size_t size)
> @@ -846,25 +863,34 @@ static ssize_t data_role_store(struct device *dev,
>   struct typec_port *port = to_typec_port(dev);
>   int ret;
>  
> - if (port->cap->type != TYPEC_PORT_DRP) {
> - dev_dbg(dev, "data role swap only supported with DRP ports\n");
> - return -EOPNOTSUPP;
> - }
> + mutex_lock(>port_type_lock);
>  
>   if (!port->cap->dr_set) {
>   dev_dbg(dev, "data role swapping not supported\n");
> - return -EOPNOTSUPP;
> + ret = -EOPNOTSUPP;
> + goto unlock_and_ret;
> + }

I think you can check this outside the lock; capabilities won't change.

> +
> + if (port->port_type != TYPEC_PORT_DRP) {
> + dev_dbg(dev, "port type fixed at \"%s\"",
> +  typec_port_types[port->port_type]);
> + ret = -EOPNOTSUPP;
> + goto unlock_and_ret;
>   }
>  
>   ret = sysfs_match_string(typec_data_roles, buf);
>   if (ret < 0)
> - return ret;
> + goto unlock_and_ret;

Move outside the lock ?

>  
>   ret = port->cap->dr_set(port->cap, ret);
>   if (ret)
> - return ret;
> + goto unlock_and_ret;
>  
> - return size;
> + ret = size;
> +
> +unlock_and_ret:
> + mutex_unlock(>port_type_lock);
> + return ret;
>  }
>  
>  static ssize_t data_role_show(struct device *dev,
> @@ -885,32 +911,47 @@ static ssize_t power_role_store(struct device *dev,
>   const char *buf, size_t size)
>  {
>   struct typec_port *port = to_typec_port(dev);
> - int ret = size;
> + int ret;
>  
> + mutex_lock(>port_type_lock);
>   if (!port->cap->pd_revision) {
>   dev_dbg(dev, "USB Power Delivery not supported\n");
> - return -EOPNOTSUPP;
> + ret = -EOPNOTSUPP;
> + goto unlock_and_ret;
>   }
>  
>   if (!port->cap->pr_set) {
>   dev_dbg(dev, "power role swapping not supported\n");
> - return -EOPNOTSUPP;
> + ret = 

[PATCHv3] usb: typec: Add a sysfs node to manage port type

[Badhri Jagan Sridharan]

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 
Change-Id: Ia76877558c47271a34d912a54eea3459655dda3c
---
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.

Changelog since v2:
- introduced a new mutex lock to protect port_type for addressing
  the race conditions identified by Geunter Roeck
- added typec_port_types_drp for printing port_type when cap->type
  is TYPE_PORT_DRP as suggested by Geunter Roeck
- Power role swap and data role swaps would be rejected unless
  port port_type == TYPE_PORT_DRP
- port_type_store would return immediately if the current port_type
  is same as the new port_type as suggested by Geunter Roeck

 drivers/usb/typec/typec.c | 127 --
 include/linux/usb/typec.h |   4 ++
 2 files changed, 116 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/typec/typec.c b/drivers/usb/typec/typec.c
index 89e540bb7ff3..a0d8887ad560 100644
--- a/drivers/usb/typec/typec.c
+++ b/drivers/usb/typec/typec.c
@@ -11,9 +11,11 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 
+
 struct typec_mode {
int index;
u32 vdo;
@@ -69,6 +71,8 @@ struct typec_port {
enum typec_role pwr_role;
enum typec_role vconn_role;
enum typec_pwr_opmode   pwr_opmode;
+   enum typec_port_typeport_type;
+   struct mutexport_type_lock;
 
const struct typec_capability   *cap;
 };
@@ -789,6 +793,19 @@ 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 const char * const typec_port_types_drp[] = {
+   [TYPEC_PORT_DFP] = "dual [source] sink",
+   [TYPEC_PORT_UFP] = "dual source [sink]",
+   [TYPEC_PORT_DRP] = "[dual] source sink",
+};
+
+
 static ssize_t
 preferred_role_store(struct device *dev, struct device_attribute *attr,
 const char *buf, size_t size)
@@ -846,25 +863,34 @@ static ssize_t data_role_store(struct device *dev,
struct typec_port *port = to_typec_port(dev);
int ret;
 
-   if (port->cap->type != TYPEC_PORT_DRP) {
-   dev_dbg(dev, "data role swap only supported with DRP ports\n");
-   return -EOPNOTSUPP;
-   }
+   mutex_lock(>port_type_lock);
 
if (!port->cap->dr_set) {
dev_dbg(dev, "data role swapping not supported\n");
-   return -EOPNOTSUPP;
+   ret = -EOPNOTSUPP;
+   goto unlock_and_ret;
+   }
+
+   if (port->port_type != TYPEC_PORT_DRP) {
+   dev_dbg(dev, "port type fixed at \"%s\"",
+typec_port_types[port->port_type]);
+   ret = -EOPNOTSUPP;
+   goto unlock_and_ret;
}
 
ret = sysfs_match_string(typec_data_roles, buf);
if (ret < 0)
-   return ret;
+   goto unlock_and_ret;
 
ret = port->cap->dr_set(port->cap, ret);
if (ret)
-   return ret;
+   goto unlock_and_ret;
 
-   return size;
+   ret = size;
+
+unlock_and_ret:
+   mutex_unlock(>port_type_lock);
+   return ret;
 }
 
 static ssize_t data_role_show(struct device *dev,
@@ -885,32 +911,47 @@ static ssize_t power_role_store(struct device *dev,
const char *buf, size_t size)
 {
struct typec_port *port = to_typec_port(dev);
-   int ret = size;
+   int ret;
 
+   mutex_lock(>port_type_lock);
if (!port->cap->pd_revision) {
dev_dbg(dev, "USB Power Delivery not supported\n");
-   return -EOPNOTSUPP;
+   ret = -EOPNOTSUPP;
+   goto unlock_and_ret;
}
 
if (!port->cap->pr_set) {
dev_dbg(dev, "power role swapping not supported\n");
-   return -EOPNOTSUPP;
+   ret = -EOPNOTSUPP;
+   goto unlock_and_ret;
+   }
+
+   if (port->port_type != TYPEC_PORT_DRP) {
+   dev_dbg(dev, "port type fixed at \"%s\"",
+typec_port_types[port->port_type]);
+   ret = -EOPNOTSUPP;
+   goto unlock_and_ret;
}
 
if (port->pwr_opmode != TYPEC_PWR_MODE_PD) {