| 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
| ||
|
Message-ID: <CANFp7mXhwMMwyqbKqxe=SgCRPUyXVhKnsJwf0xgJ2LefOvrtjg@mail.gmail.com>
Date: Thu, 31 Oct 2024 16:02:22 -0700
From: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>
To: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc: tzungbi@...nel.org, linux-usb@...r.kernel.org,
chrome-platform@...ts.linux.dev, dmitry.baryshkov@...aro.org,
jthies@...gle.com, akuchynski@...gle.com, pmalani@...omium.org,
Benson Leung <bleung@...omium.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Guenter Roeck <groeck@...omium.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/7] usb: typec: Add driver for Thunderbolt 3 Alternate Mode
On Thu, Oct 31, 2024 at 7:11 AM Heikki Krogerus
<heikki.krogerus@...ux.intel.com> wrote:
>
> Hi Abhishek,
>
> On Wed, Oct 30, 2024 at 02:28:32PM -0700, Abhishek Pandit-Subedi wrote:
> > From: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> >
> > Thunderbolt 3 Alternate Mode entry flow is described in
> > USB Type-C Specification Release 2.0.
> >
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > Co-developed-by: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>
> > ---
> >
> > Changes:
> > * Delay cable + plug checks so that the module doesn't fail to probe
> > if cable + plug information isn't available by the time the partner
> > altmode is registered.
> > * Remove unncessary brace after if (IS_ERR(plug))
> >
> > The rest of this patch should be the same as Heikki's original RFC.
> >
> >
> > Changes in v2:
> > - Use <linux/usb/typec_tbt.h> and add missing TBT_CABLE_ROUNDED
> > - Pass struct typec_thunderbolt_data to typec_altmode_notify
> > - Rename TYPEC_TBT_MODE to USB_TYPEC_TBT_MODE
> > - Use USB_TYPEC_TBT_SID and USB_TYPEC_TBT_MODE for device id
> > - Change module license to GPL due to checkpatch warning
> >
> > drivers/platform/chrome/cros_ec_typec.c | 2 +-
> > drivers/usb/typec/altmodes/Kconfig | 9 +
> > drivers/usb/typec/altmodes/Makefile | 2 +
> > drivers/usb/typec/altmodes/thunderbolt.c | 308 +++++++++++++++++++++++
> > include/linux/usb/typec_tbt.h | 3 +-
> > 5 files changed, 322 insertions(+), 2 deletions(-)
> > create mode 100644 drivers/usb/typec/altmodes/thunderbolt.c
> >
> > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> > index c7781aea0b88..53d93baa36a8 100644
> > --- a/drivers/platform/chrome/cros_ec_typec.c
> > +++ b/drivers/platform/chrome/cros_ec_typec.c
> > @@ -499,7 +499,7 @@ static int cros_typec_enable_tbt(struct cros_typec_data *typec,
> > }
> >
> > port->state.data = &data;
> > - port->state.mode = TYPEC_TBT_MODE;
> > + port->state.mode = USB_TYPEC_TBT_MODE;
> >
> > return typec_mux_set(port->mux, &port->state);
> > }
>
> The definition should be changed in a separate patch.
Ack -- will pull the rename out into its own patch.
>
> > +static const struct typec_device_id tbt_typec_id[] = {
> > + { USB_TYPEC_TBT_SID, USB_TYPEC_TBT_MODE },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(typec, tbt_typec_id);
>
> Now the mode would be the same thing as connector state, which is not
> true. The connector state is supposed to reflect the pin assignment,
> and the mode is the mode index used with the actual VDMs. For example,
> DP alt mode has several different states, but only one mode.
>
> The TBT3 altmode driver will not work with this patch alone, it will
> never bind to the partner TBT3 alt mode because the mode does not
> match.
>
> Can you reorganise this series so that the patch 2/7 comes before this
> one? Then I think you can just use the SVID unless I'm mistaken:
>
> static const struct typec_device_id tbt_typec_id[] = {
> { USB_TYPEC_TBT_SID },
> { }
> };
> MODULE_DEVICE_TABLE(typec, tbt_typec_id);
>
> Alternatively, just leave it to TYPEC_ANY_MODE for now.
>
Sure, I'll re-order the patches and get rid of the mode. I'm actually
a bit confused as to how mode is supposed to be used since typec_dp.h
defines USB_TYPEC_DP_MODE=1, typec_tbt.h defines
USB_TYPEC_TBT_MODE=TYPEC_STATE_MODAL and it looks like USB state also
starts from TYPEC_STATE_MODAL and continues.
Is this documented in the spec somewhere? How should this mode value
be used and shared between USB and various alt-modes? At least the DP
case seems clear because as you said it describes different pin
assignments. However, the term "mode" seems to be overloaded since
it's used in other areas.
> > +static struct typec_altmode_driver tbt_altmode_driver = {
> > + .id_table = tbt_typec_id,
> > + .probe = tbt_altmode_probe,
> > + .remove = tbt_altmode_remove,
> > + .driver = {
> > + .name = "typec-thunderbolt",
> > + .owner = THIS_MODULE,
> > + }
> > +};
> > +module_typec_altmode_driver(tbt_altmode_driver);
> > +
> > +MODULE_AUTHOR("Heikki Krogerus <heikki.krogerus@...ux.intel.com>");
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("Thunderbolt3 USB Type-C Alternate Mode");
> > diff --git a/include/linux/usb/typec_tbt.h b/include/linux/usb/typec_tbt.h
> > index fa97d7e00f5c..3ff82641f6a0 100644
> > --- a/include/linux/usb/typec_tbt.h
> > +++ b/include/linux/usb/typec_tbt.h
> > @@ -10,7 +10,7 @@
> > #define USB_TYPEC_TBT_SID USB_TYPEC_VENDOR_INTEL
> >
> > /* Connector state for Thunderbolt3 */
> > -#define TYPEC_TBT_MODE TYPEC_STATE_MODAL
> > +#define USB_TYPEC_TBT_MODE TYPEC_STATE_MODAL
>
> I think USB_TYPEC_STATE_TBT would be better. But please change this in
> a separate patch in any case.
Same question as above about mode vs state :)
>
> thanks,
>
> --
> heikki
Powered by blists - more mailing lists