* [PATCH 1/8] usb: typec: Add driver for Thunderbolt 3 Alternate Mode
2024-09-25 16:25 [PATCH 0/8] Thunderbolt and DP altmode support for cros-ec-typec Abhishek Pandit-Subedi
@ 2024-09-25 16:25 ` Abhishek Pandit-Subedi
2024-09-26 14:06 ` Heikki Krogerus
2024-09-26 15:14 ` kernel test robot
2024-09-25 16:25 ` [PATCH 2/8] usb: typec: altmode_match should handle TYPEC_ANY_MODE Abhishek Pandit-Subedi
` (3 subsequent siblings)
4 siblings, 2 replies; 15+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-09-25 16:25 UTC (permalink / raw)
To: heikki.krogerus, tzungbi
Cc: jthies, pmalani, akuchynski, Abhishek Pandit-Subedi,
Greg Kroah-Hartman, linux-kernel, linux-usb
From: Heikki Krogerus <heikki.krogerus@linux.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@linux.intel.com>
Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.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.
drivers/usb/typec/altmodes/Kconfig | 9 +
drivers/usb/typec/altmodes/Makefile | 2 +
drivers/usb/typec/altmodes/thunderbolt.c | 321 +++++++++++++++++++++++
3 files changed, 332 insertions(+)
create mode 100644 drivers/usb/typec/altmodes/thunderbolt.c
diff --git a/drivers/usb/typec/altmodes/Kconfig b/drivers/usb/typec/altmodes/Kconfig
index 1a6b5e872b0d..7867fa7c405d 100644
--- a/drivers/usb/typec/altmodes/Kconfig
+++ b/drivers/usb/typec/altmodes/Kconfig
@@ -23,4 +23,13 @@ config TYPEC_NVIDIA_ALTMODE
To compile this driver as a module, choose M here: the
module will be called typec_nvidia.
+config TYPEC_TBT_ALTMODE
+ tristate "Thunderbolt3 Alternate Mode driver"
+ help
+ Select this option if you have Thunderbolt3 hardware on your
+ system.
+
+ To compile this driver as a module, choose M here: the
+ module will be called typec_thunderbolt.
+
endmenu
diff --git a/drivers/usb/typec/altmodes/Makefile b/drivers/usb/typec/altmodes/Makefile
index 45717548b396..508a68351bd2 100644
--- a/drivers/usb/typec/altmodes/Makefile
+++ b/drivers/usb/typec/altmodes/Makefile
@@ -4,3 +4,5 @@ obj-$(CONFIG_TYPEC_DP_ALTMODE) += typec_displayport.o
typec_displayport-y := displayport.o
obj-$(CONFIG_TYPEC_NVIDIA_ALTMODE) += typec_nvidia.o
typec_nvidia-y := nvidia.o
+obj-$(CONFIG_TYPEC_TBT_ALTMODE) += typec_thunderbolt.o
+typec_thunderbolt-y := thunderbolt.o
diff --git a/drivers/usb/typec/altmodes/thunderbolt.c b/drivers/usb/typec/altmodes/thunderbolt.c
new file mode 100644
index 000000000000..515e775ee41a
--- /dev/null
+++ b/drivers/usb/typec/altmodes/thunderbolt.c
@@ -0,0 +1,321 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * USB Typec-C Thuderbolt3 Alternate Mode driver
+ *
+ * Copyright (C) 2019 Intel Corporation
+ * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
+ */
+
+#include <linux/delay.h>
+#include <linux/mutex.h>
+#include <linux/module.h>
+#include <linux/usb/pd_vdo.h>
+#include <linux/usb/typec_altmode.h>
+
+#define USB_TYPEC_VENDOR_INTEL 0x8087
+
+/* TBT3 Device Discover Mode VDO bits */
+#define TBT_MODE BIT(0)
+#define TBT_ADAPTER(_vdo_) (((_vdo_) & BIT(16)) >> 16)
+#define TBT_ADAPTER_LEGACY 0
+#define TBT_ADAPTER_TBT3 1
+#define TBT_INTEL_SPECIFIC_B0 BIT(26)
+#define TBT_VENDOR_SPECIFIC_B0 BIT(30)
+#define TBT_VENDOR_SPECIFIC_B1 BIT(31)
+
+/* TBT3 Cable Discover Mode VDO bits */
+#define TBT_CABLE_SPEED(_vdo_) (((_vdo_) & GENMASK(18, 16)) >> 16)
+#define TBT_CABLE_USB3_GEN1 1
+#define TBT_CABLE_USB3_PASSIVE 2
+#define TBT_CABLE_10_AND_20GBPS 3
+#define TBT_CABLE_ROUNDED BIT(19)
+#define TBT_CABLE_OPTICAL BIT(21)
+#define TBT_CABLE_RETIMER BIT(22)
+#define TBT_CABLE_LINK_TRAINING BIT(23)
+
+/* TBT3 Device Enter Mode VDO bits */
+#define TBT_ENTER_MODE_CABLE_SPEED(_s_) (((_s_) & GENMASK(2, 0)) << 16)
+#define TBT_ENTER_MODE_ACTIVE_CABLE BIT(24)
+
+enum tbt_state {
+ TBT_STATE_IDLE,
+ TBT_STATE_SOP_P_ENTER,
+ TBT_STATE_SOP_PP_ENTER,
+ TBT_STATE_ENTER,
+ TBT_STATE_EXIT,
+ TBT_STATE_SOP_PP_EXIT,
+ TBT_STATE_SOP_P_EXIT
+};
+
+struct tbt_altmode {
+ enum tbt_state state;
+ struct typec_cable *cable;
+ struct typec_altmode *alt;
+ struct typec_altmode *plug[2];
+
+ struct work_struct work;
+ struct mutex lock; /* device lock */
+};
+
+static bool tbt_ready(struct typec_altmode *alt);
+
+static int tbt_enter_mode(struct tbt_altmode *tbt)
+{
+ struct typec_altmode *plug = tbt->plug[TYPEC_PLUG_SOP_P];
+ u32 vdo;
+
+ vdo = tbt->alt->vdo & (TBT_VENDOR_SPECIFIC_B0 | TBT_VENDOR_SPECIFIC_B1);
+ vdo |= tbt->alt->vdo & TBT_INTEL_SPECIFIC_B0;
+ vdo |= TBT_MODE;
+
+ if (plug) {
+ if (typec_cable_is_active(tbt->cable))
+ vdo |= TBT_ENTER_MODE_ACTIVE_CABLE;
+
+ vdo |= TBT_ENTER_MODE_CABLE_SPEED(TBT_CABLE_SPEED(plug->vdo));
+ vdo |= plug->vdo & TBT_CABLE_ROUNDED;
+ vdo |= plug->vdo & TBT_CABLE_OPTICAL;
+ vdo |= plug->vdo & TBT_CABLE_RETIMER;
+ vdo |= plug->vdo & TBT_CABLE_LINK_TRAINING;
+ } else {
+ vdo |= TBT_ENTER_MODE_CABLE_SPEED(TBT_CABLE_USB3_PASSIVE);
+ }
+
+ return typec_altmode_enter(tbt->alt, &vdo);
+}
+
+static void tbt_altmode_work(struct work_struct *work)
+{
+ struct tbt_altmode *tbt = container_of(work, struct tbt_altmode, work);
+ int ret;
+
+ mutex_lock(&tbt->lock);
+
+ switch (tbt->state) {
+ case TBT_STATE_SOP_P_ENTER:
+ ret = typec_altmode_enter(tbt->plug[TYPEC_PLUG_SOP_P], NULL);
+ if (ret)
+ dev_dbg(&tbt->plug[TYPEC_PLUG_SOP_P]->dev,
+ "failed to enter mode (%d)\n", ret);
+ break;
+ case TBT_STATE_SOP_PP_ENTER:
+ ret = typec_altmode_enter(tbt->plug[TYPEC_PLUG_SOP_PP], NULL);
+ if (ret)
+ dev_dbg(&tbt->plug[TYPEC_PLUG_SOP_PP]->dev,
+ "failed to enter mode (%d)\n", ret);
+ break;
+ case TBT_STATE_ENTER:
+ ret = tbt_enter_mode(tbt);
+ if (ret)
+ dev_dbg(&tbt->alt->dev, "failed to enter mode (%d)\n",
+ ret);
+ break;
+ case TBT_STATE_EXIT:
+ typec_altmode_exit(tbt->alt);
+ break;
+ case TBT_STATE_SOP_PP_EXIT:
+ typec_altmode_exit(tbt->plug[TYPEC_PLUG_SOP_PP]);
+ break;
+ case TBT_STATE_SOP_P_EXIT:
+ typec_altmode_exit(tbt->plug[TYPEC_PLUG_SOP_P]);
+ break;
+ default:
+ break;
+ }
+
+ tbt->state = TBT_STATE_IDLE;
+
+ mutex_unlock(&tbt->lock);
+}
+
+static int tbt_altmode_vdm(struct typec_altmode *alt,
+ const u32 hdr, const u32 *vdo, int count)
+{
+ struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
+ int cmd_type = PD_VDO_CMDT(hdr);
+ int cmd = PD_VDO_CMD(hdr);
+
+ mutex_lock(&tbt->lock);
+
+ if (tbt->state != TBT_STATE_IDLE) {
+ mutex_unlock(&tbt->lock);
+ return -EBUSY;
+ }
+
+ switch (cmd_type) {
+ case CMDT_RSP_ACK:
+ switch (cmd) {
+ case CMD_ENTER_MODE:
+ /*
+ * Following the order describeded in USB Type-C Spec
+ * R2.0 Section 6.7.3.
+ */
+ if (alt == tbt->plug[TYPEC_PLUG_SOP_P]) {
+ if (tbt->plug[TYPEC_PLUG_SOP_PP])
+ tbt->state = TBT_STATE_SOP_PP_ENTER;
+ else
+ tbt->state = TBT_STATE_ENTER;
+ } else if (alt == tbt->plug[TYPEC_PLUG_SOP_PP]) {
+ tbt->state = TBT_STATE_ENTER;
+ } else {
+ typec_altmode_notify(alt, TYPEC_STATE_MODAL,
+ NULL);
+ }
+ break;
+ case CMD_EXIT_MODE:
+ if (alt == tbt->alt) {
+ if (tbt->plug[TYPEC_PLUG_SOP_PP])
+ tbt->state = TBT_STATE_SOP_PP_EXIT;
+ else if (tbt->plug[TYPEC_PLUG_SOP_P])
+ tbt->state = TBT_STATE_SOP_P_EXIT;
+ } else if (alt == tbt->plug[TYPEC_PLUG_SOP_PP]) {
+ tbt->state = TBT_STATE_SOP_P_EXIT;
+ }
+ break;
+ }
+ break;
+ case CMDT_RSP_NAK:
+ switch (cmd) {
+ case CMD_ENTER_MODE:
+ dev_warn(&alt->dev, "Enter Mode refused\n");
+ break;
+ default:
+ break;
+ }
+ break;
+ default:
+ break;
+ }
+
+ if (tbt->state != TBT_STATE_IDLE)
+ schedule_work(&tbt->work);
+
+ mutex_unlock(&tbt->lock);
+
+ return 0;
+}
+
+static int tbt_altmode_activate(struct typec_altmode *alt, int activate)
+{
+ struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
+ int ret;
+
+ mutex_lock(&tbt->lock);
+
+ if (!tbt_ready(alt))
+ return -ENODEV;
+
+ /* Preventing the user space from entering/exiting the cable alt mode */
+ if (alt != tbt->alt)
+ ret = -EPERM;
+ else if (activate)
+ ret = tbt_enter_mode(tbt);
+ else
+ ret = typec_altmode_exit(alt);
+
+ mutex_unlock(&tbt->lock);
+
+ return ret;
+}
+
+static const struct typec_altmode_ops tbt_altmode_ops = {
+ .vdm = tbt_altmode_vdm,
+ .activate = tbt_altmode_activate
+};
+
+static int tbt_altmode_probe(struct typec_altmode *alt)
+{
+ struct tbt_altmode *tbt;
+
+ tbt = devm_kzalloc(&alt->dev, sizeof(*tbt), GFP_KERNEL);
+ if (!tbt)
+ return -ENOMEM;
+
+ INIT_WORK(&tbt->work, tbt_altmode_work);
+ mutex_init(&tbt->lock);
+ tbt->alt = alt;
+
+ alt->desc = "Thunderbolt3";
+ typec_altmode_set_drvdata(alt, tbt);
+ typec_altmode_set_ops(alt, &tbt_altmode_ops);
+
+ if (tbt_ready(alt)) {
+ if (tbt->plug[TYPEC_PLUG_SOP_PP])
+ tbt->state = TBT_STATE_SOP_PP_ENTER;
+ else if (tbt->plug[TYPEC_PLUG_SOP_P])
+ tbt->state = TBT_STATE_SOP_P_ENTER;
+ else
+ tbt->state = TBT_STATE_ENTER;
+ schedule_work(&tbt->work);
+ }
+
+ return 0;
+}
+
+static void tbt_altmode_remove(struct typec_altmode *alt)
+{
+ struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
+
+ for (int i = TYPEC_PLUG_SOP_PP; i > 0; --i) {
+ if (tbt->plug[i])
+ typec_altmode_put_plug(tbt->plug[i]);
+ }
+
+ if (tbt->cable)
+ typec_cable_put(tbt->cable);
+}
+
+static bool tbt_ready(struct typec_altmode *alt)
+{
+ struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
+ struct typec_altmode *plug;
+
+ if (tbt->cable)
+ return true;
+
+ /* Thundebolt 3 requires a cable with eMarker */
+ tbt->cable = typec_cable_get(typec_altmode2port(tbt->alt));
+ if (!tbt->cable)
+ return false;
+
+ /* We accept systems without SOP' or SOP''. This means the port altmode
+ * driver will be responsible for properly ordering entry/exit.
+ */
+ for (int i = 0; i < TYPEC_PLUG_SOP_PP + 1; i++) {
+ plug = typec_altmode_get_plug(tbt->alt, i);
+ if (IS_ERR(plug))
+ continue;
+
+ if (!plug || plug->svid != USB_TYPEC_VENDOR_INTEL)
+ break;
+
+ plug->desc = "Thunderbolt3";
+ plug->ops = &tbt_altmode_ops;
+ typec_altmode_set_drvdata(plug, tbt);
+
+ tbt->plug[i] = plug;
+ }
+
+ return true;
+}
+
+static const struct typec_device_id tbt_typec_id[] = {
+ { USB_TYPEC_VENDOR_INTEL, TYPEC_ANY_MODE },
+ { }
+};
+MODULE_DEVICE_TABLE(typec, tbt_typec_id);
+
+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@linux.intel.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Thunderbolt3 USB Type-C Alternate Mode");
--
2.46.0.792.g87dc391469-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 1/8] usb: typec: Add driver for Thunderbolt 3 Alternate Mode
2024-09-25 16:25 ` [PATCH 1/8] usb: typec: Add driver for Thunderbolt 3 Alternate Mode Abhishek Pandit-Subedi
@ 2024-09-26 14:06 ` Heikki Krogerus
2024-09-26 15:14 ` kernel test robot
1 sibling, 0 replies; 15+ messages in thread
From: Heikki Krogerus @ 2024-09-26 14:06 UTC (permalink / raw)
To: Abhishek Pandit-Subedi
Cc: tzungbi, jthies, pmalani, akuchynski, Greg Kroah-Hartman,
linux-kernel, linux-usb
Hi Abhishek,
On Wed, Sep 25, 2024 at 09:25:02AM -0700, Abhishek Pandit-Subedi wrote:
> From: Heikki Krogerus <heikki.krogerus@linux.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@linux.intel.com>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.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.
>
>
> drivers/usb/typec/altmodes/Kconfig | 9 +
> drivers/usb/typec/altmodes/Makefile | 2 +
> drivers/usb/typec/altmodes/thunderbolt.c | 321 +++++++++++++++++++++++
> 3 files changed, 332 insertions(+)
> create mode 100644 drivers/usb/typec/altmodes/thunderbolt.c
>
> diff --git a/drivers/usb/typec/altmodes/Kconfig b/drivers/usb/typec/altmodes/Kconfig
> index 1a6b5e872b0d..7867fa7c405d 100644
> --- a/drivers/usb/typec/altmodes/Kconfig
> +++ b/drivers/usb/typec/altmodes/Kconfig
> @@ -23,4 +23,13 @@ config TYPEC_NVIDIA_ALTMODE
> To compile this driver as a module, choose M here: the
> module will be called typec_nvidia.
>
> +config TYPEC_TBT_ALTMODE
> + tristate "Thunderbolt3 Alternate Mode driver"
> + help
> + Select this option if you have Thunderbolt3 hardware on your
> + system.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called typec_thunderbolt.
> +
> endmenu
> diff --git a/drivers/usb/typec/altmodes/Makefile b/drivers/usb/typec/altmodes/Makefile
> index 45717548b396..508a68351bd2 100644
> --- a/drivers/usb/typec/altmodes/Makefile
> +++ b/drivers/usb/typec/altmodes/Makefile
> @@ -4,3 +4,5 @@ obj-$(CONFIG_TYPEC_DP_ALTMODE) += typec_displayport.o
> typec_displayport-y := displayport.o
> obj-$(CONFIG_TYPEC_NVIDIA_ALTMODE) += typec_nvidia.o
> typec_nvidia-y := nvidia.o
> +obj-$(CONFIG_TYPEC_TBT_ALTMODE) += typec_thunderbolt.o
> +typec_thunderbolt-y := thunderbolt.o
> diff --git a/drivers/usb/typec/altmodes/thunderbolt.c b/drivers/usb/typec/altmodes/thunderbolt.c
> new file mode 100644
> index 000000000000..515e775ee41a
> --- /dev/null
> +++ b/drivers/usb/typec/altmodes/thunderbolt.c
> @@ -0,0 +1,321 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * USB Typec-C Thuderbolt3 Alternate Mode driver
> + *
> + * Copyright (C) 2019 Intel Corporation
> + * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/mutex.h>
> +#include <linux/module.h>
> +#include <linux/usb/pd_vdo.h>
> +#include <linux/usb/typec_altmode.h>
> +
> +#define USB_TYPEC_VENDOR_INTEL 0x8087
> +
> +/* TBT3 Device Discover Mode VDO bits */
> +#define TBT_MODE BIT(0)
> +#define TBT_ADAPTER(_vdo_) (((_vdo_) & BIT(16)) >> 16)
> +#define TBT_ADAPTER_LEGACY 0
> +#define TBT_ADAPTER_TBT3 1
> +#define TBT_INTEL_SPECIFIC_B0 BIT(26)
> +#define TBT_VENDOR_SPECIFIC_B0 BIT(30)
> +#define TBT_VENDOR_SPECIFIC_B1 BIT(31)
> +
> +/* TBT3 Cable Discover Mode VDO bits */
> +#define TBT_CABLE_SPEED(_vdo_) (((_vdo_) & GENMASK(18, 16)) >> 16)
> +#define TBT_CABLE_USB3_GEN1 1
> +#define TBT_CABLE_USB3_PASSIVE 2
> +#define TBT_CABLE_10_AND_20GBPS 3
> +#define TBT_CABLE_ROUNDED BIT(19)
> +#define TBT_CABLE_OPTICAL BIT(21)
> +#define TBT_CABLE_RETIMER BIT(22)
> +#define TBT_CABLE_LINK_TRAINING BIT(23)
> +
> +/* TBT3 Device Enter Mode VDO bits */
> +#define TBT_ENTER_MODE_CABLE_SPEED(_s_) (((_s_) & GENMASK(2, 0)) << 16)
> +#define TBT_ENTER_MODE_ACTIVE_CABLE BIT(24)
I guess my original RFC was before the introduction of
linux/usb/typec_tbt.h. You can replace all the above definitions with:
#include <linux/usb/typec_tbt.h>
> +enum tbt_state {
> + TBT_STATE_IDLE,
> + TBT_STATE_SOP_P_ENTER,
> + TBT_STATE_SOP_PP_ENTER,
> + TBT_STATE_ENTER,
> + TBT_STATE_EXIT,
> + TBT_STATE_SOP_PP_EXIT,
> + TBT_STATE_SOP_P_EXIT
> +};
> +
> +struct tbt_altmode {
> + enum tbt_state state;
> + struct typec_cable *cable;
> + struct typec_altmode *alt;
> + struct typec_altmode *plug[2];
You really have to supply struct typec_thunderbolt_data from this
driver.
I understand that by leaving the mux handling to the alt mode drivers,
you have to refactor at least cros_ec_typec.c, but I really think you
have to do it. So please either add here:
struct typec_thunderbolt_data data;
or if we don't need to store it, store only the Enter Mode VDO:
u32 enter_vdo;
> + struct work_struct work;
> + struct mutex lock; /* device lock */
> +};
> +
> +static bool tbt_ready(struct typec_altmode *alt);
> +
> +static int tbt_enter_mode(struct tbt_altmode *tbt)
> +{
> + struct typec_altmode *plug = tbt->plug[TYPEC_PLUG_SOP_P];
> + u32 vdo;
> +
> + vdo = tbt->alt->vdo & (TBT_VENDOR_SPECIFIC_B0 | TBT_VENDOR_SPECIFIC_B1);
> + vdo |= tbt->alt->vdo & TBT_INTEL_SPECIFIC_B0;
> + vdo |= TBT_MODE;
> +
> + if (plug) {
> + if (typec_cable_is_active(tbt->cable))
> + vdo |= TBT_ENTER_MODE_ACTIVE_CABLE;
> +
> + vdo |= TBT_ENTER_MODE_CABLE_SPEED(TBT_CABLE_SPEED(plug->vdo));
> + vdo |= plug->vdo & TBT_CABLE_ROUNDED;
TBT_CABLE_ROUNDED needs to be defined in typec_tbt.h. It's missing for
some reason.
> + vdo |= plug->vdo & TBT_CABLE_OPTICAL;
> + vdo |= plug->vdo & TBT_CABLE_RETIMER;
> + vdo |= plug->vdo & TBT_CABLE_LINK_TRAINING;
> + } else {
> + vdo |= TBT_ENTER_MODE_CABLE_SPEED(TBT_CABLE_USB3_PASSIVE);
> + }
So this vdo I believe will need to be stored:
tbt->data.enter_vdo;
or
tbt->enter_vdo = vdo;
> + return typec_altmode_enter(tbt->alt, &vdo);
> +}
> +
> +static void tbt_altmode_work(struct work_struct *work)
> +{
> + struct tbt_altmode *tbt = container_of(work, struct tbt_altmode, work);
> + int ret;
> +
> + mutex_lock(&tbt->lock);
> +
> + switch (tbt->state) {
> + case TBT_STATE_SOP_P_ENTER:
> + ret = typec_altmode_enter(tbt->plug[TYPEC_PLUG_SOP_P], NULL);
> + if (ret)
> + dev_dbg(&tbt->plug[TYPEC_PLUG_SOP_P]->dev,
> + "failed to enter mode (%d)\n", ret);
> + break;
> + case TBT_STATE_SOP_PP_ENTER:
> + ret = typec_altmode_enter(tbt->plug[TYPEC_PLUG_SOP_PP], NULL);
> + if (ret)
> + dev_dbg(&tbt->plug[TYPEC_PLUG_SOP_PP]->dev,
> + "failed to enter mode (%d)\n", ret);
> + break;
> + case TBT_STATE_ENTER:
> + ret = tbt_enter_mode(tbt);
> + if (ret)
> + dev_dbg(&tbt->alt->dev, "failed to enter mode (%d)\n",
> + ret);
> + break;
> + case TBT_STATE_EXIT:
> + typec_altmode_exit(tbt->alt);
> + break;
> + case TBT_STATE_SOP_PP_EXIT:
> + typec_altmode_exit(tbt->plug[TYPEC_PLUG_SOP_PP]);
> + break;
> + case TBT_STATE_SOP_P_EXIT:
> + typec_altmode_exit(tbt->plug[TYPEC_PLUG_SOP_P]);
> + break;
> + default:
> + break;
> + }
> +
> + tbt->state = TBT_STATE_IDLE;
> +
> + mutex_unlock(&tbt->lock);
> +}
> +
> +static int tbt_altmode_vdm(struct typec_altmode *alt,
> + const u32 hdr, const u32 *vdo, int count)
> +{
> + struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
> + int cmd_type = PD_VDO_CMDT(hdr);
> + int cmd = PD_VDO_CMD(hdr);
> +
> + mutex_lock(&tbt->lock);
> +
> + if (tbt->state != TBT_STATE_IDLE) {
> + mutex_unlock(&tbt->lock);
> + return -EBUSY;
> + }
> +
> + switch (cmd_type) {
> + case CMDT_RSP_ACK:
> + switch (cmd) {
> + case CMD_ENTER_MODE:
> + /*
> + * Following the order describeded in USB Type-C Spec
> + * R2.0 Section 6.7.3.
> + */
> + if (alt == tbt->plug[TYPEC_PLUG_SOP_P]) {
> + if (tbt->plug[TYPEC_PLUG_SOP_PP])
> + tbt->state = TBT_STATE_SOP_PP_ENTER;
> + else
> + tbt->state = TBT_STATE_ENTER;
> + } else if (alt == tbt->plug[TYPEC_PLUG_SOP_PP]) {
> + tbt->state = TBT_STATE_ENTER;
> + } else {
> + typec_altmode_notify(alt, TYPEC_STATE_MODAL,
> + NULL);
typec_altmode_notify(alt, TYPEC_STATE_MODAL, &tbt->data);
or, if this is possible:
struct typec_thunderbolt_data;
data.device_vdo = tbt->alt->vdo;
data.cable_vdo = tbt->plug[TYPEC_PLUG_SOP_P].vdo;
data.enter_vdo = tbt->enter_vdo;
typec_altmode_notify(alt, TYPEC_STATE_MODAL, &data);
> + }
> + break;
> + case CMD_EXIT_MODE:
> + if (alt == tbt->alt) {
> + if (tbt->plug[TYPEC_PLUG_SOP_PP])
> + tbt->state = TBT_STATE_SOP_PP_EXIT;
> + else if (tbt->plug[TYPEC_PLUG_SOP_P])
> + tbt->state = TBT_STATE_SOP_P_EXIT;
> + } else if (alt == tbt->plug[TYPEC_PLUG_SOP_PP]) {
> + tbt->state = TBT_STATE_SOP_P_EXIT;
> + }
> + break;
> + }
> + break;
> + case CMDT_RSP_NAK:
> + switch (cmd) {
> + case CMD_ENTER_MODE:
> + dev_warn(&alt->dev, "Enter Mode refused\n");
> + break;
> + default:
> + break;
> + }
> + break;
> + default:
> + break;
> + }
> +
> + if (tbt->state != TBT_STATE_IDLE)
> + schedule_work(&tbt->work);
> +
> + mutex_unlock(&tbt->lock);
> +
> + return 0;
> +}
> +
> +static int tbt_altmode_activate(struct typec_altmode *alt, int activate)
> +{
> + struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
> + int ret;
> +
> + mutex_lock(&tbt->lock);
> +
> + if (!tbt_ready(alt))
> + return -ENODEV;
> +
> + /* Preventing the user space from entering/exiting the cable alt mode */
> + if (alt != tbt->alt)
> + ret = -EPERM;
> + else if (activate)
> + ret = tbt_enter_mode(tbt);
> + else
> + ret = typec_altmode_exit(alt);
> +
> + mutex_unlock(&tbt->lock);
> +
> + return ret;
> +}
> +
> +static const struct typec_altmode_ops tbt_altmode_ops = {
> + .vdm = tbt_altmode_vdm,
> + .activate = tbt_altmode_activate
> +};
> +
> +static int tbt_altmode_probe(struct typec_altmode *alt)
> +{
> + struct tbt_altmode *tbt;
> +
> + tbt = devm_kzalloc(&alt->dev, sizeof(*tbt), GFP_KERNEL);
> + if (!tbt)
> + return -ENOMEM;
> +
> + INIT_WORK(&tbt->work, tbt_altmode_work);
> + mutex_init(&tbt->lock);
> + tbt->alt = alt;
tbt->data.device_mode = alt->vdo;
> + alt->desc = "Thunderbolt3";
> + typec_altmode_set_drvdata(alt, tbt);
> + typec_altmode_set_ops(alt, &tbt_altmode_ops);
> +
> + if (tbt_ready(alt)) {
> + if (tbt->plug[TYPEC_PLUG_SOP_PP])
> + tbt->state = TBT_STATE_SOP_PP_ENTER;
> + else if (tbt->plug[TYPEC_PLUG_SOP_P])
> + tbt->state = TBT_STATE_SOP_P_ENTER;
> + else
> + tbt->state = TBT_STATE_ENTER;
> + schedule_work(&tbt->work);
> + }
> +
> + return 0;
> +}
> +
> +static void tbt_altmode_remove(struct typec_altmode *alt)
> +{
> + struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
> +
> + for (int i = TYPEC_PLUG_SOP_PP; i > 0; --i) {
> + if (tbt->plug[i])
> + typec_altmode_put_plug(tbt->plug[i]);
> + }
> +
> + if (tbt->cable)
> + typec_cable_put(tbt->cable);
> +}
> +
> +static bool tbt_ready(struct typec_altmode *alt)
> +{
> + struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
> + struct typec_altmode *plug;
> +
> + if (tbt->cable)
> + return true;
> +
> + /* Thundebolt 3 requires a cable with eMarker */
> + tbt->cable = typec_cable_get(typec_altmode2port(tbt->alt));
> + if (!tbt->cable)
> + return false;
> +
> + /* We accept systems without SOP' or SOP''. This means the port altmode
> + * driver will be responsible for properly ordering entry/exit.
> + */
> + for (int i = 0; i < TYPEC_PLUG_SOP_PP + 1; i++) {
> + plug = typec_altmode_get_plug(tbt->alt, i);
> + if (IS_ERR(plug))
> + continue;
> +
> + if (!plug || plug->svid != USB_TYPEC_VENDOR_INTEL)
> + break;
> +
> + plug->desc = "Thunderbolt3";
> + plug->ops = &tbt_altmode_ops;
> + typec_altmode_set_drvdata(plug, tbt);
> +
> + tbt->plug[i] = plug;
> + }
tbt->data.cable_mode = tbt->plug[TYPEC_PLUG_SOP_P].vdo;
> + return true;
> +}
> +
> +static const struct typec_device_id tbt_typec_id[] = {
> + { USB_TYPEC_VENDOR_INTEL, TYPEC_ANY_MODE },
There is also the alias: USB_TYPEC_TBT_SID
> + { }
> +};
> +MODULE_DEVICE_TABLE(typec, tbt_typec_id);
> +
> +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@linux.intel.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Thunderbolt3 USB Type-C Alternate Mode");
thanks,
--
heikki
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/8] usb: typec: Add driver for Thunderbolt 3 Alternate Mode
2024-09-25 16:25 ` [PATCH 1/8] usb: typec: Add driver for Thunderbolt 3 Alternate Mode Abhishek Pandit-Subedi
2024-09-26 14:06 ` Heikki Krogerus
@ 2024-09-26 15:14 ` kernel test robot
1 sibling, 0 replies; 15+ messages in thread
From: kernel test robot @ 2024-09-26 15:14 UTC (permalink / raw)
To: Abhishek Pandit-Subedi, heikki.krogerus, tzungbi
Cc: oe-kbuild-all, jthies, pmalani, akuchynski,
Abhishek Pandit-Subedi, Greg Kroah-Hartman, linux-kernel,
linux-usb
Hi Abhishek,
kernel test robot noticed the following build warnings:
[auto build test WARNING on chrome-platform/for-next]
[also build test WARNING on chrome-platform/for-firmware-next usb/usb-testing usb/usb-next usb/usb-linus linus/master v6.11 next-20240926]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Abhishek-Pandit-Subedi/usb-typec-Add-driver-for-Thunderbolt-3-Alternate-Mode/20240926-002931
base: https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next
patch link: https://lore.kernel.org/r/20240925092505.1.I3080b036e8de0b9957c57c1c3059db7149c5e549%40changeid
patch subject: [PATCH 1/8] usb: typec: Add driver for Thunderbolt 3 Alternate Mode
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20240926/202409262224.Bnlmjakc-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240926/202409262224.Bnlmjakc-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409262224.Bnlmjakc-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/usb/typec/altmodes/thunderbolt.c:15: warning: expecting prototype for USB Typec(). Prototype was for USB_TYPEC_VENDOR_INTEL() instead
vim +15 drivers/usb/typec/altmodes/thunderbolt.c
14
> 15 #define USB_TYPEC_VENDOR_INTEL 0x8087
16
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/8] usb: typec: altmode_match should handle TYPEC_ANY_MODE
2024-09-25 16:25 [PATCH 0/8] Thunderbolt and DP altmode support for cros-ec-typec Abhishek Pandit-Subedi
2024-09-25 16:25 ` [PATCH 1/8] usb: typec: Add driver for Thunderbolt 3 Alternate Mode Abhishek Pandit-Subedi
@ 2024-09-25 16:25 ` Abhishek Pandit-Subedi
2024-09-25 16:54 ` Dmitry Baryshkov
2024-09-25 16:25 ` [PATCH 3/8] usb: typec: intel_pmc_mux: Null check before use Abhishek Pandit-Subedi
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-09-25 16:25 UTC (permalink / raw)
To: heikki.krogerus, tzungbi
Cc: jthies, pmalani, akuchynski, Abhishek Pandit-Subedi,
Greg Kroah-Hartman, linux-kernel, linux-usb
altmode_match is used when searching for the first port altmode that
matches the partner or plug altmode. If the port registered with mode
set to TYPEC_ANY_MODE, it should always match if the SVID matches.
Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
drivers/usb/typec/class.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 9262fcd4144f..179856503d5d 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -230,7 +230,8 @@ static int altmode_match(struct device *dev, void *data)
if (!is_typec_altmode(dev))
return 0;
- return ((adev->svid == id->svid) && (adev->mode == id->mode));
+ return ((adev->svid == id->svid) &&
+ (adev->mode == id->mode || adev->mode == TYPEC_ANY_MODE));
}
static void typec_altmode_set_partner(struct altmode *altmode)
--
2.46.0.792.g87dc391469-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 2/8] usb: typec: altmode_match should handle TYPEC_ANY_MODE
2024-09-25 16:25 ` [PATCH 2/8] usb: typec: altmode_match should handle TYPEC_ANY_MODE Abhishek Pandit-Subedi
@ 2024-09-25 16:54 ` Dmitry Baryshkov
2024-09-25 17:31 ` Abhishek Pandit-Subedi
0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Baryshkov @ 2024-09-25 16:54 UTC (permalink / raw)
To: Abhishek Pandit-Subedi
Cc: heikki.krogerus, tzungbi, jthies, pmalani, akuchynski,
Greg Kroah-Hartman, linux-kernel, linux-usb
On Wed, Sep 25, 2024 at 09:25:03AM GMT, Abhishek Pandit-Subedi wrote:
> altmode_match is used when searching for the first port altmode that
> matches the partner or plug altmode. If the port registered with mode
> set to TYPEC_ANY_MODE, it should always match if the SVID matches.
>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Fixes?
> ---
>
> drivers/usb/typec/class.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index 9262fcd4144f..179856503d5d 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -230,7 +230,8 @@ static int altmode_match(struct device *dev, void *data)
> if (!is_typec_altmode(dev))
> return 0;
>
> - return ((adev->svid == id->svid) && (adev->mode == id->mode));
> + return ((adev->svid == id->svid) &&
> + (adev->mode == id->mode || adev->mode == TYPEC_ANY_MODE));
> }
>
> static void typec_altmode_set_partner(struct altmode *altmode)
> --
> 2.46.0.792.g87dc391469-goog
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/8] usb: typec: altmode_match should handle TYPEC_ANY_MODE
2024-09-25 16:54 ` Dmitry Baryshkov
@ 2024-09-25 17:31 ` Abhishek Pandit-Subedi
2024-09-26 14:35 ` Heikki Krogerus
0 siblings, 1 reply; 15+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-09-25 17:31 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: heikki.krogerus, tzungbi, jthies, pmalani, akuchynski,
Greg Kroah-Hartman, linux-kernel, linux-usb
On Wed, Sep 25, 2024 at 9:54 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Wed, Sep 25, 2024 at 09:25:03AM GMT, Abhishek Pandit-Subedi wrote:
> > altmode_match is used when searching for the first port altmode that
> > matches the partner or plug altmode. If the port registered with mode
> > set to TYPEC_ANY_MODE, it should always match if the SVID matches.
> >
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>
> Fixes?
This is new for Thunderbolt which registers as TYPEC_ANY_MODE so
there's no FIXES. I think Heikki may need to chime in on how the
`mode` is supposed to be used.
IMO, it may be appropriate to get rid of the mode check entirely.
>
> > ---
> >
> > drivers/usb/typec/class.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > index 9262fcd4144f..179856503d5d 100644
> > --- a/drivers/usb/typec/class.c
> > +++ b/drivers/usb/typec/class.c
> > @@ -230,7 +230,8 @@ static int altmode_match(struct device *dev, void *data)
> > if (!is_typec_altmode(dev))
> > return 0;
> >
> > - return ((adev->svid == id->svid) && (adev->mode == id->mode));
> > + return ((adev->svid == id->svid) &&
> > + (adev->mode == id->mode || adev->mode == TYPEC_ANY_MODE));
> > }
> >
> > static void typec_altmode_set_partner(struct altmode *altmode)
> > --
> > 2.46.0.792.g87dc391469-goog
> >
>
> --
> With best wishes
> Dmitry
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/8] usb: typec: altmode_match should handle TYPEC_ANY_MODE
2024-09-25 17:31 ` Abhishek Pandit-Subedi
@ 2024-09-26 14:35 ` Heikki Krogerus
0 siblings, 0 replies; 15+ messages in thread
From: Heikki Krogerus @ 2024-09-26 14:35 UTC (permalink / raw)
To: Abhishek Pandit-Subedi
Cc: Dmitry Baryshkov, tzungbi, jthies, pmalani, akuchynski,
Greg Kroah-Hartman, linux-kernel, linux-usb
On Wed, Sep 25, 2024 at 10:31:33AM -0700, Abhishek Pandit-Subedi wrote:
> On Wed, Sep 25, 2024 at 9:54 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Wed, Sep 25, 2024 at 09:25:03AM GMT, Abhishek Pandit-Subedi wrote:
> > > altmode_match is used when searching for the first port altmode that
> > > matches the partner or plug altmode. If the port registered with mode
> > > set to TYPEC_ANY_MODE, it should always match if the SVID matches.
> > >
> > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> >
> > Fixes?
>
> This is new for Thunderbolt which registers as TYPEC_ANY_MODE so
> there's no FIXES. I think Heikki may need to chime in on how the
> `mode` is supposed to be used.
>
> IMO, it may be appropriate to get rid of the mode check entirely.
It's probable okay to just drop it.
> >
> > > ---
> > >
> > > drivers/usb/typec/class.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > > index 9262fcd4144f..179856503d5d 100644
> > > --- a/drivers/usb/typec/class.c
> > > +++ b/drivers/usb/typec/class.c
> > > @@ -230,7 +230,8 @@ static int altmode_match(struct device *dev, void *data)
> > > if (!is_typec_altmode(dev))
> > > return 0;
> > >
> > > - return ((adev->svid == id->svid) && (adev->mode == id->mode));
> > > + return ((adev->svid == id->svid) &&
> > > + (adev->mode == id->mode || adev->mode == TYPEC_ANY_MODE));
> > > }
> > >
> > > static void typec_altmode_set_partner(struct altmode *altmode)
> > > --
> > > 2.46.0.792.g87dc391469-goog
> > >
> >
> > --
> > With best wishes
> > Dmitry
--
heikki
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/8] usb: typec: intel_pmc_mux: Null check before use
2024-09-25 16:25 [PATCH 0/8] Thunderbolt and DP altmode support for cros-ec-typec Abhishek Pandit-Subedi
2024-09-25 16:25 ` [PATCH 1/8] usb: typec: Add driver for Thunderbolt 3 Alternate Mode Abhishek Pandit-Subedi
2024-09-25 16:25 ` [PATCH 2/8] usb: typec: altmode_match should handle TYPEC_ANY_MODE Abhishek Pandit-Subedi
@ 2024-09-25 16:25 ` Abhishek Pandit-Subedi
2024-09-25 16:54 ` Dmitry Baryshkov
2024-09-26 14:37 ` Heikki Krogerus
2024-09-25 16:25 ` [PATCH 4/8] usb: typec: Auto enter control for alternate modes Abhishek Pandit-Subedi
2024-09-25 17:12 ` [PATCH 0/8] Thunderbolt and DP altmode support for cros-ec-typec Dmitry Baryshkov
4 siblings, 2 replies; 15+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-09-25 16:25 UTC (permalink / raw)
To: heikki.krogerus, tzungbi
Cc: jthies, pmalani, akuchynski, Abhishek Pandit-Subedi,
Greg Kroah-Hartman, linux-kernel, linux-usb
Make sure the data pointer in typec_mux_state is not null before
accessing it.
Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
drivers/usb/typec/mux/intel_pmc_mux.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
index 56989a0d0f43..4283fead9a69 100644
--- a/drivers/usb/typec/mux/intel_pmc_mux.c
+++ b/drivers/usb/typec/mux/intel_pmc_mux.c
@@ -331,14 +331,19 @@ static int
pmc_usb_mux_tbt(struct pmc_usb_port *port, struct typec_mux_state *state)
{
struct typec_thunderbolt_data *data = state->data;
- u8 cable_rounded = TBT_CABLE_ROUNDED_SUPPORT(data->cable_mode);
- u8 cable_speed = TBT_CABLE_SPEED(data->cable_mode);
+ u8 cable_rounded, cable_speed;
struct altmode_req req = { };
+ if (!data)
+ return 0;
+
if (IOM_PORT_ACTIVITY_IS(port->iom_status, TBT) ||
IOM_PORT_ACTIVITY_IS(port->iom_status, ALT_MODE_TBT_USB))
return 0;
+ cable_rounded = TBT_CABLE_ROUNDED_SUPPORT(data->cable_mode);
+ cable_speed = TBT_CABLE_SPEED(data->cable_mode);
+
req.usage = PMC_USB_ALT_MODE;
req.usage |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
req.mode_type = PMC_USB_MODE_TYPE_TBT << PMC_USB_MODE_TYPE_SHIFT;
--
2.46.0.792.g87dc391469-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 3/8] usb: typec: intel_pmc_mux: Null check before use
2024-09-25 16:25 ` [PATCH 3/8] usb: typec: intel_pmc_mux: Null check before use Abhishek Pandit-Subedi
@ 2024-09-25 16:54 ` Dmitry Baryshkov
2024-09-25 17:29 ` Abhishek Pandit-Subedi
2024-09-26 14:37 ` Heikki Krogerus
1 sibling, 1 reply; 15+ messages in thread
From: Dmitry Baryshkov @ 2024-09-25 16:54 UTC (permalink / raw)
To: Abhishek Pandit-Subedi
Cc: heikki.krogerus, tzungbi, jthies, pmalani, akuchynski,
Greg Kroah-Hartman, linux-kernel, linux-usb
On Wed, Sep 25, 2024 at 09:25:04AM GMT, Abhishek Pandit-Subedi wrote:
> Make sure the data pointer in typec_mux_state is not null before
> accessing it.
>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Is the a fix for an actual issue or just good-to-have thing? In the
former case it lacks a description of how the issue can be triggered and
a Fixes tag.
> ---
>
> drivers/usb/typec/mux/intel_pmc_mux.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
> index 56989a0d0f43..4283fead9a69 100644
> --- a/drivers/usb/typec/mux/intel_pmc_mux.c
> +++ b/drivers/usb/typec/mux/intel_pmc_mux.c
> @@ -331,14 +331,19 @@ static int
> pmc_usb_mux_tbt(struct pmc_usb_port *port, struct typec_mux_state *state)
> {
> struct typec_thunderbolt_data *data = state->data;
> - u8 cable_rounded = TBT_CABLE_ROUNDED_SUPPORT(data->cable_mode);
> - u8 cable_speed = TBT_CABLE_SPEED(data->cable_mode);
> + u8 cable_rounded, cable_speed;
> struct altmode_req req = { };
>
> + if (!data)
> + return 0;
> +
> if (IOM_PORT_ACTIVITY_IS(port->iom_status, TBT) ||
> IOM_PORT_ACTIVITY_IS(port->iom_status, ALT_MODE_TBT_USB))
> return 0;
>
> + cable_rounded = TBT_CABLE_ROUNDED_SUPPORT(data->cable_mode);
> + cable_speed = TBT_CABLE_SPEED(data->cable_mode);
> +
> req.usage = PMC_USB_ALT_MODE;
> req.usage |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
> req.mode_type = PMC_USB_MODE_TYPE_TBT << PMC_USB_MODE_TYPE_SHIFT;
> --
> 2.46.0.792.g87dc391469-goog
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/8] usb: typec: intel_pmc_mux: Null check before use
2024-09-25 16:54 ` Dmitry Baryshkov
@ 2024-09-25 17:29 ` Abhishek Pandit-Subedi
0 siblings, 0 replies; 15+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-09-25 17:29 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: heikki.krogerus, tzungbi, jthies, pmalani, akuchynski,
Greg Kroah-Hartman, linux-kernel, linux-usb
On Wed, Sep 25, 2024 at 9:54 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Wed, Sep 25, 2024 at 09:25:04AM GMT, Abhishek Pandit-Subedi wrote:
> > Make sure the data pointer in typec_mux_state is not null before
> > accessing it.
> >
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>
> Is the a fix for an actual issue or just good-to-have thing? In the
> former case it lacks a description of how the issue can be triggered and
> a Fixes tag.
This fixes a segfault that occurs when the new Thunderbolt driver is
used because it calls `typec_altmode_notify` with null data. I'm not
sure if that needs a `Fixes` since what's currently running upstream
doesn't actually trigger this error.
I'll update the description with why this is needed. i.e.
---
Make sure the data pointer in typec_mux_state is not null before
accessing it. The new Thunderbolt driver calls typec_altmode_notify
with a NULL pointer for data which can cause this mux configuration
to crash.
>
> > ---
> >
> > drivers/usb/typec/mux/intel_pmc_mux.c | 9 +++++++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
> > index 56989a0d0f43..4283fead9a69 100644
> > --- a/drivers/usb/typec/mux/intel_pmc_mux.c
> > +++ b/drivers/usb/typec/mux/intel_pmc_mux.c
> > @@ -331,14 +331,19 @@ static int
> > pmc_usb_mux_tbt(struct pmc_usb_port *port, struct typec_mux_state *state)
> > {
> > struct typec_thunderbolt_data *data = state->data;
> > - u8 cable_rounded = TBT_CABLE_ROUNDED_SUPPORT(data->cable_mode);
> > - u8 cable_speed = TBT_CABLE_SPEED(data->cable_mode);
> > + u8 cable_rounded, cable_speed;
> > struct altmode_req req = { };
> >
> > + if (!data)
> > + return 0;
> > +
> > if (IOM_PORT_ACTIVITY_IS(port->iom_status, TBT) ||
> > IOM_PORT_ACTIVITY_IS(port->iom_status, ALT_MODE_TBT_USB))
> > return 0;
> >
> > + cable_rounded = TBT_CABLE_ROUNDED_SUPPORT(data->cable_mode);
> > + cable_speed = TBT_CABLE_SPEED(data->cable_mode);
> > +
> > req.usage = PMC_USB_ALT_MODE;
> > req.usage |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
> > req.mode_type = PMC_USB_MODE_TYPE_TBT << PMC_USB_MODE_TYPE_SHIFT;
> > --
> > 2.46.0.792.g87dc391469-goog
> >
>
> --
> With best wishes
> Dmitry
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/8] usb: typec: intel_pmc_mux: Null check before use
2024-09-25 16:25 ` [PATCH 3/8] usb: typec: intel_pmc_mux: Null check before use Abhishek Pandit-Subedi
2024-09-25 16:54 ` Dmitry Baryshkov
@ 2024-09-26 14:37 ` Heikki Krogerus
1 sibling, 0 replies; 15+ messages in thread
From: Heikki Krogerus @ 2024-09-26 14:37 UTC (permalink / raw)
To: Abhishek Pandit-Subedi
Cc: tzungbi, jthies, pmalani, akuchynski, Greg Kroah-Hartman,
linux-kernel, linux-usb
On Wed, Sep 25, 2024 at 09:25:04AM -0700, Abhishek Pandit-Subedi wrote:
> Make sure the data pointer in typec_mux_state is not null before
> accessing it.
This really should not be necessary.
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
>
> drivers/usb/typec/mux/intel_pmc_mux.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
> index 56989a0d0f43..4283fead9a69 100644
> --- a/drivers/usb/typec/mux/intel_pmc_mux.c
> +++ b/drivers/usb/typec/mux/intel_pmc_mux.c
> @@ -331,14 +331,19 @@ static int
> pmc_usb_mux_tbt(struct pmc_usb_port *port, struct typec_mux_state *state)
> {
> struct typec_thunderbolt_data *data = state->data;
> - u8 cable_rounded = TBT_CABLE_ROUNDED_SUPPORT(data->cable_mode);
> - u8 cable_speed = TBT_CABLE_SPEED(data->cable_mode);
> + u8 cable_rounded, cable_speed;
> struct altmode_req req = { };
>
> + if (!data)
> + return 0;
> +
> if (IOM_PORT_ACTIVITY_IS(port->iom_status, TBT) ||
> IOM_PORT_ACTIVITY_IS(port->iom_status, ALT_MODE_TBT_USB))
> return 0;
>
> + cable_rounded = TBT_CABLE_ROUNDED_SUPPORT(data->cable_mode);
> + cable_speed = TBT_CABLE_SPEED(data->cable_mode);
> +
> req.usage = PMC_USB_ALT_MODE;
> req.usage |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
> req.mode_type = PMC_USB_MODE_TYPE_TBT << PMC_USB_MODE_TYPE_SHIFT;
> --
> 2.46.0.792.g87dc391469-goog
--
heikki
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 4/8] usb: typec: Auto enter control for alternate modes
2024-09-25 16:25 [PATCH 0/8] Thunderbolt and DP altmode support for cros-ec-typec Abhishek Pandit-Subedi
` (2 preceding siblings ...)
2024-09-25 16:25 ` [PATCH 3/8] usb: typec: intel_pmc_mux: Null check before use Abhishek Pandit-Subedi
@ 2024-09-25 16:25 ` Abhishek Pandit-Subedi
2024-09-25 17:12 ` [PATCH 0/8] Thunderbolt and DP altmode support for cros-ec-typec Dmitry Baryshkov
4 siblings, 0 replies; 15+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-09-25 16:25 UTC (permalink / raw)
To: heikki.krogerus, tzungbi
Cc: jthies, pmalani, akuchynski, Abhishek Pandit-Subedi,
Greg Kroah-Hartman, linux-kernel, linux-usb
Add controls for whether an alternate mode is automatically entered when
a partner connects. The auto_enter control is only available on ports
and applies immediately after a partner connects. The default behavior
is to enable auto enter and drivers must explicitly disable it.
Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
Documentation/ABI/testing/sysfs-bus-typec | 9 +++++++
drivers/usb/typec/altmodes/displayport.c | 6 +++--
drivers/usb/typec/altmodes/thunderbolt.c | 3 ++-
drivers/usb/typec/class.c | 31 +++++++++++++++++++++++
include/linux/usb/typec.h | 2 ++
include/linux/usb/typec_altmode.h | 2 ++
6 files changed, 50 insertions(+), 3 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-bus-typec b/Documentation/ABI/testing/sysfs-bus-typec
index 205d9c91e2e1..f09d05727b82 100644
--- a/Documentation/ABI/testing/sysfs-bus-typec
+++ b/Documentation/ABI/testing/sysfs-bus-typec
@@ -12,6 +12,15 @@ Description:
Valid values are boolean.
+What: /sys/bus/typec/devices/.../auto_enter
+Date: September 2024
+Contact: Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+ Controls whether a mode will be automatically entered when a partner is
+ connected.
+
+ This field is only valid and displayed on a port. Valid values are boolean.
+
What: /sys/bus/typec/devices/.../description
Date: July 2018
Contact: Heikki Krogerus <heikki.krogerus@linux.intel.com>
diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
index 92cc1b136120..7b164086bbbb 100644
--- a/drivers/usb/typec/altmodes/displayport.c
+++ b/drivers/usb/typec/altmodes/displayport.c
@@ -767,8 +767,10 @@ int dp_altmode_probe(struct typec_altmode *alt)
if (plug)
typec_altmode_set_drvdata(plug, dp);
- dp->state = plug ? DP_STATE_ENTER_PRIME : DP_STATE_ENTER;
- schedule_work(&dp->work);
+ if (port->auto_enter) {
+ dp->state = plug ? DP_STATE_ENTER_PRIME : DP_STATE_ENTER;
+ schedule_work(&dp->work);
+ }
return 0;
}
diff --git a/drivers/usb/typec/altmodes/thunderbolt.c b/drivers/usb/typec/altmodes/thunderbolt.c
index 515e775ee41a..ba2ddaf3245e 100644
--- a/drivers/usb/typec/altmodes/thunderbolt.c
+++ b/drivers/usb/typec/altmodes/thunderbolt.c
@@ -225,6 +225,7 @@ static const struct typec_altmode_ops tbt_altmode_ops = {
static int tbt_altmode_probe(struct typec_altmode *alt)
{
+ const struct typec_altmode *port = typec_altmode_get_partner(alt);
struct tbt_altmode *tbt;
tbt = devm_kzalloc(&alt->dev, sizeof(*tbt), GFP_KERNEL);
@@ -239,7 +240,7 @@ static int tbt_altmode_probe(struct typec_altmode *alt)
typec_altmode_set_drvdata(alt, tbt);
typec_altmode_set_ops(alt, &tbt_altmode_ops);
- if (tbt_ready(alt)) {
+ if (port->auto_enter && tbt_ready(alt)) {
if (tbt->plug[TYPEC_PLUG_SOP_PP])
tbt->state = TBT_STATE_SOP_PP_ENTER;
else if (tbt->plug[TYPEC_PLUG_SOP_P])
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 179856503d5d..a7ae0cdecca0 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -397,6 +397,31 @@ static ssize_t active_store(struct device *dev, struct device_attribute *attr,
}
static DEVICE_ATTR_RW(active);
+static ssize_t
+auto_enter_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct typec_altmode *alt = to_typec_altmode(dev);
+
+ return sprintf(buf, "%s\n", alt->auto_enter ? "yes" : "no");
+}
+
+static ssize_t auto_enter_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t size)
+{
+ struct typec_altmode *adev = to_typec_altmode(dev);
+ bool auto_enter;
+ int ret;
+
+ ret = kstrtobool(buf, &auto_enter);
+ if (ret)
+ return ret;
+
+ adev->auto_enter = auto_enter;
+
+ return size;
+}
+static DEVICE_ATTR_RW(auto_enter);
+
static ssize_t
supported_roles_show(struct device *dev, struct device_attribute *attr,
char *buf)
@@ -440,6 +465,7 @@ static DEVICE_ATTR_RO(svid);
static struct attribute *typec_altmode_attrs[] = {
&dev_attr_active.attr,
+ &dev_attr_auto_enter.attr,
&dev_attr_mode.attr,
&dev_attr_svid.attr,
&dev_attr_vdo.attr,
@@ -455,6 +481,10 @@ static umode_t typec_altmode_attr_is_visible(struct kobject *kobj,
if (!adev->ops || !adev->ops->activate)
return 0444;
+ if (attr == &dev_attr_auto_enter.attr)
+ if (!is_typec_port(adev->dev.parent))
+ return 0;
+
return attr->mode;
}
@@ -557,6 +587,7 @@ typec_register_altmode(struct device *parent,
if (is_port) {
alt->attrs[3] = &dev_attr_supported_roles.attr;
alt->adev.active = true; /* Enabled by default */
+ alt->adev.auto_enter = !desc->no_auto_enter;
}
sprintf(alt->group_name, "mode%d", desc->mode);
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index 549275f8ac1b..67242355f78e 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -128,6 +128,7 @@ int typec_cable_set_identity(struct typec_cable *cable);
* @svid: Standard or Vendor ID
* @mode: Index of the Mode
* @vdo: VDO returned by Discover Modes USB PD command
+ * @no_auto_enter: Only for ports. Disables auto enter which is default behavior.
* @roles: Only for ports. DRP if the mode is available in both roles
*
* Description of an Alternate Mode which a connector, cable plug or partner
@@ -137,6 +138,7 @@ struct typec_altmode_desc {
u16 svid;
u8 mode;
u32 vdo;
+ bool no_auto_enter;
/* Only used with ports */
enum typec_port_data roles;
};
diff --git a/include/linux/usb/typec_altmode.h b/include/linux/usb/typec_altmode.h
index b3c0866ea70f..ab7c3ebe4926 100644
--- a/include/linux/usb/typec_altmode.h
+++ b/include/linux/usb/typec_altmode.h
@@ -18,6 +18,7 @@ struct typec_altmode_ops;
* @mode: Index of the Mode
* @vdo: VDO returned by Discover Modes USB PD command
* @active: Tells has the mode been entered or not
+ * @auto_enter: Tells whether to auto-enter mode (only valid for port mode).
* @desc: Optional human readable description of the mode
* @ops: Operations vector from the driver
* @cable_ops: Cable operations vector from the driver.
@@ -28,6 +29,7 @@ struct typec_altmode {
int mode;
u32 vdo;
unsigned int active:1;
+ unsigned int auto_enter:1;
char *desc;
const struct typec_altmode_ops *ops;
--
2.46.0.792.g87dc391469-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 0/8] Thunderbolt and DP altmode support for cros-ec-typec
2024-09-25 16:25 [PATCH 0/8] Thunderbolt and DP altmode support for cros-ec-typec Abhishek Pandit-Subedi
` (3 preceding siblings ...)
2024-09-25 16:25 ` [PATCH 4/8] usb: typec: Auto enter control for alternate modes Abhishek Pandit-Subedi
@ 2024-09-25 17:12 ` Dmitry Baryshkov
2024-09-25 17:20 ` Abhishek Pandit-Subedi
4 siblings, 1 reply; 15+ messages in thread
From: Dmitry Baryshkov @ 2024-09-25 17:12 UTC (permalink / raw)
To: Abhishek Pandit-Subedi
Cc: heikki.krogerus, tzungbi, jthies, pmalani, akuchynski,
Benson Leung, Greg Kroah-Hartman, Guenter Roeck, chrome-platform,
linux-kernel, linux-usb
On Wed, Sep 25, 2024 at 09:25:01AM GMT, Abhishek Pandit-Subedi wrote:
>
> Hi Heikki, Tzung-Bi et al,
>
> This patch series adds support for alternate mode entry for the
> cros-ec-typec driver for Displayport and Thunderbolt.
>
> Thunderbolt support is added by adapting an RFC Heikki had posted
> previously:
>
> https://lore.kernel.org/linux-usb/20191230152857.43917-1-heikki.krogerus@linux.intel.com/
>
> A few comments on the series:
>
> * The cros-ec interface will not accept any VDOs/VDMs so we simply
> ignore any configurations we are passed (i.e. DPConfigure). This means
> the sysfs control of DP lanes won't work.
> * ChromeOS has two modes of operation for alt-modes: entirely EC driven
> or AP-driven from userspace (via the typec daemon). Thus, we never
> expect the kernel alt-mode drivers to auto-enter modes.
>
> This was tested on kernel 6.6 with a ChromeOS Brya device and compile
> tested against linux-usb (with allmodconfig).
Please test on top of the linux-usb or linux-next. 6.6 is nine months
old kernel.
Also for v2 please consider CC'ing both lists for all patches. Otherwise
it's hard to follow the changes.
>
> Thanks,
> Abhishek
>
>
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 0/8] Thunderbolt and DP altmode support for cros-ec-typec
2024-09-25 17:12 ` [PATCH 0/8] Thunderbolt and DP altmode support for cros-ec-typec Dmitry Baryshkov
@ 2024-09-25 17:20 ` Abhishek Pandit-Subedi
0 siblings, 0 replies; 15+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-09-25 17:20 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: heikki.krogerus, tzungbi, jthies, pmalani, akuchynski,
Benson Leung, Greg Kroah-Hartman, Guenter Roeck, chrome-platform,
linux-kernel, linux-usb
On Wed, Sep 25, 2024 at 10:12 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Wed, Sep 25, 2024 at 09:25:01AM GMT, Abhishek Pandit-Subedi wrote:
> >
> > Hi Heikki, Tzung-Bi et al,
> >
> > This patch series adds support for alternate mode entry for the
> > cros-ec-typec driver for Displayport and Thunderbolt.
> >
> > Thunderbolt support is added by adapting an RFC Heikki had posted
> > previously:
> >
> > https://lore.kernel.org/linux-usb/20191230152857.43917-1-heikki.krogerus@linux.intel.com/
> >
> > A few comments on the series:
> >
> > * The cros-ec interface will not accept any VDOs/VDMs so we simply
> > ignore any configurations we are passed (i.e. DPConfigure). This means
> > the sysfs control of DP lanes won't work.
> > * ChromeOS has two modes of operation for alt-modes: entirely EC driven
> > or AP-driven from userspace (via the typec daemon). Thus, we never
> > expect the kernel alt-mode drivers to auto-enter modes.
> >
> > This was tested on kernel 6.6 with a ChromeOS Brya device and compile
> > tested against linux-usb (with allmodconfig).
>
> Please test on top of the linux-usb or linux-next. 6.6 is nine months
> old kernel.
Ack -- I'm going to try to load the upstream kernel on my ChromeOS
device + test.
>
> Also for v2 please consider CC'ing both lists for all patches. Otherwise
> it's hard to follow the changes.
Really sorry about that. I'm using patman to send patches and didn't
realize it would split the patch series between the two. v2 will send
all 8 patches to both lists.
>
> >
> > Thanks,
> > Abhishek
> >
> >
> >
>
> --
> With best wishes
> Dmitry
^ permalink raw reply [flat|nested] 15+ messages in thread