On Wed, 2015-01-21 at 21:45 +0000, Jonathan Cameron wrote:
> On 07/01/15 18:24, Srinivas Pandruvada wrote:
> > Current implementation only allows one outstanding synchronous read.
> > This is a performance hit when user mode is requesting raw reads
> > of sensor attributes on multiple sensors together.
> > This change changes the mutex lock to per hid sensor hub device instead
> > of global lock. Although request to hid sensor hub is serialized, there
> > can be multiple outstanding read requests pending for responses via
> > hid reports.
> >
> > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> Seems sensible to me. I don't think this has any cross dependencies with
> the IIO patches...
Correct, no dependency with IIO. This is independent.
Thanks for the review.
-Srinivas
>
> Acked-by: Jonathan Cameron <jic23@xxxxxxxxxx>
> > ---
> > drivers/hid/hid-sensor-hub.c | 90 +++++++++++++++++++-----------------------
> > include/linux/hid-sensor-hub.h | 24 +++++++++++
> > 2 files changed, 65 insertions(+), 49 deletions(-)
> >
> > diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> > index e54ce10..865cd56 100644
> > --- a/drivers/hid/hid-sensor-hub.c
> > +++ b/drivers/hid/hid-sensor-hub.c
> > @@ -29,29 +29,10 @@
> > #define HID_SENSOR_HUB_ENUM_QUIRK 0x01
> >
> > /**
> > - * struct sensor_hub_pending - Synchronous read pending information
> > - * @status: Pending status true/false.
> > - * @ready: Completion synchronization data.
> > - * @usage_id: Usage id for physical device, E.g. Gyro usage id.
> > - * @attr_usage_id: Usage Id of a field, E.g. X-AXIS for a gyro.
> > - * @raw_size: Response size for a read request.
> > - * @raw_data: Place holder for received response.
> > - */
> > -struct sensor_hub_pending {
> > - bool status;
> > - struct completion ready;
> > - u32 usage_id;
> > - u32 attr_usage_id;
> > - int raw_size;
> > - u8 *raw_data;
> > -};
> > -
> > -/**
> > * struct sensor_hub_data - Hold a instance data for a HID hub device
> > * @hsdev: Stored hid instance for current hub device.
> > * @mutex: Mutex to serialize synchronous request.
> > * @lock: Spin lock to protect pending request structure.
> > - * @pending: Holds information of pending sync read request.
> > * @dyn_callback_list: Holds callback function
> > * @dyn_callback_lock: spin lock to protect callback list
> > * @hid_sensor_hub_client_devs: Stores all MFD cells for a hub instance.
> > @@ -61,7 +42,6 @@ struct sensor_hub_pending {
> > struct sensor_hub_data {
> > struct mutex mutex;
> > spinlock_t lock;
> > - struct sensor_hub_pending pending;
> > struct list_head dyn_callback_list;
> > spinlock_t dyn_callback_lock;
> > struct mfd_cell *hid_sensor_hub_client_devs;
> > @@ -266,40 +246,42 @@ int sensor_hub_input_attr_get_raw_value(struct hid_sensor_hub_device *hsdev,
> > struct hid_report *report;
> > int ret_val = 0;
> >
> > - mutex_lock(&data->mutex);
> > - memset(&data->pending, 0, sizeof(data->pending));
> > - init_completion(&data->pending.ready);
> > - data->pending.usage_id = usage_id;
> > - data->pending.attr_usage_id = attr_usage_id;
> > - data->pending.raw_size = 0;
> > + mutex_lock(&hsdev->mutex);
> > + memset(&hsdev->pending, 0, sizeof(hsdev->pending));
> > + init_completion(&hsdev->pending.ready);
> > + hsdev->pending.usage_id = usage_id;
> > + hsdev->pending.attr_usage_id = attr_usage_id;
> > + hsdev->pending.raw_size = 0;
> >
> > spin_lock_irqsave(&data->lock, flags);
> > - data->pending.status = true;
> > + hsdev->pending.status = true;
> > spin_unlock_irqrestore(&data->lock, flags);
> > report = sensor_hub_report(report_id, hsdev->hdev, HID_INPUT_REPORT);
> > if (!report)
> > goto err_free;
> >
> > + mutex_lock(&data->mutex);
> > hid_hw_request(hsdev->hdev, report, HID_REQ_GET_REPORT);
> > - wait_for_completion_interruptible_timeout(&data->pending.ready, HZ*5);
> > - switch (data->pending.raw_size) {
> > + mutex_unlock(&data->mutex);
> > + wait_for_completion_interruptible_timeout(&hsdev->pending.ready, HZ*5);
> > + switch (hsdev->pending.raw_size) {
> > case 1:
> > - ret_val = *(u8 *)data->pending.raw_data;
> > + ret_val = *(u8 *)hsdev->pending.raw_data;
> > break;
> > case 2:
> > - ret_val = *(u16 *)data->pending.raw_data;
> > + ret_val = *(u16 *)hsdev->pending.raw_data;
> > break;
> > case 4:
> > - ret_val = *(u32 *)data->pending.raw_data;
> > + ret_val = *(u32 *)hsdev->pending.raw_data;
> > break;
> > default:
> > ret_val = 0;
> > }
> > - kfree(data->pending.raw_data);
> > + kfree(hsdev->pending.raw_data);
> >
> > err_free:
> > - data->pending.status = false;
> > - mutex_unlock(&data->mutex);
> > + hsdev->pending.status = false;
> > + mutex_unlock(&hsdev->mutex);
> >
> > return ret_val;
> > }
> > @@ -455,16 +437,6 @@ static int sensor_hub_raw_event(struct hid_device *hdev,
> > report->field[i]->report_count)/8);
> > sz = (report->field[i]->report_size *
> > report->field[i]->report_count)/8;
> > - if (pdata->pending.status && pdata->pending.attr_usage_id ==
> > - report->field[i]->usage->hid) {
> > - hid_dbg(hdev, "data was pending ...\n");
> > - pdata->pending.raw_data = kmemdup(ptr, sz, GFP_ATOMIC);
> > - if (pdata->pending.raw_data)
> > - pdata->pending.raw_size = sz;
> > - else
> > - pdata->pending.raw_size = 0;
> > - complete(&pdata->pending.ready);
> > - }
> > collection = &hdev->collection[
> > report->field[i]->usage->collection_index];
> > hid_dbg(hdev, "collection->usage %x\n",
> > @@ -474,8 +446,21 @@ static int sensor_hub_raw_event(struct hid_device *hdev,
> > report->field[i]->physical,
> > report->field[i]->usage[0].collection_index,
> > &hsdev, &priv);
> > -
> > - if (callback && callback->capture_sample) {
> > + if (!callback) {
> > + ptr += sz;
> > + continue;
> > + }
> > + if (hsdev->pending.status && hsdev->pending.attr_usage_id ==
> > + report->field[i]->usage->hid) {
> > + hid_dbg(hdev, "data was pending ...\n");
> > + hsdev->pending.raw_data = kmemdup(ptr, sz, GFP_ATOMIC);
> > + if (hsdev->pending.raw_data)
> > + hsdev->pending.raw_size = sz;
> > + else
> > + hsdev->pending.raw_size = 0;
> > + complete(&hsdev->pending.ready);
> > + }
> > + if (callback->capture_sample) {
> > if (report->field[i]->logical)
> > callback->capture_sample(hsdev,
> > report->field[i]->logical, sz, ptr,
> > @@ -630,6 +615,8 @@ static int sensor_hub_probe(struct hid_device *hdev,
> > hsdev->hdev = hdev;
> > hsdev->vendor_id = hdev->vendor;
> > hsdev->product_id = hdev->product;
> > + hsdev->usage = collection->usage;
> > + mutex_init(&hsdev->mutex);
> > hsdev->start_collection_index = i;
> > if (last_hsdev)
> > last_hsdev->end_collection_index = i;
> > @@ -676,13 +663,18 @@ 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);
> > - if (data->pending.status)
> > - complete(&data->pending.ready);
> > + 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);
> > + }
> > spin_unlock_irqrestore(&data->lock, flags);
> > mfd_remove_devices(&hdev->dev);
> > hid_set_drvdata(hdev, NULL);
> > diff --git a/include/linux/hid-sensor-hub.h b/include/linux/hid-sensor-hub.h
> > index 4173a8f..0b21c4f 100644
> > --- a/include/linux/hid-sensor-hub.h
> > +++ b/include/linux/hid-sensor-hub.h
> > @@ -49,19 +49,43 @@ struct hid_sensor_hub_attribute_info {
> > };
> >
> > /**
> > + * struct sensor_hub_pending - Synchronous read pending information
> > + * @status: Pending status true/false.
> > + * @ready: Completion synchronization data.
> > + * @usage_id: Usage id for physical device, E.g. Gyro usage id.
> > + * @attr_usage_id: Usage Id of a field, E.g. X-AXIS for a gyro.
> > + * @raw_size: Response size for a read request.
> > + * @raw_data: Place holder for received response.
> > + */
> > +struct sensor_hub_pending {
> > + bool status;
> > + struct completion ready;
> > + u32 usage_id;
> > + u32 attr_usage_id;
> > + int raw_size;
> > + u8 *raw_data;
> > +};
> > +
> > +/**
> > * struct hid_sensor_hub_device - Stores the hub instance data
> > * @hdev: Stores the hid instance.
> > * @vendor_id: Vendor id of hub device.
> > * @product_id: Product id of hub device.
> > + * @usage: Usage id for this hub device instance.
> > * @start_collection_index: Starting index for a phy type collection
> > * @end_collection_index: Last index for a phy type collection
> > + * @mutex: synchronizing mutex.
> > + * @pending: Holds information of pending sync read request.
> > */
> > struct hid_sensor_hub_device {
> > struct hid_device *hdev;
> > u32 vendor_id;
> > u32 product_id;
> > + u32 usage;
> > int start_collection_index;
> > int end_collection_index;
> > + struct mutex mutex;
> > + struct sensor_hub_pending pending;
> > };
> >
> > /**
> >
>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
