Re: [PATCH] platform/x86: thinkpad_acpi: Add USB-C Security (USCS) support

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 11657 bytes --]

On Tue, 26 May 2026, Vishnu Sankar wrote:

> Hi Ilpo,
> 
> Thank you for the comments.
> 
> On Tue, May 26, 2026 at 2:19 AM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > On Fri, 22 May 2026, Vishnu Sankar wrote:
> >
> > > Newer ThinkPad systems expose a USB-C Security (Restricted Mode) feature.
> > > When active, USB-C data connections are disabled while power delivery is
> > > preserved. This is useful for kiosk and physically-secured deployments.
> > >
> > > Hardware interface:
> > >
> > > The HKEY device exposes a read-only ACPI method USCS():
> > >
> > >   Return value bit layout:
> > >     Bit 16 : Capability flag (1 = feature present on this SKU)
> > >     Bit  0 : Current state  (0 = security OFF, 1 = security ON)
> > >
> > > The sysfs attribute is read-only.
> > >
> > > The Fn+U followed by Fn+S hotkey chord is the only way to toggle the
> > > hardware state.
> > >
> > > Hotkey:
> > >
> > > Fn+U followed by Fn+S generates HKEY event 0x131e.
> > >
> > > sysfs interface:
> > >
> > >   /sys/devices/platform/thinkpad_acpi/usb_c_security  (read-only)
> > >   "enabled\n"  -- data connections are currently blocked
> > >   "disabled\n" -- data connections are currently allowed
> > >
> > >   The attribute is hidden on SKUs where the USCS capability bit (bit 16)
> > >   is not set, so there is no ABI impact on unsupported hardware.
> > >
> > > Suggested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> > > Signed-off-by: Vishnu Sankar <vishnuocv@gmail.com>
> > > ---
> > >  .../admin-guide/laptops/thinkpad-acpi.rst     |  24 ++++
> > >  drivers/platform/x86/lenovo/thinkpad_acpi.c   | 115 ++++++++++++++++++
> > >  2 files changed, 139 insertions(+)
> > >
> > > diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> > > index f874db31801d..db4588af0278 100644
> > > --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> > > +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> > > @@ -1543,6 +1543,30 @@ Values:
> > >
> > >       This setting can also be toggled via the Fn+doubletap hotkey.
> > >
> > > +USB-C Security
> > > +--------------
> > > +
> > > +sysfs: usb_c_security
> > > +
> > > +Reports the current state of the USB-C Security (Restricted Mode) feature
> > > +on supported ThinkPad systems. When enabled, USB-C data connections are
> > > +disabled while power delivery is preserved.
> > > +
> > > +The available command is::
> > > +
> > > +        cat /sys/devices/platform/thinkpad_acpi/usb_c_security
> > > +
> > > +Values:
> > > +
> > > +     * ``enabled``  - USB-C data connections are currently blocked
> > > +     * ``disabled`` - USB-C data connections are currently allowed
> > > +
> > > +The attribute is read-only. The USB-C Security state can only be toggled
> > > +via the Fn+U followed by Fn+S hotkey chord.
> > > +
> > > +The sysfs attribute is not created on platforms that do not support this
> > > +feature.
> > > +
> > >  Auxmac
> > >  ------
> > >
> > > diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> > > index e1cee42a1683..889db802185a 100644
> > > --- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
> > > +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> > > @@ -185,6 +185,7 @@ enum tpacpi_hkey_event_t {
> > >       TP_HKEY_EV_AMT_TOGGLE           = 0x131a, /* Toggle AMT on/off */
> > >       TP_HKEY_EV_CAMERASHUTTER_TOGGLE = 0x131b, /* Toggle Camera Shutter */
> > >       TP_HKEY_EV_DOUBLETAP_TOGGLE     = 0x131c, /* Toggle trackpoint doubletap on/off */
> > > +     TP_HKEY_EV_USB_C_SECURITY       = 0x131e, /* Toggle USB C Security ON/OFF */
> > >       TP_HKEY_EV_PROFILE_TOGGLE       = 0x131f, /* Toggle platform profile in 2024 systems */
> > >       TP_HKEY_EV_PROFILE_TOGGLE2      = 0x1401, /* Toggle platform profile in 2025 + systems */
> > >
> > > @@ -373,6 +374,8 @@ static struct {
> > >       u32 has_adaptive_kbd:1;
> > >       u32 kbd_lang:1;
> > >       u32 trackpoint_doubletap_enable:1;
> > > +     u32 usbc_security_supported:1;
> > > +     u32 usbc_security_enabled:1;
> > >       struct quirk_entry *quirks;
> > >  } tp_features;
> > >
> > > @@ -11265,6 +11268,111 @@ static struct ibm_struct hwdd_driver_data = {
> > >       .name = "hwdd",
> > >  };
> > >
> > > +/*************************************************************************
> > > + * USB-C Security subdriver
> > > + *
> > > + * HKEY.USCS(0) is a read-only ACPI method; its argument is ignored.
> > > + * It always returns:
> > > + *   bit 16 - USB-C security capability present on this SKU or not
> > > + *   bit  0 - USB-C Security state (enable or disable)
> > > + *
> > > + * Hotkey
> > > + * ------
> > > + * 0x131e (Fn+U, Fn+S): firmware toggles USBS before firing the event.
> > > + * The driver reads back the new state and notifies the sysfs attribute.
> > > + *
> > > + */
> > > +
> > > +/* USCS() return word bit layout */
> > > +#define USCS_CAP_BIT         BIT(16) /* capability: feature present on SKU */
> > > +#define USCS_STATUS_BIT              BIT(0)  /* current security state */
> > > +
> > > +static DEFINE_MUTEX(usbc_security_mutex);
> > > +
> > > +/*
> > > + * usbc_security_query - read current USB-C security state via USCS()
> > > + * @enabled: out - true when security is ON (data connections blocked)
> > > + *
> > > + * Returns true if the feature is supported and query succeeded,
> > > + * false otherwise (feature absent or ACPI call failed).
> > > + */
> > > +static bool usbc_security_query(bool *enabled)
> > > +{
> > > +     int status;
> > > +
> > > +     mutex_lock(&usbc_security_mutex);
> > > +     if (!acpi_evalf(hkey_handle, &status, "USCS", "dd", 0)) {
> > > +             mutex_unlock(&usbc_security_mutex);
> > > +             return false;
> > > +     }
> > > +     mutex_unlock(&usbc_security_mutex);
> >
> > Please use cleanup.h.
> Will do.
> Will replace the manual mutex_lock/unlock pattern with guard(mutex).
> >
> > > +
> > > +     if (!(status & USCS_CAP_BIT)) {
> > > +             pr_debug("USCS cap bit absent (raw=0x%x)\n", status);
> > > +             return false;
> > > +     }
> > > +
> > > +     *enabled = !!(status & USCS_STATUS_BIT);
> >
> > No need to do !! when assigning to bool.
> Agreed, will remove the !!.
> >
> > > +     return true;
> > > +}
> > > +
> > > +/* sysfs: /sys/devices/platform/thinkpad_acpi/usb_c_security ---------- */
> > > +static ssize_t usb_c_security_show(struct device *dev,
> > > +                                struct device_attribute *attr,
> > > +                                char *buf)
> > > +{
> > > +     return sysfs_emit(buf, "%s\n",
> > > +                       tp_features.usbc_security_enabled ? "enabled" : "disabled");
> >
> > Please use string_choices.h.
> Will use str_enabled_disabled() which is already used in
> thinkpad_acpi.c for similar pattern.
> >
> > > +}
> > > +
> > > +static DEVICE_ATTR_RO(usb_c_security);
> > > +
> > > +static struct attribute *usbc_security_attributes[] = {
> > > +     &dev_attr_usb_c_security.attr,
> > > +     NULL,
> > > +};
> > > +
> > > +static umode_t usbc_security_attr_is_visible(struct kobject *kobj,
> > > +                                          struct attribute *attr, int n)
> > > +{
> > > +     return tp_features.usbc_security_supported ? attr->mode : 0;
> > > +}
> > > +
> > > +static const struct attribute_group usbc_security_attr_group = {
> > > +     .is_visible = usbc_security_attr_is_visible,
> > > +     .attrs = usbc_security_attributes,
> > > +};
> > > +
> > > +static int tpacpi_usbc_security_init(struct ibm_init_struct *iibm)
> > > +{
> > > +     bool enabled;
> > > +
> > > +     tp_features.usbc_security_supported =
> > > +             usbc_security_query(&enabled);
> >
> > Fits to one line.
> Acked.
> >
> > > +     tp_features.usbc_security_enabled = enabled;
> >
> > I'm not sure I follow the logic here as you always seem set it enabled
> > disregarding even support or not?
> >
> > And that hotkey enabling seems deadcode with it already enabled here.
> >
> > I feel like I must blind to something obvious but cannot just find
> > what that would be.
> I'm sorry, this is a bug.
> I will change to the following:
>     tp_features.usbc_security_supported = usbc_security_query(&enabled);
>     if (tp_features.usbc_security_supported)
>         tp_features.usbc_security_enabled = enabled;
>
> The hotkey path is not dead code — it updates usbc_security_enabled
> when the user presses Fn+U Fn+S, since the EC firmware toggles the
> state and fires the event. The init only captures the state at boot;
> subsequent toggles come through the hotkey. Sorry for the confusion in
> the init code.

Oh, I see now what I was missing and it's just as obvious I was I 
thinking... I, for some reason, read these as:

	tp_features.usbc_security_enabled = true;

...but you had = enabled there. I'm sorry about my confusion.

I find the the way usbc_security_query() is architected a bit confusing 
though, it returns supported/not supported in return value and another 
boolean through the bool pointer. And that looks broken, if 
usbc_security_query() returns early, it won't set *enabled which then 
remains uninitialized (contains pseudogarbage from stack so your testing 
might have been luck and had it always working the way you wanted).

> > > +     return 0;
> > > +}
> > > +
> > > +/* tpacpi_usbc_security_hotkey - handle Fn+U Fn+S hotkey (0x131e) */
> > > +static bool tpacpi_usbc_security_hotkey(void)
> > > +{
> > > +     bool enabled;
> > > +
> > > +     if (!tp_features.usbc_security_supported)
> > > +             return false;
> > > +
> > > +     if (!usbc_security_query(&enabled))
> > > +             return false;
> > > +
> > > +     tp_features.usbc_security_enabled = enabled;
> > > +     sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, "usb_c_security");
> > > +     return true;
> > > +}
> > > +
> > > +static struct ibm_struct usbc_security_driver_data = {
> > > +     .name = "usbc_security",
> > > +};
> > > +
> > >  /* --------------------------------------------------------------------- */
> > >
> > >  static struct attribute *tpacpi_driver_attributes[] = {
> > > @@ -11325,6 +11433,7 @@ static const struct attribute_group *tpacpi_groups[] = {
> > >       &dprc_attr_group,
> > >       &auxmac_attr_group,
> > >       &hwdd_attr_group,
> > > +     &usbc_security_attr_group,
> > >       NULL,
> > >  };
> > >
> > > @@ -11479,6 +11588,8 @@ static bool tpacpi_driver_event(const unsigned int hkey_event)
> > >       case TP_HKEY_EV_PROFILE_TOGGLE2:
> > >               platform_profile_cycle();
> > >               return true;
> > > +     case TP_HKEY_EV_USB_C_SECURITY:
> > > +             return tpacpi_usbc_security_hotkey();
> > >       }
> > >
> > >       return false;
> > > @@ -11930,6 +12041,10 @@ static struct ibm_init_struct ibms_init[] __initdata = {
> > >               .init = tpacpi_hwdd_init,
> > >               .data = &hwdd_driver_data,
> > >       },
> > > +     {
> > > +             .init = tpacpi_usbc_security_init,
> > > +             .data = &usbc_security_driver_data,
> > > +     },
> > >  };
> > >
> > >  static int __init set_ibm_param(const char *val, const struct kernel_param *kp)
> > >
> >
> > --
> >  i.
> >
> 
> I will update the v2 with all these changes soon, if no further comments.
> 

-- 
 i.