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 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")
Cc: stable@vger.kernel.org
Acked-by: Benjamin Tissoires <bentiss@kernel.org>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
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 7bd86eef6ec7..4c94c03cb573 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.45.2On Thu, 7 Nov 2024, 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 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")
> Cc: stable@vger.kernel.org
> Acked-by: Benjamin Tissoires <bentiss@kernel.org>
> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Acked-by: Jiri Kosina <jkosina@suse.com>
Are you planning to merge this together with the rest of the set, or do
you want me to expedite it? I'll be happy to apply it separately as a
proper fix.
Thanks,
--
Jiri Kosina
SUSE Labs
Hi Jiri,
Am Donnerstag, 7. November 2024, 13:59:04 CET schrieb Jiri Kosina:
> On Thu, 7 Nov 2024, 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 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")
> > Cc: stable@vger.kernel.org
> > Acked-by: Benjamin Tissoires <bentiss@kernel.org>
> > Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>
> Acked-by: Jiri Kosina <jkosina@suse.com>
>
> Are you planning to merge this together with the rest of the set, or do
> you want me to expedite it? I'll be happy to apply it separately as a
> proper fix.
This change was more or less a surprise find, because I wanted to make
the platform_data pointer in the mfd_cell struct const and this the hid
sensor hub stood out as doing something strange ;-) .
So patch 2 of this series actually depends on this change to not cause
build errors.
But seeing that we're after -rc6 alredy, I would assume the brunt of the
mcu series might need to wait after 6.13-rc1 anyway - but I guess that
depends on how Lee sees things ;-) .
Heiko
On Thu, 7 Nov 2024, Heiko Stübner wrote: > This change was more or less a surprise find, because I wanted to make > the platform_data pointer in the mfd_cell struct const and this the hid > sensor hub stood out as doing something strange ;-) . > > So patch 2 of this series actually depends on this change to not cause > build errors. Ah, right. > But seeing that we're after -rc6 alredy, I would assume the brunt of the > mcu series might need to wait after 6.13-rc1 anyway - but I guess that > depends on how Lee sees things ;-) . OK, I am keeping my hands off it for the time being. Thanks, -- Jiri Kosina SUSE Labs
On Thu, 07 Nov 2024, Jiri Kosina wrote: > On Thu, 7 Nov 2024, Heiko Stübner wrote: > > > This change was more or less a surprise find, because I wanted to make > > the platform_data pointer in the mfd_cell struct const and this the hid > > sensor hub stood out as doing something strange ;-) . > > > > So patch 2 of this series actually depends on this change to not cause > > build errors. > > Ah, right. > > > But seeing that we're after -rc6 alredy, I would assume the brunt of the > > mcu series might need to wait after 6.13-rc1 anyway - but I guess that > > depends on how Lee sees things ;-) . > > OK, I am keeping my hands off it for the time being. I can take it now with an Ack. -- Lee Jones [李琼斯]
© 2016 - 2024 Red Hat, Inc.