From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pf1-f174.google.com (mail-pf1-f174.google.com [209.85.210.174]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6757A30EF6C for ; Wed, 10 Dec 2025 16:27:40 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.210.174 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765384062; cv=none; b=uOV4MchheC5zJR/48pMXkjbVth9WbqJNNg67Z5fM2ywlPE08m/Doc64DC4qj0rCfx+thl+QHREiTnb0+n2UWJ93xV8+AC22q35y0qWzUEN5aSIhIAEiVOauOQ3ufLxx0MEm4TAstj6dI67oOyP2UuNryw8CPztACaJMC8YUdAAU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765384062; c=relaxed/simple; bh=deCPl6hkb2TZ1lECZ1IKGagdVj6j5L5owCDrTSKKNI0=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=kC9MYEEKRR4n23xFmeqcjIg/NrDpK0C18yy2e5uyM6zIOYSyfC1tozv2WdxBYIdrZ0zb3ILALb6tPQ/Fn+Fu/ewM4zt8RgjrscH+hBg7GeUuL+xxCo0VR/fViSS2iGPpfIQzq48MlgpS9d3VF1ySgbwQMV+BuTjIEr3ocwvY2CQ= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=WwvcNoGu; arc=none smtp.client-ip=209.85.210.174 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="WwvcNoGu" Received: by mail-pf1-f174.google.com with SMTP id d2e1a72fcca58-7b89c1ce9easo8072149b3a.2 for ; Wed, 10 Dec 2025 08:27:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1765384060; x=1765988860; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=8KMFDVB22Zk2gVgNrK6KOYmlGrljr+AgaJp2OmrBusw=; b=WwvcNoGuNk4/XXuLzRvZQj8YiBcfxpk+XghyOIEyaDIpBR0QBqooQSVBDsxT14lh4U VSGQJ92CEbLOKaY8cb46Pb4BiCs9I2S60vveqGNMxBYkUPq4eZu3dCe4ynv0dmIPQjcM HQQiUeSPH+/qAeQvohfUVN+YK/on0SqWOYK54v/tUCHDTnVrv/6YvBBzUfvL5gxt6ZZ7 ZgbDtJa90Jn+xyzDMX32dKe24/1tLXdBlQbhStuxSdmXCBttg6Pf8woMBEwX1KDKa0S1 WhRhyQB2NfQQtGyy2M6Vdcz9pjl6t7cYzg/Qq/H//Di03+RZgdw0o1Hjb5oBMv2Bj+Km GJPA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1765384060; x=1765988860; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=8KMFDVB22Zk2gVgNrK6KOYmlGrljr+AgaJp2OmrBusw=; b=Iu0SsXo+Cti34ff5HsFecYZjzLRDd6OXvCo3FoibQEc9MCTkpJwYbBA4YjGgkYLT5W e3JUICXCQOT7vWCbMxt6kLI9ot8HUE9mfCNXtZDuzmnil6WdCQHFZiC+EbY5NNsY/d4X 95RuIfA9eVG80NpLr6LJOJG0zh+rq6ClV4NMuM1Vk0z2ndfQZejSluBIzMHm4BT2WpoM 2cbpZ9RcJs+Lc1cUJ8si9QmLQNKZgGXWOXTZJWgfYncfOgDWH9i5BjJMinMVmmXFQZ8v IuTdKXjZ6x5AlJx/uZrhYKZKdd3knHkT/UaMzO5PF2Z42unNY8r0UvD5fItSRz0CdQBi +5Lg== X-Forwarded-Encrypted: i=1; AJvYcCULPrD4x+bIiSM8z6eMMWLMNLQg0As8Yvotlz5iu3K1j5/GD9TqXPK2Me9OQjgFsypeeZswHAPtDgn3V0M=@vger.kernel.org X-Gm-Message-State: AOJu0Yw1JfqHHblVaPkhqmqu84GkLRpQsnclQCM00vMM5KHLl+X2xPcz mya9sKH3kw9AJvS0+CwwxhxlXxjZUk2g0SqrhRE+zt6qEZ+yOE4v/HXi X-Gm-Gg: AY/fxX5h0LVE5VH3pH8MhgXFDJb7e235A6YXsrxY+Z8HrKNwM29eQFqQLYuoGCAx8iG /I+x1g1zjMgJdZedVV2aAQIEFrBOShAwDvlEnN9D6Vwtz9x/hWWQGJsd+e9Ikc+hYwiUq8LRnfL ecJxbqxPkOkxQfml74s3dIug8bwiFxaPy8WaXENF9HfPYiNmg7HJ/3GyD8hYrj3vW1M5bCkYuVb X7LvgHHcPEUO/RQCHvLL7W/0HYIvuMgZ9IkOqTXHmLFL5atj5JxnrGEJN+Cmc1DG9QfEaUp/bP2 gFv3sIMBr7jjKEM+UvC7GFkgBH5F/57OwAzbqI3ynE2JnbLGmIeNWOTYr/oxgdxT4HHGWErTtIY 7Aw5SYjCoBkFixX5H1WRycMNim/1xwmZugxgqnJgkynsprdKSOLfGci24vjcVIYYg54gmCDpA4E MzLnhaiaOy06ED99QcsnSzWe44nNEcObMogQ8lwavmLZo9eO8= X-Google-Smtp-Source: AGHT+IGkWoEvNMmkOFNiqV6c3oIPVALBG20L6pooCgf0vCr2eydvUrm0K+MtQpKVmMrprDhfF1DSLw== X-Received: by 2002:a05:6a00:92a7:b0:7b8:87e1:a648 with SMTP id d2e1a72fcca58-7f22c8451admr2917696b3a.3.1765384059310; Wed, 10 Dec 2025 08:27:39 -0800 (PST) Received: from ?IPV6:240f:102:8600:1:e5ea:73e5:ae85:d6b? ([240f:102:8600:1:e5ea:73e5:ae85:d6b]) by smtp.gmail.com with ESMTPSA id d2e1a72fcca58-7f4c237f357sm28109b3a.6.2025.12.10.08.27.36 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 10 Dec 2025 08:27:38 -0800 (PST) Message-ID: Date: Thu, 11 Dec 2025 01:27:34 +0900 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 2/2] platform/x86: thinkpad_acpi: Add sysfs to display details of damaged device. To: Mario Limonciello , hansg@kernel.org, ilpo.jarvinen@linux.intel.com Cc: platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org, njoshi1@lenovo.com, Mark Pearson References: <20251210151133.7933-1-nitjoshi@gmail.com> <20251210151133.7933-2-nitjoshi@gmail.com> <21503e42-64c7-4ba1-a6b5-b27cb19af429@amd.com> Content-Language: en-US From: Nitin In-Reply-To: <21503e42-64c7-4ba1-a6b5-b27cb19af429@amd.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi Mario, Thank you for your comments. On 12/11/25 00:43, Mario Limonciello wrote: > On 12/10/25 9:11 AM, Nitin Joshi wrote: >> Add new sysfs interface to identify the impacted component with >> location of >> device. >> >> Reviewed-by: Mark Pearson >> Signed-off-by: Nitin Joshi >> --- >>   .../admin-guide/laptops/thinkpad-acpi.rst     |  13 +- >>   drivers/platform/x86/lenovo/thinkpad_acpi.c   | 112 +++++++++++++++++- >>   2 files changed, 121 insertions(+), 4 deletions(-) >> >> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst >> b/Documentation/admin-guide/laptops/thinkpad-acpi.rst >> index 94349e5f1298..3a9190ac47d0 100644 >> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst >> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst >> @@ -1580,7 +1580,7 @@ Documentation/ABI/testing/sysfs-class-power. >>   Hardware damage detection capability >>   ----------------- >>   -sysfs attributes: hwdd_status >> +sysfs attributes: hwdd_status, hwdd_detail >>     Thinkpads are adding the ability to detect and report hardware >> damage. >>   Add new sysfs interface to identify the damaged device status. >> @@ -1594,6 +1594,17 @@ This value displays status of device damaged >>   - 0 = Not Damaged >>   - 1 = Damaged >>   +The command to check location of damaged device is:: >> + >> +        cat /sys/devices/platform/thinkpad_acpi/hwdd_detail >> + >> +This value displays location of damaged device having 1 line per >> damaged "item". >> +For example: >> +if no damage is detected: >> +  No damage detected >> +if damage detected: >> +  TYPE-C: Base, Right side, Center port >> + >>   The property is read-only. If feature is not supported then sysfs >>   attribute is not created. >>   diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c >> b/drivers/platform/x86/lenovo/thinkpad_acpi.c >> index 4cf365550bcb..a092d57d995d 100644 >> --- a/drivers/platform/x86/lenovo/thinkpad_acpi.c >> +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c >> @@ -11089,8 +11089,24 @@ static const struct attribute_group >> auxmac_attr_group = { >>   #define HWDD_NOT_SUPPORTED    BIT(31) >>   #define HWDD_SUPPORT_USBC    BIT(0) >>   -#define PORT_STATUS        GENMASK(7, 4) >> -#define NUM_PORTS        4 >> +#define PORT_STATUS     GENMASK(7, 4) >> +#define LID_STATUS      GENMASK(11, 8) >> +#define BASE_STATUS     GENMASK(15, 12) >> +#define POS_STATUS      GENMASK(3, 2) >> +#define PANEL_STATUS    GENMASK(1, 0) >> + >> +#define PORT_DETAIL_OFFSET    16 >> + >> +#define PANEL_TOP    0 >> +#define PANEL_BASE    1 >> +#define PANEL_LEFT    2 >> +#define PANEL_RIGHT    3 >> + >> +#define POS_LEFT    0 >> +#define POS_CENTER    1 >> +#define POS_RIGHT    2 >> + >> +#define NUM_PORTS    4 >>     static bool hwdd_support_available; >>   static bool ucdd_supported; >> @@ -11108,7 +11124,95 @@ static int hwdd_command(int command, int >> *output) >>       return 0; >>   } >>   -/* sysfs type-c damage detection capability */ >> +static bool display_damage(char *buf, int *count, char *type, >> unsigned int dmg_status) >> +{ >> +    unsigned char lid_status, base_status, port_status; >> +    unsigned char loc_status, pos_status, panel_status; >> +    bool damage_detected = false; >> +    int i; >> + >> +    port_status = FIELD_GET(PORT_STATUS, dmg_status); >> +    lid_status = FIELD_GET(LID_STATUS, dmg_status); >> +    base_status = FIELD_GET(BASE_STATUS, dmg_status); >> +    for (i = 0; i < NUM_PORTS; i++) { >> +        if (!(dmg_status & BIT(i))) >> +            continue; >> + >> +        if (port_status & BIT(i)) { >> +            *count += sysfs_emit_at(buf, *count, "%s: ", type); >> +            loc_status = (dmg_status >> (PORT_DETAIL_OFFSET + (4 * >> i))) & 0xF; >> +            pos_status = FIELD_GET(POS_STATUS, loc_status); >> +            panel_status = FIELD_GET(PANEL_STATUS, loc_status); >> + >> +            if (lid_status & BIT(i)) >> +                *count += sysfs_emit_at(buf, *count, "Lid, "); >> +            if (base_status & BIT(i)) >> +                *count += sysfs_emit_at(buf, *count, "Base, "); >> + >> +            switch (pos_status) { >> +            case PANEL_TOP: >> +                *count += sysfs_emit_at(buf, *count, "Top, "); >> +                break; >> +            case PANEL_BASE: >> +                *count += sysfs_emit_at(buf, *count, "Bottom, "); >> +                break; >> +            case PANEL_LEFT: >> +                *count += sysfs_emit_at(buf, *count, "Left, "); >> +                break; >> +            case PANEL_RIGHT: >> +                *count += sysfs_emit_at(buf, *count, "Right, "); >> +                break; >> +            default: >> +                pr_err("Unexpected value %d in switch statement\n", >> pos_status); >> +            }; >> + >> +            switch (panel_status) { >> +            case POS_LEFT: >> +                *count += sysfs_emit_at(buf, *count, "Left port\n"); >> +                break; >> +            case POS_CENTER: >> +                *count += sysfs_emit_at(buf, *count, "Center port\n"); >> +                break; >> +            case POS_RIGHT: >> +                *count += sysfs_emit_at(buf, *count, "Right port\n"); >> +                break; >> +            default: >> +                *count += sysfs_emit_at(buf, *count, "Undefined\n"); >> +                break; >> +            }; >> +            damage_detected = true; >> +        } >> +    } >> +    return damage_detected; >> +} >> + >> +/* sysfs type-c damage detection detail */ >> +static ssize_t hwdd_detail_show(struct device *dev, >> +                struct device_attribute *attr, >> +                char *buf) >> +{ >> +    bool damage_detected = false; >> +    unsigned int damage_status; >> +    int err, count = 0; >> + >> + >> +    if (ucdd_supported) { >> +        /* Get USB TYPE-C damage status */ >> +        err = hwdd_command(HWDD_GET_DMG_USBC, &damage_status); >> +        if (err) >> +            return err; >> + >> +        if (display_damage(buf, &count, "Type-C", damage_status)) >> +            damage_detected = true; >> +    } > > Since this is always visible aren't you missing a case for > !ucdd_supported?  I would think you should be returning -ENODEV. In actual, this condition should never occur as only USB Type-C is supported  in this ASL method but i think it's ok to add this check, if there is any benefit. In this case, is it recommended to add such case like  !ucdd_supported? Also, if new device id like type-a etc..  is added in future then we need to include corresponding device id supported also in this check to make sysfs visible. > > Although arguably it would be better to control visibility of the > sysfs attribute based upon ucdd_supported.  You can simplify > hwdd_detail_show() too then. If new device id is added in future then we need to add additional flag to control visibility of sysfs . At this moment , i cant see anything obvious to be simplified in hwdd_detail_show() . Did i missed something ? > > >> + >> +    if (!damage_detected) >> +        count += sysfs_emit_at(buf, count, "No damage detected\n"); >> + >> +    return count; >> +} >> + >> +/* sysfs typc damage detection capability */ >>   static ssize_t hwdd_status_show(struct device *dev, >>                   struct device_attribute *attr, >>                   char *buf) >> @@ -11134,9 +11238,11 @@ static ssize_t hwdd_status_show(struct >> device *dev, >>       return sysfs_emit(buf, "0\n"); >>   } >>   static DEVICE_ATTR_RO(hwdd_status); >> +static DEVICE_ATTR_RO(hwdd_detail); >>     static struct attribute *hwdd_attributes[] = { >>       &dev_attr_hwdd_status.attr, >> +    &dev_attr_hwdd_detail.attr, >>       NULL >>   }; > Thank you !