Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: otbR1JLwLu1RBxt6bzxquAa1N3WDBUkrkQLoC7YsQkjB5q1cpoqinH1jMkBAwv857eZKizHW2j
 2geA9X9OjzVw==
IronPort-SDR: fsFIy+cgjj5mCIALl4pFdwQtH9jpbeYRKJ+LkG20X3MmxlB61B5wkqSOhDDYdp7CfF7ZMlVneK
 4/3UU/iW/AwA==
Date:   Thu, 12 Nov 2020 14:43:45 +0200
From:   Heikki Krogerus <heikki.krogerus@linux.intel.com>
To:     Prashant Malani <pmalani@chromium.org>
Cc:     linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
        gregkh@linuxfoundation.org, Benson Leung <bleung@chromium.org>
Subject: Re: [PATCH v3 2/2] usb: typec: Expose Product Type VDOs via sysfs
Message-ID: <20201112124345.GS1224435@kuha.fi.intel.com>
References: <20201023214328.1262883-1-pmalani@chromium.org>
 <20201023214328.1262883-2-pmalani@chromium.org>
 <20201110115453.GI1224435@kuha.fi.intel.com>
 <20201112024055.GA1367855@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201112024055.GA1367855@google.com>
Precedence: bulk

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.

> > 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?


thanks,

-- 
heikki