Re: [PATCH v2] usb: typec: Expose Product Type VDOs via sysfs

[Heikki Krogerus]

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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx> wrote:
> > > >
> > > > > > > +What:                /sys/class/typec/<port>-partner/identity/product_type_vdo
> > > > > > > +Date:                October 2020
> > > > > > > +Contact:     Prashant Malani <pmalani@xxxxxxxxxxxx>
> > > > > > > +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