| Message ID | 20241107112955.v3.3.I439cffc7bf76d94f5850eb85980f1197c4f9154c@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Thunderbolt and DP altmode support for cros-ec-typec | expand |
On Thu, Nov 07, 2024 at 11:29:56AM -0800, Abhishek Pandit-Subedi wrote: > Enforce the same requirement as when we attempt to activate altmode via > sysfs (do not enter partner mode if port mode is not active). In order > to set a default value for this field, also introduce the inactive field > in struct typec_altmode_desc. > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > > Changes in v3: > - Use port.active instead of introducing auto-enter field > - Introduce inactive field to typec_altmode_desc to set default value > for active. > - Always make port 'active' field writable > > drivers/usb/typec/altmodes/displayport.c | 7 +++++-- > drivers/usb/typec/altmodes/thunderbolt.c | 6 +++++- > drivers/usb/typec/class.c | 5 +++-- > include/linux/usb/typec.h | 2 ++ > 4 files changed, 15 insertions(+), 5 deletions(-) > > diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c > index 3245e03d59e6..f4116e96f6a1 100644 > --- a/drivers/usb/typec/altmodes/displayport.c > +++ b/drivers/usb/typec/altmodes/displayport.c > @@ -767,8 +767,11 @@ 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); > + /* Only attempt to enter altmode if port is active. */ > + if (port->active) { > + 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 a945b9d35a1d..45567abc3bb8 100644 > --- a/drivers/usb/typec/altmodes/thunderbolt.c > +++ b/drivers/usb/typec/altmodes/thunderbolt.c > @@ -212,6 +212,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); > @@ -226,7 +227,10 @@ 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)) { > + /* Only attempt to enter altmode if port is active and cable/plug > + * information is ready. > + */ > + if (port->active && 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 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..56c01771c190 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 : 1; > }; > > void typec_partner_set_pd_revision(struct typec_partner *partner, u16 pd_revision); > -- > 2.47.0.277.g8800431eea-goog
On Thu, Nov 07, 2024 at 11:29:56AM -0800, Abhishek Pandit-Subedi wrote: > Enforce the same requirement as when we attempt to activate altmode via > sysfs (do not enter partner mode if port mode is not active). In order > to set a default value for this field, also introduce the inactive field > in struct typec_altmode_desc. > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > --- > > Changes in v3: > - Use port.active instead of introducing auto-enter field > - Introduce inactive field to typec_altmode_desc to set default value > for active. > - Always make port 'active' field writable > > drivers/usb/typec/altmodes/displayport.c | 7 +++++-- > drivers/usb/typec/altmodes/thunderbolt.c | 6 +++++- > drivers/usb/typec/class.c | 5 +++-- > include/linux/usb/typec.h | 2 ++ > 4 files changed, 15 insertions(+), 5 deletions(-) > > diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c > index 3245e03d59e6..f4116e96f6a1 100644 > --- a/drivers/usb/typec/altmodes/displayport.c > +++ b/drivers/usb/typec/altmodes/displayport.c > @@ -767,8 +767,11 @@ 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); > + /* Only attempt to enter altmode if port is active. */ > + if (port->active) { > + dp->state = plug ? DP_STATE_ENTER_PRIME : DP_STATE_ENTER; > + schedule_work(&dp->work); > + } What prevents active from changing right after you checked it? > @@ -150,6 +151,7 @@ struct typec_altmode_desc { > u32 vdo; > /* Only used with ports */ > enum typec_port_data roles; > + bool inactive : 1; A boolean bitfield? That's not needed, please just do this properly. thanks, greg k-h
On Fri, Nov 8, 2024 at 5:17 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Thu, Nov 07, 2024 at 11:29:56AM -0800, Abhishek Pandit-Subedi wrote: > > Enforce the same requirement as when we attempt to activate altmode via > > sysfs (do not enter partner mode if port mode is not active). In order > > to set a default value for this field, also introduce the inactive field > > in struct typec_altmode_desc. > > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > --- > > > > Changes in v3: > > - Use port.active instead of introducing auto-enter field > > - Introduce inactive field to typec_altmode_desc to set default value > > for active. > > - Always make port 'active' field writable > > > > drivers/usb/typec/altmodes/displayport.c | 7 +++++-- > > drivers/usb/typec/altmodes/thunderbolt.c | 6 +++++- > > drivers/usb/typec/class.c | 5 +++-- > > include/linux/usb/typec.h | 2 ++ > > 4 files changed, 15 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c > > index 3245e03d59e6..f4116e96f6a1 100644 > > --- a/drivers/usb/typec/altmodes/displayport.c > > +++ b/drivers/usb/typec/altmodes/displayport.c > > @@ -767,8 +767,11 @@ 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); > > + /* Only attempt to enter altmode if port is active. */ > > + if (port->active) { > > + dp->state = plug ? DP_STATE_ENTER_PRIME : DP_STATE_ENTER; > > + schedule_work(&dp->work); > > + } > > What prevents active from changing right after you checked it? There's another check in `typec_altmode_enter` for the port itself: https://github.com/torvalds/linux/blob/master/drivers/usb/typec/bus.c#L138 If I leave this out, it will just fail in `dp_altmode_work` when it calls `typec_altmode_enter` and will leave a dev_err("failed to enter mode"). This might be more user friendly since it's visible to the user that the partner supports the mode but the port blocked entry. I'll update the message on mode entry to also print the return value (-EPERM) in the next patch. > > > @@ -150,6 +151,7 @@ struct typec_altmode_desc { > > u32 vdo; > > /* Only used with ports */ > > enum typec_port_data roles; > > + bool inactive : 1; > > A boolean bitfield? That's not needed, please just do this properly. Ack - will remove the bitfield. `struct typec_altmode` also does this -- I'll remove that in a cleanup patch too. > > thanks, > > greg k-h
diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c index 3245e03d59e6..f4116e96f6a1 100644 --- a/drivers/usb/typec/altmodes/displayport.c +++ b/drivers/usb/typec/altmodes/displayport.c @@ -767,8 +767,11 @@ 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); + /* Only attempt to enter altmode if port is active. */ + if (port->active) { + 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 a945b9d35a1d..45567abc3bb8 100644 --- a/drivers/usb/typec/altmodes/thunderbolt.c +++ b/drivers/usb/typec/altmodes/thunderbolt.c @@ -212,6 +212,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); @@ -226,7 +227,10 @@ 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)) { + /* Only attempt to enter altmode if port is active and cable/plug + * information is ready. + */ + if (port->active && 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 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..56c01771c190 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 : 1; }; void typec_partner_set_pd_revision(struct typec_partner *partner, u16 pd_revision);
Enforce the same requirement as when we attempt to activate altmode via sysfs (do not enter partner mode if port mode is not active). In order to set a default value for this field, also introduce the inactive field in struct typec_altmode_desc. Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> --- Changes in v3: - Use port.active instead of introducing auto-enter field - Introduce inactive field to typec_altmode_desc to set default value for active. - Always make port 'active' field writable drivers/usb/typec/altmodes/displayport.c | 7 +++++-- drivers/usb/typec/altmodes/thunderbolt.c | 6 +++++- drivers/usb/typec/class.c | 5 +++-- include/linux/usb/typec.h | 2 ++ 4 files changed, 15 insertions(+), 5 deletions(-)