linux-kernel - Re: [PATCH v4 5/7] platform/chrome: cros_ec

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAE-0n52dZONV2Fj=N4f85ttGRg+yyvOSXQoD50oFc9B7LBgtPA@mail.gmail.com>
Date: Mon, 16 Dec 2024 11:46:16 -0800
From: Stephen Boyd <swboyd@...omium.org>
To: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>
Cc: chrome-platform@...ts.linux.dev, heikki.krogerus@...ux.intel.com, 
	linux-usb@...r.kernel.org, tzungbi@...nel.org, akuchynski@...gle.com, 
	pmalani@...omium.org, jthies@...gle.com, dmitry.baryshkov@...aro.org, 
	badhri@...gle.com, rdbabiera@...gle.com, Benson Leung <bleung@...omium.org>, 
	Guenter Roeck <groeck@...omium.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 5/7] platform/chrome: cros_ec_typec: Displayport support

Quoting Abhishek Pandit-Subedi (2024-12-13 10:29:51)
> On Tue, Dec 10, 2024 at 4:08 PM Stephen Boyd <swboyd@...omium.org> wrote:
> >
> > Quoting Abhishek Pandit-Subedi (2024-12-06 15:38:16)
> > > diff --git a/drivers/platform/chrome/cros_typec_altmode.c b/drivers/platform/chrome/cros_typec_altmode.c
> > > new file mode 100644
> > > index 000000000000..bb7c7ad2ff6e
> > > --- /dev/null
> > > +++ b/drivers/platform/chrome/cros_typec_altmode.c
> > > @@ -0,0 +1,281 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Alt-mode implementation on ChromeOS EC.
> > > + *
> > > + * Copyright 2024 Google LLC
> > > + * Author: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>
> > > + */
> > > +#include "cros_ec_typec.h"
> > > +
> > > +#include <linux/usb/typec_dp.h>
> > > +#include <linux/usb/pd_vdo.h>
> >
> > Please include workqueue.h, mutex.h, etc. for things used in this file.
> Done. Btw, is there a script that does this for you in the kernel like
> include-what-you-use does for userspace?

I'm not aware of one.

>
> >
> > > +
> > > +#include "cros_typec_altmode.h"
> > > +
> > > +struct cros_typec_altmode_data {
> > > +       struct work_struct work;
> > > +       struct cros_typec_port *port;
> > > +       struct typec_altmode *alt;
> > > +       bool ap_mode_entry;
> >
> > The UCSI driver (drivers/usb/typec/ucsi/displayport.c) calls this
> > 'override', can it be named the same thing? I also see that the UCSI
> > driver has two bools, 'override' and 'initialized', which seems to be to
> > support a DP_CMD_CONFIGURE that will respond with an ACK and then the
> > next DP_CMD_CONFIGURE will call ucsi_altmode_update_active() to set the
> > altmode as active. Maybe the same method can be followed here so that on
> > older chromebooks where EC is in control of mode entry we can emulate
> > entering the mode?
>
> The reason it's called `override` in UCSI is because the feature is
> called "alternate mode override supported". When this optional bit is
> set, the UCSI method "SET_NEW_CAM" can be used to change what
> alternate mode is active. However, it behaves differently from cros_ec
> because even when override is set, the PD controller can/will still
> autonomously enter a mode on connection. Whereas on cros_ec_typec, if
> you set "ap-driven-mode", the EC will not enter any modes until the AP
> tells it to.

Ok, got it.

>
> Also, the reason the UCSI driver does the DP_CMD_CONFIGURE dance is
> because the UCSI command, SET_NEW_CAM, requires the DP configuration
> VDO as a parameter. Since UCSI doesn't define a VDM mechanism, the
> UCSI driver fakes the ".entry" call and then uses the first
> DP_CONFIGURE to do the actual entry. This also doesn't match the
> cros_ec driver (either ap-driven or not) because the interface does
> not allow setting the DP VDO at all. DP_CONFIGURE will be generated
> and consumed entirely within the EC and all we can really use is the
> mux update to generate a status update for the DP state machine.

Is the 'initialized' dance the fake ".entry"? I want the not ap-driven
(ec-driven?) mode to work with this series, and specifically not print
an error message. I'm guessing that to do that we should fake ".entry"
when the EC is in control of the mode entry. Or is there some way to
jump the state machine forward for the port altmode so that it is
already entered? I think to get the device to show the pin configuration
and mode like "source" vs. "sink" or "usb" we need to spoof multiple
VDMs.

>
> Right now, EC driven Chromebooks will simply report active/inactive
> for DP without reporting any configuration/status info. If you want to
> handle DP_CONFIGURE and DP_STATUS from the altmode driver, you'll need
> to fake the interactions in the port driver in a subsequent CL.
>

Ok. I'd very much like to do that because I need to make the displayport
partner altmode driver work to the point that it calls
drm_connector_oob_hotplug_event(). Do we need to spoof a
DP_CMD_STATUS_UPDATE on mode entry when the EC indicates that DP mode is
entered on a port?