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

From: Vishnu Sankar

Date: Tue Jun 09 2026 - 09:12:13 EST

Hi Ilpo,

Thanks a lot for the review comments.

On Tue, Jun 9, 2026 at 6:12 PM Ilpo Järvinen
<ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote:
>
> On Tue, 9 Jun 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>
> > ---
> > Changes since v1:
> > - Use guard(mutex) from cleanup.h instead of manual mutex_lock/unlock
> > - Revert usbc_security_query() to return int (-EIO/-ENODEV/0) instead
> > of bool to avoid uninitialized *enabled bug on unsupported platforms
> > - Remove !! when assigning to bool in usbc_security_query()
> > - Remove dead tp_features.usbc_security_supported check in show()
> > since is_visible() already gates the attribute on unsupported SKUs
> > - Use str_enabled_disabled() from string_choices.h in show()
> > - Fix uninitialized *enabled bug in tpacpi_usbc_security_init() by
> > only assigning usbc_security_enabled after a successful query
> > ---
> > .../admin-guide/laptops/thinkpad-acpi.rst | 24 ++++
> > drivers/platform/x86/lenovo/thinkpad_acpi.c | 118 ++++++++++++++++++
> > 2 files changed, 142 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..379769b62c80 100644
> > --- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
> > +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> > @@ -38,6 +38,7 @@
> > #include <linux/backlight.h>
> > #include <linux/bitfield.h>
> > #include <linux/bitops.h>
> > +#include <linux/cleanup.h>
> > #include <linux/delay.h>
> > #include <linux/dmi.h>
> > #include <linux/freezer.h>
> > @@ -66,6 +67,7 @@
> > #include <linux/seq_file.h>
> > #include <linux/slab.h>
> > #include <linux/string.h>
> > +#include <linux/string_choices.h>
> > #include <linux/string_helpers.h>
> > #include <linux/sysfs.h>
> > #include <linux/types.h>
> > @@ -185,6 +187,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 +376,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;
>
> Sashiko (sashiko.dev) warned that there may be concurrent, unprotected
> updates to these bitfields (changing bitfields require unsafe RMW). It
> looks pre-existing problem at least for trackpoint_doubletap_enable (maybe
> others).
>
> To avoid adding to the problems, usbc_security_enabled should be added
> outside the bitfield to avoid need to do locking for this bitfield.
>
> And trackpoint_doubletap_enable (and possibly others) which are touched in
> the notify context or in sysfs write should be fixed in a separate patch
> (can be done after this series as it's pre-existing problem for them).
>
> Anything that is only written during init is fine inside the bitfield.
Agreed.
usbc_security_enabled will be moved outside tp_features as a
static bool since it is written from both init and the hotkey notify
context. usbc_security_supported remains inside the bitfield as it is
only written during init.

I note that trackpoint_doubletap_enable and possibly others have the
same pre-existing issue and will address those in a separate patch as
you suggested.
>
> > struct quirk_entry *quirks;
> > } tp_features;
> >
> > @@ -11265,6 +11270,112 @@ 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.
> > + *
>
> Remove the extra line.
Acked
>
> > + */
> > +
> > +/* 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,
>
> Kerneldoc doc compatible syntax is:
>
> Returns:
>
Acked
> > + * false otherwise (feature absent or ACPI call failed).
>
> Please rewrite this as this function no longer returns true/false. :-)
>
Acked
> > + */
> > +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)) {
> > + pr_debug("USCS cap bit absent (raw=0x%x)\n", status);
> > + return -ENODEV;
> > + }
> > +
> > + *enabled = status & USCS_STATUS_BIT;
> > + return 0;
> > +}
> > +
> > +/* 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",
> > + str_enabled_disabled(tp_features.usbc_security_enabled));
> > +}
> > +
> > +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;
> > + int err;
> > +
> > + err = usbc_security_query(&enabled);
> > + if (err)
> > + return err == -ENODEV ? 0 : err;
>
> Just split this to two if () + returns for clarity.
>
Acked.
> > +
> > + tp_features.usbc_security_supported = true;
> > + tp_features.usbc_security_enabled = enabled;
> > + 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 +11436,7 @@ static const struct attribute_group *tpacpi_groups[] = {
> > &dprc_attr_group,
> > &auxmac_attr_group,
> > &hwdd_attr_group,
> > + &usbc_security_attr_group,
> > NULL,
> > };
> >
> > @@ -11479,6 +11591,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 +12044,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.
>
Thank you!

--

Regards,

Vishnu Sankar