[PATCH] usb: typec: Add num_altmodes partner attribute

[PATCH] usb: typec: Add num_altmodes partner attribute

[Prashant Malani]

Add a user-visible attribute for the number of alt modes available in a
partner. This allows userspace to determine whether there are any
remaining alt modes left to be registered by the kernel driver. It can
begin implementing any policy state machine after all available alt
modes have been registered with the connector class framework.

This value is set to "-1" initially, which is an invalid value,
signifying that a valid number of alt modes haven't been set for the
partner.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
Cc: Benson Leung <bleung@chromium.org>
---
 drivers/usb/typec/class.c | 34 ++++++++++++++++++++++++++++++++++
 include/linux/usb/typec.h |  1 +
 2 files changed, 35 insertions(+)

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index c9234748537a..680cbcfbd427 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -33,6 +33,7 @@ struct typec_partner {
 	struct usb_pd_identity		*identity;
 	enum typec_accessory		accessory;
 	struct ida			mode_ids;
+	int				num_altmodes;
 };
 
 struct typec_port {
@@ -532,9 +533,18 @@ static ssize_t supports_usb_power_delivery_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(supports_usb_power_delivery);
 
+static ssize_t num_altmodes_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct typec_partner *p = to_typec_partner(dev);
+
+	return sprintf(buf, "%d\n", p->num_altmodes);
+}
+static DEVICE_ATTR_RO(num_altmodes);
+
 static struct attribute *typec_partner_attrs[] = {
 	&dev_attr_accessory_mode.attr,
 	&dev_attr_supports_usb_power_delivery.attr,
+	&dev_attr_num_altmodes.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(typec_partner);
@@ -570,6 +580,29 @@ int typec_partner_set_identity(struct typec_partner *partner)
 }
 EXPORT_SYMBOL_GPL(typec_partner_set_identity);
 
+/**
+ * typec_partner_set_num_altmodes - Update number of available altmodes
+ * @partner: The partner to be updated
+ * @num_alt_modes: The number of altmodes we want to specify as available
+ *
+ * This routine is used to report the number of alternate modes supported by the
+ * partner. This value is *not* enforced in alt mode registration routines.
+ *
+ * @partner.num_altmodes is set to -1 on partner registration, denoting that
+ * a valid value has not been set for it yet.
+ */
+int typec_partner_set_num_altmodes(struct typec_partner *partner, int num_altmodes)
+{
+	if (num_altmodes < 0)
+		return -EINVAL;
+
+	partner->num_altmodes = num_altmodes;
+	sysfs_notify(&partner->dev.kobj, NULL, "num_altmodes");
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(typec_partner_set_num_altmodes);
+
 /**
  * typec_partner_register_altmode - Register USB Type-C Partner Alternate Mode
  * @partner: USB Type-C Partner that supports the alternate mode
@@ -612,6 +645,7 @@ struct typec_partner *typec_register_partner(struct typec_port *port,
 	ida_init(&partner->mode_ids);
 	partner->usb_pd = desc->usb_pd;
 	partner->accessory = desc->accessory;
+	partner->num_altmodes = -1;
 
 	if (desc->identity) {
 		/*
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index 5daa1c49761c..ab523caa23a4 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -112,6 +112,7 @@ struct typec_altmode_desc {
 	enum typec_port_data	roles;
 };
 
+int typec_partner_set_num_altmodes(struct typec_partner *partner, int num_altmodes);
 struct typec_altmode
 *typec_partner_register_altmode(struct typec_partner *partner,
 				const struct typec_altmode_desc *desc);
-- 
2.27.0.212.ge8ba1cc988-goog

Re: [PATCH] usb: typec: Add num_altmodes partner attribute

[Heikki Krogerus]

Hi Prashant,

On Tue, Jun 30, 2020 at 05:31:48PM -0700, Prashant Malani wrote:
> Add a user-visible attribute for the number of alt modes available in a
> partner. This allows userspace to determine whether there are any
> remaining alt modes left to be registered by the kernel driver. It can
> begin implementing any policy state machine after all available alt
> modes have been registered with the connector class framework.
> 
> This value is set to "-1" initially, which is an invalid value,
> signifying that a valid number of alt modes haven't been set for the
> partner.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> Cc: Benson Leung <bleung@chromium.org>
> ---
>  drivers/usb/typec/class.c | 34 ++++++++++++++++++++++++++++++++++
>  include/linux/usb/typec.h |  1 +
>  2 files changed, 35 insertions(+)

You need to add documentation for the file. Add an entry for it to
Documentation/ABI/testing/sysfs-class-typec

> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index c9234748537a..680cbcfbd427 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -33,6 +33,7 @@ struct typec_partner {
>  	struct usb_pd_identity		*identity;
>  	enum typec_accessory		accessory;
>  	struct ida			mode_ids;
> +	int				num_altmodes;
>  };
>  
>  struct typec_port {
> @@ -532,9 +533,18 @@ static ssize_t supports_usb_power_delivery_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(supports_usb_power_delivery);
>  
> +static ssize_t num_altmodes_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct typec_partner *p = to_typec_partner(dev);
> +
> +	return sprintf(buf, "%d\n", p->num_altmodes);
> +}
> +static DEVICE_ATTR_RO(num_altmodes);

This is up to you, but please consider naming the file
"number_of_alternate_modes".

So now user space needs to understand that value -1 means "unknown",
right?

If so, then I think it would probable be better to just hide the file
until the actual number of alternate modes is known.

>  static struct attribute *typec_partner_attrs[] = {
>  	&dev_attr_accessory_mode.attr,
>  	&dev_attr_supports_usb_power_delivery.attr,
> +	&dev_attr_num_altmodes.attr,
>  	NULL
>  };
>  ATTRIBUTE_GROUPS(typec_partner);

-ATTRIBUTE_GROUPS(typec_partner);

static umode_t typec_partner_attr_is_visible(struct kobject *kobj,
                                             struct attribute *attr, int n)
{
        struct typec_partner *partner = to_typec_partner(kobj_to_dev(kobj));

        if (attr == &dev_attr_num_altmodes.attr) {
                if (partner->num_altmodes < 0)
                        return 0;
        }

        return attr->mode;
}

static struct attribute_group typec_partner_group = {
        .is_visible = typec_partner_attr_is_visible,
        .attrs = typec_partner_attrs
};

static const struct attribute_group *typec_partner_groups[] = {
        &typec_partner_group,
        NULL
};

> @@ -570,6 +580,29 @@ int typec_partner_set_identity(struct typec_partner *partner)
>  }
>  EXPORT_SYMBOL_GPL(typec_partner_set_identity);
>  
> +/**
> + * typec_partner_set_num_altmodes - Update number of available altmodes
> + * @partner: The partner to be updated
> + * @num_alt_modes: The number of altmodes we want to specify as available
> + *
> + * This routine is used to report the number of alternate modes supported by the
> + * partner. This value is *not* enforced in alt mode registration routines.
> + *
> + * @partner.num_altmodes is set to -1 on partner registration, denoting that
> + * a valid value has not been set for it yet.
> + */
> +int typec_partner_set_num_altmodes(struct typec_partner *partner, int num_altmodes)
> +{
> +	if (num_altmodes < 0)
> +		return -EINVAL;
> +
> +	partner->num_altmodes = num_altmodes;
> +	sysfs_notify(&partner->dev.kobj, NULL, "num_altmodes");
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(typec_partner_set_num_altmodes);
> +
>  /**
>   * typec_partner_register_altmode - Register USB Type-C Partner Alternate Mode
>   * @partner: USB Type-C Partner that supports the alternate mode
> @@ -612,6 +645,7 @@ struct typec_partner *typec_register_partner(struct typec_port *port,
>  	ida_init(&partner->mode_ids);
>  	partner->usb_pd = desc->usb_pd;
>  	partner->accessory = desc->accessory;
> +	partner->num_altmodes = -1;
>  
>  	if (desc->identity) {
>  		/*
> diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
> index 5daa1c49761c..ab523caa23a4 100644
> --- a/include/linux/usb/typec.h
> +++ b/include/linux/usb/typec.h
> @@ -112,6 +112,7 @@ struct typec_altmode_desc {
>  	enum typec_port_data	roles;
>  };
>  
> +int typec_partner_set_num_altmodes(struct typec_partner *partner, int num_altmodes);
>  struct typec_altmode
>  *typec_partner_register_altmode(struct typec_partner *partner,
>  				const struct typec_altmode_desc *desc);

thanks,

-- 
heikki

Re: [PATCH] usb: typec: Add num_altmodes partner attribute

[Greg Kroah-Hartman]

On Tue, Jun 30, 2020 at 05:31:48PM -0700, Prashant Malani wrote:
> Add a user-visible attribute for the number of alt modes available in a
> partner. This allows userspace to determine whether there are any
> remaining alt modes left to be registered by the kernel driver. It can
> begin implementing any policy state machine after all available alt
> modes have been registered with the connector class framework.
> 
> This value is set to "-1" initially, which is an invalid value,
> signifying that a valid number of alt modes haven't been set for the
> partner.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> Cc: Benson Leung <bleung@chromium.org>
> ---
>  drivers/usb/typec/class.c | 34 ++++++++++++++++++++++++++++++++++
>  include/linux/usb/typec.h |  1 +
>  2 files changed, 35 insertions(+)
> 
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index c9234748537a..680cbcfbd427 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -33,6 +33,7 @@ struct typec_partner {
>  	struct usb_pd_identity		*identity;
>  	enum typec_accessory		accessory;
>  	struct ida			mode_ids;
> +	int				num_altmodes;
>  };
>  
>  struct typec_port {
> @@ -532,9 +533,18 @@ static ssize_t supports_usb_power_delivery_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(supports_usb_power_delivery);
>  
> +static ssize_t num_altmodes_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct typec_partner *p = to_typec_partner(dev);
> +
> +	return sprintf(buf, "%d\n", p->num_altmodes);
> +}
> +static DEVICE_ATTR_RO(num_altmodes);
> +
>  static struct attribute *typec_partner_attrs[] = {
>  	&dev_attr_accessory_mode.attr,
>  	&dev_attr_supports_usb_power_delivery.attr,
> +	&dev_attr_num_altmodes.attr,
>  	NULL
>  };
>  ATTRIBUTE_GROUPS(typec_partner);
> @@ -570,6 +580,29 @@ int typec_partner_set_identity(struct typec_partner *partner)
>  }
>  EXPORT_SYMBOL_GPL(typec_partner_set_identity);
>  
> +/**
> + * typec_partner_set_num_altmodes - Update number of available altmodes
> + * @partner: The partner to be updated
> + * @num_alt_modes: The number of altmodes we want to specify as available
> + *
> + * This routine is used to report the number of alternate modes supported by the
> + * partner. This value is *not* enforced in alt mode registration routines.
> + *
> + * @partner.num_altmodes is set to -1 on partner registration, denoting that
> + * a valid value has not been set for it yet.
> + */
> +int typec_partner_set_num_altmodes(struct typec_partner *partner, int num_altmodes)
> +{
> +	if (num_altmodes < 0)
> +		return -EINVAL;
> +
> +	partner->num_altmodes = num_altmodes;
> +	sysfs_notify(&partner->dev.kobj, NULL, "num_altmodes");
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(typec_partner_set_num_altmodes);

Who is calling this?  We need a user before we can add a newly exported
function.

thanks,

greg k-h

Re: [PATCH] usb: typec: Add num_altmodes partner attribute

[Prashant Malani]

Hi Greg,

Thanks a lot for looking at the patch.

(trimming text)
On Wed, Jul 01, 2020 at 02:01:31PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Jun 30, 2020 at 05:31:48PM -0700, Prashant Malani wrote:
> > Add a user-visible attribute for the number of alt modes available in a
> > partner. This allows userspace to determine whether there are any
> > remaining alt modes left to be registered by the kernel driver. It can
> > begin implementing any policy state machine after all available alt
> > modes have been registered with the connector class framework.
> > 
> > This value is set to "-1" initially, which is an invalid value,
> > signifying that a valid number of alt modes haven't been set for the
> > partner.
> > 
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > Cc: Benson Leung <bleung@chromium.org>
> > + */
> > +int typec_partner_set_num_altmodes(struct typec_partner *partner, int num_altmodes)
> > +{
> > +	if (num_altmodes < 0)
> > +		return -EINVAL;
> > +
> > +	partner->num_altmodes = num_altmodes;
> > +	sysfs_notify(&partner->dev.kobj, NULL, "num_altmodes");
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(typec_partner_set_num_altmodes);
> 
> Who is calling this?  We need a user before we can add a newly exported
> function.

cros-ec-typec (drivers/platform/chrome/cros_ec_typec.c), but the patch for that
isn't ready yet. I will submit v2 when that patch is ready for review.

My apologies for not considering this earlier.

Best regards,

> 
> thanks,
> 
> greg k-h

Re: [PATCH] usb: typec: Add num_altmodes partner attribute

[Prashant Malani]

Hi Heikki,

Thanks a lot for reviewing the patch.

On Wed, Jul 01, 2020 at 11:22:30AM +0300, Heikki Krogerus wrote:
> Hi Prashant,
> 
> On Tue, Jun 30, 2020 at 05:31:48PM -0700, Prashant Malani wrote:
> > Add a user-visible attribute for the number of alt modes available in a
> > partner. This allows userspace to determine whether there are any
> > remaining alt modes left to be registered by the kernel driver. It can
> > begin implementing any policy state machine after all available alt
> > modes have been registered with the connector class framework.
> > 
> > This value is set to "-1" initially, which is an invalid value,
> > signifying that a valid number of alt modes haven't been set for the
> > partner.
> > 
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > Cc: Benson Leung <bleung@chromium.org>
> > ---
> >  drivers/usb/typec/class.c | 34 ++++++++++++++++++++++++++++++++++
> >  include/linux/usb/typec.h |  1 +
> >  2 files changed, 35 insertions(+)
> 
> You need to add documentation for the file. Add an entry for it to
> Documentation/ABI/testing/sysfs-class-typec
> 

Ack.
Will add this in v2.
> > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > index c9234748537a..680cbcfbd427 100644
> > --- a/drivers/usb/typec/class.c
> > +++ b/drivers/usb/typec/class.c
> > @@ -33,6 +33,7 @@ struct typec_partner {
> >  	struct usb_pd_identity		*identity;
> >  	enum typec_accessory		accessory;
> >  	struct ida			mode_ids;
> > +	int				num_altmodes;
> >  };
> >  
> >  struct typec_port {
> > @@ -532,9 +533,18 @@ static ssize_t supports_usb_power_delivery_show(struct device *dev,
> >  }
> >  static DEVICE_ATTR_RO(supports_usb_power_delivery);
> >  
> > +static ssize_t num_altmodes_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +	struct typec_partner *p = to_typec_partner(dev);
> > +
> > +	return sprintf(buf, "%d\n", p->num_altmodes);
> > +}
> > +static DEVICE_ATTR_RO(num_altmodes);
> 
> This is up to you, but please consider naming the file
> "number_of_alternate_modes".
> 
Ack. Will update the name in v2.

> So now user space needs to understand that value -1 means "unknown",
> right?
> 
> If so, then I think it would probable be better to just hide the file
> until the actual number of alternate modes is known.
> 

SGTM.
> >  static struct attribute *typec_partner_attrs[] = {
> >  	&dev_attr_accessory_mode.attr,
> >  	&dev_attr_supports_usb_power_delivery.attr,
> > +	&dev_attr_num_altmodes.attr,
> >  	NULL
> >  };
> >  ATTRIBUTE_GROUPS(typec_partner);
> 
> -ATTRIBUTE_GROUPS(typec_partner);
> 
> static umode_t typec_partner_attr_is_visible(struct kobject *kobj,
>                                              struct attribute *attr, int n)
> {
>         struct typec_partner *partner = to_typec_partner(kobj_to_dev(kobj));
> 
>         if (attr == &dev_attr_num_altmodes.attr) {
>                 if (partner->num_altmodes < 0)
>                         return 0;
>         }
> 
>         return attr->mode;
> }
> 
> static struct attribute_group typec_partner_group = {
>         .is_visible = typec_partner_attr_is_visible,
>         .attrs = typec_partner_attrs
> };
> 
> static const struct attribute_group *typec_partner_groups[] = {
>         &typec_partner_group,
>         NULL
> };
> 

Nice, this is cool. Will make the modification in v2.

Best regards,

> > @@ -570,6 +580,29 @@ int typec_partner_set_identity(struct typec_partner *partner)
> >  }
> >  EXPORT_SYMBOL_GPL(typec_partner_set_identity);
> >  
> > +/**
> > + * typec_partner_set_num_altmodes - Update number of available altmodes
> > + * @partner: The partner to be updated
> > + * @num_alt_modes: The number of altmodes we want to specify as available
> > + *
> > + * This routine is used to report the number of alternate modes supported by the
> > + * partner. This value is *not* enforced in alt mode registration routines.
> > + *
> > + * @partner.num_altmodes is set to -1 on partner registration, denoting that
> > + * a valid value has not been set for it yet.
> > + */
> > +int typec_partner_set_num_altmodes(struct typec_partner *partner, int num_altmodes)
> > +{
> > +	if (num_altmodes < 0)
> > +		return -EINVAL;
> > +
> > +	partner->num_altmodes = num_altmodes;
> > +	sysfs_notify(&partner->dev.kobj, NULL, "num_altmodes");
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(typec_partner_set_num_altmodes);
> > +
> >  /**
> >   * typec_partner_register_altmode - Register USB Type-C Partner Alternate Mode
> >   * @partner: USB Type-C Partner that supports the alternate mode
> > @@ -612,6 +645,7 @@ struct typec_partner *typec_register_partner(struct typec_port *port,
> >  	ida_init(&partner->mode_ids);
> >  	partner->usb_pd = desc->usb_pd;
> >  	partner->accessory = desc->accessory;
> > +	partner->num_altmodes = -1;
> >  
> >  	if (desc->identity) {
> >  		/*
> > diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
> > index 5daa1c49761c..ab523caa23a4 100644
> > --- a/include/linux/usb/typec.h
> > +++ b/include/linux/usb/typec.h
> > @@ -112,6 +112,7 @@ struct typec_altmode_desc {
> >  	enum typec_port_data	roles;
> >  };
> >  
> > +int typec_partner_set_num_altmodes(struct typec_partner *partner, int num_altmodes);
> >  struct typec_altmode
> >  *typec_partner_register_altmode(struct typec_partner *partner,
> >  				const struct typec_altmode_desc *desc);
> 
> thanks,
> 
> -- 
> heikki