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@xxxxxxxxxxxxxxxxxxx> wrote: > From: Jim Keir <jimkeir@xxxxxxxxxxx> > Signed-off-by: Jim Keir <jimkeir@xxxxxxxxxxx> > 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@xxxxxxxxxxxxxxx > 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-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
