[PATCH 0/3] AMD_SFH: Allow overriding sensor-mask by module-param or DMI-quirk

Hi All,

There are several bug-reports about the new AMD_SFH driver not working
on various HP ENVY x360 Convertible models. The problem is that the
driver expects the BIOS to program which sensors are present into the
active bits of the AMD_P2C_MSG3 register; and the BIOS on these models
does not do this:

https://bugzilla.kernel.org/show_bug.cgi?id=199715
https://bugzilla.redhat.com/show_bug.cgi?id=1651886

This patch-set adds a module-parameter + DMI-quirk mechanism to override
the settings read back from the AMD_P2C_MSG3 register, to work around
this problem. The DMI-quirk table is populated with 2 HP ENVY x360
Convertible models which are know to need this workaround.

There also is a much larger refactoring patch-set pending from
Richard Neumann, who is also involved in the bugzilla.kernel.org bug.

But it looks to me like that will need a bit more more work before
it is ready for merging, where as (IMHO) this set is ready for
merging now. So IMHO it would be good to first merge this patch-set
to get this fix into the hands of end-users of these devices.

Note there still is an open issue on these devices where the
sensors stop working after a suspend/resume cycle.

I wonder if the driver should perhaps not only not use the
active bits of the AMD_P2C_MSG3 register for determining which
sensors are there, but if it should actually write to those bots
with the correct settings.

Sandeep, do you have any ideas what might be the problem here?
Should I ask the reporters to test a patch which actually
updates the active bits?

Regards,

Hans



Hans de Goede (3):
  AMD_SFH: Removed unused activecontrolstatus member from the
    amd_mp2_dev struct
  AMD_SFH: Add sensor_mask module parameter
  AMD_SFH: Add DMI quirk table for BIOS-es which don't set the
    activestatus bits

 drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 40 ++++++++++++++++++++++++--
 drivers/hid/amd-sfh-hid/amd_sfh_pcie.h |  1 -
 2 files changed, 37 insertions(+), 4 deletions(-)

-- 
2.29.2

Some BIOS-es do not initialize the activestatus bits of the AMD_P2C_MSG3
register. This cause the AMD_SFH driver to not register any sensors even
though the laptops in question do have sensors.

Add a DMI quirk-table for specifying sensor-mask overrides based on
DMI match, to make the sensors work OOTB on these laptop models.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199715
BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1651886
Fixes: 4f567b9f8141 ("SFH: PCIe driver to add support of AMD sensor fusion hub")
Signed-off-by: Hans de Goede <redacted>
---
 drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
index ab0a9443e252..ddecc84fd6f0 100644
--- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
+++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c

@@ -10,6 +10,7 @@
 #include <linux/bitops.h>
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
+#include <linux/dmi.h>
 #include <linux/interrupt.h>
 #include <linux/io-64-nonatomic-lo-hi.h>
 #include <linux/module.h>

@@ -77,11 +78,34 @@ void amd_stop_all_sensors(struct amd_mp2_dev *privdata)
 	writel(cmd_base.ul, privdata->mmio + AMD_C2P_MSG0);
 }
 
+static const struct dmi_system_id dmi_sensor_mask_overrides[] = {
+	{
+		.matches = {
+			DMI_MATCH(DMI_PRODUCT_NAME, "HP ENVY x360 Convertible 13-ag0xxx"),
+		},
+		.driver_data = (void *)(ACEL_EN | MAGNO_EN),
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_PRODUCT_NAME, "HP ENVY x360 Convertible 15-cp0xxx"),
+		},
+		.driver_data = (void *)(ACEL_EN | MAGNO_EN),
+	},
+	{ }
+};
+
 int amd_mp2_get_sensor_num(struct amd_mp2_dev *privdata, u8 *sensor_id)
 {
 	int activestatus, num_of_sensors = 0;
+	const struct dmi_system_id *dmi_id;
 	u32 activecontrolstatus;
 
+	if (sensor_mask_override == -1) {
+		dmi_id = dmi_first_match(dmi_sensor_mask_overrides);
+		if (dmi_id)
+			sensor_mask_override = (long)dmi_id->driver_data;
+	}
+
 	if (sensor_mask_override >= 0) {
 		activestatus = sensor_mask_override;
 	} else {

-- 
2.29.2

Add a sensor_mask module parameter which can be used to override the
sensor-mask read from the activestatus bits of the AMD_P2C_MSG3
registers. Some BIOS-es do not program the activestatus bits, leading
to the AMD-SFH driver not registering any HID devices even though the
laptop in question does actually have sensors.

While at it also fix the wrong indentation of the MAGNO_EN define.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199715
BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1651886
Fixes: 4f567b9f8141 ("SFH: PCIe driver to add support of AMD sensor fusion hub")
Suggested-by: Richard Neumann <redacted>
Signed-off-by: Hans de Goede <redacted>
---
 drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
index f3cdb4ea33da..ab0a9443e252 100644
--- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
+++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c

@@ -22,9 +22,13 @@
 
 #define ACEL_EN		BIT(0)
 #define GYRO_EN		BIT(1)
-#define MAGNO_EN		BIT(2)
+#define MAGNO_EN	BIT(2)
 #define ALS_EN		BIT(19)
 
+static int sensor_mask_override = -1;
+module_param_named(sensor_mask, sensor_mask_override, int, 0444);
+MODULE_PARM_DESC(sensor_mask, "override the detected sensors mask");
+
 void amd_start_sensor(struct amd_mp2_dev *privdata, struct amd_mp2_sensor_info info)
 {
 	union sfh_cmd_param cmd_param;

@@ -78,8 +82,12 @@ int amd_mp2_get_sensor_num(struct amd_mp2_dev *privdata, u8 *sensor_id)
 	int activestatus, num_of_sensors = 0;
 	u32 activecontrolstatus;
 
-	activecontrolstatus = readl(privdata->mmio + AMD_P2C_MSG3);
-	activestatus = activecontrolstatus >> 4;
+	if (sensor_mask_override >= 0) {
+		activestatus = sensor_mask_override;
+	} else {
+		activecontrolstatus = readl(privdata->mmio + AMD_P2C_MSG3);
+		activestatus = activecontrolstatus >> 4;
+	}
 
 	if (ACEL_EN  & activestatus)
 		sensor_id[num_of_sensors++] = accel_idx;

-- 
2.29.2

This value is only used once inside amd_mp2_get_sensor_num(),
so there is no need to store this in the amd_mp2_dev struct,
amd_mp2_get_sensor_num() can simple use a local variable for this.

Fixes: 4f567b9f8141 ("SFH: PCIe driver to add support of AMD sensor fusion hub")
Signed-off-by: Hans de Goede <redacted>
---
 drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 6 ++++--
 drivers/hid/amd-sfh-hid/amd_sfh_pcie.h | 1 -
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
index dbac16641662..f3cdb4ea33da 100644
--- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
+++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c

@@ -76,9 +76,11 @@ void amd_stop_all_sensors(struct amd_mp2_dev *privdata)
 int amd_mp2_get_sensor_num(struct amd_mp2_dev *privdata, u8 *sensor_id)
 {
 	int activestatus, num_of_sensors = 0;
+	u32 activecontrolstatus;
+
+	activecontrolstatus = readl(privdata->mmio + AMD_P2C_MSG3);
+	activestatus = activecontrolstatus >> 4;
 
-	privdata->activecontrolstatus = readl(privdata->mmio + AMD_P2C_MSG3);
-	activestatus = privdata->activecontrolstatus >> 4;
 	if (ACEL_EN  & activestatus)
 		sensor_id[num_of_sensors++] = accel_idx;
 

diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.h b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.h
index 8f8d19b2cfe5..489415f7c22c 100644
--- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.h
+++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.h

@@ -61,7 +61,6 @@ struct amd_mp2_dev {
 	struct pci_dev *pdev;
 	struct amdtp_cl_data *cl_data;
 	void __iomem *mmio;
-	u32 activecontrolstatus;
 };
 
 struct amd_mp2_sensor_info {

-- 
2.29.2

Hi Hans,

sorry, I missed to include you in the recipients of the patch I
submitted yesterday: 
https://patchwork.kernel.org/project/linux-input/list/?series=423055

I was focused on the recipient returned by the get_maintainer.pl
script.
My patch, however, does the DMI matching a little differently and does
not utilize the driver_data field of the DMI IDs.
Also I did not include a patch to add a module parameter.

I have too few knowledge which one is better. I don't think that we
need the module parameter right now, if we have the DMI quirks, but
I'll leave this decision to the experts.

It is true, that my fully refactored patch will need more review, and I
changed some things already since I submitted it. But I will wait for
some comments before I consider submitting it again.

Kind regards,

Richard

Am Donnerstag, dem 28.01.2021 um 13:12 +0100 schrieb Hans de Goede:

Hi All,

There are several bug-reports about the new AMD_SFH driver not
working
on various HP ENVY x360 Convertible models. The problem is that the
driver expects the BIOS to program which sensors are present into the
active bits of the AMD_P2C_MSG3 register; and the BIOS on these
models
does not do this:

https://bugzilla.kernel.org/show_bug.cgi?id=199715
https://bugzilla.redhat.com/show_bug.cgi?id=1651886

This patch-set adds a module-parameter + DMI-quirk mechanism to
override
the settings read back from the AMD_P2C_MSG3 register, to work around
this problem. The DMI-quirk table is populated with 2 HP ENVY x360
Convertible models which are know to need this workaround.

There also is a much larger refactoring patch-set pending from
Richard Neumann, who is also involved in the bugzilla.kernel.org bug.

But it looks to me like that will need a bit more more work before
it is ready for merging, where as (IMHO) this set is ready for
merging now. So IMHO it would be good to first merge this patch-set
to get this fix into the hands of end-users of these devices.

Note there still is an open issue on these devices where the
sensors stop working after a suspend/resume cycle.

I wonder if the driver should perhaps not only not use the
active bits of the AMD_P2C_MSG3 register for determining which
sensors are there, but if it should actually write to those bots
with the correct settings.

Sandeep, do you have any ideas what might be the problem here?
Should I ask the reporters to test a patch which actually
updates the active bits?

Regards,

Hans



Hans de Goede (3):
  AMD_SFH: Removed unused activecontrolstatus member from the
    amd_mp2_dev struct
  AMD_SFH: Add sensor_mask module parameter
  AMD_SFH: Add DMI quirk table for BIOS-es which don't set the
    activestatus bits

 drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 40
++++++++++++++++++++++++--
 drivers/hid/amd-sfh-hid/amd_sfh_pcie.h |  1 -
 2 files changed, 37 insertions(+), 4 deletions(-)

Hi Hans,
On 1/28/2021 5:42 PM, Hans de Goede wrote:

Can you please confirm that HP Envy x360  is whether ryzen 4000 (Renior based) series or ryzen 3000 (Raven based) series? As of now current upstream code does not have support for Ryzen 4000 series
for which work is in progress. However, for Ryzen 3000 based series this patch looks fine and thanks for the contribution.


Regards

Nehal

My 13" model is a Raven-Ridge device:

$ grep -i model /proc/cpuinfo | head -2
model		: 17
model name	: AMD Ryzen 5 2500U with Radeon Vega Mobile Gfx

Since the accelerometer is also reported working on the 15" device, I'd
assume that the corresponding DMI record is also fine.

However, I don't know whether there are newer models on the market with
the same product names, which is why in my patch I used the board's
serial number for DMI matching instead.
This, however, seems unlikely since the series "13-ag0xxx" is included
in the product name, so we should be fine here.

Am Montag, dem 15.02.2021 um 13:54 +0530 schrieb Shah, Nehal-
bakulchandra:

Hi Hans,
On 1/28/2021 5:42 PM, Hans de Goede wrote:

quoted

Some BIOS-es do not initialize the activestatus bits of the
AMD_P2C_MSG3
register. This cause the AMD_SFH driver to not register any sensors
even
though the laptops in question do have sensors.

Add a DMI quirk-table for specifying sensor-mask overrides based on
DMI match, to make the sensors work OOTB on these laptop models.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199715
BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1651886
Fixes: 4f567b9f8141 ("SFH: PCIe driver to add support of AMD sensor
fusion hub")
Signed-off-by: Hans de Goede <redacted>
---
 drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c

b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
index ab0a9443e252..ddecc84fd6f0 100644

--- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
+++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c

@@ -10,6 +10,7 @@

 #include <linux/bitops.h>
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
+#include <linux/dmi.h>
 #include <linux/interrupt.h>
 #include <linux/io-64-nonatomic-lo-hi.h>
 #include <linux/module.h>

@@ -77,11 +78,34 @@ void amd_stop_all_sensors(struct amd_mp2_dev

*privdata)
        writel(cmd_base.ul, privdata->mmio + AMD_C2P_MSG0);
 }
 
+static const struct dmi_system_id dmi_sensor_mask_overrides[] = {
+       {
+               .matches = {
+                       DMI_MATCH(DMI_PRODUCT_NAME, "HP ENVY x360
Convertible 13-ag0xxx"),
+               },
+               .driver_data = (void *)(ACEL_EN | MAGNO_EN),
+       },
+       {
+               .matches = {
+                       DMI_MATCH(DMI_PRODUCT_NAME, "HP ENVY x360
Convertible 15-cp0xxx"),
+               },
+               .driver_data = (void *)(ACEL_EN | MAGNO_EN),
+       },
+       { }
+};
+
 int amd_mp2_get_sensor_num(struct amd_mp2_dev *privdata, u8
*sensor_id)
 {
        int activestatus, num_of_sensors = 0;
+       const struct dmi_system_id *dmi_id;
        u32 activecontrolstatus;
 
+       if (sensor_mask_override == -1) {
+               dmi_id = dmi_first_match(dmi_sensor_mask_overrides);
+               if (dmi_id)
+                       sensor_mask_override = (long)dmi_id-

quoted

driver_data;

+       }
+
        if (sensor_mask_override >= 0) {
                activestatus = sensor_mask_override;
        } else {

Can you please confirm that HP Envy x360  is whether ryzen 4000 (Renior
based) series or ryzen 3000 (Raven based) series? As of now current
upstream code does not have support for Ryzen 4000 series
for which work is in progress. However, for Ryzen 3000 based series
this patch looks fine and thanks for the contribution.


Regards

Nehal

Hi,

On 2/15/21 9:24 AM, Shah, Nehal-bakulchandra wrote:

Hi Hans,
On 1/28/2021 5:42 PM, Hans de Goede wrote:

quoted

Some BIOS-es do not initialize the activestatus bits of the AMD_P2C_MSG3
register. This cause the AMD_SFH driver to not register any sensors even
though the laptops in question do have sensors.

Add a DMI quirk-table for specifying sensor-mask overrides based on
DMI match, to make the sensors work OOTB on these laptop models.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199715
BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1651886
Fixes: 4f567b9f8141 ("SFH: PCIe driver to add support of AMD sensor fusion hub")
Signed-off-by: Hans de Goede <redacted>
---
 drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
index ab0a9443e252..ddecc84fd6f0 100644
--- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
+++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c

@@ -10,6 +10,7 @@
 #include <linux/bitops.h>
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
+#include <linux/dmi.h>
 #include <linux/interrupt.h>
 #include <linux/io-64-nonatomic-lo-hi.h>
 #include <linux/module.h>

@@ -77,11 +78,34 @@ void amd_stop_all_sensors(struct amd_mp2_dev *privdata)
 	writel(cmd_base.ul, privdata->mmio + AMD_C2P_MSG0);
 }
 
+static const struct dmi_system_id dmi_sensor_mask_overrides[] = {
+	{
+		.matches = {
+			DMI_MATCH(DMI_PRODUCT_NAME, "HP ENVY x360 Convertible 13-ag0xxx"),
+		},
+		.driver_data = (void *)(ACEL_EN | MAGNO_EN),
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_PRODUCT_NAME, "HP ENVY x360 Convertible 15-cp0xxx"),
+		},
+		.driver_data = (void *)(ACEL_EN | MAGNO_EN),
+	},
+	{ }
+};
+
 int amd_mp2_get_sensor_num(struct amd_mp2_dev *privdata, u8 *sensor_id)
 {
 	int activestatus, num_of_sensors = 0;
+	const struct dmi_system_id *dmi_id;
 	u32 activecontrolstatus;
 
+	if (sensor_mask_override == -1) {
+		dmi_id = dmi_first_match(dmi_sensor_mask_overrides);
+		if (dmi_id)
+			sensor_mask_override = (long)dmi_id->driver_data;
+	}
+
 	if (sensor_mask_override >= 0) {
 		activestatus = sensor_mask_override;
 	} else {

Can you please confirm that HP Envy x360  is whether ryzen 4000 (Renior based) series or ryzen 3000 (Raven based) series? As of now current upstream code does not have support for Ryzen 4000 series
for which work is in progress. However, for Ryzen 3000 based series this patch looks fine and thanks for the contribution.

Both models added to the dmi_sensor_mask_overrides table here use Raven-Ridge SoCs,
they ship with the following CPU options:

Ryzen 3 2300U
Ryzen 5 2500U
Ryzen 7 2700U

So I think we should be good to go wrt merging this set.

Since this set is basically a bugfix set it would be nice if we can get
this merged into one of the 5.12-rc#s.

Regards,

Hans

On Thu, 28 Jan 2021, Hans de Goede wrote:

Hi All,

There are several bug-reports about the new AMD_SFH driver not working
on various HP ENVY x360 Convertible models. The problem is that the
driver expects the BIOS to program which sensors are present into the
active bits of the AMD_P2C_MSG3 register; and the BIOS on these models
does not do this:

https://bugzilla.kernel.org/show_bug.cgi?id=199715
https://bugzilla.redhat.com/show_bug.cgi?id=1651886

This patch-set adds a module-parameter + DMI-quirk mechanism to override
the settings read back from the AMD_P2C_MSG3 register, to work around
this problem. The DMI-quirk table is populated with 2 HP ENVY x360
Convertible models which are know to need this workaround.

Applied, thanks.

-- 
Jiri Kosina
SUSE Labs