| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <0000014b0ce57b01-4b49f79e-984b-43dd-b81d-01664133976f-000000@email.amazonses.com>
Date: Wed, 21 Jan 2015 14:29:00 +0000
From: Jim Keir <jimkeir@...cledbadirect.com>
To: Benjamin Tissoires <benjamin.tissoires@...il.com>
CC: linux-input <linux-input@...r.kernel.org>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
Jiri Kosina <jkosina@...e.cz>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCHv2 001/002] usbhid: Fix initialisation for the Microsoft
Sidewinder Force Feedback Pro 2 joystick
Hi,
Thanks for the feedback. I've included a replacement patch here for the
first issue, and will send a separate message with a patch for the third
issue. I've removed the second as per Alan's request. This diff is
against the latest version of the hid branch rather than the 3.13 branch
I was using before. Apologies for the mistakes, I'm still feeling my way
around here.
---
Allow the Microsoft Sidewinder Force Feedback 2 joystick to perform the
required communication with the device during initialisation.
Signed-off-by: Jim Keir <jimkeir@...cledbadirect.com>
---
diff --git a/drivers/hid/usbhid/hid-pidff.c b/drivers/hid/usbhid/hid-pidff.c
index 10b6167..1b3fa70 100644
--- a/drivers/hid/usbhid/hid-pidff.c
+++ b/drivers/hid/usbhid/hid-pidff.c
@@ -1252,6 +1258,8 @@ int hid_pidff_init(struct hid_device *hid)
pidff->hid = hid;
+ hid_device_io_start(hid);
+
pidff_find_reports(hid, HID_OUTPUT_REPORT, pidff);
pidff_find_reports(hid, HID_FEATURE_REPORT, pidff);
@@ -1315,9 +1323,13 @@ int hid_pidff_init(struct hid_device *hid)
hid_info(dev, "Force feedback for USB HID PID devices by Anssi
Hannula <anssi.hannula@...il.com>\n");
+ hid_device_io_stop(hid);
+
return 0;
fail:
+ hid_device_io_stop(hid);
+
kfree(pidff);
return error;
}
---
On 20/01/2015 14:09, Benjamin Tissoires wrote:
> Hi,
>
> Jim, in addition to what Alan said, here are some comments that I
> would like to be fixed in the v2.
>
> On Sun, Jan 18, 2015 at 11:07 AM, Jim Keir <jimkeir@...cledbadirect.com> wrote:
>> From: Jim Keir <jimkeir@...oo.co.uk>
>> Signed-off-by: Jim Keir <jimkeir@...oo.co.uk>
>>
> The Signed-off-by line is generally at the end of the commit message.
> This way, if someone else adds new changes to the patch, we can trace
> which modifications belongs to which.
>
>> Currently the SWFF2 driver fails during initialisation, making the force
>> capability of the joystick unusable. Further, there is a long-standing
>> bug in the same driver where commands to update force parameters are
>> addressed to the last-created force effect instead of the specified one,
>> making it impossible to modify effects after their creation.
>>
>> Three bugs are addressed:
>>
>> 1) The FF2 driver (usbhid/hid-pidff.c) sends commands to the stick
>> during ff_init. However, this is called inside a block where
>> driver_input_lock is locked, so the results of these initial commands
>> are discarded. This one is the "killer", without this nothing else works.
>>
>> ff_init issues commands using "hid_hw_request". This eventually goes to
>> hid_input_report, which returns -EBUSY because driver_input_lock is
>> locked. The change is to delay the ff_init call in hid-core.c until
>> after this lock has been released.
>>
>> 2) The usbhid driver ignores an endpoint stall when sending control
>> commands, causing the first few commands of the hid-pidff.c
>> initialisation to get lost.
>>
>> usbhid/hid-core.c has been modified by copying lines into "hid_ctrl"
>> from the "hid_irq_in" function in the same file.
>>
>> 3) The FF2 driver (usbhid/hid-pidff.c) does not set the effect ID when
>> uploading an effect. The result is that the initial upload works but
>> subsequent uploads to modify effect parameters are all directed at the
>> last-created effect.
>>
> Fully agree that you should split the commit in 3 if there are 3
> issues (and to the rest Alan said also, but this is the most important
> I think).
>
>
>> The targeted effect ID must be passed back to the device when effect
>> parameters are changed. This is done at the start of
>> "pidff_set_condition_report", "pidff_set_periodic_report" etc. based on
>> the value of "pidff->block_load[PID_EFFECT_BLOCK_INDEX].value[0]".
>> However, this value is only ever set during pidff_request_effect_upload.
>> The result is stored in "pidff->pid_id[effect->id]" at the end of
>> pid_upload_effect, for later use. However, if an effect is modified and
>> re-sent then this identifier is not being copied back from
>> pidff->pid_id[effect->id] before sending the command to the device. The
>> fix is to do this at the start of pidff_upload_effect.
>>
>> This patch taken against kernel 3.13.0
>>
>> ---
>>
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 905e40a..a608ee6 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -1546,9 +1546,8 @@ int hid_connect(struct hid_device *hdev, unsigned int
>> connect_mask)
> On my local tree, hid_connect is at 1562. Is your patch based on the
> for-next branch of the HID tree?
> https://git.kernel.org/cgit/linux/kernel/git/jikos/hid.git
>
>
>> return -ENODEV;
>> }
>>
>> - if ((hdev->claimed & HID_CLAIMED_INPUT) &&
>> - (connect_mask & HID_CONNECT_FF) && hdev->ff_init)
>> - hdev->ff_init(hdev);
>> + /* Removed ff_init() call from here. It does device I/O but this
>> + * is blocked because driver_input_lock is currently locked. */
> Please don't. If the feedback driver needs to have access to the IO
> earlier, it needs to call hid_device_io_start() (and eventually
> hid_device_io_stop() if some other initialization are required).
>
>> len = 0;
>> if (hdev->claimed & HID_CLAIMED_INPUT)
>> @@ -2029,6 +2028,13 @@ static int hid_device_probe(struct device *dev)
>> unlock:
>> if (!hdev->io_started)
>> up(&hdev->driver_input_lock);
>> +
>> + if ((hdev->claimed & HID_CLAIMED_INPUT) && hdev->ff_init) {
>> + /* Late init of PID force-feedback drivers moved to after
>> + * unlock of driver_input_lock */
>> + hdev->ff_init(hdev);
>> + }
>> +
> Same comment as above.
>
>> unlock_driver_lock:
>> up(&hdev->driver_lock);
>> return ret;
>> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
>> index 029965e..5d34dd7 100644
>> --- a/drivers/hid/usbhid/hid-core.c
>> +++ b/drivers/hid/usbhid/hid-core.c
>> @@ -505,7 +505,12 @@ static void hid_ctrl(struct urb *urb)
>> case -EPROTO: /* protocol error or unplug */
>> case -ECONNRESET: /* unlink */
>> case -ENOENT:
>> + break;
>> case -EPIPE: /* report not available */
>> + usbhid_mark_busy(usbhid);
>> + clear_bit(HID_IN_RUNNING, &usbhid->iofl);
>> + set_bit(HID_CLEAR_HALT, &usbhid->iofl);
>> + schedule_work(&usbhid->reset_work);
>> break;
>> default: /* error */
>> hid_warn(urb->dev, "ctrl urb status %d received\n", status);
>> diff --git a/drivers/hid/usbhid/hid-pidff.c b/drivers/hid/usbhid/hid-pidff.c
>> index 10b6167..3f8ea63 100644
>> --- a/drivers/hid/usbhid/hid-pidff.c
>> +++ b/drivers/hid/usbhid/hid-pidff.c
>> @@ -568,6 +568,13 @@ static int pidff_upload_effect(struct input_dev *dev,
>> struct ff_effect *effect,
>> int type_id;
>> int error;
>>
>> + pidff->block_load[PID_EFFECT_BLOCK_INDEX].value[0] = 0;
>> +
>> + if (old && effect) {
>> + pidff->block_load[PID_EFFECT_BLOCK_INDEX].value[0] =
>> + pidff->pid_id[effect->id];
>> + }
>> +
>> switch (effect->type) {
>> case FF_CONSTANT:
>> if (!old) {
>> @@ -701,10 +708,14 @@ static int pidff_upload_effect(struct input_dev *dev,
>> struct ff_effect *effect,
>> return -EINVAL;
>> }
>>
>> - if (!old)
>> + if (!old) {
>> pidff->pid_id[effect->id] =
>> pidff->block_load[PID_EFFECT_BLOCK_INDEX].value[0];
>>
>> + hid_dbg(pidff->hid, "Created new effect of type 0x%02x with h/w ID
>> %d, driver ID %d\n",
>> + effect->type, pidff->pid_id[effect->id], effect->id);
>> + }
>> +
> Not sure this is absolutely required, but it shouldn't hurt so you can
> keep it I guess.
>
> Cheers,
> Benjamin
>
>> hid_dbg(pidff->hid, "uploaded\n");
>>
>> return 0;
>>
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@...r.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists