| 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: <CANFp7mUOxyYfkr6Ce93aLzJXRopbvfEjq52tsa+DhKi-Y90Uvw@mail.gmail.com>
Date: Fri, 13 Dec 2024 09:55:06 -0800
From: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>
To: Stephen Boyd <swboyd@...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,
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
On Tue, Dec 10, 2024 at 4:21 PM Stephen Boyd <swboyd@...omium.org> wrote:
>
> 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.
Done
>
> > + * 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?
Compiles without it so I'm guessing no.
>
> > +#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
Done
>
> > +
> > +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?
This looks like it's protecting control flow (enter/exit/vdm can all
be triggered on probe, via .activate or potentially autonomously via
port driver triggering the alt-mode).
>
> > +};
> [...]
> > +
> > +/* MUST HOLD tbt->lock.
>
> Use lockdep_assert_held(tbt->lock) and remove the comment?
Done.
>
> > + *
> > + * 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?
Clang format missed it :(
>
> > + 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.
I think earlier in the series, they were its own patch -- got merged
down into this over several refactors. I'll pull it out into its own
patch.
Powered by blists - more mailing lists