On Fri, Aug 06, 2021 at 08:45:06AM -0600, Aaron Bieber wrote:
>
> joshua stein writes:
>
> > On Thu, 05 Aug 2021 at 12:41:56 -0600, Aaron Bieber wrote:
> >>
> >> Martin Pieuchot writes:
> >>
> >> > On 05/08/21(Thu) 09:41, Aaron Bieber wrote:
> >> >>
> >> >> Stuart Henderson writes:
> >> >>
> >> >> > On 2021/07/13 01:29, Anindya Mukherjee wrote:
> >> >> >> I have a Cyber Power CP1500PFCLCDa UPS and get exactly the same
> >> >> >> crash on the
> >> >> >> latest snapshot if the USB cable is unplugged. My dmesg is very
> >> >> >> similar so I've
> >> >> >> omitted it, but I'd also be happy to help debug this issue.
> >> >> >>
> >> >> >> Regards,
> >> >> >> Anindya
> >> >> >>
> >> >> >
> >> >> > First try backing out the "Allow uhidev child devices to claim
> >> >> > selective
> >> >> > report ids" commit from March.
> >> >>
> >> >> Here is a diff that reverts:
> >> >>
> >> >> 2021-03-08 6545f693 jcs Add another Type Cover device
> >> >> 2021-03-08 1f85050a jcs regen
> >> >> 2021-03-08 fc9d2605 jcs Add Surface Pro Type Cover
> >> >> 2021-03-08 f31b43ce jcs Allow uhidev child devices to claim
> >> >> selective report ids
> >> >>
> >> >> With them reverted, my UPS(s) no longer trigger panics on disconnect.
> >> >
> >> > The bugs lies in upd_match() and has been introduced with the change
> >> > containing UHIDEV_CLAIM_MULTIPLE_REPORTID.
> >> >
> >> > upd(4) owns all the reportID of a USB device, so a "uha.claimed" logic
> >> > similar to what has been written for umt_match() is missing.
> >> >
> >>
> >> The following diff fixes it for me:
> >>
> >> diff 2f84ac1381d01579f2b25c83b3dec5f908503765 /usr/src
> >> blob - ad65b77718b50851c1537a7048eb7b54aba91203
> >> file + sys/dev/usb/upd.c
> >> --- sys/dev/usb/upd.c
> >> +++ sys/dev/usb/upd.c
> >> @@ -167,6 +167,7 @@ upd_match(struct device *parent, void *match, void *au
> >> if (upd_lookup_usage_entry(desc, size,
> >> upd_usage_roots + i, &item)) {
> >> ret = UMATCH_VENDOR_PRODUCT;
> >> + uha->claimed[item.report_ID] = 1;
> >> break;
> >> }
> >
> > I think it needs to loop through all of the report IDs like it does
> > in upd_attach_sensor_tree to mark them claimed, so it should not
> > break after the first result.
>
> Ah, ty! Version with break removed:
>
> diff --git a/sys/dev/usb/upd.c b/sys/dev/usb/upd.c
> index ad65b77718b..cfffab765a3 100644
> --- a/sys/dev/usb/upd.c
> +++ b/sys/dev/usb/upd.c
> @@ -167,7 +167,7 @@ upd_match(struct device *parent, void *match, void *aux)
> if (upd_lookup_usage_entry(desc, size,
> upd_usage_roots + i, &item)) {
> ret = UMATCH_VENDOR_PRODUCT;
> - break;
> + uha->claimed[item.report_ID] = 1;
> }
>
> return (ret);
Thanks for chasing this down. Had bee on my back burner for a while and never
got to it.
-ml
