| Message ID | 20190906094158.8854-1-streetwalkermc@gmail.com |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | David Miller |
| Headers | show |
| Series | Bluetooth: hidp: Fix error checks in hidp_get/set_raw_report | expand |
On Fri, Sep 06, 2019 at 12:41:57PM +0300, Dan Elkouby wrote: > Commit 48d9cc9d85dd ("Bluetooth: hidp: Let hidp_send_message return > number of queued bytes") changed hidp_send_message to return non-zero > values on success, which some other bits did not expect. This caused > spurious errors to be propagated through the stack, breaking some (all?) > drivers, such as hid-sony for the Dualshock 4 in Bluetooth mode. > > Signed-off-by: Dan Elkouby <streetwalkermc@gmail.com> I think we also need to update update ms_ff_worker() which assumes that hid_hw_output_report() returns zero on success. Please use the Fixes tag for this since a lot of scripts rely on it to decide what to backport. Fixes: 48d9cc9d85dd ("Bluetooth: hidp: Let hidp_send_message return number of queued bytes") Otherwise, it looks good. Thanks for catching this. regards, dan carpenter
On Fri, Sep 6, 2019 at 1:14 PM Dan Carpenter wrote: > I think we also need to update update ms_ff_worker() which assumes that > hid_hw_output_report() returns zero on success. Yes, it looks like that's the case. Should I amend my patch to include this fix, or should it be a separate patch? I don't have access to any hardware covered by hid-microsoft, so I won't be able to test it. > Please use the Fixes > tag for this since a lot of scripts rely on it to decide what to > backport. > > Fixes: 48d9cc9d85dd ("Bluetooth: hidp: Let hidp_send_message return number of queued bytes") Will do. > Otherwise, it looks good. Thanks for catching this. Thanks for taking a look! (Sorry for sending this twice, I'm not used to mailing lists and forgot to reply to all.)
On Fri, Sep 06, 2019 at 01:40:15PM +0300, Dan Elkouby wrote: > On Fri, Sep 6, 2019 at 1:14 PM Dan Carpenter wrote: > > I think we also need to update update ms_ff_worker() which assumes that > > hid_hw_output_report() returns zero on success. > > Yes, it looks like that's the case. Should I amend my patch to include > this fix, or should it be a separate patch? I don't have access to any > hardware covered by hid-microsoft, so I won't be able to test it. > Yes. Please amend the patch. We all understand that you don't have the hardware so it's not a problem. If you want to blame me in the commit message that's fine. "Dan Carpenter pointed out a related issue in ms_ff_worker()". But we're only silencing a warning so it can't really break anything. You can add my Reviewed-by tag as well when you resend. regards, dan carpenter
Hi Dan, > Commit 48d9cc9d85dd ("Bluetooth: hidp: Let hidp_send_message return > number of queued bytes") changed hidp_send_message to return non-zero > values on success, which some other bits did not expect. This caused > spurious errors to be propagated through the stack, breaking some (all?) > drivers, such as hid-sony for the Dualshock 4 in Bluetooth mode. > > Signed-off-by: Dan Elkouby <streetwalkermc@gmail.com> > --- > net/bluetooth/hidp/core.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) patch has been applied to bluetooth-next tree. Regards Marcel
On Fri, Sep 6, 2019 at 4:56 PM Marcel Holtmann <marcel@holtmann.org> wrote: > patch has been applied to bluetooth-next tree. > Thanks a lot!
On Fri, Sep 06, 2019 at 03:56:29PM +0200, Marcel Holtmann wrote: > Hi Dan, > > > Commit 48d9cc9d85dd ("Bluetooth: hidp: Let hidp_send_message return > > number of queued bytes") changed hidp_send_message to return non-zero > > values on success, which some other bits did not expect. This caused > > spurious errors to be propagated through the stack, breaking some (all?) > > drivers, such as hid-sony for the Dualshock 4 in Bluetooth mode. > > > > Signed-off-by: Dan Elkouby <streetwalkermc@gmail.com> > > --- > > net/bluetooth/hidp/core.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > patch has been applied to bluetooth-next tree. > The v2 added an additional fix and used the Fixes tag. Could you apply that instead? regards, dan carpenter
Hi Dan, >>> Commit 48d9cc9d85dd ("Bluetooth: hidp: Let hidp_send_message return >>> number of queued bytes") changed hidp_send_message to return non-zero >>> values on success, which some other bits did not expect. This caused >>> spurious errors to be propagated through the stack, breaking some (all?) >>> drivers, such as hid-sony for the Dualshock 4 in Bluetooth mode. >>> >>> Signed-off-by: Dan Elkouby <streetwalkermc@gmail.com> >>> --- >>> net/bluetooth/hidp/core.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> patch has been applied to bluetooth-next tree. >> > > The v2 added an additional fix and used the Fixes tag. Could you apply > that instead? see my reply to Jiri. I replied to the wrong patch, but actually applied to the updated one. Regards Marcel
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c index 8d889969ae7e..bef84b95e2c4 100644 --- a/net/bluetooth/hidp/core.c +++ b/net/bluetooth/hidp/core.c @@ -267,7 +267,7 @@ static int hidp_get_raw_report(struct hid_device *hid, set_bit(HIDP_WAITING_FOR_RETURN, &session->flags); data[0] = report_number; ret = hidp_send_ctrl_message(session, report_type, data, 1); - if (ret) + if (ret < 0) goto err; /* Wait for the return of the report. The returned report @@ -343,7 +343,7 @@ static int hidp_set_raw_report(struct hid_device *hid, unsigned char reportnum, data[0] = reportnum; set_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags); ret = hidp_send_ctrl_message(session, report_type, data, count); - if (ret) + if (ret < 0) goto err; /* Wait for the ACK from the device. */
Commit 48d9cc9d85dd ("Bluetooth: hidp: Let hidp_send_message return number of queued bytes") changed hidp_send_message to return non-zero values on success, which some other bits did not expect. This caused spurious errors to be propagated through the stack, breaking some (all?) drivers, such as hid-sony for the Dualshock 4 in Bluetooth mode. Signed-off-by: Dan Elkouby <streetwalkermc@gmail.com> --- net/bluetooth/hidp/core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)