platform-x86: lenovo-wmi: Add fixes and enhancement

Adds LWMI_ATTR_ID macro. In the same vein as LWMI_ATTR_ID_FAN_RPM, but as a generic, to de-duplicate attribute_id assignment biolerplate.

Reviewed-by Mark Pearson <[email protected]>

Signed-off-by Derek J. Clark <[email protected]>

---
 drivers/platform/x86/lenovo/wmi-capdata.h  |  6 ++++
 drivers/platform/x86/lenovo/wmi-gamezone.h |  1 +
 drivers/platform/x86/lenovo/wmi-other.c    | 36 ++++++----------------
 3 files changed, 17 insertions(+), 26 deletions(-)

drivers/platform/x86/lenovo/wmi-capdata.h +0 -0

index 8c1df3efcc55..27202e2dc8a5 100644

drivers/platform/x86/lenovo/wmi-capdata.h +1 -0

+
 #define LWMI_DEVICE_ID_FAN	0x04
 
 struct component_match;

drivers/platform/x86/lenovo/wmi-gamezone.h +1 -0

index 6b163a5eeb95..ddb919cf6c36 100644
@@ -10,6 +10,7 @@ enum gamezone_events_type {
 };
 
 enum thermal_mode {
+	LWMI_GZ_THERMAL_MODE_NONE =	   0x00,
 	LWMI_GZ_THERMAL_MODE_QUIET =	   0x01,
 	LWMI_GZ_THERMAL_MODE_BALANCED =	   0x02,
 	LWMI_GZ_THERMAL_MODE_PERFORMANCE = 0x03,

drivers/platform/x86/lenovo/wmi-other.c +10 -26

index 6040f45aa2b0..95886df39c8d 100644
@@ -71,10 +71,9 @@
 #define LWMI_FAN_NR 4
 #define LWMI_FAN_ID(x) ((x) + LWMI_FAN_ID_BASE)
 
-#define LWMI_ATTR_ID_FAN_RPM(x)						\
-	(FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, LWMI_DEVICE_ID_FAN) |	\
-	 FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, LWMI_FEATURE_ID_FAN_RPM) |	\
-	 FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, LWMI_FAN_ID(x)))
+#define LWMI_ATTR_ID_FAN_RPM(x)                                   \
+	LWMI_ATTR_ID(LWMI_DEVICE_ID_FAN, LWMI_FEATURE_ID_FAN_RPM, \
+		     LWMI_GZ_THERMAL_MODE_NONE, LWMI_FAN_ID(x))
 
 #define LWMI_FAN_DIV 100
 

@@ -716,12 +715,8 @@ static ssize_t attr_capdata01_show(struct kobject *kobj,
 	u32 attribute_id;
 	int value, ret;
 
-	attribute_id =
-		FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, tunable_attr->device_id) |
-		FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, tunable_attr->feature_id) |
-		FIELD_PREP(LWMI_ATTR_MODE_ID_MASK,
-			   LWMI_GZ_THERMAL_MODE_CUSTOM) |
-		FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, tunable_attr->type_id);
+	attribute_id = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
+				    LWMI_GZ_THERMAL_MODE_CUSTOM, tunable_attr->type_id);
 
 	ret = lwmi_cd01_get_data(priv->cd01_list, attribute_id, &capdata);
 	if (ret)

@@ -776,7 +771,6 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
 	struct wmi_method_args_32 args;
 	struct capdata01 capdata;
 	enum thermal_mode mode;
-	u32 attribute_id;
 	u32 value;
 	int ret;
 

@@ -787,13 +781,10 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
 	if (mode != LWMI_GZ_THERMAL_MODE_CUSTOM)
 		return -EBUSY;
 
-	attribute_id =
-		FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, tunable_attr->device_id) |
-		FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, tunable_attr->feature_id) |
-		FIELD_PREP(LWMI_ATTR_MODE_ID_MASK, mode) |
-		FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, tunable_attr->type_id);
+	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
+				 mode, tunable_attr->type_id);
 
-	ret = lwmi_cd01_get_data(priv->cd01_list, attribute_id, &capdata);
+	ret = lwmi_cd01_get_data(priv->cd01_list, args.arg0, &capdata);
 	if (ret)
 		return ret;
 

@@ -804,7 +795,6 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
 	if (value < capdata.min_value || value > capdata.max_value)
 		return -EINVAL;
 
-	args.arg0 = attribute_id;
 	args.arg1 = value;
 
 	ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_SET,

@@ -838,7 +828,6 @@ static ssize_t attr_current_value_show(struct kobject *kobj,
 	struct lwmi_om_priv *priv = dev_get_drvdata(tunable_attr->dev);
 	struct wmi_method_args_32 args;
 	enum thermal_mode mode;
-	u32 attribute_id;
 	int retval;
 	int ret;
 

@@ -846,13 +835,8 @@ static ssize_t attr_current_value_show(struct kobject *kobj,
 	if (ret)
 		return ret;
 
-	attribute_id =
-		FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, tunable_attr->device_id) |
-		FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, tunable_attr->feature_id) |
-		FIELD_PREP(LWMI_ATTR_MODE_ID_MASK, mode) |
-		FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, tunable_attr->type_id);
-
-	args.arg0 = attribute_id;
+	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
+				 mode, tunable_attr->type_id);
 
 	ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_GET,
 				    (unsigned char *)&args, sizeof(args),

Quoted 19 lines · Derek J. Clark

Adds LWMI_ATTR_ID macro. In the same vein as LWMI_ATTR_ID_FAN_RPM, but as a generic, to de-duplicate attribute_id assignment biolerplate.

Reviewed-by Mark Pearson <[email protected]>

Signed-off-by Derek J. Clark <[email protected]>

---
 drivers/platform/x86/lenovo/wmi-capdata.h  |  6 ++++
 drivers/platform/x86/lenovo/wmi-gamezone.h |  1 +
 drivers/platform/x86/lenovo/wmi-other.c    | 36 ++++++----------------
 3 files changed, 17 insertions(+), 26 deletions(-)

drivers/platform/x86/lenovo/wmi-capdata.h +0 -0

index 8c1df3efcc55..27202e2dc8a5 100644
@@ -17,6 +17,12 @@
 #define LWMI_ATTR_MODE_ID_MASK	GENMASK(15, 8)
 #define LWMI_ATTR_TYPE_ID_MASK	GENMASK(7, 0)
 

drivers/platform/x86/lenovo/wmi-capdata.h line 24

Quoted 5 lines

drivers/platform/x86/lenovo/wmi-capdata.h +5 -0

+#define LWMI_ATTR_ID(dev, feat, mode, type) \
+	(FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, dev) | \
+	 FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, feat) | \
+	 FIELD_PREP(LWMI_ATTR_MODE_ID_MASK, mode) | \
+	 FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, type))

Why does this need to be macro? Static inline would have proper types.

Quoted 107 lines

drivers/platform/x86/lenovo/wmi-capdata.h +1 -0

+
 #define LWMI_DEVICE_ID_FAN	0x04
 
 struct component_match;

drivers/platform/x86/lenovo/wmi-gamezone.h +1 -0

index 6b163a5eeb95..ddb919cf6c36 100644
@@ -10,6 +10,7 @@ enum gamezone_events_type {
 };
 
 enum thermal_mode {
+	LWMI_GZ_THERMAL_MODE_NONE =	   0x00,
 	LWMI_GZ_THERMAL_MODE_QUIET =	   0x01,
 	LWMI_GZ_THERMAL_MODE_BALANCED =	   0x02,
 	LWMI_GZ_THERMAL_MODE_PERFORMANCE = 0x03,

drivers/platform/x86/lenovo/wmi-other.c +10 -26

index 6040f45aa2b0..95886df39c8d 100644
@@ -71,10 +71,9 @@
 #define LWMI_FAN_NR 4
 #define LWMI_FAN_ID(x) ((x) + LWMI_FAN_ID_BASE)
 
-#define LWMI_ATTR_ID_FAN_RPM(x)						\
-	(FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, LWMI_DEVICE_ID_FAN) |	\
-	 FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, LWMI_FEATURE_ID_FAN_RPM) |	\
-	 FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, LWMI_FAN_ID(x)))
+#define LWMI_ATTR_ID_FAN_RPM(x)                                   \
+	LWMI_ATTR_ID(LWMI_DEVICE_ID_FAN, LWMI_FEATURE_ID_FAN_RPM, \
+		     LWMI_GZ_THERMAL_MODE_NONE, LWMI_FAN_ID(x))
 
 #define LWMI_FAN_DIV 100
 

@@ -716,12 +715,8 @@ static ssize_t attr_capdata01_show(struct kobject *kobj,
 	u32 attribute_id;
 	int value, ret;
 
-	attribute_id =
-		FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, tunable_attr->device_id) |
-		FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, tunable_attr->feature_id) |
-		FIELD_PREP(LWMI_ATTR_MODE_ID_MASK,
-			   LWMI_GZ_THERMAL_MODE_CUSTOM) |
-		FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, tunable_attr->type_id);
+	attribute_id = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
+				    LWMI_GZ_THERMAL_MODE_CUSTOM, tunable_attr->type_id);
 
 	ret = lwmi_cd01_get_data(priv->cd01_list, attribute_id, &capdata);
 	if (ret)

@@ -776,7 +771,6 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
 	struct wmi_method_args_32 args;
 	struct capdata01 capdata;
 	enum thermal_mode mode;
-	u32 attribute_id;
 	u32 value;
 	int ret;
 

@@ -787,13 +781,10 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
 	if (mode != LWMI_GZ_THERMAL_MODE_CUSTOM)
 		return -EBUSY;
 
-	attribute_id =
-		FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, tunable_attr->device_id) |
-		FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, tunable_attr->feature_id) |
-		FIELD_PREP(LWMI_ATTR_MODE_ID_MASK, mode) |
-		FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, tunable_attr->type_id);
+	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
+				 mode, tunable_attr->type_id);
 
-	ret = lwmi_cd01_get_data(priv->cd01_list, attribute_id, &capdata);
+	ret = lwmi_cd01_get_data(priv->cd01_list, args.arg0, &capdata);
 	if (ret)
 		return ret;
 

@@ -804,7 +795,6 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
 	if (value < capdata.min_value || value > capdata.max_value)
 		return -EINVAL;
 
-	args.arg0 = attribute_id;
 	args.arg1 = value;
 
 	ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_SET,

@@ -838,7 +828,6 @@ static ssize_t attr_current_value_show(struct kobject *kobj,
 	struct lwmi_om_priv *priv = dev_get_drvdata(tunable_attr->dev);
 	struct wmi_method_args_32 args;
 	enum thermal_mode mode;
-	u32 attribute_id;
 	int retval;
 	int ret;
 

@@ -846,13 +835,8 @@ static ssize_t attr_current_value_show(struct kobject *kobj,
 	if (ret)
 		return ret;
 
-	attribute_id =
-		FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, tunable_attr->device_id) |
-		FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, tunable_attr->feature_id) |
-		FIELD_PREP(LWMI_ATTR_MODE_ID_MASK, mode) |
-		FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, tunable_attr->type_id);
-
-	args.arg0 = attribute_id;
+	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
+				 mode, tunable_attr->type_id);
 
 	ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_GET,
 				    (unsigned char *)&args, sizeof(args),

Quoted 21 lines · Ilpo Järvinen

Quoted 19 lines · Derek J. Clark

Adds LWMI_ATTR_ID macro. In the same vein as LWMI_ATTR_ID_FAN_RPM, but as a generic, to de-duplicate attribute_id assignment biolerplate.

Reviewed-by Mark Pearson <[email protected]>

Signed-off-by Derek J. Clark <[email protected]>

---
 drivers/platform/x86/lenovo/wmi-capdata.h  |  6 ++++
 drivers/platform/x86/lenovo/wmi-gamezone.h |  1 +
 drivers/platform/x86/lenovo/wmi-other.c    | 36 ++++++----------------
 3 files changed, 17 insertions(+), 26 deletions(-)

drivers/platform/x86/lenovo/wmi-capdata.h +0 -0

index 8c1df3efcc55..27202e2dc8a5 100644
@@ -17,6 +17,12 @@
 #define LWMI_ATTR_MODE_ID_MASK	GENMASK(15, 8)
 #define LWMI_ATTR_TYPE_ID_MASK	GENMASK(7, 0)
 

Quoted 8 lines

drivers/platform/x86/lenovo/wmi-capdata.h line 24

Quoted 5 lines

drivers/platform/x86/lenovo/wmi-capdata.h +5 -0

+#define LWMI_ATTR_ID(dev, feat, mode, type) \
+	(FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, dev) | \
+	 FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, feat) | \
+	 FIELD_PREP(LWMI_ATTR_MODE_ID_MASK, mode) | \
+	 FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, type))

Why does this need to be macro? Static inline would have proper types.

Hi Ilpo,

It doesn't, I was just keeping it consistent with other previous uses. It's not a problem to change it. I'll need to move it somewhere else since this is a header.

Adds LWMI_ATTR_ID macro. In the same vein as LWMI_ATTR_ID_FAN_RPM,

but as a generic, to de-duplicate attribute_id assignment biolerplate.

Reviewed-by Mark Pearson <[email protected]>

Signed-off-by Derek J. Clark <[email protected]>

---
 drivers/platform/x86/lenovo/wmi-capdata.h  |  6 ++++
 drivers/platform/x86/lenovo/wmi-gamezone.h |  1 +
 drivers/platform/x86/lenovo/wmi-other.c    | 36 ++++++----------------
 3 files changed, 17 insertions(+), 26 deletions(-)

drivers/platform/x86/lenovo/wmi-capdata.h +6 -0

index 8c1df3efcc55..27202e2dc8a5 100644
@@ -17,6 +17,12 @@
 #define LWMI_ATTR_MODE_ID_MASK	GENMASK(15, 8)
 #define LWMI_ATTR_TYPE_ID_MASK	GENMASK(7, 0)
 
+#define LWMI_ATTR_ID(dev, feat, mode, type) \
+	(FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, dev) | \
+	 FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, feat) | \
+	 FIELD_PREP(LWMI_ATTR_MODE_ID_MASK, mode) | \
+	 FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, type))
+
 #define LWMI_DEVICE_ID_FAN	0x04
 
 struct component_match;

drivers/platform/x86/lenovo/wmi-gamezone.h +1 -0

index 6b163a5eeb95..ddb919cf6c36 100644
@@ -10,6 +10,7 @@ enum gamezone_events_type {
 };
 
 enum thermal_mode {
+	LWMI_GZ_THERMAL_MODE_NONE =	   0x00,
 	LWMI_GZ_THERMAL_MODE_QUIET =	   0x01,
 	LWMI_GZ_THERMAL_MODE_BALANCED =	   0x02,
 	LWMI_GZ_THERMAL_MODE_PERFORMANCE = 0x03,

drivers/platform/x86/lenovo/wmi-other.c +10 -26

index 6040f45aa2b0..95886df39c8d 100644
@@ -71,10 +71,9 @@
 #define LWMI_FAN_NR 4
 #define LWMI_FAN_ID(x) ((x) + LWMI_FAN_ID_BASE)
 
-#define LWMI_ATTR_ID_FAN_RPM(x)						\
-	(FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, LWMI_DEVICE_ID_FAN) |	\
-	 FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, LWMI_FEATURE_ID_FAN_RPM) |	\
-	 FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, LWMI_FAN_ID(x)))
+#define LWMI_ATTR_ID_FAN_RPM(x)                                   \
+	LWMI_ATTR_ID(LWMI_DEVICE_ID_FAN, LWMI_FEATURE_ID_FAN_RPM, \
+		     LWMI_GZ_THERMAL_MODE_NONE, LWMI_FAN_ID(x))
 
 #define LWMI_FAN_DIV 100
 

@@ -716,12 +715,8 @@ static ssize_t attr_capdata01_show(struct kobject *kobj,
 	u32 attribute_id;
 	int value, ret;
 
-	attribute_id =
-		FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, tunable_attr->device_id) |
-		FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, tunable_attr->feature_id) |
-		FIELD_PREP(LWMI_ATTR_MODE_ID_MASK,
-			   LWMI_GZ_THERMAL_MODE_CUSTOM) |
-		FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, tunable_attr->type_id);
+	attribute_id = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
+				    LWMI_GZ_THERMAL_MODE_CUSTOM, tunable_attr->type_id);
 
 	ret = lwmi_cd01_get_data(priv->cd01_list, attribute_id, &capdata);
 	if (ret)

@@ -776,7 +771,6 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
 	struct wmi_method_args_32 args;
 	struct capdata01 capdata;
 	enum thermal_mode mode;
-	u32 attribute_id;
 	u32 value;
 	int ret;
 

@@ -787,13 +781,10 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
 	if (mode != LWMI_GZ_THERMAL_MODE_CUSTOM)
 		return -EBUSY;
 
-	attribute_id =
-		FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, tunable_attr->device_id) |
-		FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, tunable_attr->feature_id) |
-		FIELD_PREP(LWMI_ATTR_MODE_ID_MASK, mode) |
-		FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, tunable_attr->type_id);
+	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
+				 mode, tunable_attr->type_id);
 
-	ret = lwmi_cd01_get_data(priv->cd01_list, attribute_id, &capdata);
+	ret = lwmi_cd01_get_data(priv->cd01_list, args.arg0, &capdata);
 	if (ret)
 		return ret;
 

@@ -804,7 +795,6 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
 	if (value < capdata.min_value || value > capdata.max_value)
 		return -EINVAL;
 
-	args.arg0 = attribute_id;
 	args.arg1 = value;
 
 	ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_SET,

@@ -838,7 +828,6 @@ static ssize_t attr_current_value_show(struct kobject *kobj,
 	struct lwmi_om_priv *priv = dev_get_drvdata(tunable_attr->dev);
 	struct wmi_method_args_32 args;
 	enum thermal_mode mode;
-	u32 attribute_id;
 	int retval;
 	int ret;
 

@@ -846,13 +835,8 @@ static ssize_t attr_current_value_show(struct kobject *kobj,
 	if (ret)
 		return ret;
 
-	attribute_id =
-		FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, tunable_attr->device_id) |
-		FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, tunable_attr->feature_id) |
-		FIELD_PREP(LWMI_ATTR_MODE_ID_MASK, mode) |
-		FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, tunable_attr->type_id);
-
-	args.arg0 = attribute_id;
+	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
+				 mode, tunable_attr->type_id);
 
 	ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_GET,
 				    (unsigned char *)&args, sizeof(args),

Adds lwmi_is_attr_01_supported, and only creates the attribute subfolder if the attribute is supported by the hardware. Due to some poorly implemented BIOS, this is a multi-step sequence of events. This is because:
- Some BIOS support getting the capability data from custom mode (0xff), while others only support it in no-mode (0x00).
- Similarly, some BIOS support get/set for the current value from custom mode (0xff), while others only support it in no-mode (0x00).
- Some BIOS report capability data for a method that is not fully implemented.
- Some BIOS have methods fully implemented, but no complimentary capability data.

To ensure we only expose fully implemented methods with corresponding capability data, we check each outcome before reporting that an attribute can be supported.

Checking for lwmi_is_attr_01_supported during remove is not done to ensure that we don't attempt to call cd01 or send WMI events if one of the interfaces being removed was the cause of the driver unloading.

While adding members to tunable_attr_01, remove unused capdata pointer and limit size of all ID's to the appropriate size.

Reviewed-by Mark Pearson <[email protected]>

Reported-by Kurt Borja <[email protected]>

Closes https://lore.kernel.org/platform-driver-x86/[email protected]/ open here

Signed-off-by Derek J. Clark <[email protected]>

---
 drivers/platform/x86/lenovo/wmi-other.c | 117 +++++++++++++++++++++---
 1 file changed, 102 insertions(+), 15 deletions(-)

drivers/platform/x86/lenovo/wmi-other.c +38 -8

index 95886df39c8d..f3f12303e379 100644
@@ -545,11 +545,12 @@ static void lwmi_om_fan_info_collect_cd_fan(struct device *dev, struct cd_list *
 /* ======== fw_attributes (component: lenovo-wmi-capdata 01) ======== */
 
 struct tunable_attr_01 {
-	struct capdata01 *capdata;
 	struct device *dev;
-	u32 feature_id;
-	u32 device_id;
-	u32 type_id;
+	u8 feature_id;
+	u8 device_id;
+	u8 type_id;
+	u8 cd_mode_id; /* mode arg for searching capdata */
+	u8 cv_mode_id; /* mode arg for set/get current_value */
 };
 
 static struct tunable_attr_01 ppt_pl1_spl = {

@@ -716,7 +717,7 @@ static ssize_t attr_capdata01_show(struct kobject *kobj,
 	int value, ret;
 
 	attribute_id = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
-				    LWMI_GZ_THERMAL_MODE_CUSTOM, tunable_attr->type_id);
+				    tunable_attr->cd_mode_id, tunable_attr->type_id);
 
 	ret = lwmi_cd01_get_data(priv->cd01_list, attribute_id, &capdata);
 	if (ret)

@@ -782,7 +783,7 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
 		return -EBUSY;
 
 	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
-				 mode, tunable_attr->type_id);
+				 tunable_attr->cd_mode_id, tunable_attr->type_id);
 
 	ret = lwmi_cd01_get_data(priv->cd01_list, args.arg0, &capdata);
 	if (ret)

@@ -795,6 +796,8 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
 	if (value < capdata.min_value || value > capdata.max_value)
 		return -EINVAL;
 
+	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
+				 tunable_attr->cv_mode_id, tunable_attr->type_id);
 	args.arg1 = value;
 
 	ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_SET,

@@ -828,13 +831,16 @@ static ssize_t attr_current_value_show(struct kobject *kobj,
 	struct lwmi_om_priv *priv = dev_get_drvdata(tunable_attr->dev);
 	struct wmi_method_args_32 args;
 	enum thermal_mode mode;
-	int retval;
-	int ret;
+	int retval, ret;
 
 	ret = lwmi_om_notifier_call(&mode);
 	if (ret)
 		return ret;
 
+	/* If "no-mode" is the supported mode, ensure we never send current mode */
+	if (tunable_attr->cv_mode_id == LWMI_GZ_THERMAL_MODE_NONE)
+		mode = tunable_attr->cv_mode_id;
+
 	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
 				 mode, tunable_attr->type_id);
 

@@ -847,6 +853,85 @@ static ssize_t attr_current_value_show(struct kobject *kobj,
 	return sysfs_emit(buf, "%d\n", retval);
 }
 
+/**
+ * lwmi_attr_01_is_supported() - Determine if the given attribute is supported.
+ * @tunable_attr: The attribute to verify.
+ *
+ * First check if the attribute has a corresponding capdata01 table in the cd01
+ * module under the "custom" mode (0xff). If that is not present then check if
+ * there is a corresponding "no-mode" (0x00) entry. If either of those passes,
+ * check capdata->supported for values > 0. If capdata is available, attempt to
+ * determine the set/get mode for the current value property using a similar
+ * pattern. If the value returned by either custom or no-mode is 0, or we get
+ * an error, we assume that mode is not supported. If any of the above checks
+ * fail then the attribute is not fully supported.
+ *
+ * The probed cd_mode_id/cv_mode_id are stored on the tunable_attr for later
+ * reference.
+ *
+ * Return: Support level, or an error code.
+ */
+static int lwmi_attr_01_is_supported(struct tunable_attr_01 *tunable_attr)
+{
+	struct lwmi_om_priv *priv = dev_get_drvdata(tunable_attr->dev);
+	u8 mode = LWMI_GZ_THERMAL_MODE_CUSTOM;
+	struct wmi_method_args_32 args;
+	struct capdata01 capdata;

drivers/platform/x86/lenovo/wmi-other.c +13 -0

+	}
+	if (ret)
+		goto not_supported;
+	if (!capdata.supported) {
+		ret = -EOPNOTSUPP;
+		goto not_supported;
+	}
+
+	tunable_attr->cd_mode_id = mode;
+
+	/* Determine tunable_attr->cv_mode_id */
+	mode = LWMI_GZ_THERMAL_MODE_CUSTOM;
+no_mode_fallback_2:

drivers/platform/x86/lenovo/wmi-other.c +20 -0

+	}
+	if (ret)
+		goto not_supported;
+	if (retval == 0) {
+		ret = -EOPNOTSUPP;
+		goto not_supported;
+	}
+
+	tunable_attr->cv_mode_id = mode;
+	dev_dbg(tunable_attr->dev, "cd_mode_id: %02x%02x%02x%02x, cv_mode_id: %#08x attribute support level: %x\n",
+		tunable_attr->device_id, tunable_attr->feature_id, tunable_attr->cd_mode_id,
+		tunable_attr->type_id, args.arg0, capdata.supported);
+
+	return capdata.supported;
+
+not_supported:
+	dev_dbg(tunable_attr->dev, "Attribute id %x not supported\n", args.arg0);
+	return ret;
+}
+
 /* Lenovo WMI Other Mode Attribute macros */
 #define __LWMI_ATTR_RO(_func, _name)                                  \
 	{                                                             \

drivers/platform/x86/lenovo/wmi-other.c +0 -0

 	kset_unregister(priv->fw_attr_kset);
 
 

Quoted 17 lines · Derek J. Clark

Adds lwmi_is_attr_01_supported, and only creates the attribute subfolder if the attribute is supported by the hardware. Due to some poorly implemented BIOS, this is a multi-step sequence of events. This is because:
- Some BIOS support getting the capability data from custom mode (0xff), while others only support it in no-mode (0x00).
- Similarly, some BIOS support get/set for the current value from custom mode (0xff), while others only support it in no-mode (0x00).
- Some BIOS report capability data for a method that is not fully implemented.
- Some BIOS have methods fully implemented, but no complimentary capability data.

To ensure we only expose fully implemented methods with corresponding capability data, we check each outcome before reporting that an attribute can be supported.

Quoted 7 lines

Checking for lwmi_is_attr_01_supported during remove is not done to ensure that we don't attempt to call cd01 or send WMI events if one of the interfaces being removed was the cause of the driver unloading.

While adding members to tunable_attr_01, remove unused capdata pointer and limit size of all ID's to the appropriate size.

Please don't mix changes like this. Create a seprate patch for it.

Quoted 103 lines

Reviewed-by Mark Pearson <[email protected]>

Reported-by Kurt Borja <[email protected]>

Closes https://lore.kernel.org/platform-driver-x86/[email protected]/ open here

Signed-off-by Derek J. Clark <[email protected]>

---
 drivers/platform/x86/lenovo/wmi-other.c | 117 +++++++++++++++++++++---
 1 file changed, 102 insertions(+), 15 deletions(-)

drivers/platform/x86/lenovo/wmi-other.c +38 -8

index 95886df39c8d..f3f12303e379 100644
@@ -545,11 +545,12 @@ static void lwmi_om_fan_info_collect_cd_fan(struct device *dev, struct cd_list *
 /* ======== fw_attributes (component: lenovo-wmi-capdata 01) ======== */
 
 struct tunable_attr_01 {
-	struct capdata01 *capdata;
 	struct device *dev;
-	u32 feature_id;
-	u32 device_id;
-	u32 type_id;
+	u8 feature_id;
+	u8 device_id;
+	u8 type_id;
+	u8 cd_mode_id; /* mode arg for searching capdata */
+	u8 cv_mode_id; /* mode arg for set/get current_value */
 };
 
 static struct tunable_attr_01 ppt_pl1_spl = {

@@ -716,7 +717,7 @@ static ssize_t attr_capdata01_show(struct kobject *kobj,
 	int value, ret;
 
 	attribute_id = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
-				    LWMI_GZ_THERMAL_MODE_CUSTOM, tunable_attr->type_id);
+				    tunable_attr->cd_mode_id, tunable_attr->type_id);
 
 	ret = lwmi_cd01_get_data(priv->cd01_list, attribute_id, &capdata);
 	if (ret)

@@ -782,7 +783,7 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
 		return -EBUSY;
 
 	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
-				 mode, tunable_attr->type_id);
+				 tunable_attr->cd_mode_id, tunable_attr->type_id);
 
 	ret = lwmi_cd01_get_data(priv->cd01_list, args.arg0, &capdata);
 	if (ret)

@@ -795,6 +796,8 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
 	if (value < capdata.min_value || value > capdata.max_value)
 		return -EINVAL;
 
+	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
+				 tunable_attr->cv_mode_id, tunable_attr->type_id);
 	args.arg1 = value;
 
 	ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_SET,

@@ -828,13 +831,16 @@ static ssize_t attr_current_value_show(struct kobject *kobj,
 	struct lwmi_om_priv *priv = dev_get_drvdata(tunable_attr->dev);
 	struct wmi_method_args_32 args;
 	enum thermal_mode mode;
-	int retval;
-	int ret;
+	int retval, ret;
 
 	ret = lwmi_om_notifier_call(&mode);
 	if (ret)
 		return ret;
 
+	/* If "no-mode" is the supported mode, ensure we never send current mode */
+	if (tunable_attr->cv_mode_id == LWMI_GZ_THERMAL_MODE_NONE)
+		mode = tunable_attr->cv_mode_id;
+
 	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
 				 mode, tunable_attr->type_id);
 

@@ -847,6 +853,85 @@ static ssize_t attr_current_value_show(struct kobject *kobj,
 	return sysfs_emit(buf, "%d\n", retval);
 }
 
+/**
+ * lwmi_attr_01_is_supported() - Determine if the given attribute is supported.
+ * @tunable_attr: The attribute to verify.
+ *
+ * First check if the attribute has a corresponding capdata01 table in the cd01
+ * module under the "custom" mode (0xff). If that is not present then check if
+ * there is a corresponding "no-mode" (0x00) entry. If either of those passes,
+ * check capdata->supported for values > 0. If capdata is available, attempt to
+ * determine the set/get mode for the current value property using a similar
+ * pattern. If the value returned by either custom or no-mode is 0, or we get
+ * an error, we assume that mode is not supported. If any of the above checks
+ * fail then the attribute is not fully supported.
+ *
+ * The probed cd_mode_id/cv_mode_id are stored on the tunable_attr for later
+ * reference.
+ *
+ * Return: Support level, or an error code.
+ */
+static int lwmi_attr_01_is_supported(struct tunable_attr_01 *tunable_attr)
+{
+	struct lwmi_om_priv *priv = dev_get_drvdata(tunable_attr->dev);
+	u8 mode = LWMI_GZ_THERMAL_MODE_CUSTOM;
+	struct wmi_method_args_32 args;
+	struct capdata01 capdata;

drivers/platform/x86/lenovo/wmi-other.c static ssize_t attr_current_value_show(struct kobject *kobj, · line 889

Quoted 10 lines

drivers/platform/x86/lenovo/wmi-other.c +10 -0

+	int retval, ret;
+
+	/* Determine tunable_attr->cd_mode_id */
+no_mode_fallback_1:
+	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
+				 mode, tunable_attr->type_id);
+
+	ret = lwmi_cd01_get_data(priv->cd01_list, args.arg0, &capdata);
+	if (ret && mode) {
+		dev_dbg(tunable_attr->dev, "Attribute id %x not supported\n", args.arg0);

Add include.

drivers/platform/x86/lenovo/wmi-other.c static ssize_t attr_current_value_show(struct kobject *kobj, · line 891

Quoted 2 lines

drivers/platform/x86/lenovo/wmi-other.c +2 -0

+		mode = LWMI_GZ_THERMAL_MODE_NONE;
+		goto no_mode_fallback_1;

Is it possible to make a helper so you don't need these back gotos?

Quoted 13 lines

drivers/platform/x86/lenovo/wmi-other.c +13 -0

+	}
+	if (ret)
+		goto not_supported;
+	if (!capdata.supported) {
+		ret = -EOPNOTSUPP;
+		goto not_supported;
+	}
+
+	tunable_attr->cd_mode_id = mode;
+
+	/* Determine tunable_attr->cv_mode_id */
+	mode = LWMI_GZ_THERMAL_MODE_CUSTOM;
+no_mode_fallback_2:

drivers/platform/x86/lenovo/wmi-other.c static ssize_t attr_current_value_show(struct kobject *kobj, · line 914

Quoted 10 lines

drivers/platform/x86/lenovo/wmi-other.c +10 -0

+	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
+				 mode, tunable_attr->type_id);
+
+	ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_GET,
+				    (unsigned char *)&args, sizeof(args),
+				    &retval);
+	if ((ret && mode) || (!retval && mode)) {
+		dev_dbg(tunable_attr->dev, "Attribute id %x not supported\n", args.arg0);
+		mode = LWMI_GZ_THERMAL_MODE_NONE;
+		goto no_mode_fallback_2;

Same question here?

Quoted 24 lines

drivers/platform/x86/lenovo/wmi-other.c +20 -0

+	}
+	if (ret)
+		goto not_supported;
+	if (retval == 0) {
+		ret = -EOPNOTSUPP;
+		goto not_supported;
+	}
+
+	tunable_attr->cv_mode_id = mode;
+	dev_dbg(tunable_attr->dev, "cd_mode_id: %02x%02x%02x%02x, cv_mode_id: %#08x attribute support level: %x\n",
+		tunable_attr->device_id, tunable_attr->feature_id, tunable_attr->cd_mode_id,
+		tunable_attr->type_id, args.arg0, capdata.supported);
+
+	return capdata.supported;
+
+not_supported:
+	dev_dbg(tunable_attr->dev, "Attribute id %x not supported\n", args.arg0);
+	return ret;
+}
+
 /* Lenovo WMI Other Mode Attribute macros */
 #define __LWMI_ATTR_RO(_func, _name)                                  \
 	{                                                             \

drivers/platform/x86/lenovo/wmi-other.c static int lwmi_om_fw_attr_add(struct lwmi_om_priv *priv) · line 1059

Quoted 10 lines

drivers/platform/x86/lenovo/wmi-other.c +1 -5

@@ -970,19 +1055,21 @@ static int lwmi_om_fw_attr_add(struct lwmi_om_priv *priv)
 	}
 
 	for (i = 0; i < ARRAY_SIZE(cd01_attr_groups) - 1; i++) {
-		err = sysfs_create_group(&priv->fw_attr_kset->kobj,
-					 cd01_attr_groups[i].attr_group);
-		if (err)
-			goto err_remove_groups;
-
 		cd01_attr_groups[i].tunable_attr->dev = &priv->wdev->dev;
+		if (lwmi_attr_01_is_supported(cd01_attr_groups[i].tunable_attr) > 0) {

Reverse logic and use continue.

drivers/platform/x86/lenovo/wmi-other.c static int lwmi_om_fw_attr_add(struct lwmi_om_priv *priv) · line 1070

Quoted 13 lines

drivers/platform/x86/lenovo/wmi-other.c +6 -2

+			err = sysfs_create_group(&priv->fw_attr_kset->kobj,
+						 cd01_attr_groups[i].attr_group);
+			if (err)
+				goto err_remove_groups;
+		}
 	}
 	return 0;
 
 err_remove_groups:
 	while (i--)
-		sysfs_remove_group(&priv->fw_attr_kset->kobj,
-				   cd01_attr_groups[i].attr_group);
+		if (lwmi_attr_01_is_supported(cd01_attr_groups[i].tunable_attr) > 0)

Reverse logic + continue.

drivers/platform/x86/lenovo/wmi-other.c static int lwmi_om_fw_attr_add(struct lwmi_om_priv *priv) · line 1072

Quoted 2 lines

drivers/platform/x86/lenovo/wmi-other.c +2 -0

+			sysfs_remove_group(&priv->fw_attr_kset->kobj,
+					   cd01_attr_groups[i].attr_group);

You need to add braces for multiline constructs.

Quoted 4 lines

drivers/platform/x86/lenovo/wmi-other.c +0 -0

 	kset_unregister(priv->fw_attr_kset);
 
 

Quoted 145 lines · Ilpo Järvinen

Quoted 17 lines · Derek J. Clark

Adds lwmi_is_attr_01_supported, and only creates the attribute subfolder if the attribute is supported by the hardware. Due to some poorly implemented BIOS, this is a multi-step sequence of events. This is because:
- Some BIOS support getting the capability data from custom mode (0xff), while others only support it in no-mode (0x00).
- Similarly, some BIOS support get/set for the current value from custom mode (0xff), while others only support it in no-mode (0x00).
- Some BIOS report capability data for a method that is not fully implemented.
- Some BIOS have methods fully implemented, but no complimentary capability data.

To ensure we only expose fully implemented methods with corresponding capability data, we check each outcome before reporting that an attribute can be supported.

Quoted 7 lines

Checking for lwmi_is_attr_01_supported during remove is not done to ensure that we don't attempt to call cd01 or send WMI events if one of the interfaces being removed was the cause of the driver unloading.

While adding members to tunable_attr_01, remove unused capdata pointer and limit size of all ID's to the appropriate size.

Please don't mix changes like this. Create a seprate patch for it.

Quoted 103 lines

Reviewed-by Mark Pearson <[email protected]>

Reported-by Kurt Borja <[email protected]>

Closes https://lore.kernel.org/platform-driver-x86/[email protected]/ open here

Signed-off-by Derek J. Clark <[email protected]>

---
 drivers/platform/x86/lenovo/wmi-other.c | 117 +++++++++++++++++++++---
 1 file changed, 102 insertions(+), 15 deletions(-)

drivers/platform/x86/lenovo/wmi-other.c +38 -8

index 95886df39c8d..f3f12303e379 100644
@@ -545,11 +545,12 @@ static void lwmi_om_fan_info_collect_cd_fan(struct device *dev, struct cd_list *
 /* ======== fw_attributes (component: lenovo-wmi-capdata 01) ======== */
 
 struct tunable_attr_01 {
-	struct capdata01 *capdata;
 	struct device *dev;
-	u32 feature_id;
-	u32 device_id;
-	u32 type_id;
+	u8 feature_id;
+	u8 device_id;
+	u8 type_id;
+	u8 cd_mode_id; /* mode arg for searching capdata */
+	u8 cv_mode_id; /* mode arg for set/get current_value */
 };
 
 static struct tunable_attr_01 ppt_pl1_spl = {

@@ -716,7 +717,7 @@ static ssize_t attr_capdata01_show(struct kobject *kobj,
 	int value, ret;
 
 	attribute_id = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
-				    LWMI_GZ_THERMAL_MODE_CUSTOM, tunable_attr->type_id);
+				    tunable_attr->cd_mode_id, tunable_attr->type_id);
 
 	ret = lwmi_cd01_get_data(priv->cd01_list, attribute_id, &capdata);
 	if (ret)

@@ -782,7 +783,7 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
 		return -EBUSY;
 
 	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
-				 mode, tunable_attr->type_id);
+				 tunable_attr->cd_mode_id, tunable_attr->type_id);
 
 	ret = lwmi_cd01_get_data(priv->cd01_list, args.arg0, &capdata);
 	if (ret)

@@ -795,6 +796,8 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
 	if (value < capdata.min_value || value > capdata.max_value)
 		return -EINVAL;
 
+	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
+				 tunable_attr->cv_mode_id, tunable_attr->type_id);
 	args.arg1 = value;
 
 	ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_SET,

@@ -828,13 +831,16 @@ static ssize_t attr_current_value_show(struct kobject *kobj,
 	struct lwmi_om_priv *priv = dev_get_drvdata(tunable_attr->dev);
 	struct wmi_method_args_32 args;
 	enum thermal_mode mode;
-	int retval;
-	int ret;
+	int retval, ret;
 
 	ret = lwmi_om_notifier_call(&mode);
 	if (ret)
 		return ret;
 
+	/* If "no-mode" is the supported mode, ensure we never send current mode */
+	if (tunable_attr->cv_mode_id == LWMI_GZ_THERMAL_MODE_NONE)
+		mode = tunable_attr->cv_mode_id;
+
 	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
 				 mode, tunable_attr->type_id);
 

@@ -847,6 +853,85 @@ static ssize_t attr_current_value_show(struct kobject *kobj,
 	return sysfs_emit(buf, "%d\n", retval);
 }
 
+/**
+ * lwmi_attr_01_is_supported() - Determine if the given attribute is supported.
+ * @tunable_attr: The attribute to verify.
+ *
+ * First check if the attribute has a corresponding capdata01 table in the cd01
+ * module under the "custom" mode (0xff). If that is not present then check if
+ * there is a corresponding "no-mode" (0x00) entry. If either of those passes,
+ * check capdata->supported for values > 0. If capdata is available, attempt to
+ * determine the set/get mode for the current value property using a similar
+ * pattern. If the value returned by either custom or no-mode is 0, or we get
+ * an error, we assume that mode is not supported. If any of the above checks
+ * fail then the attribute is not fully supported.
+ *
+ * The probed cd_mode_id/cv_mode_id are stored on the tunable_attr for later
+ * reference.
+ *
+ * Return: Support level, or an error code.
+ */
+static int lwmi_attr_01_is_supported(struct tunable_attr_01 *tunable_attr)
+{
+	struct lwmi_om_priv *priv = dev_get_drvdata(tunable_attr->dev);
+	u8 mode = LWMI_GZ_THERMAL_MODE_CUSTOM;
+	struct wmi_method_args_32 args;
+	struct capdata01 capdata;

drivers/platform/x86/lenovo/wmi-other.c static ssize_t attr_current_value_show(struct kobject *kobj, · line 889

Quoted 10 lines

drivers/platform/x86/lenovo/wmi-other.c +10 -0

+	int retval, ret;
+
+	/* Determine tunable_attr->cd_mode_id */
+no_mode_fallback_1:
+	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
+				 mode, tunable_attr->type_id);
+
+	ret = lwmi_cd01_get_data(priv->cd01_list, args.arg0, &capdata);
+	if (ret && mode) {
+		dev_dbg(tunable_attr->dev, "Attribute id %x not supported\n", args.arg0);

Add include.

Quoted 5 lines

drivers/platform/x86/lenovo/wmi-other.c static ssize_t attr_current_value_show(struct kobject *kobj, · line 891

Quoted 2 lines

drivers/platform/x86/lenovo/wmi-other.c +2 -0

+		mode = LWMI_GZ_THERMAL_MODE_NONE;
+		goto no_mode_fallback_1;

Is it possible to make a helper so you don't need these back gotos?

Sure. How about I put both modes into an array and loop through them, passing the values to a function that will evaluate it. I can break if there is a match and throw an error if there's no match at the end.

Quoted 89 lines

Quoted 13 lines

drivers/platform/x86/lenovo/wmi-other.c +13 -0

+	}
+	if (ret)
+		goto not_supported;
+	if (!capdata.supported) {
+		ret = -EOPNOTSUPP;
+		goto not_supported;
+	}
+
+	tunable_attr->cd_mode_id = mode;
+
+	/* Determine tunable_attr->cv_mode_id */
+	mode = LWMI_GZ_THERMAL_MODE_CUSTOM;
+no_mode_fallback_2:

drivers/platform/x86/lenovo/wmi-other.c static ssize_t attr_current_value_show(struct kobject *kobj, · line 914

Quoted 10 lines

drivers/platform/x86/lenovo/wmi-other.c +10 -0

+	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
+				 mode, tunable_attr->type_id);
+
+	ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_GET,
+				    (unsigned char *)&args, sizeof(args),
+				    &retval);
+	if ((ret && mode) || (!retval && mode)) {
+		dev_dbg(tunable_attr->dev, "Attribute id %x not supported\n", args.arg0);
+		mode = LWMI_GZ_THERMAL_MODE_NONE;
+		goto no_mode_fallback_2;

Same question here?

Quoted 24 lines

drivers/platform/x86/lenovo/wmi-other.c +20 -0

+	}
+	if (ret)
+		goto not_supported;
+	if (retval == 0) {
+		ret = -EOPNOTSUPP;
+		goto not_supported;
+	}
+
+	tunable_attr->cv_mode_id = mode;
+	dev_dbg(tunable_attr->dev, "cd_mode_id: %02x%02x%02x%02x, cv_mode_id: %#08x attribute support level: %x\n",
+		tunable_attr->device_id, tunable_attr->feature_id, tunable_attr->cd_mode_id,
+		tunable_attr->type_id, args.arg0, capdata.supported);
+
+	return capdata.supported;
+
+not_supported:
+	dev_dbg(tunable_attr->dev, "Attribute id %x not supported\n", args.arg0);
+	return ret;
+}
+
 /* Lenovo WMI Other Mode Attribute macros */
 #define __LWMI_ATTR_RO(_func, _name)                                  \
 	{                                                             \

drivers/platform/x86/lenovo/wmi-other.c static int lwmi_om_fw_attr_add(struct lwmi_om_priv *priv) · line 1059

Quoted 10 lines

drivers/platform/x86/lenovo/wmi-other.c +1 -5

@@ -970,19 +1055,21 @@ static int lwmi_om_fw_attr_add(struct lwmi_om_priv *priv)
 	}
 
 	for (i = 0; i < ARRAY_SIZE(cd01_attr_groups) - 1; i++) {
-		err = sysfs_create_group(&priv->fw_attr_kset->kobj,
-					 cd01_attr_groups[i].attr_group);
-		if (err)
-			goto err_remove_groups;
-
 		cd01_attr_groups[i].tunable_attr->dev = &priv->wdev->dev;
+		if (lwmi_attr_01_is_supported(cd01_attr_groups[i].tunable_attr) > 0) {

Reverse logic and use continue.

drivers/platform/x86/lenovo/wmi-other.c static int lwmi_om_fw_attr_add(struct lwmi_om_priv *priv) · line 1070

Quoted 13 lines

drivers/platform/x86/lenovo/wmi-other.c +6 -2

+			err = sysfs_create_group(&priv->fw_attr_kset->kobj,
+						 cd01_attr_groups[i].attr_group);
+			if (err)
+				goto err_remove_groups;
+		}
 	}
 	return 0;
 
 err_remove_groups:
 	while (i--)
-		sysfs_remove_group(&priv->fw_attr_kset->kobj,
-				   cd01_attr_groups[i].attr_group);
+		if (lwmi_attr_01_is_supported(cd01_attr_groups[i].tunable_attr) > 0)

Reverse logic + continue.

drivers/platform/x86/lenovo/wmi-other.c static int lwmi_om_fw_attr_add(struct lwmi_om_priv *priv) · line 1072

Quoted 2 lines

drivers/platform/x86/lenovo/wmi-other.c +2 -0

+			sysfs_remove_group(&priv->fw_attr_kset->kobj,
+					   cd01_attr_groups[i].attr_group);

You need to add braces for multiline constructs.

Quoted 4 lines

drivers/platform/x86/lenovo/wmi-other.c +0 -0

 	kset_unregister(priv->fw_attr_kset);
 
 

Adds lwmi_is_attr_01_supported, and only creates the attribute subfolder if the attribute is supported by the hardware. Due to some poorly implemented BIOS, this is a multi-step sequence of events. This is because:
- Some BIOS support getting the capability data from custom mode (0xff), while others only support it in no-mode (0x00).
- Similarly, some BIOS support get/set for the current value from custom mode (0xff), while others only support it in no-mode (0x00).
- Some BIOS report capability data for a method that is not fully implemented.
- Some BIOS have methods fully implemented, but no complimentary capability data.

To ensure we only expose fully implemented methods with corresponding capability data, we check each outcome before reporting that an attribute can be supported.

Checking for lwmi_is_attr_01_supported during remove is not done to ensure that we don't attempt to call cd01 or send WMI events if one of the interfaces being removed was the cause of the driver unloading.

While adding members to tunable_attr_01, remove unused capdata pointer and limit size of all ID's to the appropriate size.

Reviewed-by Mark Pearson <[email protected]>

Reported-by Kurt Borja <[email protected]>

Closes https://lore.kernel.org/platform-driver-x86/[email protected]/ open here

Signed-off-by Derek J. Clark <[email protected]>

---
 drivers/platform/x86/lenovo/wmi-other.c | 117 +++++++++++++++++++++---
 1 file changed, 102 insertions(+), 15 deletions(-)

drivers/platform/x86/lenovo/wmi-other.c +71 -8

index 95886df39c8d..f3f12303e379 100644
@@ -545,11 +545,12 @@ static void lwmi_om_fan_info_collect_cd_fan(struct device *dev, struct cd_list *
 /* ======== fw_attributes (component: lenovo-wmi-capdata 01) ======== */
 
 struct tunable_attr_01 {
-	struct capdata01 *capdata;
 	struct device *dev;
-	u32 feature_id;
-	u32 device_id;
-	u32 type_id;
+	u8 feature_id;
+	u8 device_id;
+	u8 type_id;
+	u8 cd_mode_id; /* mode arg for searching capdata */
+	u8 cv_mode_id; /* mode arg for set/get current_value */
 };
 
 static struct tunable_attr_01 ppt_pl1_spl = {

@@ -716,7 +717,7 @@ static ssize_t attr_capdata01_show(struct kobject *kobj,
 	int value, ret;
 
 	attribute_id = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
-				    LWMI_GZ_THERMAL_MODE_CUSTOM, tunable_attr->type_id);
+				    tunable_attr->cd_mode_id, tunable_attr->type_id);
 
 	ret = lwmi_cd01_get_data(priv->cd01_list, attribute_id, &capdata);
 	if (ret)

@@ -782,7 +783,7 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
 		return -EBUSY;
 
 	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
-				 mode, tunable_attr->type_id);
+				 tunable_attr->cd_mode_id, tunable_attr->type_id);
 
 	ret = lwmi_cd01_get_data(priv->cd01_list, args.arg0, &capdata);
 	if (ret)

@@ -795,6 +796,8 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
 	if (value < capdata.min_value || value > capdata.max_value)
 		return -EINVAL;
 
+	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
+				 tunable_attr->cv_mode_id, tunable_attr->type_id);
 	args.arg1 = value;
 
 	ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_SET,

@@ -828,13 +831,16 @@ static ssize_t attr_current_value_show(struct kobject *kobj,
 	struct lwmi_om_priv *priv = dev_get_drvdata(tunable_attr->dev);
 	struct wmi_method_args_32 args;
 	enum thermal_mode mode;
-	int retval;
-	int ret;
+	int retval, ret;
 
 	ret = lwmi_om_notifier_call(&mode);
 	if (ret)
 		return ret;
 
+	/* If "no-mode" is the supported mode, ensure we never send current mode */
+	if (tunable_attr->cv_mode_id == LWMI_GZ_THERMAL_MODE_NONE)
+		mode = tunable_attr->cv_mode_id;
+
 	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
 				 mode, tunable_attr->type_id);
 

@@ -847,6 +853,85 @@ static ssize_t attr_current_value_show(struct kobject *kobj,
 	return sysfs_emit(buf, "%d\n", retval);
 }
 
+/**
+ * lwmi_attr_01_is_supported() - Determine if the given attribute is supported.
+ * @tunable_attr: The attribute to verify.
+ *
+ * First check if the attribute has a corresponding capdata01 table in the cd01
+ * module under the "custom" mode (0xff). If that is not present then check if
+ * there is a corresponding "no-mode" (0x00) entry. If either of those passes,
+ * check capdata->supported for values > 0. If capdata is available, attempt to
+ * determine the set/get mode for the current value property using a similar
+ * pattern. If the value returned by either custom or no-mode is 0, or we get
+ * an error, we assume that mode is not supported. If any of the above checks
+ * fail then the attribute is not fully supported.
+ *
+ * The probed cd_mode_id/cv_mode_id are stored on the tunable_attr for later
+ * reference.
+ *
+ * Return: Support level, or an error code.
+ */
+static int lwmi_attr_01_is_supported(struct tunable_attr_01 *tunable_attr)
+{
+	struct lwmi_om_priv *priv = dev_get_drvdata(tunable_attr->dev);
+	u8 mode = LWMI_GZ_THERMAL_MODE_CUSTOM;
+	struct wmi_method_args_32 args;
+	struct capdata01 capdata;
+	int retval, ret;
+
+	/* Determine tunable_attr->cd_mode_id */
+no_mode_fallback_1:
+	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
+				 mode, tunable_attr->type_id);
+
+	ret = lwmi_cd01_get_data(priv->cd01_list, args.arg0, &capdata);
+	if (ret && mode) {
+		dev_dbg(tunable_attr->dev, "Attribute id %x not supported\n", args.arg0);
+		mode = LWMI_GZ_THERMAL_MODE_NONE;
+		goto no_mode_fallback_1;
+	}
+	if (ret)
+		goto not_supported;
+	if (!capdata.supported) {
+		ret = -EOPNOTSUPP;
+		goto not_supported;
+	}
+
+	tunable_attr->cd_mode_id = mode;
+
+	/* Determine tunable_attr->cv_mode_id */
+	mode = LWMI_GZ_THERMAL_MODE_CUSTOM;
+no_mode_fallback_2:
+	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
+				 mode, tunable_attr->type_id);
+
+	ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_GET,
+				    (unsigned char *)&args, sizeof(args),
+				    &retval);
+	if ((ret && mode) || (!retval && mode)) {
+		dev_dbg(tunable_attr->dev, "Attribute id %x not supported\n", args.arg0);

drivers/platform/x86/lenovo/wmi-other.c +14 -5

+		tunable_attr->device_id, tunable_attr->feature_id, tunable_attr->cd_mode_id,
+		tunable_attr->type_id, args.arg0, capdata.supported);
+
+	return capdata.supported;
+
+not_supported:
+	dev_dbg(tunable_attr->dev, "Attribute id %x not supported\n", args.arg0);
+	return ret;
+}
+
 /* Lenovo WMI Other Mode Attribute macros */
 #define __LWMI_ATTR_RO(_func, _name)                                  \
 	{                                                             \

@@ -970,19 +1055,21 @@ static int lwmi_om_fw_attr_add(struct lwmi_om_priv *priv)
 	}
 
 	for (i = 0; i < ARRAY_SIZE(cd01_attr_groups) - 1; i++) {
-		err = sysfs_create_group(&priv->fw_attr_kset->kobj,
-					 cd01_attr_groups[i].attr_group);
-		if (err)
-			goto err_remove_groups;
-
 		cd01_attr_groups[i].tunable_attr->dev = &priv->wdev->dev;
+		if (lwmi_attr_01_is_supported(cd01_attr_groups[i].tunable_attr) > 0) {
+			err = sysfs_create_group(&priv->fw_attr_kset->kobj,
+						 cd01_attr_groups[i].attr_group);
+			if (err)

Hi Derek,

Quoted 160 lines · Derek J. Clark

Adds lwmi_is_attr_01_supported, and only creates the attribute subfolder if the attribute is supported by the hardware. Due to some poorly implemented BIOS, this is a multi-step sequence of events. This is because:
- Some BIOS support getting the capability data from custom mode (0xff), while others only support it in no-mode (0x00).
- Similarly, some BIOS support get/set for the current value from custom mode (0xff), while others only support it in no-mode (0x00).
- Some BIOS report capability data for a method that is not fully implemented.
- Some BIOS have methods fully implemented, but no complimentary capability data.

To ensure we only expose fully implemented methods with corresponding capability data, we check each outcome before reporting that an attribute can be supported.

Checking for lwmi_is_attr_01_supported during remove is not done to ensure that we don't attempt to call cd01 or send WMI events if one of the interfaces being removed was the cause of the driver unloading.

While adding members to tunable_attr_01, remove unused capdata pointer and limit size of all ID's to the appropriate size.

Reviewed-by Mark Pearson <[email protected]>

Reported-by Kurt Borja <[email protected]>

Closes https://lore.kernel.org/platform-driver-x86/[email protected]/ open here

Signed-off-by Derek J. Clark <[email protected]>

---
 drivers/platform/x86/lenovo/wmi-other.c | 117 +++++++++++++++++++++---
 1 file changed, 102 insertions(+), 15 deletions(-)

drivers/platform/x86/lenovo/wmi-other.c +71 -8

index 95886df39c8d..f3f12303e379 100644
@@ -545,11 +545,12 @@ static void lwmi_om_fan_info_collect_cd_fan(struct device *dev, struct cd_list *
 /* ======== fw_attributes (component: lenovo-wmi-capdata 01) ======== */
 
 struct tunable_attr_01 {
-	struct capdata01 *capdata;
 	struct device *dev;
-	u32 feature_id;
-	u32 device_id;
-	u32 type_id;
+	u8 feature_id;
+	u8 device_id;
+	u8 type_id;
+	u8 cd_mode_id; /* mode arg for searching capdata */
+	u8 cv_mode_id; /* mode arg for set/get current_value */
 };
 
 static struct tunable_attr_01 ppt_pl1_spl = {

@@ -716,7 +717,7 @@ static ssize_t attr_capdata01_show(struct kobject *kobj,
 	int value, ret;
 
 	attribute_id = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
-				    LWMI_GZ_THERMAL_MODE_CUSTOM, tunable_attr->type_id);
+				    tunable_attr->cd_mode_id, tunable_attr->type_id);
 
 	ret = lwmi_cd01_get_data(priv->cd01_list, attribute_id, &capdata);
 	if (ret)

@@ -782,7 +783,7 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
 		return -EBUSY;
 
 	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
-				 mode, tunable_attr->type_id);
+				 tunable_attr->cd_mode_id, tunable_attr->type_id);
 
 	ret = lwmi_cd01_get_data(priv->cd01_list, args.arg0, &capdata);
 	if (ret)

@@ -795,6 +796,8 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
 	if (value < capdata.min_value || value > capdata.max_value)
 		return -EINVAL;
 
+	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
+				 tunable_attr->cv_mode_id, tunable_attr->type_id);
 	args.arg1 = value;
 
 	ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_SET,

@@ -828,13 +831,16 @@ static ssize_t attr_current_value_show(struct kobject *kobj,
 	struct lwmi_om_priv *priv = dev_get_drvdata(tunable_attr->dev);
 	struct wmi_method_args_32 args;
 	enum thermal_mode mode;
-	int retval;
-	int ret;
+	int retval, ret;
 
 	ret = lwmi_om_notifier_call(&mode);
 	if (ret)
 		return ret;
 
+	/* If "no-mode" is the supported mode, ensure we never send current mode */
+	if (tunable_attr->cv_mode_id == LWMI_GZ_THERMAL_MODE_NONE)
+		mode = tunable_attr->cv_mode_id;
+
 	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
 				 mode, tunable_attr->type_id);
 

@@ -847,6 +853,85 @@ static ssize_t attr_current_value_show(struct kobject *kobj,
 	return sysfs_emit(buf, "%d\n", retval);
 }
 
+/**
+ * lwmi_attr_01_is_supported() - Determine if the given attribute is supported.
+ * @tunable_attr: The attribute to verify.
+ *
+ * First check if the attribute has a corresponding capdata01 table in the cd01
+ * module under the "custom" mode (0xff). If that is not present then check if
+ * there is a corresponding "no-mode" (0x00) entry. If either of those passes,
+ * check capdata->supported for values > 0. If capdata is available, attempt to
+ * determine the set/get mode for the current value property using a similar
+ * pattern. If the value returned by either custom or no-mode is 0, or we get
+ * an error, we assume that mode is not supported. If any of the above checks
+ * fail then the attribute is not fully supported.
+ *
+ * The probed cd_mode_id/cv_mode_id are stored on the tunable_attr for later
+ * reference.
+ *
+ * Return: Support level, or an error code.
+ */
+static int lwmi_attr_01_is_supported(struct tunable_attr_01 *tunable_attr)
+{
+	struct lwmi_om_priv *priv = dev_get_drvdata(tunable_attr->dev);
+	u8 mode = LWMI_GZ_THERMAL_MODE_CUSTOM;
+	struct wmi_method_args_32 args;
+	struct capdata01 capdata;
+	int retval, ret;
+
+	/* Determine tunable_attr->cd_mode_id */
+no_mode_fallback_1:
+	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
+				 mode, tunable_attr->type_id);
+
+	ret = lwmi_cd01_get_data(priv->cd01_list, args.arg0, &capdata);
+	if (ret && mode) {
+		dev_dbg(tunable_attr->dev, "Attribute id %x not supported\n", args.arg0);
+		mode = LWMI_GZ_THERMAL_MODE_NONE;
+		goto no_mode_fallback_1;
+	}
+	if (ret)
+		goto not_supported;
+	if (!capdata.supported) {
+		ret = -EOPNOTSUPP;
+		goto not_supported;
+	}
+
+	tunable_attr->cd_mode_id = mode;
+
+	/* Determine tunable_attr->cv_mode_id */
+	mode = LWMI_GZ_THERMAL_MODE_CUSTOM;
+no_mode_fallback_2:
+	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
+				 mode, tunable_attr->type_id);
+
+	ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_GET,
+				    (unsigned char *)&args, sizeof(args),
+				    &retval);
+	if ((ret && mode) || (!retval && mode)) {
+		dev_dbg(tunable_attr->dev, "Attribute id %x not supported\n", args.arg0);

drivers/platform/x86/lenovo/wmi-other.c static ssize_t attr_current_value_show(struct kobject *kobj, · line 922

Quoted 10 lines

drivers/platform/x86/lenovo/wmi-other.c +10 -0

+		mode = LWMI_GZ_THERMAL_MODE_NONE;
+		goto no_mode_fallback_2;
+	}
+	if (ret)
+		goto not_supported;
+	if (retval == 0) {
+		ret = -EOPNOTSUPP;
+		goto not_supported;
+	}
+

I agree with Ilpo that backward gotos are hard to read and should be avoided if possible.

drivers/platform/x86/lenovo/wmi-other.c static ssize_t attr_current_value_show(struct kobject *kobj, · line 924

Quoted 2 lines

drivers/platform/x86/lenovo/wmi-other.c +2 -0

+	tunable_attr->cv_mode_id = mode;
+	dev_dbg(tunable_attr->dev, "cd_mode_id: %02x%02x%02x%02x, cv_mode_id: %#08x attribute support level: %x\n",

It's a long line. Wrap at the first comma?

Quoted 27 lines

drivers/platform/x86/lenovo/wmi-other.c +14 -5

+		tunable_attr->device_id, tunable_attr->feature_id, tunable_attr->cd_mode_id,
+		tunable_attr->type_id, args.arg0, capdata.supported);
+
+	return capdata.supported;
+
+not_supported:
+	dev_dbg(tunable_attr->dev, "Attribute id %x not supported\n", args.arg0);
+	return ret;
+}
+
 /* Lenovo WMI Other Mode Attribute macros */
 #define __LWMI_ATTR_RO(_func, _name)                                  \
 	{                                                             \

@@ -970,19 +1055,21 @@ static int lwmi_om_fw_attr_add(struct lwmi_om_priv *priv)
 	}
 
 	for (i = 0; i < ARRAY_SIZE(cd01_attr_groups) - 1; i++) {
-		err = sysfs_create_group(&priv->fw_attr_kset->kobj,
-					 cd01_attr_groups[i].attr_group);
-		if (err)
-			goto err_remove_groups;
-
 		cd01_attr_groups[i].tunable_attr->dev = &priv->wdev->dev;
+		if (lwmi_attr_01_is_supported(cd01_attr_groups[i].tunable_attr) > 0) {
+			err = sysfs_create_group(&priv->fw_attr_kset->kobj,
+						 cd01_attr_groups[i].attr_group);
+			if (err)

drivers/platform/x86/lenovo/wmi-other.c static int lwmi_om_fw_attr_add(struct lwmi_om_priv *priv) · line 1070

Quoted 10 lines

drivers/platform/x86/lenovo/wmi-other.c +3 -2

+				goto err_remove_groups;
+		}
 	}
 	return 0;
 
 err_remove_groups:
 	while (i--)
-		sysfs_remove_group(&priv->fw_attr_kset->kobj,
-				   cd01_attr_groups[i].attr_group);
+		if (lwmi_attr_01_is_supported(cd01_attr_groups[i].tunable_attr) > 0)

I don't see lwmi_om_fw_attr_remove() doing the same. If we care about symmetric cleanup, I would imagine a bitmap or so recording what have been created is used there.

Also it's doubtful if the firmware always return the same result on the same parameters. If a broken firmware somehow returns different results here, some attributes may be left uncleaned.

That being said, I am not sure if we really need symmetric cleanup. IIUC, sysfs_remove_group() is a no-op if we pass an attr_group that has not been created.

drivers/platform/x86/lenovo/wmi-other.c static int lwmi_om_fw_attr_add(struct lwmi_om_priv *priv) · line 1075

Quoted 5 lines

drivers/platform/x86/lenovo/wmi-other.c +2 -0

+			sysfs_remove_group(&priv->fw_attr_kset->kobj,
+					   cd01_attr_groups[i].attr_group);
 
 	kset_unregister(priv->fw_attr_kset);
 

Thanks,
Rong

Quoted 228 lines · Rong Zhang

Hi Derek,

Quoted 160 lines · Derek J. Clark

Adds lwmi_is_attr_01_supported, and only creates the attribute subfolder if the attribute is supported by the hardware. Due to some poorly implemented BIOS, this is a multi-step sequence of events. This is because:
- Some BIOS support getting the capability data from custom mode (0xff), while others only support it in no-mode (0x00).
- Similarly, some BIOS support get/set for the current value from custom mode (0xff), while others only support it in no-mode (0x00).
- Some BIOS report capability data for a method that is not fully implemented.
- Some BIOS have methods fully implemented, but no complimentary capability data.

To ensure we only expose fully implemented methods with corresponding capability data, we check each outcome before reporting that an attribute can be supported.

Checking for lwmi_is_attr_01_supported during remove is not done to ensure that we don't attempt to call cd01 or send WMI events if one of the interfaces being removed was the cause of the driver unloading.

While adding members to tunable_attr_01, remove unused capdata pointer and limit size of all ID's to the appropriate size.

Reviewed-by Mark Pearson <[email protected]>

Reported-by Kurt Borja <[email protected]>

Closes https://lore.kernel.org/platform-driver-x86/[email protected]/ open here

Signed-off-by Derek J. Clark <[email protected]>

---
 drivers/platform/x86/lenovo/wmi-other.c | 117 +++++++++++++++++++++---
 1 file changed, 102 insertions(+), 15 deletions(-)

drivers/platform/x86/lenovo/wmi-other.c +71 -8

index 95886df39c8d..f3f12303e379 100644
@@ -545,11 +545,12 @@ static void lwmi_om_fan_info_collect_cd_fan(struct device *dev, struct cd_list *
 /* ======== fw_attributes (component: lenovo-wmi-capdata 01) ======== */
 
 struct tunable_attr_01 {
-	struct capdata01 *capdata;
 	struct device *dev;
-	u32 feature_id;
-	u32 device_id;
-	u32 type_id;
+	u8 feature_id;
+	u8 device_id;
+	u8 type_id;
+	u8 cd_mode_id; /* mode arg for searching capdata */
+	u8 cv_mode_id; /* mode arg for set/get current_value */
 };
 
 static struct tunable_attr_01 ppt_pl1_spl = {

@@ -716,7 +717,7 @@ static ssize_t attr_capdata01_show(struct kobject *kobj,
 	int value, ret;
 
 	attribute_id = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
-				    LWMI_GZ_THERMAL_MODE_CUSTOM, tunable_attr->type_id);
+				    tunable_attr->cd_mode_id, tunable_attr->type_id);
 
 	ret = lwmi_cd01_get_data(priv->cd01_list, attribute_id, &capdata);
 	if (ret)

@@ -782,7 +783,7 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
 		return -EBUSY;
 
 	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
-				 mode, tunable_attr->type_id);
+				 tunable_attr->cd_mode_id, tunable_attr->type_id);
 
 	ret = lwmi_cd01_get_data(priv->cd01_list, args.arg0, &capdata);
 	if (ret)

@@ -795,6 +796,8 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
 	if (value < capdata.min_value || value > capdata.max_value)
 		return -EINVAL;
 
+	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
+				 tunable_attr->cv_mode_id, tunable_attr->type_id);
 	args.arg1 = value;
 
 	ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_SET,

@@ -828,13 +831,16 @@ static ssize_t attr_current_value_show(struct kobject *kobj,
 	struct lwmi_om_priv *priv = dev_get_drvdata(tunable_attr->dev);
 	struct wmi_method_args_32 args;
 	enum thermal_mode mode;
-	int retval;
-	int ret;
+	int retval, ret;
 
 	ret = lwmi_om_notifier_call(&mode);
 	if (ret)
 		return ret;
 
+	/* If "no-mode" is the supported mode, ensure we never send current mode */
+	if (tunable_attr->cv_mode_id == LWMI_GZ_THERMAL_MODE_NONE)
+		mode = tunable_attr->cv_mode_id;
+
 	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
 				 mode, tunable_attr->type_id);
 

@@ -847,6 +853,85 @@ static ssize_t attr_current_value_show(struct kobject *kobj,
 	return sysfs_emit(buf, "%d\n", retval);
 }
 
+/**
+ * lwmi_attr_01_is_supported() - Determine if the given attribute is supported.
+ * @tunable_attr: The attribute to verify.
+ *
+ * First check if the attribute has a corresponding capdata01 table in the cd01
+ * module under the "custom" mode (0xff). If that is not present then check if
+ * there is a corresponding "no-mode" (0x00) entry. If either of those passes,
+ * check capdata->supported for values > 0. If capdata is available, attempt to
+ * determine the set/get mode for the current value property using a similar
+ * pattern. If the value returned by either custom or no-mode is 0, or we get
+ * an error, we assume that mode is not supported. If any of the above checks
+ * fail then the attribute is not fully supported.
+ *
+ * The probed cd_mode_id/cv_mode_id are stored on the tunable_attr for later
+ * reference.
+ *
+ * Return: Support level, or an error code.
+ */
+static int lwmi_attr_01_is_supported(struct tunable_attr_01 *tunable_attr)
+{
+	struct lwmi_om_priv *priv = dev_get_drvdata(tunable_attr->dev);
+	u8 mode = LWMI_GZ_THERMAL_MODE_CUSTOM;
+	struct wmi_method_args_32 args;
+	struct capdata01 capdata;
+	int retval, ret;
+
+	/* Determine tunable_attr->cd_mode_id */
+no_mode_fallback_1:
+	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
+				 mode, tunable_attr->type_id);
+
+	ret = lwmi_cd01_get_data(priv->cd01_list, args.arg0, &capdata);
+	if (ret && mode) {
+		dev_dbg(tunable_attr->dev, "Attribute id %x not supported\n", args.arg0);
+		mode = LWMI_GZ_THERMAL_MODE_NONE;
+		goto no_mode_fallback_1;
+	}
+	if (ret)
+		goto not_supported;
+	if (!capdata.supported) {
+		ret = -EOPNOTSUPP;
+		goto not_supported;
+	}
+
+	tunable_attr->cd_mode_id = mode;
+
+	/* Determine tunable_attr->cv_mode_id */
+	mode = LWMI_GZ_THERMAL_MODE_CUSTOM;
+no_mode_fallback_2:
+	args.arg0 = LWMI_ATTR_ID(tunable_attr->device_id, tunable_attr->feature_id,
+				 mode, tunable_attr->type_id);
+
+	ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_GET,
+				    (unsigned char *)&args, sizeof(args),
+				    &retval);
+	if ((ret && mode) || (!retval && mode)) {
+		dev_dbg(tunable_attr->dev, "Attribute id %x not supported\n", args.arg0);

drivers/platform/x86/lenovo/wmi-other.c static ssize_t attr_current_value_show(struct kobject *kobj, · line 922

Quoted 10 lines

drivers/platform/x86/lenovo/wmi-other.c +10 -0

+		mode = LWMI_GZ_THERMAL_MODE_NONE;
+		goto no_mode_fallback_2;
+	}
+	if (ret)
+		goto not_supported;
+	if (retval == 0) {
+		ret = -EOPNOTSUPP;
+		goto not_supported;
+	}
+

I agree with Ilpo that backward gotos are hard to read and should be avoided if possible.

drivers/platform/x86/lenovo/wmi-other.c static ssize_t attr_current_value_show(struct kobject *kobj, · line 924

Quoted 2 lines

drivers/platform/x86/lenovo/wmi-other.c +2 -0

+	tunable_attr->cv_mode_id = mode;
+	dev_dbg(tunable_attr->dev, "cd_mode_id: %02x%02x%02x%02x, cv_mode_id: %#08x attribute support level: %x\n",

It's a long line. Wrap at the first comma?

Quoted 27 lines

drivers/platform/x86/lenovo/wmi-other.c +14 -5

+		tunable_attr->device_id, tunable_attr->feature_id, tunable_attr->cd_mode_id,
+		tunable_attr->type_id, args.arg0, capdata.supported);
+
+	return capdata.supported;
+
+not_supported:
+	dev_dbg(tunable_attr->dev, "Attribute id %x not supported\n", args.arg0);
+	return ret;
+}
+
 /* Lenovo WMI Other Mode Attribute macros */
 #define __LWMI_ATTR_RO(_func, _name)                                  \
 	{                                                             \

@@ -970,19 +1055,21 @@ static int lwmi_om_fw_attr_add(struct lwmi_om_priv *priv)
 	}
 
 	for (i = 0; i < ARRAY_SIZE(cd01_attr_groups) - 1; i++) {
-		err = sysfs_create_group(&priv->fw_attr_kset->kobj,
-					 cd01_attr_groups[i].attr_group);
-		if (err)
-			goto err_remove_groups;
-
 		cd01_attr_groups[i].tunable_attr->dev = &priv->wdev->dev;
+		if (lwmi_attr_01_is_supported(cd01_attr_groups[i].tunable_attr) > 0) {
+			err = sysfs_create_group(&priv->fw_attr_kset->kobj,
+						 cd01_attr_groups[i].attr_group);
+			if (err)

drivers/platform/x86/lenovo/wmi-other.c static int lwmi_om_fw_attr_add(struct lwmi_om_priv *priv) · line 1070

Quoted 10 lines

drivers/platform/x86/lenovo/wmi-other.c +3 -2

+				goto err_remove_groups;
+		}
 	}
 	return 0;
 
 err_remove_groups:
 	while (i--)
-		sysfs_remove_group(&priv->fw_attr_kset->kobj,
-				   cd01_attr_groups[i].attr_group);
+		if (lwmi_attr_01_is_supported(cd01_attr_groups[i].tunable_attr) > 0)

I don't see lwmi_om_fw_attr_remove() doing the same. If we care about symmetric cleanup, I would imagine a bitmap or so recording what have been created is used there.

Also it's doubtful if the firmware always return the same result on the same parameters. If a broken firmware somehow returns different results here, some attributes may be left uncleaned.

Quoted 3 lines

That being said, I am not sure if we really need symmetric cleanup. IIUC, sysfs_remove_group() is a no-op if we pass an attr_group that has not been created.

Hi Rong,
We don't want to query the interface if it was removed, and the no-op is harmless. It doesn't seem worth it to me to retain that in memory to achieve the same results.

Quoted 8 lines

drivers/platform/x86/lenovo/wmi-other.c static int lwmi_om_fw_attr_add(struct lwmi_om_priv *priv) · line 1075

Quoted 5 lines

drivers/platform/x86/lenovo/wmi-other.c +2 -0

+			sysfs_remove_group(&priv->fw_attr_kset->kobj,
+					   cd01_attr_groups[i].attr_group);
 
 	kset_unregister(priv->fw_attr_kset);
 

Thanks,
Rong

Use an enum for device ID's and CPU attribute feature ID's, add missing CPU attributes.

Reviewed-by Mark Pearson <[email protected]>

Signed-off-by Derek J. Clark <[email protected]>

---
 .../wmi/devices/lenovo-wmi-other.rst          | 10 ++
 drivers/platform/x86/lenovo/wmi-capdata.h     |  5 +-
 drivers/platform/x86/lenovo/wmi-other.c       | 98 ++++++++++++++++++-
 3 files changed, 107 insertions(+), 6 deletions(-)

Documentation/wmi/devices/lenovo-wmi-other.rst +10 -0

index 01d471156738..f4763ed66cc6 100644
@@ -68,9 +68,19 @@ Each attribute has the following properties:
  - type
 
 The following firmware-attributes are implemented:
+ - cpu_oc_stat: CPU Overlocking Status
+ - cpu_temp: CPU Thermal Load Limit
+ - ppt_cpu_cl: CPU Cross Loading Power Limit
+ - ppt_pl1_apu_spl: Platform Profile Tracking APU Sustained Power Limit
  - ppt_pl1_spl: Platform Profile Tracking Sustained Power Limit
+ - ppt_pl1_spl_cl: Platform Profile Tracking Cross Loading Sustained Power Limit
+ - ppt_pl1_tau: Exceed Duration for Platform Profile Tracking Sustained Power Limit
  - ppt_pl2_sppt: Platform Profile Tracking Slow Package Power Tracking
+ - ppt_pl2_sppt_cl: Platform Profile Tracking Cross Loading Slow Package Tracking
  - ppt_pl3_fppt: Platform Profile Tracking Fast Package Power Tracking
+ - ppt_pl3_fppt_cl: Platform Profile Tracking Cross Loading Fast Package Power Tracking
+ - ppt_pl4_ipl: Platform Profile Trakcing Instantaneous Power Limit
+ - ppt_pl4_ipl_cl: Platform Profile Tracking Cross Loading Instantaneous Power Limit
 
 LENOVO_FAN_TEST_DATA
 -------------------------

drivers/platform/x86/lenovo/wmi-capdata.h +4 -1

index 27202e2dc8a5..aa48f43cbb43 100644
@@ -23,7 +23,10 @@
 	 FIELD_PREP(LWMI_ATTR_MODE_ID_MASK, mode) | \
 	 FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, type))
 
-#define LWMI_DEVICE_ID_FAN	0x04
+enum lwmi_device_id {
+	LWMI_DEVICE_ID_CPU = 0x01,
+	LWMI_DEVICE_ID_FAN = 0x04,
+};
 
 struct component_match;
 struct device;

drivers/platform/x86/lenovo/wmi-other.c +0 -4

index f3f12303e379..87aba244da84 100644
@@ -54,15 +54,21 @@
 
 #define LENOVO_OTHER_MODE_GUID "DC2A8805-3A8C-41BA-A6F7-092E0089CD3B"
 
-#define LWMI_DEVICE_ID_CPU 0x01
-
-#define LWMI_FEATURE_ID_CPU_SPPT 0x01
-#define LWMI_FEATURE_ID_CPU_SPL 0x02

drivers/platform/x86/lenovo/wmi-other.c +84 -0

+};
 
 #define LWMI_FEATURE_ID_FAN_RPM 0x03
 
 #define LWMI_TYPE_ID_NONE 0x00
+#define LWMI_TYPE_ID_CROSSLOAD 0x01
 
 #define LWMI_FEATURE_VALUE_GET 17
 #define LWMI_FEATURE_VALUE_SET 18

@@ -559,18 +565,72 @@ static struct tunable_attr_01 ppt_pl1_spl = {
 	.type_id = LWMI_TYPE_ID_NONE,
 };
 
+static struct tunable_attr_01 ppt_pl1_spl_cl = {
+	.device_id = LWMI_DEVICE_ID_CPU,
+	.feature_id = LWMI_FEATURE_ID_CPU_SPL,
+	.type_id = LWMI_TYPE_ID_CROSSLOAD,
+};
+
 static struct tunable_attr_01 ppt_pl2_sppt = {
 	.device_id = LWMI_DEVICE_ID_CPU,
 	.feature_id = LWMI_FEATURE_ID_CPU_SPPT,
 	.type_id = LWMI_TYPE_ID_NONE,
 };
 
+static struct tunable_attr_01 ppt_pl2_sppt_cl = {
+	.device_id = LWMI_DEVICE_ID_CPU,
+	.feature_id = LWMI_FEATURE_ID_CPU_SPPT,
+	.type_id = LWMI_TYPE_ID_CROSSLOAD,
+};
+
 static struct tunable_attr_01 ppt_pl3_fppt = {
 	.device_id = LWMI_DEVICE_ID_CPU,
 	.feature_id = LWMI_FEATURE_ID_CPU_FPPT,
 	.type_id = LWMI_TYPE_ID_NONE,
 };
 
+static struct tunable_attr_01 ppt_pl3_fppt_cl = {
+	.device_id = LWMI_DEVICE_ID_CPU,
+	.feature_id = LWMI_FEATURE_ID_CPU_FPPT,
+	.type_id = LWMI_TYPE_ID_CROSSLOAD,
+};
+
+static struct tunable_attr_01 cpu_temp = {
+	.device_id = LWMI_DEVICE_ID_CPU,
+	.feature_id = LWMI_FEATURE_ID_CPU_TEMP,
+	.type_id = LWMI_TYPE_ID_NONE,
+};
+
+static struct tunable_attr_01 ppt_pl1_apu_spl = {
+	.device_id = LWMI_DEVICE_ID_CPU,
+	.feature_id = LWMI_FEATURE_ID_CPU_APU,
+	.type_id = LWMI_TYPE_ID_NONE,
+};
+
+static struct tunable_attr_01 ppt_cpu_cl = {
+	.device_id = LWMI_DEVICE_ID_CPU,
+	.feature_id = LWMI_FEATURE_ID_CPU_CL,
+	.type_id = LWMI_TYPE_ID_NONE,
+};
+
+static struct tunable_attr_01 ppt_pl1_tau = {
+	.device_id = LWMI_DEVICE_ID_CPU,
+	.feature_id = LWMI_FEATURE_ID_CPU_TAU,
+	.type_id = LWMI_TYPE_ID_NONE,
+};
+
+static struct tunable_attr_01 ppt_pl4_ipl = {
+	.device_id = LWMI_DEVICE_ID_CPU,
+	.feature_id = LWMI_FEATURE_ID_CPU_IPL,
+	.type_id = LWMI_TYPE_ID_NONE,
+};
+
+static struct tunable_attr_01 ppt_pl4_ipl_cl = {
+	.device_id = LWMI_DEVICE_ID_CPU,
+	.feature_id = LWMI_FEATURE_ID_CPU_IPL,
+	.type_id = LWMI_TYPE_ID_CROSSLOAD,
+};
+
 struct capdata01_attr_group {
 	const struct attribute_group *attr_group;
 	struct tunable_attr_01 *tunable_attr;

@@ -1009,17 +1069,45 @@ static int lwmi_attr_01_is_supported(struct tunable_attr_01 *tunable_attr)
 		.name = _fsname, .attrs = _attrname##_attrs               \
 	}
 
+LWMI_ATTR_GROUP_TUNABLE_CAP01(cpu_temp, "cpu_temp",
+			      "Set the CPU thermal load limit");
+LWMI_ATTR_GROUP_TUNABLE_CAP01(ppt_cpu_cl, "ppt_cpu_cl",
+			      "Set the CPU cross loading power limit");
+LWMI_ATTR_GROUP_TUNABLE_CAP01(ppt_pl1_apu_spl, "ppt_pl1_apu_spl",
+			      "Set the APU sustained power limit");
 LWMI_ATTR_GROUP_TUNABLE_CAP01(ppt_pl1_spl, "ppt_pl1_spl",
 			      "Set the CPU sustained power limit");
+LWMI_ATTR_GROUP_TUNABLE_CAP01(ppt_pl1_spl_cl, "ppt_pl1_spl_cl",
+			      "Set the CPU cross loading sustained power limit");
 LWMI_ATTR_GROUP_TUNABLE_CAP01(ppt_pl2_sppt, "ppt_pl2_sppt",
 			      "Set the CPU slow package power tracking limit");
+LWMI_ATTR_GROUP_TUNABLE_CAP01(ppt_pl2_sppt_cl, "ppt_pl2_sppt_cl",
+			      "Set the CPU cross loading slow package power tracking limit");
 LWMI_ATTR_GROUP_TUNABLE_CAP01(ppt_pl3_fppt, "ppt_pl3_fppt",
 			      "Set the CPU fast package power tracking limit");
+LWMI_ATTR_GROUP_TUNABLE_CAP01(ppt_pl3_fppt_cl, "ppt_pl3_fppt_cl",
+			      "Set the CPU cross loading fast package power tracking limit");
+LWMI_ATTR_GROUP_TUNABLE_CAP01(ppt_pl1_tau, "ppt_pl1_tau",
+			      "Set the CPU sustained power limit exceed duration");
+LWMI_ATTR_GROUP_TUNABLE_CAP01(ppt_pl4_ipl, "ppt_pl4_ipl",
+			      "Set the CPU instantaneous power limit");
+LWMI_ATTR_GROUP_TUNABLE_CAP01(ppt_pl4_ipl_cl, "ppt_pl4_ipl_cl",
+			      "Set the CPU cross loading instantaneous power limit");
+
 
 static struct capdata01_attr_group cd01_attr_groups[] = {
+	{ &cpu_temp_attr_group, &cpu_temp },
+	{ &ppt_cpu_cl_attr_group, &ppt_cpu_cl },
+	{ &ppt_pl1_apu_spl_attr_group, &ppt_pl1_apu_spl },
 	{ &ppt_pl1_spl_attr_group, &ppt_pl1_spl },
+	{ &ppt_pl1_spl_cl_attr_group, &ppt_pl1_spl_cl },
+	{ &ppt_pl1_tau_attr_group, &ppt_pl1_tau },
 	{ &ppt_pl2_sppt_attr_group, &ppt_pl2_sppt },
+	{ &ppt_pl2_sppt_cl_attr_group, &ppt_pl2_sppt_cl },
 	{ &ppt_pl3_fppt_attr_group, &ppt_pl3_fppt },
+	{ &ppt_pl3_fppt_cl_attr_group, &ppt_pl3_fppt_cl },
+	{ &ppt_pl4_ipl_attr_group, &ppt_pl4_ipl },
+	{ &ppt_pl4_ipl_cl_attr_group, &ppt_pl4_ipl_cl },
 	{},
 };
 

Use an enum for all GPU attribute feature ID's and add GPU attributes.

Reviewed-by Mark Pearson <[email protected]>

Signed-off-by Derek J. Clark <[email protected]>

---

.../wmi/devices/lenovo-wmi-other.rst          |  11 ++
drivers/platform/x86/lenovo/wmi-capdata.h     |   1 +
drivers/platform/x86/lenovo/wmi-other.c       | 105 ++++++++++++++++++
3 files changed, 117 insertions(+)

Documentation/wmi/devices/lenovo-wmi-other.rst +0 -0

index f4763ed66cc6..f7564b23bb7f 100644
@@ -70,6 +70,17 @@ Each attribute has the following properties:

The following firmware-attributes are implemented:
- cpu_oc_stat: CPU Overlocking Status

- cpu_temp: CPU Thermal Load Limit
+ - dgpu_boost_clk: Dedicated GPU Boost Clock
+ - dgpu_enable: Dedicated GPU Enabled Status
+ - gpu_didvid: GPU Device Identifier and Vendor Identifier
+ - gpu_mode: GPU Mode by Power Limit
+ - gpu_nv_ac_offset: Nvidia GPU AC Total Processing Power Baseline Offset
+ - gpu_nv_bpl: Nvidia GPU Base Power Limit
+ - gpu_nv_cpu_boost: Nvidia GPU to CPU Dynamic Boost Limit
+ - gpu_nv_ctgp: Nvidia GPU Configurable Total Graphics Power
+ - gpu_nv_ppab: Nvidia GPU Power Performance Aware Boost Limit
+ - gpu_oc_stat: GPU Overclocking Status

+ - gpu_temp: GPU Thermal Load Limit
- ppt_cpu_cl: CPU Cross Loading Power Limit
- ppt_pl1_apu_spl: Platform Profile Tracking APU Sustained Power Limit
- ppt_pl1_spl: Platform Profile Tracking Sustained Power Limit

drivers/platform/x86/lenovo/wmi-capdata.h +1 -0

index aa48f43cbb43..b7f9ee7b301a 100644
@@ -25,6 +25,7 @@
 
 enum lwmi_device_id {
 	LWMI_DEVICE_ID_CPU = 0x01,
+	LWMI_DEVICE_ID_GPU = 0x02,
 	LWMI_DEVICE_ID_FAN = 0x04,
 };
 

drivers/platform/x86/lenovo/wmi-other.c +105 -0

index 87aba244da84..67768f6a50e0 100644
@@ -65,6 +65,19 @@ enum lwmi_feature_id_cpu {
 	LWMI_FEATURE_ID_CPU_IPL = 0x09,
 };
 
+enum lwmi_feature_id_gpu {
+	LWMI_FEATURE_ID_GPU_NV_PPAB = 0x01,
+	LWMI_FEATURE_ID_GPU_NV_CTGP = 0x02,
+	LWMI_FEATURE_ID_GPU_TEMP = 0x03,
+	LWMI_FEATURE_ID_GPU_AC_OFFSET = 0x04,
+	LWMI_FEATURE_ID_DGPU_BOOST_CLK = 0x06,
+	LWMI_FEATURE_ID_DGPU_EN = 0x07,
+	LWMI_FEATURE_ID_GPU_MODE = 0x08,
+	LWMI_FEATURE_ID_DGPU_DIDVID = 0x09,
+	LWMI_FEATURE_ID_GPU_NV_BPL = 0x0a,
+	LWMI_FEATURE_ID_GPU_NV_CPU_BOOST = 0x0b,
+};
+
 #define LWMI_FEATURE_ID_FAN_RPM 0x03
 
 #define LWMI_TYPE_ID_NONE 0x00

@@ -631,6 +644,66 @@ static struct tunable_attr_01 ppt_pl4_ipl_cl = {
 	.type_id = LWMI_TYPE_ID_CROSSLOAD,
 };
 
+static struct tunable_attr_01 gpu_nv_ppab = {
+	.device_id = LWMI_DEVICE_ID_GPU,
+	.feature_id = LWMI_FEATURE_ID_GPU_NV_PPAB,
+	.type_id = LWMI_TYPE_ID_NONE,
+};
+
+static struct tunable_attr_01 gpu_nv_ctgp = {
+	.device_id = LWMI_DEVICE_ID_GPU,
+	.feature_id = LWMI_FEATURE_ID_GPU_NV_CTGP,
+	.type_id = LWMI_TYPE_ID_NONE,
+};
+
+static struct tunable_attr_01 gpu_temp = {
+	.device_id = LWMI_DEVICE_ID_GPU,
+	.feature_id = LWMI_FEATURE_ID_GPU_TEMP,
+	.type_id = LWMI_TYPE_ID_NONE,
+};
+
+static struct tunable_attr_01 gpu_nv_ac_offset = {
+	.device_id = LWMI_DEVICE_ID_GPU,
+	.feature_id = LWMI_FEATURE_ID_GPU_AC_OFFSET,
+	.type_id = LWMI_TYPE_ID_NONE,
+};
+
+static struct tunable_attr_01 dgpu_boost_clk = {
+	.device_id = LWMI_DEVICE_ID_GPU,
+	.feature_id = LWMI_FEATURE_ID_DGPU_BOOST_CLK,
+	.type_id = LWMI_TYPE_ID_NONE,
+};
+
+static struct tunable_attr_01 dgpu_enable = {
+	.device_id = LWMI_DEVICE_ID_GPU,
+	.feature_id = LWMI_FEATURE_ID_DGPU_EN,
+	.type_id = LWMI_TYPE_ID_NONE,
+};
+
+static struct tunable_attr_01 gpu_mode = {
+	.device_id = LWMI_DEVICE_ID_GPU,
+	.feature_id = LWMI_FEATURE_ID_GPU_MODE,
+	.type_id = LWMI_TYPE_ID_NONE,
+};
+
+static struct tunable_attr_01 dgpu_didvid = {
+	.device_id = LWMI_DEVICE_ID_GPU,
+	.feature_id = LWMI_FEATURE_ID_DGPU_DIDVID,
+	.type_id = LWMI_TYPE_ID_NONE,
+};
+
+static struct tunable_attr_01 gpu_nv_bpl = {
+	.device_id = LWMI_DEVICE_ID_GPU,
+	.feature_id = LWMI_FEATURE_ID_GPU_NV_BPL,
+	.type_id = LWMI_TYPE_ID_NONE,
+};
+
+static struct tunable_attr_01 gpu_nv_cpu_boost = {
+	.device_id = LWMI_DEVICE_ID_GPU,
+	.feature_id = LWMI_FEATURE_ID_GPU_NV_CPU_BOOST,
+	.type_id = LWMI_TYPE_ID_NONE,
+};
+
 struct capdata01_attr_group {
 	const struct attribute_group *attr_group;
 	struct tunable_attr_01 *tunable_attr;

@@ -1069,6 +1142,7 @@ static int lwmi_attr_01_is_supported(struct tunable_attr_01 *tunable_attr)
 		.name = _fsname, .attrs = _attrname##_attrs               \
 	}
 
+/* CPU tunable attributes */
 LWMI_ATTR_GROUP_TUNABLE_CAP01(cpu_temp, "cpu_temp",
 			      "Set the CPU thermal load limit");
 LWMI_ATTR_GROUP_TUNABLE_CAP01(ppt_cpu_cl, "ppt_cpu_cl",

@@ -1094,9 +1168,40 @@ LWMI_ATTR_GROUP_TUNABLE_CAP01(ppt_pl4_ipl, "ppt_pl4_ipl",
 LWMI_ATTR_GROUP_TUNABLE_CAP01(ppt_pl4_ipl_cl, "ppt_pl4_ipl_cl",
 			      "Set the CPU cross loading instantaneous power limit");
 
+/* GPU tunable attributes */
+LWMI_ATTR_GROUP_TUNABLE_CAP01(dgpu_boost_clk, "gpu_boost_clk",
+			      "Set the dedicated GPU boost clock");
+LWMI_ATTR_GROUP_TUNABLE_CAP01(dgpu_didvid, "gpu_didvid",
+			      "Get the GPU device identifier and vendor identifier");
+LWMI_ATTR_GROUP_TUNABLE_CAP01(dgpu_enable, "dgpu_enable",
+			      "Set the dedicated Nvidia GPU enabled status");
+LWMI_ATTR_GROUP_TUNABLE_CAP01(gpu_mode, "gpu_mode",
+			      "Set the GPU mode by power limit");
+LWMI_ATTR_GROUP_TUNABLE_CAP01(gpu_nv_ac_offset, "gpu_nv_ac_offset",
+			      "Set the Nvidia GPU AC total processing power baseline offset");
+LWMI_ATTR_GROUP_TUNABLE_CAP01(gpu_nv_bpl, "gpu_nv_bpl",
+			      "Set the Nvidia GPU base power limit");
+LWMI_ATTR_GROUP_TUNABLE_CAP01(gpu_nv_cpu_boost, "gpu_nv_cpu_boost",
+			      "Set the Nvidia GPU to CPU dynamic boost limit");
+LWMI_ATTR_GROUP_TUNABLE_CAP01(gpu_nv_ctgp, "gpu_nv_ctgp",
+			      "Set the GPU configurable total graphics power");
+LWMI_ATTR_GROUP_TUNABLE_CAP01(gpu_nv_ppab, "gpu_nv_ppab",
+			      "Set the Nvidia GPU power performance aware boost limit");
+LWMI_ATTR_GROUP_TUNABLE_CAP01(gpu_temp, "gpu_temp",
+			      "Set the GPU thermal load limit");
 
 static struct capdata01_attr_group cd01_attr_groups[] = {
 	{ &cpu_temp_attr_group, &cpu_temp },
+	{ &dgpu_boost_clk_attr_group, &dgpu_boost_clk },
+	{ &dgpu_didvid_attr_group, &dgpu_didvid },
+	{ &dgpu_enable_attr_group, &dgpu_enable },
+	{ &gpu_mode_attr_group, &gpu_mode },
+	{ &gpu_nv_ac_offset_attr_group, &gpu_nv_ac_offset },
+	{ &gpu_nv_bpl_attr_group, &gpu_nv_bpl },
+	{ &gpu_nv_cpu_boost_attr_group, &gpu_nv_cpu_boost },
+	{ &gpu_nv_ctgp_attr_group, &gpu_nv_ctgp },
+	{ &gpu_nv_ppab_attr_group, &gpu_nv_ppab },
+	{ &gpu_temp_attr_group, &gpu_temp },
 	{ &ppt_cpu_cl_attr_group, &ppt_cpu_cl },
 	{ &ppt_pl1_apu_spl_attr_group, &ppt_pl1_apu_spl },
 	{ &ppt_pl1_spl_attr_group, &ppt_pl1_spl },

Use an enum for all GPU attribute feature ID's and add GPU attributes.

Reviewed-by Mark Pearson <[email protected]>

Signed-off-by Derek J. Clark <[email protected]>

---
 .../wmi/devices/lenovo-wmi-other.rst          |  11 ++
 drivers/platform/x86/lenovo/wmi-capdata.h     |   1 +
 drivers/platform/x86/lenovo/wmi-other.c       | 105 ++++++++++++++++++
 3 files changed, 117 insertions(+)

Documentation/wmi/devices/lenovo-wmi-other.rst +11 -0

index f4763ed66cc6..f7564b23bb7f 100644
@@ -70,6 +70,17 @@ Each attribute has the following properties:
 The following firmware-attributes are implemented:
  - cpu_oc_stat: CPU Overlocking Status
  - cpu_temp: CPU Thermal Load Limit
+ - dgpu_boost_clk: Dedicated GPU Boost Clock
+ - dgpu_enable: Dedicated GPU Enabled Status
+ - gpu_didvid: GPU Device Identifier and Vendor Identifier
+ - gpu_mode: GPU Mode by Power Limit
+ - gpu_nv_ac_offset: Nvidia GPU AC Total Processing Power Baseline Offset
+ - gpu_nv_bpl: Nvidia GPU Base Power Limit
+ - gpu_nv_cpu_boost: Nvidia GPU to CPU Dynamic Boost Limit
+ - gpu_nv_ctgp: Nvidia GPU Configurable Total Graphics Power
+ - gpu_nv_ppab: Nvidia GPU Power Performance Aware Boost Limit
+ - gpu_oc_stat: GPU Overclocking Status
+ - gpu_temp: GPU Thermal Load Limit
  - ppt_cpu_cl: CPU Cross Loading Power Limit
  - ppt_pl1_apu_spl: Platform Profile Tracking APU Sustained Power Limit
  - ppt_pl1_spl: Platform Profile Tracking Sustained Power Limit

drivers/platform/x86/lenovo/wmi-capdata.h +1 -0

index aa48f43cbb43..b7f9ee7b301a 100644
@@ -25,6 +25,7 @@
 
 enum lwmi_device_id {
 	LWMI_DEVICE_ID_CPU = 0x01,
+	LWMI_DEVICE_ID_GPU = 0x02,
 	LWMI_DEVICE_ID_FAN = 0x04,
 };
 

drivers/platform/x86/lenovo/wmi-other.c +1 -0

index 87aba244da84..67768f6a50e0 100644
@@ -65,6 +65,19 @@ enum lwmi_feature_id_cpu {
 	LWMI_FEATURE_ID_CPU_IPL = 0x09,
 };
 
+enum lwmi_feature_id_gpu {

drivers/platform/x86/lenovo/wmi-other.c +94 -0

+};
+
 #define LWMI_FEATURE_ID_FAN_RPM 0x03
 
 #define LWMI_TYPE_ID_NONE 0x00

@@ -631,6 +644,66 @@ static struct tunable_attr_01 ppt_pl4_ipl_cl = {
 	.type_id = LWMI_TYPE_ID_CROSSLOAD,
 };
 
+static struct tunable_attr_01 gpu_nv_ppab = {
+	.device_id = LWMI_DEVICE_ID_GPU,
+	.feature_id = LWMI_FEATURE_ID_GPU_NV_PPAB,
+	.type_id = LWMI_TYPE_ID_NONE,
+};
+
+static struct tunable_attr_01 gpu_nv_ctgp = {
+	.device_id = LWMI_DEVICE_ID_GPU,
+	.feature_id = LWMI_FEATURE_ID_GPU_NV_CTGP,
+	.type_id = LWMI_TYPE_ID_NONE,
+};
+
+static struct tunable_attr_01 gpu_temp = {
+	.device_id = LWMI_DEVICE_ID_GPU,
+	.feature_id = LWMI_FEATURE_ID_GPU_TEMP,
+	.type_id = LWMI_TYPE_ID_NONE,
+};
+
+static struct tunable_attr_01 gpu_nv_ac_offset = {
+	.device_id = LWMI_DEVICE_ID_GPU,
+	.feature_id = LWMI_FEATURE_ID_GPU_AC_OFFSET,
+	.type_id = LWMI_TYPE_ID_NONE,
+};
+
+static struct tunable_attr_01 dgpu_boost_clk = {
+	.device_id = LWMI_DEVICE_ID_GPU,
+	.feature_id = LWMI_FEATURE_ID_DGPU_BOOST_CLK,
+	.type_id = LWMI_TYPE_ID_NONE,
+};
+
+static struct tunable_attr_01 dgpu_enable = {
+	.device_id = LWMI_DEVICE_ID_GPU,
+	.feature_id = LWMI_FEATURE_ID_DGPU_EN,
+	.type_id = LWMI_TYPE_ID_NONE,
+};
+
+static struct tunable_attr_01 gpu_mode = {
+	.device_id = LWMI_DEVICE_ID_GPU,
+	.feature_id = LWMI_FEATURE_ID_GPU_MODE,
+	.type_id = LWMI_TYPE_ID_NONE,
+};
+
+static struct tunable_attr_01 dgpu_didvid = {
+	.device_id = LWMI_DEVICE_ID_GPU,
+	.feature_id = LWMI_FEATURE_ID_DGPU_DIDVID,
+	.type_id = LWMI_TYPE_ID_NONE,
+};
+
+static struct tunable_attr_01 gpu_nv_bpl = {
+	.device_id = LWMI_DEVICE_ID_GPU,
+	.feature_id = LWMI_FEATURE_ID_GPU_NV_BPL,
+	.type_id = LWMI_TYPE_ID_NONE,
+};
+
+static struct tunable_attr_01 gpu_nv_cpu_boost = {
+	.device_id = LWMI_DEVICE_ID_GPU,
+	.feature_id = LWMI_FEATURE_ID_GPU_NV_CPU_BOOST,
+	.type_id = LWMI_TYPE_ID_NONE,
+};
+
 struct capdata01_attr_group {
 	const struct attribute_group *attr_group;
 	struct tunable_attr_01 *tunable_attr;

@@ -1069,6 +1142,7 @@ static int lwmi_attr_01_is_supported(struct tunable_attr_01 *tunable_attr)
 		.name = _fsname, .attrs = _attrname##_attrs               \
 	}
 
+/* CPU tunable attributes */
 LWMI_ATTR_GROUP_TUNABLE_CAP01(cpu_temp, "cpu_temp",
 			      "Set the CPU thermal load limit");
 LWMI_ATTR_GROUP_TUNABLE_CAP01(ppt_cpu_cl, "ppt_cpu_cl",

@@ -1094,9 +1168,40 @@ LWMI_ATTR_GROUP_TUNABLE_CAP01(ppt_pl4_ipl, "ppt_pl4_ipl",
 LWMI_ATTR_GROUP_TUNABLE_CAP01(ppt_pl4_ipl_cl, "ppt_pl4_ipl_cl",
 			      "Set the CPU cross loading instantaneous power limit");
 
+/* GPU tunable attributes */
+LWMI_ATTR_GROUP_TUNABLE_CAP01(dgpu_boost_clk, "gpu_boost_clk",
+			      "Set the dedicated GPU boost clock");
+LWMI_ATTR_GROUP_TUNABLE_CAP01(dgpu_didvid, "gpu_didvid",
+			      "Get the GPU device identifier and vendor identifier");
+LWMI_ATTR_GROUP_TUNABLE_CAP01(dgpu_enable, "dgpu_enable",
+			      "Set the dedicated Nvidia GPU enabled status");
+LWMI_ATTR_GROUP_TUNABLE_CAP01(gpu_mode, "gpu_mode",
+			      "Set the GPU mode by power limit");
+LWMI_ATTR_GROUP_TUNABLE_CAP01(gpu_nv_ac_offset, "gpu_nv_ac_offset",
+			      "Set the Nvidia GPU AC total processing power baseline offset");
+LWMI_ATTR_GROUP_TUNABLE_CAP01(gpu_nv_bpl, "gpu_nv_bpl",
+			      "Set the Nvidia GPU base power limit");
+LWMI_ATTR_GROUP_TUNABLE_CAP01(gpu_nv_cpu_boost, "gpu_nv_cpu_boost",
+			      "Set the Nvidia GPU to CPU dynamic boost limit");
+LWMI_ATTR_GROUP_TUNABLE_CAP01(gpu_nv_ctgp, "gpu_nv_ctgp",
+			      "Set the GPU configurable total graphics power");
+LWMI_ATTR_GROUP_TUNABLE_CAP01(gpu_nv_ppab, "gpu_nv_ppab",
+			      "Set the Nvidia GPU power performance aware boost limit");
+LWMI_ATTR_GROUP_TUNABLE_CAP01(gpu_temp, "gpu_temp",
+			      "Set the GPU thermal load limit");
 
 static struct capdata01_attr_group cd01_attr_groups[] = {
 	{ &cpu_temp_attr_group, &cpu_temp },
+	{ &dgpu_boost_clk_attr_group, &dgpu_boost_clk },
+	{ &dgpu_didvid_attr_group, &dgpu_didvid },
+	{ &dgpu_enable_attr_group, &dgpu_enable },
+	{ &gpu_mode_attr_group, &gpu_mode },
+	{ &gpu_nv_ac_offset_attr_group, &gpu_nv_ac_offset },
+	{ &gpu_nv_bpl_attr_group, &gpu_nv_bpl },
+	{ &gpu_nv_cpu_boost_attr_group, &gpu_nv_cpu_boost },
+	{ &gpu_nv_ctgp_attr_group, &gpu_nv_ctgp },
+	{ &gpu_nv_ppab_attr_group, &gpu_nv_ppab },
+	{ &gpu_temp_attr_group, &gpu_temp },
 	{ &ppt_cpu_cl_attr_group, &ppt_cpu_cl },
 	{ &ppt_pl1_apu_spl_attr_group, &ppt_pl1_apu_spl },
 	{ &ppt_pl1_spl_attr_group, &ppt_pl1_spl },

In the next patch a power supply extension is added which requires a name attribute. Instead of creating another const macro with the same information, replace LWMI_OM_FW_ATTR_BASE_PATH with LWMI_OM_NAME and use that for firmware attributes and power supply

extension.

Reviewed-by Mark Pearson <[email protected]>

Signed-off-by Derek J. Clark <[email protected]>

---
 drivers/platform/x86/lenovo/wmi-other.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

drivers/platform/x86/lenovo/wmi-other.c +0 -0

index 67768f6a50e0..7f0d5a17b44f 100644

drivers/platform/x86/lenovo/wmi-other.c +2 -4

-
-#define LWMI_OM_FW_ATTR_BASE_PATH "lenovo-wmi-other"
+#define LWMI_OM_SYSFS_NAME "lenovo-wmi-other"
 #define LWMI_OM_HWMON_NAME "lenovo_wmi_other"
 
 static BLOCKING_NOTIFIER_HEAD(om_chain_head);

@@ -1233,8 +1233,7 @@ static int lwmi_om_fw_attr_add(struct lwmi_om_priv *priv)
 
 	priv->fw_attr_dev = device_create(&firmware_attributes_class, NULL,
 					  MKDEV(0, 0), NULL, "%s-%u",
-					  LWMI_OM_FW_ATTR_BASE_PATH,
-					  priv->ida_id);
+					  LWMI_OM_SYSFS_NAME, priv->ida_id);
 	if (IS_ERR(priv->fw_attr_dev)) {
 		err = PTR_ERR(priv->fw_attr_dev);
 		goto err_free_ida;

Quoted 4 lines · Derek J. Clark

In the next patch a power supply extension is added which requires a name attribute. Instead of creating another const macro with the same information, replace LWMI_OM_FW_ATTR_BASE_PATH with LWMI_OM_NAME and use that for firmware attributes and power supply

This doesn't match the code change??

Quoted 13 lines

extension.

Reviewed-by Mark Pearson <[email protected]>

Signed-off-by Derek J. Clark <[email protected]>

---
 drivers/platform/x86/lenovo/wmi-other.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

drivers/platform/x86/lenovo/wmi-other.c +0 -0

index 67768f6a50e0..7f0d5a17b44f 100644

drivers/platform/x86/lenovo/wmi-other.c enum lwmi_feature_id_gpu { · line 97

Quoted 10 lines

drivers/platform/x86/lenovo/wmi-other.c +2 -1

@@ -90,13 +90,13 @@ enum lwmi_feature_id_gpu {
 #define LWMI_FAN_NR 4
 #define LWMI_FAN_ID(x) ((x) + LWMI_FAN_ID_BASE)
 
+#define LWMI_FAN_DIV 100
+
 #define LWMI_ATTR_ID_FAN_RPM(x)                                   \
 	LWMI_ATTR_ID(LWMI_DEVICE_ID_FAN, LWMI_FEATURE_ID_FAN_RPM, \
 		     LWMI_GZ_THERMAL_MODE_NONE, LWMI_FAN_ID(x))
 
-#define LWMI_FAN_DIV 100

Unrelated change.

Quoted 17 lines

drivers/platform/x86/lenovo/wmi-other.c +2 -4

-
-#define LWMI_OM_FW_ATTR_BASE_PATH "lenovo-wmi-other"
+#define LWMI_OM_SYSFS_NAME "lenovo-wmi-other"
 #define LWMI_OM_HWMON_NAME "lenovo_wmi_other"
 
 static BLOCKING_NOTIFIER_HEAD(om_chain_head);

@@ -1233,8 +1233,7 @@ static int lwmi_om_fw_attr_add(struct lwmi_om_priv *priv)
 
 	priv->fw_attr_dev = device_create(&firmware_attributes_class, NULL,
 					  MKDEV(0, 0), NULL, "%s-%u",
-					  LWMI_OM_FW_ATTR_BASE_PATH,
-					  priv->ida_id);
+					  LWMI_OM_SYSFS_NAME, priv->ida_id);
 	if (IS_ERR(priv->fw_attr_dev)) {
 		err = PTR_ERR(priv->fw_attr_dev);
 		goto err_free_ida;

In the next patch a power supply extension is added which requires a name attribute. Instead of creating another const macro with the same information, replace LWMI_OM_FW_ATTR_BASE_PATH with LWMI_OM_NAME and use that for firmware attributes and power supply extension.

Reviewed-by Mark Pearson <[email protected]>

Signed-off-by Derek J. Clark <[email protected]>

---
 drivers/platform/x86/lenovo/wmi-other.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

drivers/platform/x86/lenovo/wmi-other.c +0 -0

index 67768f6a50e0..7f0d5a17b44f 100644

drivers/platform/x86/lenovo/wmi-other.c +1 -2

-
-#define LWMI_OM_FW_ATTR_BASE_PATH "lenovo-wmi-other"
+#define LWMI_OM_SYSFS_NAME "lenovo-wmi-other"
 #define LWMI_OM_HWMON_NAME "lenovo_wmi_other"
 
 static BLOCKING_NOTIFIER_HEAD(om_chain_head);

Hi Derek,

Quoted 26 lines · Derek J. Clark

In the next patch a power supply extension is added which requires a name attribute. Instead of creating another const macro with the same information, replace LWMI_OM_FW_ATTR_BASE_PATH with LWMI_OM_NAME and use that for firmware attributes and power supply extension.

Reviewed-by Mark Pearson <[email protected]>

Signed-off-by Derek J. Clark <[email protected]>

---
 drivers/platform/x86/lenovo/wmi-other.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

drivers/platform/x86/lenovo/wmi-other.c +2 -0

index 67768f6a50e0..7f0d5a17b44f 100644
@@ -90,13 +90,13 @@ enum lwmi_feature_id_gpu {
 #define LWMI_FAN_NR 4
 #define LWMI_FAN_ID(x) ((x) + LWMI_FAN_ID_BASE)
 
+#define LWMI_FAN_DIV 100
+
 #define LWMI_ATTR_ID_FAN_RPM(x)                                   \
 	LWMI_ATTR_ID(LWMI_DEVICE_ID_FAN, LWMI_FEATURE_ID_FAN_RPM, \
 		     LWMI_GZ_THERMAL_MODE_NONE, LWMI_FAN_ID(x))
 

drivers/platform/x86/lenovo/wmi-other.c enum lwmi_feature_id_gpu { · line 97

Quoted 1 line

drivers/platform/x86/lenovo/wmi-other.c +0 -1

-#define LWMI_FAN_DIV 100

I placed it here intentionally as it has nothing to do with LWMI_ATTR_ID (contrary to other fan-related macros) and is just a descriptive macro.

Quoted 7 lines

drivers/platform/x86/lenovo/wmi-other.c +1 -2

-
-#define LWMI_OM_FW_ATTR_BASE_PATH "lenovo-wmi-other"
+#define LWMI_OM_SYSFS_NAME "lenovo-wmi-other"
 #define LWMI_OM_HWMON_NAME "lenovo_wmi_other"
 
 static BLOCKING_NOTIFIER_HEAD(om_chain_head);

drivers/platform/x86/lenovo/wmi-other.c static int lwmi_om_fw_attr_add(struct lwmi_om_priv *priv) · line 1239

Quoted 9 lines

drivers/platform/x86/lenovo/wmi-other.c +1 -2

@@ -1233,8 +1233,7 @@ static int lwmi_om_fw_attr_add(struct lwmi_om_priv *priv)
 
 	priv->fw_attr_dev = device_create(&firmware_attributes_class, NULL,
 					  MKDEV(0, 0), NULL, "%s-%u",
-					  LWMI_OM_FW_ATTR_BASE_PATH,
-					  priv->ida_id);
+					  LWMI_OM_SYSFS_NAME, priv->ida_id);
 	if (IS_ERR(priv->fw_attr_dev)) {
 		err = PTR_ERR(priv->fw_attr_dev);
 		goto err_free_ida;

Thanks,
Rong

Add charge-type power supply extension for devices that support WMI based charge enable/disable. Lenovo Legion devices that implement function ID and capdata 00 ID 0x03010001 are able to enable or disable charging through the lenovo-wmi-other interface. The ideapad_laptop driver conflicts with this if it can also provide the attribute, so we have to get the acpi_handle and check for the same ACPI methods that enable the feature in that driver. The ACPI method is more reliable from my testing when both are present, so there is no need to modify the ideapad_laptop driver instead.

Reviewed-by Mark Pearson <[email protected]>

Signed-off-by Derek J. Clark <[email protected]>

---
 drivers/platform/x86/lenovo/wmi-capdata.h |   1 +
 drivers/platform/x86/lenovo/wmi-other.c   | 230 ++++++++++++++++++++++
 2 files changed, 231 insertions(+)

drivers/platform/x86/lenovo/wmi-capdata.h +1 -0

index b7f9ee7b301a..00471551e7d6 100644
@@ -26,6 +26,7 @@
 enum lwmi_device_id {
 	LWMI_DEVICE_ID_CPU = 0x01,
 	LWMI_DEVICE_ID_GPU = 0x02,
+	LWMI_DEVICE_ID_PSU = 0x03,
 	LWMI_DEVICE_ID_FAN = 0x04,
 };
 

drivers/platform/x86/lenovo/wmi-other.c +3 -0

index 7f0d5a17b44f..b2daff1b45c2 100644
@@ -42,9 +42,12 @@
 #include <linux/module.h>
 #include <linux/notifier.h>
 #include <linux/platform_profile.h>
+#include <linux/power_supply.h>
 #include <linux/types.h>
 #include <linux/wmi.h>
 
+#include <acpi/battery.h>
+
 #include "wmi-capdata.h"
 #include "wmi-events.h"
 #include "wmi-gamezone.h"

drivers/platform/x86/lenovo/wmi-other.c +102 -0

 #define LWMI_FEATURE_VALUE_GET 17
 #define LWMI_FEATURE_VALUE_SET 18

@@ -92,10 +102,17 @@ enum lwmi_feature_id_gpu {
 
 #define LWMI_FAN_DIV 100
 
+#define LWMI_CHARGE_MODE_ENABLED	0x00
+#define LWMI_CHARGE_MODE_DISABLED	0x01
+
 #define LWMI_ATTR_ID_FAN_RPM(x)                                   \
 	LWMI_ATTR_ID(LWMI_DEVICE_ID_FAN, LWMI_FEATURE_ID_FAN_RPM, \
 		     LWMI_GZ_THERMAL_MODE_NONE, LWMI_FAN_ID(x))
 
+#define LWMI_ATTR_ID_PSU(feat, type)		\
+	LWMI_ATTR_ID(LWMI_DEVICE_ID_PSU, feat,		\
+		     LWMI_GZ_THERMAL_MODE_NONE, type)
+
 #define LWMI_OM_SYSFS_NAME "lenovo-wmi-other"
 #define LWMI_OM_HWMON_NAME "lenovo_wmi_other"
 

@@ -137,6 +154,8 @@ struct lwmi_om_priv {
 		bool capdata00_collected : 1;
 		bool capdata_fan_collected : 1;
 	} fan_flags;
+
+	struct acpi_battery_hook battery_hook;
 };
 
 /*

@@ -561,6 +580,216 @@ static void lwmi_om_fan_info_collect_cd_fan(struct device *dev, struct cd_list *
 	lwmi_om_hwmon_add(priv);
 }
 
+/* ======== Power Supply Extension (component: lenovo-wmi-capdata 00) ======== */
+
+/**
+ * lwmi_psy_ext_get_prop() - Get a power_supply_ext property
+ * @ps: The battery that was extended
+ * @ext: The extension
+ * @ext_data: Pointer the lwmi_om_priv drvdata
+ * @prop: The property to read
+ * @val: The value to return
+ *
+ * Writes the given value to the power_supply_ext property
+ *
+ * Return: 0 on success, or an error
+ */
+static int lwmi_psy_ext_get_prop(struct power_supply *ps,
+				 const struct power_supply_ext *ext,
+				 void *ext_data,
+				 enum power_supply_property prop,
+				 union power_supply_propval *val)
+{
+	struct lwmi_om_priv *priv = ext_data;
+	struct wmi_method_args_32 args;
+	u32 retval;
+	int ret;
+
+	args.arg0 = LWMI_ATTR_ID_PSU(LWMI_FEATURE_ID_PSU_INSTANT_MODE, LWMI_TYPE_ID_PSU_AC);
+
+	ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_GET,
+				    (unsigned char *)&args, sizeof(args),
+				    &retval);
+	if (ret)
+		return ret;
+
+	dev_dbg(&priv->wdev->dev, "Got return value %x for property %x\n", retval, prop);
+
+	if (retval == LWMI_CHARGE_MODE_DISABLED)
+		val->intval = POWER_SUPPLY_CHARGE_TYPE_LONGLIFE;
+	else
+		val->intval = POWER_SUPPLY_CHARGE_TYPE_STANDARD;
+
+	return 0;
+}
+
+/**
+ * lwmi_psy_ext_set_prop() - Set a power_supply_ext property
+ * @ps: The battery that was extended
+ * @ext: The extension
+ * @ext_data: Pointer the lwmi_om_priv drvdata
+ * @prop: The property to write
+ * @val: The value to write
+ *
+ * Writes the given value to the power_supply_ext property
+ *
+ * Return: 0 on success, or an error
+ */
+static int lwmi_psy_ext_set_prop(struct power_supply *ps,
+				 const struct power_supply_ext *ext,
+				 void *ext_data,
+				 enum power_supply_property prop,
+				 const union power_supply_propval *val)
+{
+	struct lwmi_om_priv *priv = ext_data;
+	struct wmi_method_args_32 args;
+
+	args.arg0 = LWMI_ATTR_ID_PSU(LWMI_FEATURE_ID_PSU_INSTANT_MODE, LWMI_TYPE_ID_PSU_AC);
+	if (val->intval == POWER_SUPPLY_CHARGE_TYPE_LONGLIFE)
+		args.arg1 = LWMI_CHARGE_MODE_DISABLED;
+	else
+		args.arg1 = LWMI_CHARGE_MODE_ENABLED;
+
+	dev_dbg(&priv->wdev->dev, "Attempting to set %#08x for property %x to %x\n",
+		args.arg0, prop, args.arg1);
+
+	return lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_SET,
+				     (unsigned char *)&args, sizeof(args), NULL);
+}
+
+/**
+ * lwmi_psy_prop_is_writeable() - Determine if the property is supported
+ * @ps: The battery that was extended
+ * @ext: The extension
+ * @ext_data: Pointer the lwmi_om_priv drvdata
+ * @prop: The property to check
+ *
+ * Checks capdata 00 to determine if the property is supported.
+ *
+ * Return: Support level, or false
+ */
+static int lwmi_psy_prop_is_writeable(struct power_supply *ps,
+				      const struct power_supply_ext *ext,
+				      void *ext_data,
+				      enum power_supply_property prop)
+{

drivers/platform/x86/lenovo/wmi-other.c +56 -0

+		attribute_id, capdata.supported);
+
+	return capdata.supported;
+}
+
+static const enum power_supply_property lwmi_psy_ext_props[] = {
+	POWER_SUPPLY_PROP_CHARGE_TYPES,
+};
+
+static const struct power_supply_ext lwmi_psy_ext = {
+	.name			= LWMI_OM_SYSFS_NAME,
+	.properties		= lwmi_psy_ext_props,
+	.num_properties		= ARRAY_SIZE(lwmi_psy_ext_props),
+	.charge_types		= (BIT(POWER_SUPPLY_CHARGE_TYPE_STANDARD) |
+				   BIT(POWER_SUPPLY_CHARGE_TYPE_LONGLIFE)),
+	.get_property		= lwmi_psy_ext_get_prop,
+	.set_property		= lwmi_psy_ext_set_prop,
+	.property_is_writeable	= lwmi_psy_prop_is_writeable,
+};
+
+/**
+ * lwmi_add_battery() - Connect the power_supply_ext
+ * @battery: The battery to extend
+ * @hook: The driver hook used to extend the battery
+ *
+ * Return: 0 on success, or an error.
+ */
+static int lwmi_add_battery(struct power_supply *battery, struct acpi_battery_hook *hook)
+{
+	struct lwmi_om_priv *priv = container_of(hook, struct lwmi_om_priv, battery_hook);
+
+	return power_supply_register_extension(battery, &lwmi_psy_ext, &priv->wdev->dev, priv);
+}
+
+/**
+ * lwmi_remove_battery() - Disconnect the power_supply_ext
+ * @battery: The battery that was extended
+ * @hook: The driver hook used to extend the battery
+ *
+ * Return: 0 on success, or an error.
+ */
+static int lwmi_remove_battery(struct power_supply *battery, struct acpi_battery_hook *hook)
+{
+	power_supply_unregister_extension(battery, &lwmi_psy_ext);
+	return 0;
+}
+
+/**
+ * lwmi_acpi_match() - Attempts to return the ideapad acpi handle
+ * @handle: The ACPI handle that manages battery charging
+ * @lvl: Unused
+ * @context: Void pointer to the acpi_handle object to return
+ * @retval: Unused
+ *
+ * Checks if the ideapad_laptop driver is going to manage charge_type first,
+ * then if not, hooks the battery to our WMI methods.

drivers/platform/x86/lenovo/wmi-other.c +25 -0

+	*ahand = handle;
+
+	return AE_CTRL_TERMINATE;
+}
+
+/**
+ * lwmi_om_ps_ext_init() - Hooks power supply extension to device battery
+ * @priv: Driver private data
+ *
+ * Checks if the ideapad_laptop driver is going to manage charge_type first,
+ * then if not, hooks the battery to our WMI methods.
+ */
+static void lwmi_om_ps_ext_init(struct lwmi_om_priv *priv)
+{
+	static const char * const ideapad_hid = "VPC2004";
+	acpi_handle handle = NULL;
+	int ret;
+
+	/* Deconflict ideapad_laptop driver */
+	ret = acpi_get_devices(ideapad_hid, lwmi_acpi_match, &handle, NULL);
+	if (ret)
+		return;
+
+	if (!handle)
+		return;

drivers/platform/x86/lenovo/wmi-other.c +7 -0

+
+	ret = devm_battery_hook_register(&priv->wdev->dev, &priv->battery_hook);
+	if (ret)
+		dev_err(&priv->wdev->dev, "Error during battery hook: %i\n", ret);
+}
+
 /* ======== fw_attributes (component: lenovo-wmi-capdata 01) ======== */
 
 struct tunable_attr_01 {

@@ -1325,6 +1554,7 @@ static int lwmi_om_master_bind(struct device *dev)
 		return -ENODEV;
 
 	lwmi_om_fan_info_collect_cd00(priv);
+	lwmi_om_ps_ext_init(priv);
 
 	return lwmi_om_fw_attr_add(priv);
 }

Quoted 47 lines · Derek J. Clark

Add charge-type power supply extension for devices that support WMI based charge enable/disable. Lenovo Legion devices that implement function ID and capdata 00 ID 0x03010001 are able to enable or disable charging through the lenovo-wmi-other interface. The ideapad_laptop driver conflicts with this if it can also provide the attribute, so we have to get the acpi_handle and check for the same ACPI methods that enable the feature in that driver. The ACPI method is more reliable from my testing when both are present, so there is no need to modify the ideapad_laptop driver instead.

Reviewed-by Mark Pearson <[email protected]>

Signed-off-by Derek J. Clark <[email protected]>

---
 drivers/platform/x86/lenovo/wmi-capdata.h |   1 +
 drivers/platform/x86/lenovo/wmi-other.c   | 230 ++++++++++++++++++++++
 2 files changed, 231 insertions(+)

drivers/platform/x86/lenovo/wmi-capdata.h +1 -0

index b7f9ee7b301a..00471551e7d6 100644
@@ -26,6 +26,7 @@
 enum lwmi_device_id {
 	LWMI_DEVICE_ID_CPU = 0x01,
 	LWMI_DEVICE_ID_GPU = 0x02,
+	LWMI_DEVICE_ID_PSU = 0x03,
 	LWMI_DEVICE_ID_FAN = 0x04,
 };
 

drivers/platform/x86/lenovo/wmi-other.c +3 -0

index 7f0d5a17b44f..b2daff1b45c2 100644
@@ -42,9 +42,12 @@
 #include <linux/module.h>
 #include <linux/notifier.h>
 #include <linux/platform_profile.h>
+#include <linux/power_supply.h>
 #include <linux/types.h>
 #include <linux/wmi.h>
 
+#include <acpi/battery.h>
+
 #include "wmi-capdata.h"
 #include "wmi-events.h"
 #include "wmi-gamezone.h"

drivers/platform/x86/lenovo/wmi-other.c enum lwmi_feature_id_gpu { · line 86

Quoted 6 lines

drivers/platform/x86/lenovo/wmi-other.c +3 -0

@@ -78,10 +81,17 @@ enum lwmi_feature_id_gpu {
 	LWMI_FEATURE_ID_GPU_NV_CPU_BOOST = 0x0b,
 };
 
+enum lwmi_feature_id_psu {
+	LWMI_FEATURE_ID_PSU_INSTANT_MODE = 0x01,
+	LWMI_FEATURE_ID_PSU_CHARGE_MODE = 0x02,

Align.

Quoted 4 lines

drivers/platform/x86/lenovo/wmi-other.c +2 -0

+};
+
 #define LWMI_FEATURE_ID_FAN_RPM 0x03
 

drivers/platform/x86/lenovo/wmi-other.c enum lwmi_feature_id_gpu { · line 94

Quoted 4 lines

drivers/platform/x86/lenovo/wmi-other.c +2 -0

 #define LWMI_TYPE_ID_NONE 0x00
 #define LWMI_TYPE_ID_CROSSLOAD 0x01
+#define LWMI_TYPE_ID_PSU_AC 0x01
+#define LWMI_TYPE_ID_PSU_PD 0x02

These should be aligned as well but that will impact more than this patch to avoid back and forth changes within the series.

Quoted 133 lines

drivers/platform/x86/lenovo/wmi-other.c +108 -0

 #define LWMI_FEATURE_VALUE_GET 17
 #define LWMI_FEATURE_VALUE_SET 18

@@ -92,10 +102,17 @@ enum lwmi_feature_id_gpu {
 
 #define LWMI_FAN_DIV 100
 
+#define LWMI_CHARGE_MODE_ENABLED	0x00
+#define LWMI_CHARGE_MODE_DISABLED	0x01
+
 #define LWMI_ATTR_ID_FAN_RPM(x)                                   \
 	LWMI_ATTR_ID(LWMI_DEVICE_ID_FAN, LWMI_FEATURE_ID_FAN_RPM, \
 		     LWMI_GZ_THERMAL_MODE_NONE, LWMI_FAN_ID(x))
 
+#define LWMI_ATTR_ID_PSU(feat, type)		\
+	LWMI_ATTR_ID(LWMI_DEVICE_ID_PSU, feat,		\
+		     LWMI_GZ_THERMAL_MODE_NONE, type)
+
 #define LWMI_OM_SYSFS_NAME "lenovo-wmi-other"
 #define LWMI_OM_HWMON_NAME "lenovo_wmi_other"
 

@@ -137,6 +154,8 @@ struct lwmi_om_priv {
 		bool capdata00_collected : 1;
 		bool capdata_fan_collected : 1;
 	} fan_flags;
+
+	struct acpi_battery_hook battery_hook;
 };
 
 /*

@@ -561,6 +580,216 @@ static void lwmi_om_fan_info_collect_cd_fan(struct device *dev, struct cd_list *
 	lwmi_om_hwmon_add(priv);
 }
 
+/* ======== Power Supply Extension (component: lenovo-wmi-capdata 00) ======== */
+
+/**
+ * lwmi_psy_ext_get_prop() - Get a power_supply_ext property
+ * @ps: The battery that was extended
+ * @ext: The extension
+ * @ext_data: Pointer the lwmi_om_priv drvdata
+ * @prop: The property to read
+ * @val: The value to return
+ *
+ * Writes the given value to the power_supply_ext property
+ *
+ * Return: 0 on success, or an error
+ */
+static int lwmi_psy_ext_get_prop(struct power_supply *ps,
+				 const struct power_supply_ext *ext,
+				 void *ext_data,
+				 enum power_supply_property prop,
+				 union power_supply_propval *val)
+{
+	struct lwmi_om_priv *priv = ext_data;
+	struct wmi_method_args_32 args;
+	u32 retval;
+	int ret;
+
+	args.arg0 = LWMI_ATTR_ID_PSU(LWMI_FEATURE_ID_PSU_INSTANT_MODE, LWMI_TYPE_ID_PSU_AC);
+
+	ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_GET,
+				    (unsigned char *)&args, sizeof(args),
+				    &retval);
+	if (ret)
+		return ret;
+
+	dev_dbg(&priv->wdev->dev, "Got return value %x for property %x\n", retval, prop);
+
+	if (retval == LWMI_CHARGE_MODE_DISABLED)
+		val->intval = POWER_SUPPLY_CHARGE_TYPE_LONGLIFE;
+	else
+		val->intval = POWER_SUPPLY_CHARGE_TYPE_STANDARD;
+
+	return 0;
+}
+
+/**
+ * lwmi_psy_ext_set_prop() - Set a power_supply_ext property
+ * @ps: The battery that was extended
+ * @ext: The extension
+ * @ext_data: Pointer the lwmi_om_priv drvdata
+ * @prop: The property to write
+ * @val: The value to write
+ *
+ * Writes the given value to the power_supply_ext property
+ *
+ * Return: 0 on success, or an error
+ */
+static int lwmi_psy_ext_set_prop(struct power_supply *ps,
+				 const struct power_supply_ext *ext,
+				 void *ext_data,
+				 enum power_supply_property prop,
+				 const union power_supply_propval *val)
+{
+	struct lwmi_om_priv *priv = ext_data;
+	struct wmi_method_args_32 args;
+
+	args.arg0 = LWMI_ATTR_ID_PSU(LWMI_FEATURE_ID_PSU_INSTANT_MODE, LWMI_TYPE_ID_PSU_AC);
+	if (val->intval == POWER_SUPPLY_CHARGE_TYPE_LONGLIFE)
+		args.arg1 = LWMI_CHARGE_MODE_DISABLED;
+	else
+		args.arg1 = LWMI_CHARGE_MODE_ENABLED;
+
+	dev_dbg(&priv->wdev->dev, "Attempting to set %#08x for property %x to %x\n",
+		args.arg0, prop, args.arg1);
+
+	return lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_SET,
+				     (unsigned char *)&args, sizeof(args), NULL);
+}
+
+/**
+ * lwmi_psy_prop_is_writeable() - Determine if the property is supported
+ * @ps: The battery that was extended
+ * @ext: The extension
+ * @ext_data: Pointer the lwmi_om_priv drvdata
+ * @prop: The property to check
+ *
+ * Checks capdata 00 to determine if the property is supported.
+ *
+ * Return: Support level, or false
+ */
+static int lwmi_psy_prop_is_writeable(struct power_supply *ps,
+				      const struct power_supply_ext *ext,
+				      void *ext_data,
+				      enum power_supply_property prop)
+{
+	struct lwmi_om_priv *priv = ext_data;
+	struct capdata00 capdata;
+	u32 attribute_id = LWMI_ATTR_ID_PSU(LWMI_FEATURE_ID_PSU_INSTANT_MODE, LWMI_TYPE_ID_PSU_AC);
+	int ret;
+
+	ret = lwmi_cd00_get_data(priv->cd00_list, attribute_id, &capdata);