| 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: <CAE-0n52KWd5twmXk5fHf=kfdNm27QTJMPLrU3CRb9SnrtAF_UA@mail.gmail.com>
Date: Tue, 10 Dec 2024 16:21:36 -0800
From: Stephen Boyd <swboyd@...omium.org>
To: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>, chrome-platform@...ts.linux.dev,
heikki.krogerus@...ux.intel.com, linux-usb@...r.kernel.org,
tzungbi@...nel.org
Cc: akuchynski@...gle.com, pmalani@...omium.org, jthies@...gle.com,
dmitry.baryshkov@...aro.org, badhri@...gle.com, rdbabiera@...gle.com,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/7] usb: typec: Add driver for Thunderbolt 3 Alternate Mode
Quoting Abhishek Pandit-Subedi (2024-12-06 15:38:13)
> diff --git a/drivers/usb/typec/altmodes/thunderbolt.c b/drivers/usb/typec/altmodes/thunderbolt.c
> new file mode 100644
> index 000000000000..14e89e9a7691
> --- /dev/null
> +++ b/drivers/usb/typec/altmodes/thunderbolt.c
> @@ -0,0 +1,387 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
Remove extra *, this isn't kerneldoc.
> + * USB Typec-C Thuderbolt3 Alternate Mode driver
> + *
> + * Copyright (C) 2019 Intel Corporation
> + * Author: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> + */
> +
> +#include <linux/delay.h>
Is this include used?
> +#include <linux/mutex.h>
> +#include <linux/module.h>
> +#include <linux/usb/pd_vdo.h>
> +#include <linux/usb/typec_altmode.h>
> +#include <linux/usb/typec_tbt.h>
Please include workqueue.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];
> + u32 enter_vdo;
> +
> + struct work_struct work;
> + struct mutex lock; /* device lock */
What does it protect? The whole struct tbt_altmode?
> +};
[...]
> +
> +/* MUST HOLD tbt->lock.
Use lockdep_assert_held(tbt->lock) and remove the comment?
> + *
> + * If SOP' is available, enter that first (which will trigger a VDM response
> + * that will enter SOP" if available and then the port). If entering SOP' fails,
> + * stop attempting to enter either cable altmode (probably not supported) and
> + * directly enter the port altmode.
> + */
> +static int tbt_enter_modes_ordered(struct typec_altmode *alt)
> +{
> + struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
> + int ret = 0;
> +
> + if (!tbt_ready(tbt->alt))
> + return -ENODEV;
> +
> + if (tbt->plug[TYPEC_PLUG_SOP_P]) {
> + ret = typec_cable_altmode_enter(alt, TYPEC_PLUG_SOP_P, NULL);
> + if (ret < 0) {
> + for (int i = TYPEC_PLUG_SOP_PP; i > 0; --i) {
> + if (tbt->plug[i])
> + typec_altmode_put_plug(tbt->plug[i]);
> +
> + tbt->plug[i] = NULL;
> + }
> + } else {
> + return ret;
> + }
> + }
> +
> + return tbt_enter_mode(tbt);
> +}
> +
> +static int tbt_cable_altmode_vdm(struct typec_altmode *alt,
> + enum typec_plug_index sop, const u32 hdr,
> + const u32 *vdo, int count)
> +{
[...]
> + case CMD_EXIT_MODE:
> + /* Exit in opposite order: Port, SOP", then SOP'. */
> + if (sop == TYPEC_PLUG_SOP_PP)
> + tbt->state = TBT_STATE_SOP_P_EXIT;
> + break;
> + }
> + break;
> + default:
> + break;
> + }
> +
> + if (tbt->state != TBT_STATE_IDLE)
> + schedule_work(&tbt->work);
> +
> +
Nitpick: Why two newlines?
> + mutex_unlock(&tbt->lock);
> + return 0;
> +}
> +
[...]
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index febe453b96be..b5e67a57762c 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -458,7 +458,8 @@ static umode_t typec_altmode_attr_is_visible(struct kobject *kobj,
> struct typec_altmode *adev = to_typec_altmode(kobj_to_dev(kobj));
>
> if (attr == &dev_attr_active.attr)
> - if (!adev->ops || !adev->ops->activate)
> + if (!is_typec_port(adev->dev.parent) &&
> + (!adev->ops || !adev->ops->activate))
> return 0444;
>
> return attr->mode;
> @@ -563,7 +564,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.active = !desc->inactive; /* Enabled by default */
> }
>
> sprintf(alt->group_name, "mode%d", desc->mode);
> diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
> index d616b8807000..252af3f77039 100644
> --- a/include/linux/usb/typec.h
> +++ b/include/linux/usb/typec.h
> @@ -140,6 +140,7 @@ int typec_cable_set_identity(struct typec_cable *cable);
> * @mode: Index of the Mode
> * @vdo: VDO returned by Discover Modes USB PD command
> * @roles: Only for ports. DRP if the mode is available in both roles
> + * @inactive: Only for ports. Make this port inactive (default is active).
> *
> * Description of an Alternate Mode which a connector, cable plug or partner
> * supports.
> @@ -150,6 +151,7 @@ struct typec_altmode_desc {
> u32 vdo;
> /* Only used with ports */
> enum typec_port_data roles;
> + bool inactive;
> };
>
> void typec_partner_set_pd_revision(struct typec_partner *partner, u16 pd_revision);
These two files look like they can be a different patch? Or the commit
text can describe these changes.
Powered by blists - more mailing lists