Re: [PATCH v3 2/2] usb: typec: Expose Product Type VDOs via sysfs

[Prashant Malani]

Hi Heikki,

On Thu, Nov 12, 2020 at 4:43 AM Heikki Krogerus
<heikki.krogerus@xxxxxxxxxxxxxxx> wrote:
>
> On Wed, Nov 11, 2020 at 06:40:55PM -0800, Prashant Malani wrote:
> > Hi Heikki,
> >
> > On Tue, Nov 10, 2020 at 01:54:53PM +0200, Heikki Krogerus wrote:
> > > On Fri, Oct 23, 2020 at 02:43:28PM -0700, Prashant Malani wrote:
> > >
> > > I've now come to the conclusion that this is not the correct approach.
> > > Instead, the whole identity, all six VDOs, should be supplied
> > > separately with a "raw" sysfs attribute file after all.
> > >
> > > The three attribute files that we already have - so id_header,
> > > cert_stat and product - can always supply the actual VDO as is,
> > > regardless of the product type, so they are fine. But these new
> > > attribute files, product_type_vdoX, would behave differently as they
> > > supply different information depending on the product type. That just
> > > does not feel right to me.
> >
> > OOI: I'd like to understand the reservations around this approach. Can't
> > userspace just read these and then interpret them appropriately according
> > to the id_header as well as PD revision (and version number) if that's exposed?
> > The only thing I see changing is how we name those product_type_vdoX
> > sysfs files, i.e product_type_vdo0 == passive_cable_vdo OR active_cable_vdo1
> > depending on the product type.
> >
> > That said, perhaps I'm missing some aspect of this.
>
> I don't think the userspace should have to interpret any of these
> VDOs. If the userspace has to interpret the information, then the
> userspace should interpret everything for the sake of consistency (so
> the "raw" attribute file).
>
> But I still think that defining separate device types for every
> product type would be the best way to handle the identity. We could
> then have sysfs attribute files that are specific for each product
> type. It does not even matter that some of the product types are going
> to be removed. We will have to handle all of them in any case,
> including the ones that were removed. This way things would be much
> more clear for the userspace.
>
> The only problem IMO with the separate device types for each product
> type is that we don't always have access to the Discover Identity
> result. It means depending on your system we will claim the
> partner device type is "default" (no identity information) or the
> actual product type. That is also a bit inconsistent, but is is
> acceptable? I would really like to here what Greg thinks about all
> this.

Thanks for explaining the rationale.
Of course, I defer to Greg & your decision on this :)

I'm yet unable to grasp what benefit userspace gets from having the kernel parse
and present this data in appropriately named sysfs files when the userspace has
enough info to do so itself.

For that reason and also because the "raw" approach is IMO a bit more
resilient to the changes
we talk about (some product type VDOs being dropped off across PD spec
uprevs [1] etc)
the "raw" proposal sounded appealing to me.

>
> > > So lets just add the "raw" sysfs attribute file. We can think about
> > > extracting some other details from the product type VDOs once the
> > > specification has settled down a bit and we can be quite certain that
> > > those details will always be available.
> > >
> > > Would this be OK to you? I think we should be able to dump the data to
> > > the "raw" sysfs attribute file with something like hex_dump_to_buffer().
> >
> > FWIW, "raw" option SGTM (the product type VDOs can be parsed from the
> > buffer since the format is fixed).
>
> Well, I'm starting to think that what if we just prepare patches where
> we propose separate device type for every product type? Of course, if
> they are OK to you?
>
SG. To clarify, will you prepare these patches?

Thanks & best regards,

-Prashant

[1]: https://lore.kernel.org/linux-usb/CANLzEkskrWXWLC+csObYwB+JUFdH+p6V6giMHtsKY-L61cTG9g@xxxxxxxxxxxxxx/