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@xxxxxxxxxx>
Cheers,
Benjamin
> > Signed-off-by: Heiko Stuebner <heiko@xxxxxxxxx>
> > ---
> > 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 [李琼斯]
