diff mbox series

[v8,1/9] HID: hid-sensor-hub: don't use stale platform-data on remove

Message ID 20240908210803.3339919-2-heiko@sntech.de (mailing list archive)
State New, archived
Headers show
Series Drivers to support the MCU on QNAP NAS devices | expand

Commit Message

Heiko Stübner Sept. 8, 2024, 9:07 p.m. UTC
The hid-sensor-hub creates the individual device structs and transfers them
to the created mfd platform-devices via the platform_data in the mfd_cell.

Before commit e651a1da442a ("HID: hid-sensor-hub: Allow parallel synchronous reads")
the sensor-hub was managing access centrally, with one "completion" in the
hub's data structure, which needed to be finished on removal at the latest.

The mentioned commit then moved this central management to each hid sensor
device, resulting on a completion in each struct hid_sensor_hub_device.
The remove procedure was adapted to go through all sensor devices and
finish any pending "completion".

What this didn't take into account was, platform_device_add_data() that is
used by mfd_add{_hotplug}_devices() does a kmemdup on the submitted
platform-data. So the data the platform-device gets is a copy of the
original data, meaning that the device worked on a different completion
than what sensor_hub_remove() currently wants to access.

To fix that, use device_for_each_child() to go through each child-device
similar to how mfd_remove_devices() unregisters the devices later and
with that get the live platform_data to finalize the correct completion.

Fixes: e651a1da442a ("HID: hid-sensor-hub: Allow parallel synchronous reads")
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/hid/hid-sensor-hub.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

Comments

Lee Jones Oct. 2, 2024, 1:58 p.m. UTC | #1
Intentional top-post!

Just this patch to be reviewed now.

Any of the HID people around?

> The hid-sensor-hub creates the individual device structs and transfers them
> to the created mfd platform-devices via the platform_data in the mfd_cell.
> 
> Before commit e651a1da442a ("HID: hid-sensor-hub: Allow parallel synchronous reads")
> the sensor-hub was managing access centrally, with one "completion" in the
> hub's data structure, which needed to be finished on removal at the latest.
> 
> The mentioned commit then moved this central management to each hid sensor
> device, resulting on a completion in each struct hid_sensor_hub_device.
> The remove procedure was adapted to go through all sensor devices and
> finish any pending "completion".
> 
> What this didn't take into account was, platform_device_add_data() that is
> used by mfd_add{_hotplug}_devices() does a kmemdup on the submitted
> platform-data. So the data the platform-device gets is a copy of the
> original data, meaning that the device worked on a different completion
> than what sensor_hub_remove() currently wants to access.
> 
> To fix that, use device_for_each_child() to go through each child-device
> similar to how mfd_remove_devices() unregisters the devices later and
> with that get the live platform_data to finalize the correct completion.
> 
> Fixes: e651a1da442a ("HID: hid-sensor-hub: Allow parallel synchronous reads")
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  drivers/hid/hid-sensor-hub.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> index 26e93a331a51..3cd00afa453a 100644
> --- a/drivers/hid/hid-sensor-hub.c
> +++ b/drivers/hid/hid-sensor-hub.c
> @@ -730,23 +730,30 @@ static int sensor_hub_probe(struct hid_device *hdev,
>  	return ret;
>  }
>  
> +static int sensor_hub_finalize_pending_fn(struct device *dev, void *data)
> +{
> +	struct hid_sensor_hub_device *hsdev = dev->platform_data;
> +
> +	if (hsdev->pending.status)
> +		complete(&hsdev->pending.ready);
> +
> +	return 0;
> +}
> +
>  static void sensor_hub_remove(struct hid_device *hdev)
>  {
>  	struct sensor_hub_data *data = hid_get_drvdata(hdev);
>  	unsigned long flags;
> -	int i;
>  
>  	hid_dbg(hdev, " hardware removed\n");
>  	hid_hw_close(hdev);
>  	hid_hw_stop(hdev);
> +
>  	spin_lock_irqsave(&data->lock, flags);
> -	for (i = 0; i < data->hid_sensor_client_cnt; ++i) {
> -		struct hid_sensor_hub_device *hsdev =
> -			data->hid_sensor_hub_client_devs[i].platform_data;
> -		if (hsdev->pending.status)
> -			complete(&hsdev->pending.ready);
> -	}
> +	device_for_each_child(&hdev->dev, NULL,
> +			      sensor_hub_finalize_pending_fn);
>  	spin_unlock_irqrestore(&data->lock, flags);
> +
>  	mfd_remove_devices(&hdev->dev);
>  	mutex_destroy(&data->mutex);
>  }
> -- 
> 2.43.0
>
Benjamin Tissoires Oct. 2, 2024, 2:29 p.m. UTC | #2
On Oct 02 2024, Lee Jones wrote:
> Intentional top-post!
> 
> Just this patch to be reviewed now.
> 
> Any of the HID people around?

Sure.

> 
> > The hid-sensor-hub creates the individual device structs and transfers them
> > to the created mfd platform-devices via the platform_data in the mfd_cell.
> > 
> > Before commit e651a1da442a ("HID: hid-sensor-hub: Allow parallel synchronous reads")
> > the sensor-hub was managing access centrally, with one "completion" in the
> > hub's data structure, which needed to be finished on removal at the latest.
> > 
> > The mentioned commit then moved this central management to each hid sensor
> > device, resulting on a completion in each struct hid_sensor_hub_device.
> > The remove procedure was adapted to go through all sensor devices and
> > finish any pending "completion".
> > 
> > What this didn't take into account was, platform_device_add_data() that is
> > used by mfd_add{_hotplug}_devices() does a kmemdup on the submitted
> > platform-data. So the data the platform-device gets is a copy of the
> > original data, meaning that the device worked on a different completion
> > than what sensor_hub_remove() currently wants to access.
> > 
> > To fix that, use device_for_each_child() to go through each child-device
> > similar to how mfd_remove_devices() unregisters the devices later and
> > with that get the live platform_data to finalize the correct completion.
> > 
> > Fixes: e651a1da442a ("HID: hid-sensor-hub: Allow parallel synchronous reads")

That commit was included in v4.1. Don't we want to cc stable here as
well?

Besides that, with the limited knowledge I have of MFDs and the commit
description above, this is:
Acked-by: Benjamin Tissoires <bentiss@kernel.org>

Cheers,
Benjamin

> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> >  drivers/hid/hid-sensor-hub.c | 21 ++++++++++++++-------
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> > index 26e93a331a51..3cd00afa453a 100644
> > --- a/drivers/hid/hid-sensor-hub.c
> > +++ b/drivers/hid/hid-sensor-hub.c
> > @@ -730,23 +730,30 @@ static int sensor_hub_probe(struct hid_device *hdev,
> >  	return ret;
> >  }
> >  
> > +static int sensor_hub_finalize_pending_fn(struct device *dev, void *data)
> > +{
> > +	struct hid_sensor_hub_device *hsdev = dev->platform_data;
> > +
> > +	if (hsdev->pending.status)
> > +		complete(&hsdev->pending.ready);
> > +
> > +	return 0;
> > +}
> > +
> >  static void sensor_hub_remove(struct hid_device *hdev)
> >  {
> >  	struct sensor_hub_data *data = hid_get_drvdata(hdev);
> >  	unsigned long flags;
> > -	int i;
> >  
> >  	hid_dbg(hdev, " hardware removed\n");
> >  	hid_hw_close(hdev);
> >  	hid_hw_stop(hdev);
> > +
> >  	spin_lock_irqsave(&data->lock, flags);
> > -	for (i = 0; i < data->hid_sensor_client_cnt; ++i) {
> > -		struct hid_sensor_hub_device *hsdev =
> > -			data->hid_sensor_hub_client_devs[i].platform_data;
> > -		if (hsdev->pending.status)
> > -			complete(&hsdev->pending.ready);
> > -	}
> > +	device_for_each_child(&hdev->dev, NULL,
> > +			      sensor_hub_finalize_pending_fn);
> >  	spin_unlock_irqrestore(&data->lock, flags);
> > +
> >  	mfd_remove_devices(&hdev->dev);
> >  	mutex_destroy(&data->mutex);
> >  }
> > -- 
> > 2.43.0
> > 
> 
> -- 
> Lee Jones [李琼斯]
srinivas pandruvada Oct. 3, 2024, 12:16 a.m. UTC | #3
On Sun, 2024-09-08 at 23:07 +0200, Heiko Stuebner wrote:
> The hid-sensor-hub creates the individual device structs and
> transfers them
> to the created mfd platform-devices via the platform_data in the
> mfd_cell.
> 
> Before commit e651a1da442a ("HID: hid-sensor-hub: Allow parallel
> synchronous reads")
> the sensor-hub was managing access centrally, with one "completion"
> in the
> hub's data structure, which needed to be finished on removal at the
> latest.
> 
> The mentioned commit then moved this central management to each hid
> sensor
> device, resulting on a completion in each struct
> hid_sensor_hub_device.
> The remove procedure was adapted to go through all sensor devices and
> finish any pending "completion".
> 
> What this didn't take into account was, platform_device_add_data()
> that is
> used by mfd_add{_hotplug}_devices() does a kmemdup on the submitted
> platform-data. So the data the platform-device gets is a copy of the
> original data, meaning that the device worked on a different
> completion
> than what sensor_hub_remove() currently wants to access.
> 
> To fix that, use device_for_each_child() to go through each child-
> device
> similar to how mfd_remove_devices() unregisters the devices later and
> with that get the live platform_data to finalize the correct
> completion.
> 
> Fixes: e651a1da442a ("HID: hid-sensor-hub: Allow parallel synchronous
> reads")
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

> ---
>  drivers/hid/hid-sensor-hub.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-
> hub.c
> index 26e93a331a51..3cd00afa453a 100644
> --- a/drivers/hid/hid-sensor-hub.c
> +++ b/drivers/hid/hid-sensor-hub.c
> @@ -730,23 +730,30 @@ static int sensor_hub_probe(struct hid_device
> *hdev,
>  	return ret;
>  }
>  
> +static int sensor_hub_finalize_pending_fn(struct device *dev, void
> *data)
> +{
> +	struct hid_sensor_hub_device *hsdev = dev->platform_data;
> +
> +	if (hsdev->pending.status)
> +		complete(&hsdev->pending.ready);
> +
> +	return 0;
> +}
> +
>  static void sensor_hub_remove(struct hid_device *hdev)
>  {
>  	struct sensor_hub_data *data = hid_get_drvdata(hdev);
>  	unsigned long flags;
> -	int i;
>  
>  	hid_dbg(hdev, " hardware removed\n");
>  	hid_hw_close(hdev);
>  	hid_hw_stop(hdev);
> +
>  	spin_lock_irqsave(&data->lock, flags);
> -	for (i = 0; i < data->hid_sensor_client_cnt; ++i) {
> -		struct hid_sensor_hub_device *hsdev =
> -			data-
> >hid_sensor_hub_client_devs[i].platform_data;
> -		if (hsdev->pending.status)
> -			complete(&hsdev->pending.ready);
> -	}
> +	device_for_each_child(&hdev->dev, NULL,
> +			      sensor_hub_finalize_pending_fn);
>  	spin_unlock_irqrestore(&data->lock, flags);
> +
>  	mfd_remove_devices(&hdev->dev);
>  	mutex_destroy(&data->mutex);
>  }
diff mbox series

Patch

diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
index 26e93a331a51..3cd00afa453a 100644
--- a/drivers/hid/hid-sensor-hub.c
+++ b/drivers/hid/hid-sensor-hub.c
@@ -730,23 +730,30 @@  static int sensor_hub_probe(struct hid_device *hdev,
 	return ret;
 }
 
+static int sensor_hub_finalize_pending_fn(struct device *dev, void *data)
+{
+	struct hid_sensor_hub_device *hsdev = dev->platform_data;
+
+	if (hsdev->pending.status)
+		complete(&hsdev->pending.ready);
+
+	return 0;
+}
+
 static void sensor_hub_remove(struct hid_device *hdev)
 {
 	struct sensor_hub_data *data = hid_get_drvdata(hdev);
 	unsigned long flags;
-	int i;
 
 	hid_dbg(hdev, " hardware removed\n");
 	hid_hw_close(hdev);
 	hid_hw_stop(hdev);
+
 	spin_lock_irqsave(&data->lock, flags);
-	for (i = 0; i < data->hid_sensor_client_cnt; ++i) {
-		struct hid_sensor_hub_device *hsdev =
-			data->hid_sensor_hub_client_devs[i].platform_data;
-		if (hsdev->pending.status)
-			complete(&hsdev->pending.ready);
-	}
+	device_for_each_child(&hdev->dev, NULL,
+			      sensor_hub_finalize_pending_fn);
 	spin_unlock_irqrestore(&data->lock, flags);
+
 	mfd_remove_devices(&hdev->dev);
 	mutex_destroy(&data->mutex);
 }