On Wed, Nov 03, 2021 at 09:43:54AM +0100, Damien Couderc wrote:
> Le 03/11/2021 à 07:44, Anton Lindqvist a écrit :
> > Hi,
> > In an attempt to fix a bug related to upd(4) I discovered that the
> > pseudo report id UHIDEV_CLAIM_MULTIPLE_REPORTID is conflicting with an
> > actual report id. The previous fix, reverted by now, avoided the
> > conflict by incrementing the pseudo report id was not a good idea
> > since a report id is expected to be represented using a single unsigned
> > byte elsewhere in the usb stack.
> >
> > Here's a second attempt. Report id zero is according to the USB HID
> > specification reserved and should not be used. We can define
> > UHIDEV_CLAIM_MULTIPLE_REPORTID as zero removing the need to use a larger
> > integer type than the existing uint8_t for the report id. Another
> > correction is also needed. Some descriptors only supports a single
> > report and the report id is in this case optional. Internally, uhidev
> > uses report id zero for such devices. It's therefore of importance to
> > not perform a claim multiple report ids attachment on such devices as
> > there's only one report id to claim.
> >
> > Testing would be much appreciated.
> >
> > Comments? OK?
> >
> > diff --git sys/dev/usb/uhidev.c sys/dev/usb/uhidev.c
> > index 5fe2f702e21..f5cc5984b59 100644
> > --- sys/dev/usb/uhidev.c
> > +++ sys/dev/usb/uhidev.c
> > @@ -247,29 +247,36 @@ uhidev_attach(struct device *parent, struct device
> > *self, void *aux)
> > sc->sc_isize += (nrepid != 1); /* one byte for the report ID */
> > DPRINTF(("uhidev_attach: isize=%d\n", sc->sc_isize));
> > + memset(&uha, 0, sizeof(uha));
> > uha.uaa = uaa;
> > uha.parent = sc;
> > - uha.reportid = UHIDEV_CLAIM_MULTIPLE_REPORTID;
> > - uha.nreports = nrepid;
> > - uha.claimed = malloc(nrepid, M_TEMP, M_WAITOK|M_ZERO);
> > /* Look for a driver claiming multiple report IDs first. */
> > - dev = config_found_sm(self, &uha, NULL, uhidevsubmatch);
> > - if (dev != NULL) {
> > - for (repid = 0; repid < nrepid; repid++) {
> > - /*
> > - * Could already be assigned by uhidev_set_report_dev().
> > - */
> > - if (sc->sc_subdevs[repid] != NULL)
> > - continue;
> > -
> > - if (uha.claimed[repid])
> > + if (nrepid > 1) {
> > + uha.reportid = UHIDEV_CLAIM_MULTIPLE_REPORTID;
> > + uha.nreports = nrepid;
> > + uha.claimed = malloc(nrepid, M_TEMP, M_WAITOK|M_ZERO);
> > +
> > + dev = config_found_sm(self, &uha, NULL, uhidevsubmatch);
> > + if (dev != NULL) {
> > + for (repid = 0; repid < nrepid; repid++) {
> > + /*
> > + * Could already be assigned by
> > + * uhidev_set_report_dev().
> > + */
> > + if (sc->sc_subdevs[repid] != NULL)
> > + continue;
> > +
> > + if (!uha.claimed[repid])
> > + continue;
> > sc->sc_subdevs[repid] = (struct uhidev *)dev;
> > + }
> > }
> > - }
> > - free(uha.claimed, M_TEMP, nrepid);
> > - uha.claimed = NULL;
> > + free(uha.claimed, M_TEMP, nrepid);
> > + uha.nreports = 0;
> > + uha.claimed = NULL;
> > + }
> > for (repid = 0; repid < nrepid; repid++) {
> > DPRINTF(("%s: try repid=%d\n", __func__, repid));
> > diff --git sys/dev/usb/uhidev.h sys/dev/usb/uhidev.h
> > index 6ce75b1f49d..9ae85e1ab9f 100644
> > --- sys/dev/usb/uhidev.h
> > +++ sys/dev/usb/uhidev.h
> > @@ -81,8 +81,8 @@ struct uhidev_attach_arg {
> > struct usb_attach_arg *uaa;
> > struct uhidev_softc *parent;
> > uint8_t reportid;
> > -#define UHIDEV_CLAIM_MULTIPLE_REPORTID 255
> > - uint8_t nreports;
> > +#define UHIDEV_CLAIM_MULTIPLE_REPORTID 0
> > + u_int nreports;
> > uint8_t *claimed;
> > };
>
> Hi,
>
> It works flawlessly for me and unplugging is fine too.
Thanks, committed.
