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: <ZyOQJmF-PcFHgmeq@kuha.fi.intel.com>
Date: Thu, 31 Oct 2024 16:11:50 +0200
From: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>
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

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.

> +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.

> +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.

thanks,

-- 
heikki

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ