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

[Vishnu Sankar]

On Tue, May 26, 2026 at 6:16 PM Ilpo Järvinen
<ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> > > > Signed-off-by: Vishnu Sankar <vishnuocv@xxxxxxxxx>
> > > > ---
> > > >  .../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).
Yes, the usbc_security_query() is confusing and does have the
uninitialized *enabled bug.
I only tested on systems that have the USB-C Security feature, so the
unsupported path
was never exercised and the bug was not caught.

I will do something like this:

    static int usbc_security_query(bool *enabled)
    {
        int status;

        guard(mutex)(&usbc_security_mutex);
        if (!acpi_evalf(hkey_handle, &status, "USCS", "dd", 0))
            return -EIO;

        if (!(status & USCS_CAP_BIT))
            return -ENODEV;

        *enabled = status & USCS_STATUS_BIT;
        return 0;
    }

With this, *enabled is only ever read after a successful return 0, so
the uninitialized value issue may not happen. Init and hotkey paths
become straightforward as well.
Hope this is fine.
>
> > > > +     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.



-- 

Regards,

      Vishnu Sankar