From patchwork Wed Jan 21 14:29:00 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jim Keir X-Patchwork-Id: 5679571 X-Patchwork-Delegate: jikos@jikos.cz Return-Path: X-Original-To: patchwork-linux-input@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id CCCC1C058D for ; Wed, 21 Jan 2015 17:06:54 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 863D020412 for ; Wed, 21 Jan 2015 17:06:53 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 1BF7620260 for ; Wed, 21 Jan 2015 17:06:52 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753371AbbAURGr (ORCPT ); Wed, 21 Jan 2015 12:06:47 -0500 Received: from a8-77.smtp-out.amazonses.com ([54.240.8.77]:39477 "EHLO a8-77.smtp-out.amazonses.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932096AbbAURGN (ORCPT ); Wed, 21 Jan 2015 12:06:13 -0500 X-Greylist: delayed 8455 seconds by postgrey-1.27 at vger.kernel.org; Wed, 21 Jan 2015 12:06:12 EST DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/simple; s=224i4yxa5dv7c2xz3womw6peuasteono; d=amazonses.com; t=1421850540; h=Message-ID:Date:From:MIME-Version:To:CC:Subject:References:In-Reply-To:Content-Type:Content-Transfer-Encoding:Feedback-ID; bh=GeKnHelkL2K7eWcIHXMx3dgwn8AI5WpnHYaNpw9cgtg=; b=XSiTT7XhrVWtAMrsDx+zA5fRYAwEAyjSelbbSqtPhUudLVJo6KkWmVFIW0y+2OMx IF7kwzQdoVj00js2UAoXmBYbNhMWtcAP8nz+qTJ2RHhVXnd2GN2/tXrFARnUA4HM54f tAnmW1nmUS6W7QWICvgrvHJsKsqzBN4vkX2cQnJI= DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/simple; s=wskikwaygy6dq7o6mewsvwbivuod352d; d=oracledbadirect.com; t=1421850540; h=Message-ID:Date:From:MIME-Version:To:CC:Subject:References:In-Reply-To:Content-Type:Content-Transfer-Encoding; bh=GeKnHelkL2K7eWcIHXMx3dgwn8AI5WpnHYaNpw9cgtg=; b=CevwBa3ANk3OqGeEYdIzL5eCQqFEXbI7BsLd7ZYtyj/o34hSpiCHL1Ccs7YPR+aR 9NMqy0ErGPE3W9jb65NmIrU8owUUKMDrRuQIfLP1+2vrSpc2uiv5zqzwJDFMVH4nnw0 sW9FQUY8e83Ip1X4Ifj56+A3i5uLVfmd+Sy0pTnw= X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID,T_RP_MATCHES_RCVD,UNPARSEABLE_RELAY autolearn=ham version=3.3.1 Message-ID: <0000014b0ce57b01-4b49f79e-984b-43dd-b81d-01664133976f-000000@email.amazonses.com> Date: Wed, 21 Jan 2015 14:29:00 +0000 From: Jim Keir User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Benjamin Tissoires CC: linux-input , "linux-usb@vger.kernel.org" , Jiri Kosina , "linux-kernel@vger.kernel.org" Subject: Re: [PATCHv2 001/002] usbhid: Fix initialisation for the Microsoft Sidewinder Force Feedback Pro 2 joystick References: <54BBD8BA.7090906@yahoo.co.uk> <0000014afdcc643d-a943b494-4c10-4fa0-a85b-d36e89d6b09a-000000@email.amazonses.com> In-Reply-To: X-SES-Outgoing: 2015.01.21-54.240.8.77 Feedback-ID: us-east-1.amlta2VpckBvcmFjbGVkYmFkaXJlY3QuY29t:AmazonSES Sender: linux-input-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-input@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 --- } --- 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 wrote: >> From: Jim Keir >> Signed-off-by: Jim Keir >> > 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@vger.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-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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 \n"); + hid_device_io_stop(hid); + return 0; fail: + hid_device_io_stop(hid); + kfree(pidff); return error;