| Message ID | 20201022061554.3418060-1-pmalani@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] usb: typec: Expose Product Type VDOs via sysfs | expand |
On Wed, Oct 21, 2020 at 11:15:54PM -0700, Prashant Malani wrote: > A PD-capable device can return up to 3 Product Type VDOs as part of its > DiscoverIdentity Response (USB PD Spec, Rev 3.0, Version 2.0, Section > 6.4.4.3.1). Add a sysfs attribute to expose these to userspace. > > Cc: Benson Leung <bleung@chromium.org> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Signed-off-by: Prashant Malani <pmalani@chromium.org> > --- > > NOTE: I didn't include Benson's Reviewed-by from v1, since this version > introduced the sysfs_notify() call. > > Changes in v2: > - Added sysfs_notify() call for the attribute. > - Added description for the attribute in > Documentation/ABI/testing/sysfs-class-typec. > > Documentation/ABI/testing/sysfs-class-typec | 17 +++++++++++++++++ > drivers/usb/typec/class.c | 11 +++++++++++ > 2 files changed, 28 insertions(+) > > diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec > index b834671522d6..16440a236b66 100644 > --- a/Documentation/ABI/testing/sysfs-class-typec > +++ b/Documentation/ABI/testing/sysfs-class-typec > @@ -170,6 +170,14 @@ Description: > will show 0 until Discover Identity command result becomes > available. The value can be polled. > > +What: /sys/class/typec/<port>-partner/identity/product_type_vdo > +Date: October 2020 > +Contact: Prashant Malani <pmalani@chromium.org> > +Description: > + Product Type VDOs part of Discover Identity command result. 3 values > + are displayed (for the 3 possible Product Type VDOs), one per line. sysfs is "one value per file", not "one value per line". This is not ok. > + The values will show 0s until Discover Identity command result becomes > + available. The values can be polled. It can be polled? Did you try that? I don't see the logic for that in your patch. > > USB Type-C cable devices (eg. /sys/class/typec/port0-cable/) > > @@ -230,6 +238,15 @@ Description: > will show 0 until Discover Identity command result becomes > available. The value can be polled. > > +What: /sys/class/typec/<port>-cable/identity/product_type_vdo > +Date: October 2020 > +Contact: Prashant Malani <pmalani@chromium.org> > +Description: > + Product Type VDOs part of Discover Identity command result. 3 values > + are displayed (for the 3 possible Product Type VDOs), one per line. > + The values will show 0s until Discover Identity command result becomes > + available. The values can be polled. Why are you describing the same value in two different locations? > + > > USB Type-C port alternate mode devices. > > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c > index 35eec707cb51..37fa4501e75f 100644 > --- a/drivers/usb/typec/class.c > +++ b/drivers/usb/typec/class.c > @@ -122,10 +122,20 @@ static ssize_t product_show(struct device *dev, struct device_attribute *attr, > } > static DEVICE_ATTR_RO(product); > > +static ssize_t product_type_vdo_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + struct usb_pd_identity *id = get_pd_identity(dev); > + > + return sprintf(buf, "0x%08x\n0x%08x\n0x%08x\n", id->vdo[0], id->vdo[1], id->vdo[2]); Note, for future sysfs stuff, always use sysfs_emit(). But again, this is not allowed as you have multiple values per a single file. thanks, greg k-h
Thanks for reviewing the patch, Greg. On Wed, Oct 21, 2020 at 11:56 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Wed, Oct 21, 2020 at 11:15:54PM -0700, Prashant Malani wrote: > > > > diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec > > index b834671522d6..16440a236b66 100644 > > --- a/Documentation/ABI/testing/sysfs-class-typec > > +++ b/Documentation/ABI/testing/sysfs-class-typec > > @@ -170,6 +170,14 @@ Description: > > will show 0 until Discover Identity command result becomes > > available. The value can be polled. > > > > +What: /sys/class/typec/<port>-partner/identity/product_type_vdo > > +Date: October 2020 > > +Contact: Prashant Malani <pmalani@chromium.org> > > +Description: > > + Product Type VDOs part of Discover Identity command result. 3 values > > + are displayed (for the 3 possible Product Type VDOs), one per line. > > sysfs is "one value per file", not "one value per line". This is not > ok. I see. Would listing these out as three separate vdos (i.e vdo0, vdo1, vdo2) be better? > > > + The values will show 0s until Discover Identity command result becomes > > + available. The values can be polled. > > It can be polled? Did you try that? I don't see the logic for that in > your patch. To be honest, no. I followed the pattern used by the other identity VDOs (cert_stat, id_header and product), and re-used their description assuming it to be accurate. > > > > > > USB Type-C cable devices (eg. /sys/class/typec/port0-cable/) > > > > @@ -230,6 +238,15 @@ Description: > > will show 0 until Discover Identity command result becomes > > available. The value can be polled. > > > > +What: /sys/class/typec/<port>-cable/identity/product_type_vdo > > +Date: October 2020 > > +Contact: Prashant Malani <pmalani@chromium.org> > > +Description: > > + Product Type VDOs part of Discover Identity command result. 3 values > > + are displayed (for the 3 possible Product Type VDOs), one per line. > > + The values will show 0s until Discover Identity command result becomes > > + available. The values can be polled. > > Why are you describing the same value in two different locations? Both cable and partner can have Discover Identity VDOs and they are listed separately in sysfs. The other VDOs (id_header, cert_stat and product) have separate descriptions for cable and partner too. Perhaps there is a case for consolidating the listings here (factor out the ones which are common to cable and partner). > > > + > > > > USB Type-C port alternate mode devices. > > > > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c > > index 35eec707cb51..37fa4501e75f 100644 > > --- a/drivers/usb/typec/class.c > > +++ b/drivers/usb/typec/class.c > > @@ -122,10 +122,20 @@ static ssize_t product_show(struct device *dev, struct device_attribute *attr, > > } > > static DEVICE_ATTR_RO(product); > > > > +static ssize_t product_type_vdo_show(struct device *dev, struct device_attribute *attr, > > + char *buf) > > +{ > > + struct usb_pd_identity *id = get_pd_identity(dev); > > + > > + return sprintf(buf, "0x%08x\n0x%08x\n0x%08x\n", id->vdo[0], id->vdo[1], id->vdo[2]); > > Note, for future sysfs stuff, always use sysfs_emit(). > > But again, this is not allowed as you have multiple values per a single > file. Noted. I will keep that in mind for future versions. Best regards, > > thanks, > > greg k-h
On Thu, Oct 22, 2020 at 12:13:54AM -0700, Prashant Malani wrote: > Thanks for reviewing the patch, Greg. > > On Wed, Oct 21, 2020 at 11:56 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Wed, Oct 21, 2020 at 11:15:54PM -0700, Prashant Malani wrote: > > > > > > diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec > > > index b834671522d6..16440a236b66 100644 > > > --- a/Documentation/ABI/testing/sysfs-class-typec > > > +++ b/Documentation/ABI/testing/sysfs-class-typec > > > @@ -170,6 +170,14 @@ Description: > > > will show 0 until Discover Identity command result becomes > > > available. The value can be polled. > > > > > > +What: /sys/class/typec/<port>-partner/identity/product_type_vdo > > > +Date: October 2020 > > > +Contact: Prashant Malani <pmalani@chromium.org> > > > +Description: > > > + Product Type VDOs part of Discover Identity command result. 3 values > > > + are displayed (for the 3 possible Product Type VDOs), one per line. > > > > sysfs is "one value per file", not "one value per line". This is not > > ok. > > I see. Would listing these out as three separate vdos (i.e vdo0, vdo1, > vdo2) be better? Given that your current implementation is not acceptable, something has to change :) thanks, greg k-h
Hi Greg, On Thu, Oct 22, 2020 at 12:17 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > +What: /sys/class/typec/<port>-partner/identity/product_type_vdo > > > > +Date: October 2020 > > > > +Contact: Prashant Malani <pmalani@chromium.org> > > > > +Description: > > > > + Product Type VDOs part of Discover Identity command result. 3 values > > > > + are displayed (for the 3 possible Product Type VDOs), one per line. > > > > > > sysfs is "one value per file", not "one value per line". This is not > > > ok. > > > > I see. Would listing these out as three separate vdos (i.e vdo0, vdo1, > > vdo2) be better? > > Given that your current implementation is not acceptable, something has > to change :) Got it. I'd like to see if Heikki has any suggestions on naming these entries better. Thanks again and best regards, -Prashant > > thanks, > > greg k-h
On Thu, Oct 22, 2020 at 12:25:07AM -0700, Prashant Malani wrote: > Hi Greg, > > On Thu, Oct 22, 2020 at 12:17 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > > +What: /sys/class/typec/<port>-partner/identity/product_type_vdo > > > > > +Date: October 2020 > > > > > +Contact: Prashant Malani <pmalani@chromium.org> > > > > > +Description: > > > > > + Product Type VDOs part of Discover Identity command result. 3 values > > > > > + are displayed (for the 3 possible Product Type VDOs), one per line. > > > > > > > > sysfs is "one value per file", not "one value per line". This is not > > > > ok. > > > > > > I see. Would listing these out as three separate vdos (i.e vdo0, vdo1, > > > vdo2) be better? > > > > Given that your current implementation is not acceptable, something has > > to change :) > > Got it. I'd like to see if Heikki has any suggestions on naming these > entries better. Why not have product type specific attribute files? So if the partner is UFP, then we expose ufp1 and ufp2 files that return the UFP1 and UFP2 VDO values and hide the other files: % ls /sys/class/typec/port0-partner/identity/ id_header cert_stat product ufp1 ufp2 If the partner is DFP, then you expose the dfp file and hide everything else: % ls /sys/class/typec/port0-partner/identity/ id_header cert_stat product dfp And so on. thanks, -- heikki
Hi Heikki, Thanks for your feedback. On Thu, Oct 22, 2020 at 5:43 AM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > On Thu, Oct 22, 2020 at 12:25:07AM -0700, Prashant Malani wrote: > > Hi Greg, > > > > On Thu, Oct 22, 2020 at 12:17 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > Given that your current implementation is not acceptable, something has > > > to change :) > > > > Got it. I'd like to see if Heikki has any suggestions on naming these > > entries better. > > Why not have product type specific attribute files? > > So if the partner is UFP, then we expose ufp1 and ufp2 files that > return the UFP1 and UFP2 VDO values and hide the other files: > > % ls /sys/class/typec/port0-partner/identity/ > id_header cert_stat product ufp1 ufp2 > > If the partner is DFP, then you expose the dfp file and hide > everything else: > > % ls /sys/class/typec/port0-partner/identity/ > id_header cert_stat product dfp > > And so on. > Makes sense, thanks! The only query I have here is , does the kernel *need* to implement this logic? Userspace can read id_header VDO and figure this on its own (parse the Product Type specific VDOs accordingly). Apart from that, I can work on implementing this if there are no concerns. Best regards, > thanks, > > -- > heikki
Hi Heikki, On Thu, Oct 22, 2020 at 5:43 AM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > On Thu, Oct 22, 2020 at 12:25:07AM -0700, Prashant Malani wrote: > > Hi Greg, > > > > On Thu, Oct 22, 2020 at 12:17 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > > > > +What: /sys/class/typec/<port>-partner/identity/product_type_vdo > > > > > > +Date: October 2020 > > > > > > +Contact: Prashant Malani <pmalani@chromium.org> > > > > > > +Description: > > > > > > + Product Type VDOs part of Discover Identity command result. 3 values > > > > > > + are displayed (for the 3 possible Product Type VDOs), one per line. > > > > > > > > > > sysfs is "one value per file", not "one value per line". This is not > > > > > ok. > > > > > > > > I see. Would listing these out as three separate vdos (i.e vdo0, vdo1, > > > > vdo2) be better? > > > > > > Given that your current implementation is not acceptable, something has > > > to change :) > > > > Got it. I'd like to see if Heikki has any suggestions on naming these > > entries better. > > Why not have product type specific attribute files? > > So if the partner is UFP, then we expose ufp1 and ufp2 files that > return the UFP1 and UFP2 VDO values and hide the other files: > > % ls /sys/class/typec/port0-partner/identity/ > id_header cert_stat product ufp1 ufp2 > > If the partner is DFP, then you expose the dfp file and hide > everything else: > > % ls /sys/class/typec/port0-partner/identity/ > id_header cert_stat product dfp > > And so on. I would caution against any decoding of the VDO contents in the kernel and making assumptions about the # or the names of these three individual objects. Since PD 2.0 through PD 3.0, and PD 3.0's different subrevisions (1.0, 1.3, 2.0), the # of VDOs that have been supported has changed in the various spec versions. PD R3.0 V2.0 actually added extra objects here (UFP VDO1 UFP VDO2, DFP VDO), but thanks to some troublemaker (me, actually...), the PD spec's next version deprecates and deletes two of them (the AMA VDO and the UFP VDO2 are gone, thanks to an ECR I put into USB PD). (If you've got USB PD working group access, the two ECRs in question are: https://groups.usb.org/wg/powerdelivery/document/11007 and https://groups.usb.org/wg/powerdelivery/document/10967). Since the different spec versions need to all be supported (since the firmware of PD devices are baked for a particular version of the PD spec at the time they are released and don't change in practice), the software on USB PD hosts should provide these objects up to the next layer without adding any extra decoding, and the upper layer (userspace) can figure out the specifics based on comparing different revision and version fields to figure out what vdo1, vdo2, and vdo3 are. Anyway, hope this helps, and sorry in advance for making this section of the PD spec more complicated to handle over time... Benson > > thanks, > > -- > heikki -- Benson Leung Staff Software Engineer Chrome OS Kernel Google Inc. bleung@google.com Chromium OS Project bleung@chromium.org
On Thu, Oct 22, 2020 at 04:25:23PM -0700, Benson Leung wrote: > Hi Heikki, > > On Thu, Oct 22, 2020 at 5:43 AM Heikki Krogerus > <heikki.krogerus@linux.intel.com> wrote: > > > > On Thu, Oct 22, 2020 at 12:25:07AM -0700, Prashant Malani wrote: > > > Hi Greg, > > > > > > On Thu, Oct 22, 2020 at 12:17 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > > > > > > +What: /sys/class/typec/<port>-partner/identity/product_type_vdo > > > > > > > +Date: October 2020 > > > > > > > +Contact: Prashant Malani <pmalani@chromium.org> > > > > > > > +Description: > > > > > > > + Product Type VDOs part of Discover Identity command result. 3 values > > > > > > > + are displayed (for the 3 possible Product Type VDOs), one per line. > > > > > > > > > > > > sysfs is "one value per file", not "one value per line". This is not > > > > > > ok. > > > > > > > > > > I see. Would listing these out as three separate vdos (i.e vdo0, vdo1, > > > > > vdo2) be better? > > > > > > > > Given that your current implementation is not acceptable, something has > > > > to change :) > > > > > > Got it. I'd like to see if Heikki has any suggestions on naming these > > > entries better. > > > > Why not have product type specific attribute files? > > > > So if the partner is UFP, then we expose ufp1 and ufp2 files that > > return the UFP1 and UFP2 VDO values and hide the other files: > > > > % ls /sys/class/typec/port0-partner/identity/ > > id_header cert_stat product ufp1 ufp2 > > > > If the partner is DFP, then you expose the dfp file and hide > > everything else: > > > > % ls /sys/class/typec/port0-partner/identity/ > > id_header cert_stat product dfp > > > > And so on. > > I would caution against any decoding of the VDO contents in the kernel > and making assumptions about the # or the names of these three > individual objects. > > Since PD 2.0 through PD 3.0, and PD 3.0's different subrevisions (1.0, > 1.3, 2.0), the # of VDOs that have been supported has changed in the > various spec versions. > > PD R3.0 V2.0 actually added extra objects here (UFP VDO1 UFP VDO2, DFP > VDO), but thanks to some troublemaker (me, actually...), the PD spec's > next version deprecates and deletes two of them (the AMA VDO and the > UFP VDO2 are gone, thanks to an ECR I put into USB PD). > > (If you've got USB PD working group access, the two ECRs in question > are: https://groups.usb.org/wg/powerdelivery/document/11007 and > https://groups.usb.org/wg/powerdelivery/document/10967). > > Since the different spec versions need to all be supported (since the > firmware of PD devices are baked for a particular version of the PD > spec at the time they are released and don't change in practice), the > software on USB PD hosts should provide these objects up to the next > layer without adding any extra decoding, and the upper layer > (userspace) can figure out the specifics based on comparing different > revision and version fields to figure out what vdo1, vdo2, and vdo3 > are. Agreed. This is a good point. This reminds me why I never exposed the product type VDOs in the first place. > Anyway, hope this helps, and sorry in advance for making this section > of the PD spec more complicated to handle over time... Thanks for the heads up. For the record, I don't think you are the only troublemaker :-). Some of the parts in the USB PD specification and many parts in the USB Type-C specification have been a bit of a moving target (though, I was hoping that things would have settled down by now). This is btw. the reason why I wanted to use interpreted kernel specific attribute files for example with the roles instead of trying to expose things like the specs says. I never accepted terms like UFP, DFP, DRP or DRD, and I'm really clad I didn't - the meaning of those has changed over the time in the USB Type-C specifications. Because of the constantly changing specifications, the goal remains the same. We pick only the details that we need, and that we are pretty certain will not change, and expose those in our own format (which ideally is human readable as well as machine readable) instead of exposing all the data that those details are part of in the raw format that follows the specification of today. But I guess we can't pick any more specific details out of the response to the Discover Identity. We are going to have to dump the whole thing to the user as it is. thanks, -- heikki
On Thu, Oct 22, 2020 at 12:13 AM Prashant Malani <pmalani@chromium.org> wrote: > > Thanks for reviewing the patch, Greg. > > On Wed, Oct 21, 2020 at 11:56 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > > > +What: /sys/class/typec/<port>-cable/identity/product_type_vdo > > > +Date: October 2020 > > > +Contact: Prashant Malani <pmalani@chromium.org> > > > +Description: > > > + Product Type VDOs part of Discover Identity command result. 3 values > > > + are displayed (for the 3 possible Product Type VDOs), one per line. > > > + The values will show 0s until Discover Identity command result becomes > > > + available. The values can be polled. > > > > Why are you describing the same value in two different locations? > > Both cable and partner can have Discover Identity VDOs and they are > listed separately in sysfs. > The other VDOs (id_header, cert_stat and product) have separate > descriptions for cable and partner too. > Perhaps there is a case for consolidating the listings here (factor > out the ones which are common to cable and partner). > Just an update: I added a patch to the series which consolidates these repeat identity entries into 1 location: https://lore.kernel.org/linux-usb/20201023214328.1262883-1-pmalani@chromium.org/ Thanks and best regards,
diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec index b834671522d6..16440a236b66 100644 --- a/Documentation/ABI/testing/sysfs-class-typec +++ b/Documentation/ABI/testing/sysfs-class-typec @@ -170,6 +170,14 @@ Description: will show 0 until Discover Identity command result becomes available. The value can be polled. +What: /sys/class/typec/<port>-partner/identity/product_type_vdo +Date: October 2020 +Contact: Prashant Malani <pmalani@chromium.org> +Description: + Product Type VDOs part of Discover Identity command result. 3 values + are displayed (for the 3 possible Product Type VDOs), one per line. + The values will show 0s until Discover Identity command result becomes + available. The values can be polled. USB Type-C cable devices (eg. /sys/class/typec/port0-cable/) @@ -230,6 +238,15 @@ Description: will show 0 until Discover Identity command result becomes available. The value can be polled. +What: /sys/class/typec/<port>-cable/identity/product_type_vdo +Date: October 2020 +Contact: Prashant Malani <pmalani@chromium.org> +Description: + Product Type VDOs part of Discover Identity command result. 3 values + are displayed (for the 3 possible Product Type VDOs), one per line. + The values will show 0s until Discover Identity command result becomes + available. The values can be polled. + USB Type-C port alternate mode devices. diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c index 35eec707cb51..37fa4501e75f 100644 --- a/drivers/usb/typec/class.c +++ b/drivers/usb/typec/class.c @@ -122,10 +122,20 @@ static ssize_t product_show(struct device *dev, struct device_attribute *attr, } static DEVICE_ATTR_RO(product); +static ssize_t product_type_vdo_show(struct device *dev, struct device_attribute *attr, + char *buf) +{ + struct usb_pd_identity *id = get_pd_identity(dev); + + return sprintf(buf, "0x%08x\n0x%08x\n0x%08x\n", id->vdo[0], id->vdo[1], id->vdo[2]); +} +static DEVICE_ATTR_RO(product_type_vdo); + static struct attribute *usb_pd_id_attrs[] = { &dev_attr_id_header.attr, &dev_attr_cert_stat.attr, &dev_attr_product.attr, + &dev_attr_product_type_vdo.attr, NULL }; @@ -144,6 +154,7 @@ static void typec_report_identity(struct device *dev) sysfs_notify(&dev->kobj, "identity", "id_header"); sysfs_notify(&dev->kobj, "identity", "cert_stat"); sysfs_notify(&dev->kobj, "identity", "product"); + sysfs_notify(&dev->kobj, "identity", "product_type_vdo"); } /* ------------------------------------------------------------------------- */
A PD-capable device can return up to 3 Product Type VDOs as part of its DiscoverIdentity Response (USB PD Spec, Rev 3.0, Version 2.0, Section 6.4.4.3.1). Add a sysfs attribute to expose these to userspace. Cc: Benson Leung <bleung@chromium.org> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> Signed-off-by: Prashant Malani <pmalani@chromium.org> --- NOTE: I didn't include Benson's Reviewed-by from v1, since this version introduced the sysfs_notify() call. Changes in v2: - Added sysfs_notify() call for the attribute. - Added description for the attribute in Documentation/ABI/testing/sysfs-class-typec. Documentation/ABI/testing/sysfs-class-typec | 17 +++++++++++++++++ drivers/usb/typec/class.c | 11 +++++++++++ 2 files changed, 28 insertions(+)