Hello David,
On 05/12/14(Fri) 14:02, David Higgs wrote:
> On Dec 4, 2014, at 4:30 PM, David Higgs <hig...@gmail.com> wrote:
>
> > I am trying to figure out how to handle the buggy USB firmware in my UPS
> > (see misc@ thread from last week). With some kernel debug enabled, I see
> > "usb_transfer_complete: short transfer 3<5" messages. Since the
> > BatteryPresent sensor could not be read, all battery-related sensors were
> > forced into UNKNOWN state.
> >
> > It appears that apcupsd adjusts the expected HID report length as soon as a
> > bogus request is detected:
> > http://sourceforge.net/p/apcupsd/svn/2273/tree/branches/Branch-3_14/src/drivers/usb/generic/generic-usb.c#l254
> >
> > The patch below handles my particular case but is probably too
> > generic/sloppy. Is there any interest in workarounds, either like this or
> > something less terrible?
>
> Below is an updated diff against -current, and dmesg when applied to -stable
> (no change). On my non-virtual hardware, upd(4) connects via ohci(4), which
> didn’t report USBD_SHORT_XFER. Also, I fixed a format string warning when
> UPD_DEBUG was enabled.
The ohci(4) diff is almost fine, USBD_SHORT_XFER should only be set in
usbd_transfer_complete() so the HCD should only set the status to
USBD_NORMAL_COMPLETION, see below.
Concerning your broken firmware, what we should do is change the
uhidev_*_report() function to somehow return the number of transferred
bytes (actlen). Since this involves calling usbd_do_request_flag() and
update all the consumers of this API, here's a first step.
Could you test the diff below and tell me if it still works for you?
You'll still need your upd(4) diff. If that works, I'll commit it and
send a second diff to deal with your broken firmware.
Index: ohci.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/ohci.c,v
retrieving revision 1.140
diff -u -p -r1.140 ohci.c
--- ohci.c 5 Oct 2014 08:40:29 -0000 1.140
+++ ohci.c 6 Dec 2014 12:21:17 -0000
@@ -1311,6 +1311,8 @@ ohci_softintr(void *v)
if (cc == OHCI_CC_STALL)
xfer->status = USBD_STALLED;
+ else if (cc == OHCI_CC_DATA_UNDERRUN)
+ xfer->status = USBD_NORMAL_COMPLETION;
else
xfer->status = USBD_IOERROR;
s = splusb();
Index: uhidev.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/uhidev.c,v
retrieving revision 1.63
diff -u -p -r1.63 uhidev.c
--- uhidev.c 10 Aug 2014 12:48:43 -0000 1.63
+++ uhidev.c 6 Dec 2014 13:37:02 -0000
@@ -264,12 +264,16 @@ int
uhidev_use_rdesc(struct uhidev_softc *sc, int vendor, int product,
void **descp, int *sizep)
{
- static uByte reportbuf[] = {2, 2, 2};
+ static uByte reportbuf[] = {2, 2};
const void *descptr = NULL;
void *desc;
int size;
if (vendor == USB_VENDOR_WACOM) {
+ struct uhidev wacom;
+
+ /* XXX until we pass the parent directly. */
+ wacom.sc_parent = sc;
/* The report descriptor for the Wacom Graphire is broken. */
switch (product) {
@@ -279,9 +283,8 @@ uhidev_use_rdesc(struct uhidev_softc *sc
break;
case USB_PRODUCT_WACOM_GRAPHIRE3_4X5:
case USB_PRODUCT_WACOM_GRAPHIRE4_4X5:
- usbd_set_report(sc->sc_udev, sc->sc_ifaceno,
- UHID_FEATURE_REPORT, 2, &reportbuf,
- sizeof(reportbuf));
+ uhidev_set_report(&wacom, UHID_FEATURE_REPORT,
+ 2, &reportbuf, sizeof(reportbuf));
size = sizeof(uhid_graphire3_4x5_report_descr);
descptr = uhid_graphire3_4x5_report_descr;
break;
@@ -629,23 +632,30 @@ usbd_status
uhidev_set_report(struct uhidev *scd, int type, int id, void *data, int len)
{
struct uhidev_softc *sc = scd->sc_parent;
- char *buf;
- usbd_status retstat;
+ usb_device_request_t req;
+ usbd_status err;
+ char *buf = data;
- if (id == 0)
- return usbd_set_report(sc->sc_udev, sc->sc_ifaceno, type,
- id, data, len);
+ /* Prepend the reportID. */
+ if (id > 0) {
+ len++;
+ buf = malloc(len, M_TEMP, M_WAITOK);
+ buf[0] = id;
+ memcpy(buf + 1, data, len - 1);
+ }
- buf = malloc(len + 1, M_TEMP, M_WAITOK);
- buf[0] = id;
- memcpy(buf+1, data, len);
+ req.bmRequestType = UT_WRITE_CLASS_INTERFACE;
+ req.bRequest = UR_SET_REPORT;
+ USETW2(req.wValue, type, id);
+ USETW(req.wIndex, sc->sc_ifaceno);
+ USETW(req.wLength, len);
- retstat = usbd_set_report(sc->sc_udev, sc->sc_ifaceno, type,
- id, buf, len + 1);
+ err = usbd_do_request(sc->sc_udev, &req, buf);
- free(buf, M_TEMP, 0);
+ if (id > 0)
+ free(buf, M_TEMP, len);
- return retstat;
+ return (err);
}
usbd_status
@@ -653,39 +663,52 @@ uhidev_set_report_async(struct uhidev *s
int len)
{
struct uhidev_softc *sc = scd->sc_parent;
- char *buf;
- usbd_status retstat;
+ usb_device_request_t req;
+ usbd_status err;
+ char *buf = data;
- if (id == 0)
- return usbd_set_report_async(sc->sc_udev, sc->sc_ifaceno, type,
- id, data, len);
-
- buf = malloc(len + 1, M_TEMP, M_NOWAIT);
- if (buf == NULL)
- return (USBD_NOMEM);
- buf[0] = id;
- memcpy(buf+1, data, len);
+ /* Prepend the reportID. */
+ if (id > 0) {
+ len++;
+ buf = malloc(len, M_TEMP, M_NOWAIT);
+ if (buf == NULL)
+ return (USBD_NOMEM);
+ buf[0] = id;
+ memcpy(buf + 1, data, len - 1);
+ }
+
+ req.bmRequestType = UT_WRITE_CLASS_INTERFACE;
+ req.bRequest = UR_SET_REPORT;
+ USETW2(req.wValue, type, id);
+ USETW(req.wIndex, sc->sc_ifaceno);
+ USETW(req.wLength, len);
- retstat = usbd_set_report_async(sc->sc_udev, sc->sc_ifaceno, type,
- id, buf, len + 1);
+ err = usbd_do_request_async(sc->sc_udev, &req, buf);
/*
* Since report requests are write-only it is safe to free
* the buffer right after submitting the transfer because
* it won't be used afterward.
*/
- free(buf, M_TEMP, 0);
+ if (id > 0)
+ free(buf, M_TEMP, len);
- return retstat;
+ return (err);
}
usbd_status
uhidev_get_report(struct uhidev *scd, int type, int id, void *data, int len)
{
struct uhidev_softc *sc = scd->sc_parent;
+ usb_device_request_t req;
+
+ req.bmRequestType = UT_READ_CLASS_INTERFACE;
+ req.bRequest = UR_GET_REPORT;
+ USETW2(req.wValue, type, id);
+ USETW(req.wIndex, sc->sc_ifaceno);
+ USETW(req.wLength, len);
- return usbd_get_report(sc->sc_udev, sc->sc_ifaceno, type,
- id, data, len);
+ return (usbd_do_request(sc->sc_udev, &req, data));
}
usbd_status
Index: usbdi_util.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/usbdi_util.c,v
retrieving revision 1.39
diff -u -p -r1.39 usbdi_util.c
--- usbdi_util.c 7 Nov 2014 13:56:29 -0000 1.39
+++ usbdi_util.c 6 Dec 2014 13:01:06 -0000
@@ -207,51 +207,6 @@ usbd_set_port_feature(struct usbd_device
}
usbd_status
-usbd_set_report(struct usbd_device *dev, int ifaceno, int type, int id,
- void *data, int len)
-{
- usb_device_request_t req;
-
- DPRINTFN(4, ("usbd_set_report: len=%d\n", len));
- req.bmRequestType = UT_WRITE_CLASS_INTERFACE;
- req.bRequest = UR_SET_REPORT;
- USETW2(req.wValue, type, id);
- USETW(req.wIndex, ifaceno);
- USETW(req.wLength, len);
- return (usbd_do_request(dev, &req, data));
-}
-
-usbd_status
-usbd_set_report_async(struct usbd_device *dev, int ifaceno, int type, int id,
- void *data, int len)
-{
- usb_device_request_t req;
-
- DPRINTFN(4, ("usbd_set_report_async: len=%d\n", len));
- req.bmRequestType = UT_WRITE_CLASS_INTERFACE;
- req.bRequest = UR_SET_REPORT;
- USETW2(req.wValue, type, id);
- USETW(req.wIndex, ifaceno);
- USETW(req.wLength, len);
- return (usbd_do_request_async(dev, &req, data));
-}
-
-usbd_status
-usbd_get_report(struct usbd_device *dev, int ifaceno, int type, int id,
- void *data, int len)
-{
- usb_device_request_t req;
-
- DPRINTFN(4, ("usbd_get_report: len=%d\n", len));
- req.bmRequestType = UT_READ_CLASS_INTERFACE;
- req.bRequest = UR_GET_REPORT;
- USETW2(req.wValue, type, id);
- USETW(req.wIndex, ifaceno);
- USETW(req.wLength, len);
- return (usbd_do_request(dev, &req, data));
-}
-
-usbd_status
usbd_set_idle(struct usbd_device *dev, int ifaceno, int duration, int id)
{
usb_device_request_t req;
Index: usbdi_util.h
===================================================================
RCS file: /cvs/src/sys/dev/usb/usbdi_util.h,v
retrieving revision 1.28
diff -u -p -r1.28 usbdi_util.h
--- usbdi_util.h 7 Nov 2014 13:56:29 -0000 1.28
+++ usbdi_util.h 6 Dec 2014 13:01:02 -0000
@@ -49,12 +49,6 @@ usbd_status usbd_get_hub_ss_descriptor(s
usb_hub_ss_descriptor_t *, uint8_t);
struct usb_hid_descriptor *usbd_get_hid_descriptor(struct usbd_device *,
usb_interface_descriptor_t *);
-usbd_status usbd_get_report(struct usbd_device *, int, int, int, void *,
- int);
-usbd_status usbd_set_report(struct usbd_device *, int, int, int, void *,
- int);
-usbd_status usbd_set_report_async(struct usbd_device *, int, int, int,
- void *, int);
usbd_status usbd_set_idle(struct usbd_device *, int, int, int);
usbd_status usbd_get_report_descriptor(struct usbd_device *, int, void *,
int);
