Hi Frank,
On Tue, Nov 11, 2014 at 01:01:53PM -0500, Frank Praznik wrote:
> Replace stack buffers with kernel allocated buffers when sending
> and receiving HID reports to prevent issues with DMA transfers
> on certain hardware.
>
> Output report buffers are allocated at initialization time to avoid
> excessive kmalloc and kfree calls.
>
> Signed-off-by: Frank Praznik <[email protected]>
> ---
>
> I'm CC-ing the original bug reporter to see if he can test this patch
> on whatever platform was giving him trouble as I can't replicate the
> reported error on my end.
>
> drivers/hid/hid-sony.c | 135
> ++++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 105 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index bc4269e..28f2849 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -798,6 +798,9 @@ union sixaxis_output_report_01 {
> __u8 buf[36];
> };
>
> +#define DS4_REPORT5_SIZE 32
> +#define DS4_REPORT17_SIZE 78
> +
> static spinlock_t sony_dev_list_lock;
> static LIST_HEAD(sony_device_list);
> static DEFINE_IDA(sony_device_id_allocator);
> @@ -811,6 +814,7 @@ struct sony_sc {
> struct work_struct state_worker;
> struct power_supply battery;
> int device_id;
> + __u8 *output_report_dmabuf;
Just to confirm as I haven't looked at the entire driver: there is no
possibility of 2 requests being submitted at the same time so that one
will overwrite other's data?
>
> #ifdef CONFIG_SONY_FF
> __u8 left;
> @@ -1142,9 +1146,19 @@ static int sixaxis_set_operational_usb(struct
> hid_device *hdev)
>
> static int sixaxis_set_operational_bt(struct hid_device *hdev)
> {
> - unsigned char buf[] = { 0xf4, 0x42, 0x03, 0x00, 0x00 };
> - return hid_hw_raw_request(hdev, buf[0], buf, sizeof(buf),
> + static const __u8 report[] = { 0xf4, 0x42, 0x03, 0x00, 0x00 };
> + __u8 *buf = kmemdup(report, sizeof(report), GFP_KERNEL);
> + int ret;
> +
> + if (!buf)
Please keep allocation and check together, i.e.
buf = kmemdup(report, sizeof(report), GFP_KERNEL);
if (!buf)
...
> + return -ENOMEM;
> +
> + ret = hid_hw_raw_request(hdev, buf[0], buf, sizeof(buf),
sizeof(buf) is either 4 or 8, not 5!
> HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
> +
> + kfree(buf);
> +
> + return ret;
> }
>
> /*
> @@ -1153,10 +1167,18 @@ static int sixaxis_set_operational_bt(struct
> hid_device *hdev)
> */
> static int dualshock4_set_operational_bt(struct hid_device *hdev)
> {
> - __u8 buf[37] = { 0 };
> + __u8 *buf = kmalloc(37, GFP_KERNEL);
No magic constants please.
> + int ret;
>
> - return hid_hw_raw_request(hdev, 0x02, buf, sizeof(buf),
> - HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> + if (!buf)
> + return -ENOMEM;
> +
> + ret = hid_hw_raw_request(hdev, 0x02, buf, 37, HID_FEATURE_REPORT,
> + HID_REQ_GET_REPORT);
> +
> + kfree(buf);
> +
> + return ret;
> }
>
> static void sixaxis_set_leds_from_id(int id, __u8 values[MAX_LEDS])
> @@ -1471,9 +1493,7 @@ error_leds:
>
> static void sixaxis_state_worker(struct work_struct *work)
> {
> - struct sony_sc *sc = container_of(work, struct sony_sc, state_worker);
> - int n;
> - union sixaxis_output_report_01 report = {
> + static const union sixaxis_output_report_01 default_report = {
> .buf = {
> 0x01,
> 0x00, 0xff, 0x00, 0xff, 0x00,
> @@ -1485,20 +1505,27 @@ static void sixaxis_state_worker(struct work_struct
> *work)
> 0x00, 0x00, 0x00, 0x00, 0x00
> }
> };
> + struct sony_sc *sc = container_of(work, struct sony_sc, state_worker);
> + struct sixaxis_output_report *report =
> + (struct sixaxis_output_report *)sc->output_report_dmabuf;
> + int n;
> +
> + /* Initialize the report with default values */
> + memcpy(report, &default_report, sizeof(struct sixaxis_output_report));
>
> #ifdef CONFIG_SONY_FF
> - report.data.rumble.right_motor_on = sc->right ? 1 : 0;
> - report.data.rumble.left_motor_force = sc->left;
> + report->rumble.right_motor_on = sc->right ? 1 : 0;
> + report->rumble.left_motor_force = sc->left;
> #endif
>
> - report.data.leds_bitmap |= sc->led_state[0] << 1;
> - report.data.leds_bitmap |= sc->led_state[1] << 2;
> - report.data.leds_bitmap |= sc->led_state[2] << 3;
> - report.data.leds_bitmap |= sc->led_state[3] << 4;
> + report->leds_bitmap |= sc->led_state[0] << 1;
> + report->leds_bitmap |= sc->led_state[1] << 2;
> + report->leds_bitmap |= sc->led_state[2] << 3;
> + report->leds_bitmap |= sc->led_state[3] << 4;
>
> /* Set flag for all leds off, required for 3rd party INTEC controller */
> - if ((report.data.leds_bitmap & 0x1E) == 0)
> - report.data.leds_bitmap |= 0x20;
> + if ((report->leds_bitmap & 0x1E) == 0)
> + report->leds_bitmap |= 0x20;
>
> /*
> * The LEDs in the report are indexed in reverse order to their
> @@ -1511,13 +1538,14 @@ static void sixaxis_state_worker(struct work_struct
> *work)
> */
> for (n = 0; n < 4; n++) {
> if (sc->led_delay_on[n] || sc->led_delay_off[n]) {
> - report.data.led[3 - n].duty_off = sc->led_delay_off[n];
> - report.data.led[3 - n].duty_on = sc->led_delay_on[n];
> + report->led[3 - n].duty_off = sc->led_delay_off[n];
> + report->led[3 - n].duty_on = sc->led_delay_on[n];
> }
> }
>
> - hid_hw_raw_request(sc->hdev, report.data.report_id, report.buf,
> - sizeof(report), HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
> + hid_hw_raw_request(sc->hdev, report->report_id, (__u8 *)report,
> + sizeof(struct sixaxis_output_report),
> + HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
> }
>
> static void dualshock4_state_worker(struct work_struct *work)
> @@ -1526,13 +1554,15 @@ static void dualshock4_state_worker(struct
> work_struct *work)
> struct hid_device *hdev = sc->hdev;
> int offset;
>
> - __u8 buf[78] = { 0 };
> + __u8 *buf = sc->output_report_dmabuf;
>
> if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) {
> + memset(buf, 0, DS4_REPORT5_SIZE);
> buf[0] = 0x05;
> buf[1] = 0xFF;
> offset = 4;
> } else {
> + memset(buf, 0, DS4_REPORT17_SIZE);
> buf[0] = 0x11;
> buf[1] = 0xB0;
> buf[3] = 0x0F;
> @@ -1560,12 +1590,33 @@ static void dualshock4_state_worker(struct
> work_struct *work)
> buf[offset++] = sc->led_delay_off[3];
>
> if (sc->quirks & DUALSHOCK4_CONTROLLER_USB)
> - hid_hw_output_report(hdev, buf, 32);
> + hid_hw_output_report(hdev, buf, DS4_REPORT5_SIZE);
> else
> - hid_hw_raw_request(hdev, 0x11, buf, 78,
> + hid_hw_raw_request(hdev, 0x11, buf, DS4_REPORT17_SIZE,
> HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
> }
>
> +static int sony_allocate_output_report(struct sony_sc *sc)
> +{
> + if (sc->quirks & SIXAXIS_CONTROLLER)
> + sc->output_report_dmabuf =
> + kmalloc(sizeof(union sixaxis_output_report_01),
> + GFP_KERNEL);
> + else if (sc->quirks & DUALSHOCK4_CONTROLLER_BT)
> + sc->output_report_dmabuf = kmalloc(DS4_REPORT17_SIZE,
> + GFP_KERNEL);
> + else if (sc->quirks & DUALSHOCK4_CONTROLLER_USB)
> + sc->output_report_dmabuf = kmalloc(DS4_REPORT5_SIZE,
> + GFP_KERNEL);
> + else
> + return 0;
> +
> + if (!sc->output_report_dmabuf)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> #ifdef CONFIG_SONY_FF
> static int sony_play_effect(struct input_dev *dev, void *data,
> struct ff_effect *effect)
> @@ -1754,6 +1805,7 @@ static int sony_get_bt_devaddr(struct sony_sc *sc)
>
> static int sony_check_add(struct sony_sc *sc)
> {
> + __u8 *buf = NULL;
> int n, ret;
>
> if ((sc->quirks & DUALSHOCK4_CONTROLLER_BT) ||
> @@ -1769,36 +1821,44 @@ static int sony_check_add(struct sony_sc *sc)
> return 0;
> }
> } else if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) {
> - __u8 buf[7];
> + buf = kmalloc(7, GFP_KERNEL);
> +
> + if (!buf)
> + return -ENOMEM;
>
> /*
> * The MAC address of a DS4 controller connected via USB can be
> * retrieved with feature report 0x81. The address begins at
> * offset 1.
> */
> - ret = hid_hw_raw_request(sc->hdev, 0x81, buf, sizeof(buf),
> + ret = hid_hw_raw_request(sc->hdev, 0x81, buf, 7,
> HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
>
> if (ret != 7) {
> hid_err(sc->hdev, "failed to retrieve feature report
> 0x81 with the DualShock 4 MAC address\n");
> - return ret < 0 ? ret : -EINVAL;
> + ret = ret < 0 ? ret : -EINVAL;
> + goto out_free;
> }
>
> memcpy(sc->mac_address, &buf[1], sizeof(sc->mac_address));
> } else if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
> - __u8 buf[18];
> + buf = kmalloc(18, GFP_KERNEL);
> +
> + if (!buf)
> + return -ENOMEM;
>
> /*
> * The MAC address of a Sixaxis controller connected via USB can
> * be retrieved with feature report 0xf2. The address begins at
> * offset 4.
> */
> - ret = hid_hw_raw_request(sc->hdev, 0xf2, buf, sizeof(buf),
> + ret = hid_hw_raw_request(sc->hdev, 0xf2, buf, 18,
> HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
>
> if (ret != 18) {
> hid_err(sc->hdev, "failed to retrieve feature report
> 0xf2 with the Sixaxis MAC address\n");
> - return ret < 0 ? ret : -EINVAL;
> + ret = ret < 0 ? ret : -EINVAL;
> + goto out_free;
> }
>
> /*
> @@ -1811,7 +1871,13 @@ static int sony_check_add(struct sony_sc *sc)
> return 0;
> }
>
> - return sony_check_add_dev_list(sc);
> + ret = sony_check_add_dev_list(sc);
> +
> +out_free:
> +
> + kfree(buf);
> +
> + return ret;
> }
>
> static int sony_set_device_id(struct sony_sc *sc)
> @@ -1895,6 +1961,12 @@ static int sony_probe(struct hid_device *hdev, const
> struct hid_device_id *id)
> return ret;
> }
>
> + ret = sony_allocate_output_report(sc);
> + if (ret < 0) {
> + hid_err(hdev, "failed to allocate the output report buffer\n");
> + goto err_stop;
> + }
> +
> ret = sony_set_device_id(sc);
> if (ret < 0) {
> hid_err(hdev, "failed to allocate the device id\n");
> @@ -1984,6 +2056,7 @@ err_stop:
> if (sc->quirks & SONY_BATTERY_SUPPORT)
> sony_battery_remove(sc);
> sony_cancel_work_sync(sc);
> + kfree(sc->output_report_dmabuf);
> sony_remove_dev_list(sc);
> sony_release_device_id(sc);
> hid_hw_stop(hdev);
> @@ -2004,6 +2077,8 @@ static void sony_remove(struct hid_device *hdev)
>
> sony_cancel_work_sync(sc);
>
> + kfree(sc->output_report_dmabuf);
> +
> sony_remove_dev_list(sc);
>
> sony_release_device_id(sc);
> --
> 2.1.0
>
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
