Re: [PATCH] platform/x86: msi-wmi-platform: Workaround a ACPI firmware bug

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 6750 bytes --]

On Sat, 12 Apr 2025, Armin Wolf wrote:

> Am 11.04.25 um 15:53 schrieb Ilpo Järvinen:
> 
> > On Thu, 10 Apr 2025, Armin Wolf wrote:
> > 
> > > The ACPI byte code inside the ACPI control method responsible for
> > > handling the WMI method calls uses a global buffer for constructing
> > > the return value, yet the ACPI control method itself is not marked
> > > as "Serialized".
> > > This means that calling WMI methods on this WMI device is not
> > > thread-safe, as concurrent WMI method calls will corrupt the global
> > > buffer.
> > Please avoid non-full lines in middle of a paragraph. Either make things
> > truly own paragraphs or reflow the text in the paragraph.
> > 
> > > Fix this by serializing the WMI method calls using a mutex.
> > > 
> > > Fixes: 9c0beb6b29e7 ("platform/x86: wmi: Add MSI WMI Platform driver")
> > > Tested-by: Antheas Kapenekakis <lkml@antheas.dev>
> > > Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> > > ---
> > >   .../wmi/devices/msi-wmi-platform.rst          |  4 +
> > >   drivers/platform/x86/msi-wmi-platform.c       | 99 ++++++++++++-------
> > >   2 files changed, 67 insertions(+), 36 deletions(-)
> > > 
> > > diff --git a/Documentation/wmi/devices/msi-wmi-platform.rst
> > > b/Documentation/wmi/devices/msi-wmi-platform.rst
> > > index 31a136942892..73197b31926a 100644
> > > --- a/Documentation/wmi/devices/msi-wmi-platform.rst
> > > +++ b/Documentation/wmi/devices/msi-wmi-platform.rst
> > > @@ -138,6 +138,10 @@ input data, the meaning of which depends on the
> > > subfeature being accessed.
> > >   The output buffer contains a single byte which signals success or
> > > failure (``0x00`` on failure)
> > >   and 31 bytes of output data, the meaning if which depends on the
> > > subfeature being accessed.
> > > 
> > > +.. note::
> > > +   The ACPI control method responsible for handling the WMI method calls
> > > is not thread-safe.
> > > +   This is a firmware bug that needs to be handled inside the driver
> > > itself.
> > > +
> > >   WMI method Get_EC()
> > >   -------------------
> > > 
> > > diff --git a/drivers/platform/x86/msi-wmi-platform.c
> > > b/drivers/platform/x86/msi-wmi-platform.c
> > > index 9b5c7f8c79b0..dc5e9878cb68 100644
> > > --- a/drivers/platform/x86/msi-wmi-platform.c
> > > +++ b/drivers/platform/x86/msi-wmi-platform.c
> > > @@ -10,6 +10,7 @@
> > >   #include <linux/acpi.h>
> > >   #include <linux/bits.h>
> > >   #include <linux/bitfield.h>
> > > +#include <linux/cleanup.h>
> > >   #include <linux/debugfs.h>
> > >   #include <linux/device.h>
> > >   #include <linux/device/driver.h>
> > > @@ -17,6 +18,7 @@
> > >   #include <linux/hwmon.h>
> > >   #include <linux/kernel.h>
> > >   #include <linux/module.h>
> > > +#include <linux/mutex.h>
> > >   #include <linux/printk.h>
> > >   #include <linux/rwsem.h>
> > >   #include <linux/types.h>
> > > @@ -76,8 +78,13 @@ enum msi_wmi_platform_method {
> > >   	MSI_PLATFORM_GET_WMI		= 0x1d,
> > >   };
> > > 
> > > -struct msi_wmi_platform_debugfs_data {
> > > +struct msi_wmi_platform_data {
> > >   	struct wmi_device *wdev;
> > > +	struct mutex wmi_lock;	/* Necessary when calling WMI methods */
> > > +};
> > > +
> > > +struct msi_wmi_platform_debugfs_data {
> > > +	struct msi_wmi_platform_data *data;
> > >   	enum msi_wmi_platform_method method;
> > >   	struct rw_semaphore buffer_lock;	/* Protects debugfs buffer */
> > >   	size_t length;
> > > @@ -132,8 +139,9 @@ static int msi_wmi_platform_parse_buffer(union
> > > acpi_object *obj, u8 *output, siz
> > >   	return 0;
> > >   }
> > > 
> > > -static int msi_wmi_platform_query(struct wmi_device *wdev, enum
> > > msi_wmi_platform_method method,
> > > -				  u8 *input, size_t input_length, u8 *output,
> > > size_t output_length)
> > > +static int msi_wmi_platform_query(struct msi_wmi_platform_data *data,
> > > +				  enum msi_wmi_platform_method method, u8
> > > *input,
> > > +				  size_t input_length, u8 *output, size_t
> > > output_length)
> > >   {
> > >   	struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
> > >   	struct acpi_buffer in = {
> > > @@ -147,9 +155,15 @@ static int msi_wmi_platform_query(struct wmi_device
> > > *wdev, enum msi_wmi_platform
> > >   	if (!input_length || !output_length)
> > >   		return -EINVAL;
> > > 
> > > -	status = wmidev_evaluate_method(wdev, 0x0, method, &in, &out);
> > > -	if (ACPI_FAILURE(status))
> > > -		return -EIO;
> > > +	/*
> > > +	 * The ACPI control method responsible for handling the WMI method
> > > calls
> > > +	 * is not thread-safe. Because of this we have to do the locking
> > > ourself.
> > > +	 */
> > > +	scoped_guard(mutex, &data->wmi_lock) {
> > > +		status = wmidev_evaluate_method(data->wdev, 0x0, method, &in,
> > > &out);
> > > +		if (ACPI_FAILURE(status))
> > > +			return -EIO;
> > > +	}
> > > 
> > >   	obj = out.pointer;
> > >   	if (!obj)
> > > @@ -170,22 +184,22 @@ static umode_t msi_wmi_platform_is_visible(const
> > > void *drvdata, enum hwmon_senso
> > >   static int msi_wmi_platform_read(struct device *dev, enum
> > > hwmon_sensor_types type, u32 attr,
> > >   				 int channel, long *val)
> > >   {
> > > -	struct wmi_device *wdev = dev_get_drvdata(dev);
> > > +	struct msi_wmi_platform_data *data = dev_get_drvdata(dev);
> > >   	u8 input[32] = { 0 };
> > >   	u8 output[32];
> > > -	u16 data;
> > > +	u16 value;
> > >   	int ret;
> > > 
> > > -	ret = msi_wmi_platform_query(wdev, MSI_PLATFORM_GET_FAN, input,
> > > sizeof(input), output,
> > > +	ret = msi_wmi_platform_query(data, MSI_PLATFORM_GET_FAN, input,
> > > sizeof(input), output,
> > >   				     sizeof(output));
> > >   	if (ret < 0)
> > >   		return ret;
> > > 
> > > -	data = get_unaligned_be16(&output[channel * 2 + 1]);
> > > -	if (!data)
> > > +	value = get_unaligned_be16(&output[channel * 2 + 1]);
> > > +	if (!value)
> > >   		*val = 0;
> > >   	else
> > > -		*val = 480000 / data;
> > > +		*val = 480000 / value;
> > Please put this variable rename into own patch before the actual fix.
> 
> Hi,
> 
> the variable rename is necessary because there would be a naming conflict with
> the struct msi_wmi_platform_data *data.
> Since the rename is rather small i would prefer keeping this as a single patch
> to make it easier for the stable
> team to backport.

Hi,

I'm not buying it's "easier" when due to the rename, the patch ends up 
having one long context like that. Stable team is perfectly able to 
backport prerequisite patches.

Please just split the rename into own patch, it will make the fix related 
changes more obvious here, and the fix itself is quite long even without 
the rename.

-- 
 i.