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

[Benjamin Tissoires]

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 [李琼斯]