| Message ID | 1374153691-25100-2-git-send-email-nlopezcasad@logitech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
On Thu, Jul 18, 2013 at 3:21 PM, Nestor Lopez Casado <nlopezcasad@logitech.com> wrote: > Set querying_devices flag to true when we start the enumeration > process. > > This was missing from the original patch. It never produced > undesirable effects as it is highly improbable to have a second > enumeration triggered while a first one was still in progress. > > Signed-off-by: Nestor Lopez Casado <nlopezcasad@logitech.com> > --- > drivers/hid/hid-logitech-dj.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c > index 0d13389..d4657a5 100644 > --- a/drivers/hid/hid-logitech-dj.c > +++ b/drivers/hid/hid-logitech-dj.c > @@ -488,6 +488,8 @@ static int logi_dj_recv_query_paired_devices(struct dj_receiver_dev *djrcv_dev) > if (djrcv_dev->querying_devices) > return 0; > > + djrcv_dev->querying_devices = true; > + Unfortunately, this breaks the fallback mechanism :( We tried to add the two patches in Fedora [1], but this doesn't fix the bug because the driver actually things that it already asked for the enumeration, but as we get the -EPIPE error, the request was never sent. So, Jiri, if you were to submit that series to Linus (or Greg) for fixing the bug, please just drop this second patch. Cheers, Benjamin [1] https://bugzilla.redhat.com/show_bug.cgi?id=989138 > dj_report = kzalloc(sizeof(struct dj_report), GFP_KERNEL); > if (!dj_report) > return -ENOMEM; > -- > 1.7.9.5 > -- 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
On Thu, 1 Aug 2013, Benjamin Tissoires wrote: > > Set querying_devices flag to true when we start the enumeration > > process. > > > > This was missing from the original patch. It never produced > > undesirable effects as it is highly improbable to have a second > > enumeration triggered while a first one was still in progress. > > > > Signed-off-by: Nestor Lopez Casado <nlopezcasad@logitech.com> > > --- > > drivers/hid/hid-logitech-dj.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c > > index 0d13389..d4657a5 100644 > > --- a/drivers/hid/hid-logitech-dj.c > > +++ b/drivers/hid/hid-logitech-dj.c > > @@ -488,6 +488,8 @@ static int logi_dj_recv_query_paired_devices(struct dj_receiver_dev *djrcv_dev) > > if (djrcv_dev->querying_devices) > > return 0; > > > > + djrcv_dev->querying_devices = true; > > + > > Unfortunately, this breaks the fallback mechanism :( > We tried to add the two patches in Fedora [1], but this doesn't fix > the bug because the driver actually things that it already asked for > the enumeration, but as we get the -EPIPE error, the request was never > sent. > > So, Jiri, if you were to submit that series to Linus (or Greg) for > fixing the bug, please just drop this second patch. It's already on its way to Linus (he hasn't pulled yet though) ... which is not a big deal per se, I can always push a revert, but I have to admit I don't understand the breakage it is causing at all. Could you please elaborate? (and put an elaborate description to revert commit log perhaps?) Thanks,
On Fri, Aug 2, 2013 at 3:11 AM, Jiri Kosina <jkosina@suse.cz> wrote: > On Thu, 1 Aug 2013, Benjamin Tissoires wrote: > >> > Set querying_devices flag to true when we start the enumeration >> > process. >> > >> > This was missing from the original patch. It never produced >> > undesirable effects as it is highly improbable to have a second >> > enumeration triggered while a first one was still in progress. >> > >> > Signed-off-by: Nestor Lopez Casado <nlopezcasad@logitech.com> >> > --- >> > drivers/hid/hid-logitech-dj.c | 2 ++ >> > 1 file changed, 2 insertions(+) >> > >> > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c >> > index 0d13389..d4657a5 100644 >> > --- a/drivers/hid/hid-logitech-dj.c >> > +++ b/drivers/hid/hid-logitech-dj.c >> > @@ -488,6 +488,8 @@ static int logi_dj_recv_query_paired_devices(struct dj_receiver_dev *djrcv_dev) >> > if (djrcv_dev->querying_devices) >> > return 0; >> > >> > + djrcv_dev->querying_devices = true; >> > + >> >> Unfortunately, this breaks the fallback mechanism :( >> We tried to add the two patches in Fedora [1], but this doesn't fix >> the bug because the driver actually things that it already asked for >> the enumeration, but as we get the -EPIPE error, the request was never >> sent. >> >> So, Jiri, if you were to submit that series to Linus (or Greg) for >> fixing the bug, please just drop this second patch. > > It's already on its way to Linus (he hasn't pulled yet though) ... which > is not a big deal per se, I can always push a revert, but I have to admit > I don't understand the breakage it is causing at all. > > Could you please elaborate? (and put an elaborate description to revert > commit log perhaps?) Sure, so here is the revert commit log: -- Commit "HID: hid-logitech-dj, querying_devices was never set" activate a flag which guarantees that we do not ask the receiver for too many enumeration. When the flag is set, each following enumeration call is discarded (the usb request is not forwarded to the receiver). The flag is then released when the driver receive a pairing information event, which normally follows the enumeration request. However, the USB3 bug makes the driver think the enumeration request has been forwarded to the receiver. However, it is actually not the case because the USB stack returns -EPIPE. So, when a new unknown device appears, the workaround consisting in asking for a new enumeration is not working anymore: this new enumeration is discarded because of the flag, which is never reset. A solution could be to trigger a timeout before releasing it, but for now, let's just revert the patch. -- Does that makes it more understandable? I'm sorry I was not clear last time, I was trying to catch this up between two sessions at GUADEC. Cheers, Benjamin -- 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
On Fri, 2 Aug 2013, Benjamin Tissoires wrote: > > Could you please elaborate? (and put an elaborate description to revert > > commit log perhaps?) > > Sure, so here is the revert commit log: > > -- > > Commit "HID: hid-logitech-dj, querying_devices was never set" activate > a flag which guarantees that we do not ask the receiver for too many > enumeration. When the flag is set, each following enumeration call is > discarded (the usb request is not forwarded to the receiver). The flag > is then released when the driver receive a pairing information event, > which normally follows the enumeration request. > However, the USB3 bug makes the driver think the enumeration request > has been forwarded to the receiver. However, it is actually not the > case because the USB stack returns -EPIPE. So, when a new unknown > device appears, the workaround consisting in asking for a new > enumeration is not working anymore: this new enumeration is discarded > because of the flag, which is never reset. > > A solution could be to trigger a timeout before releasing it, but for > now, let's just revert the patch. > > -- Thanks Benjamin. I'd like to have a bit more clarification about the USB3 bug, as this whole issue is not completely clear to me. To be more specific -- when exactly do we receive -EPIPE, why do we receive it and why do we not handle it properly? Thanks again,
On Mon, Aug 5, 2013 at 3:22 PM, Jiri Kosina <jkosina@suse.cz> wrote: > On Fri, 2 Aug 2013, Benjamin Tissoires wrote: > >> > Could you please elaborate? (and put an elaborate description to revert >> > commit log perhaps?) >> >> Sure, so here is the revert commit log: >> >> -- >> >> Commit "HID: hid-logitech-dj, querying_devices was never set" activate >> a flag which guarantees that we do not ask the receiver for too many >> enumeration. When the flag is set, each following enumeration call is >> discarded (the usb request is not forwarded to the receiver). The flag >> is then released when the driver receive a pairing information event, >> which normally follows the enumeration request. >> However, the USB3 bug makes the driver think the enumeration request >> has been forwarded to the receiver. However, it is actually not the >> case because the USB stack returns -EPIPE. So, when a new unknown >> device appears, the workaround consisting in asking for a new >> enumeration is not working anymore: this new enumeration is discarded >> because of the flag, which is never reset. >> >> A solution could be to trigger a timeout before releasing it, but for >> now, let's just revert the patch. >> >> -- > > Thanks Benjamin. > > I'd like to have a bit more clarification about the USB3 bug, as this > whole issue is not completely clear to me. > > To be more specific -- when exactly do we receive -EPIPE, why do we > receive it and why do we not handle it properly? Sure, I'll try (though the more I think of it, the more it seems blurry to me :( ). So the initial probe function in hid-logitech-dj was implemented by using a direct call to hid_output_raw_report(). This call was synchronous, so we did get the -EPIPE return code. Then, the probe() function returns the -EPIPE error, cleaning the receiver and unregister it from the hid bus. However, now, we use hid_hw_request(), which is asynchronous (from what I can read). At least, this code returns "void" as the set_report command seems to be scheduled for later handling. In usbhid, when the queue is flushed, I did not found a way to retrieve the error code... So basically, the -EPIPE is received in usbhid_restart_ctrl_queue(), but nothing notifies hid-logitech-dj from the error. In the end, the probe() function returns without error code, but the receiver never received the notification. Cheers, Benjamin -- 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
Being affected by this bug, I can confirm that Linux 3.11-rc4 still exhibits the unwanted behaviour for me, but that commenting out the single line from the second patch makes it work. Thus, for requesting a revert on that line, you are most welcome to put me down as a "Tested-By". Best regards, Sune Mølgaard Benjamin Tissoires wrote: > On Mon, Aug 5, 2013 at 3:22 PM, Jiri Kosina <jkosina@suse.cz> wrote: >> On Fri, 2 Aug 2013, Benjamin Tissoires wrote: >> >>>> Could you please elaborate? (and put an elaborate description to revert >>>> commit log perhaps?) >>> >>> Sure, so here is the revert commit log: >>> >>> -- >>> >>> Commit "HID: hid-logitech-dj, querying_devices was never set" activate >>> a flag which guarantees that we do not ask the receiver for too many >>> enumeration. When the flag is set, each following enumeration call is >>> discarded (the usb request is not forwarded to the receiver). The flag >>> is then released when the driver receive a pairing information event, >>> which normally follows the enumeration request. >>> However, the USB3 bug makes the driver think the enumeration request >>> has been forwarded to the receiver. However, it is actually not the >>> case because the USB stack returns -EPIPE. So, when a new unknown >>> device appears, the workaround consisting in asking for a new >>> enumeration is not working anymore: this new enumeration is discarded >>> because of the flag, which is never reset. >>> >>> A solution could be to trigger a timeout before releasing it, but for >>> now, let's just revert the patch. >>> >>> -- >> >> Thanks Benjamin. >> >> I'd like to have a bit more clarification about the USB3 bug, as this >> whole issue is not completely clear to me. >> >> To be more specific -- when exactly do we receive -EPIPE, why do we >> receive it and why do we not handle it properly? > > Sure, I'll try (though the more I think of it, the more it seems > blurry to me :( ). > > So the initial probe function in hid-logitech-dj was implemented by > using a direct call to hid_output_raw_report(). This call was > synchronous, so we did get the -EPIPE return code. Then, the probe() > function returns the -EPIPE error, cleaning the receiver and > unregister it from the hid bus. > > However, now, we use hid_hw_request(), which is asynchronous (from > what I can read). At least, this code returns "void" as the set_report > command seems to be scheduled for later handling. In usbhid, when the > queue is flushed, I did not found a way to retrieve the error code... > > So basically, the -EPIPE is received in usbhid_restart_ctrl_queue(), > but nothing notifies hid-logitech-dj from the error. In the end, the > probe() function returns without error code, but the receiver never > received the notification. > > Cheers, > Benjamin > -- > 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/ > >
On Tue, 6 Aug 2013, Sune Mølgaard wrote: > Being affected by this bug, I can confirm that Linux 3.11-rc4 still > exhibits the unwanted behaviour for me, but that commenting out the > single line from the second patch makes it work. > > Thus, for requesting a revert on that line, you are most welcome to put > me down as a "Tested-By". Okay, this is now queued, and I will be pushing it to Linus. Thanks,
On Tue, 6 Aug 2013, Sune Mølgaard wrote: > Being affected by this bug, I can confirm that Linux 3.11-rc4 still > exhibits the unwanted behaviour for me, but that commenting out the > single line from the second patch makes it work. > > Thus, for requesting a revert on that line, you are most welcome to put > me down as a "Tested-By". Will be reverted in 3.11-rc5 (8e5654ce69). Thanks,
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c index 0d13389..d4657a5 100644 --- a/drivers/hid/hid-logitech-dj.c +++ b/drivers/hid/hid-logitech-dj.c @@ -488,6 +488,8 @@ static int logi_dj_recv_query_paired_devices(struct dj_receiver_dev *djrcv_dev) if (djrcv_dev->querying_devices) return 0; + djrcv_dev->querying_devices = true; + dj_report = kzalloc(sizeof(struct dj_report), GFP_KERNEL); if (!dj_report) return -ENOMEM;
Set querying_devices flag to true when we start the enumeration process. This was missing from the original patch. It never produced undesirable effects as it is highly improbable to have a second enumeration triggered while a first one was still in progress. Signed-off-by: Nestor Lopez Casado <nlopezcasad@logitech.com> --- drivers/hid/hid-logitech-dj.c | 2 ++ 1 file changed, 2 insertions(+)