Date: Mon, 18 Apr 2005 21:42:52 -0400 From: "Adam Kropelin" <akropel1@rochester.rr.com> To: "Mike Tancsa" <mike@sentex.net>, <freebsd-usb@freebsd.org> Subject: Re: ugen lockups with apcupsd Message-ID: <064001c54481$19ea1b90$03c8a8c0@kroptech.com> References: <6.2.1.2.0.20050418133919.03102d90@64.7.153.2>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format.
------=_NextPart_000_063D_01C5445F.92CA23B0
Content-Type: text/plain;
format=flowed;
charset="iso-8859-1";
reply-type=response
Content-Transfer-Encoding: 7bit
Mike Tancsa wrote:
> Hi,
> I have been trying out the apcupsd program with an APC
> RS-1500 on FreeBSD via the ugen interface. On my VIA box, the machine
> locks up
> solid within a short period of time after starting the daemon.
This is a known bug and is fixed by a patch I posted to apcupsd-users. The
BSD UHCI driver has a tendency to leave transfers on the queue when they
complete 'short' (i.e., with fewer bytes than expected). This happens often
in apcupsd-3.10.17 due to some poor coding on my part, but can also happen
on UPSes having broken firmware that sends fewer bytes than the report
descriptor claims. (These UPSes do not deadlock on Linux, so I think there
is still a BSD UHCI bug here, but luckily it can be worked around in
apcupsd.)
Patch against apcupsd-3.10.17 attached. Let me know if it solves the problem
for you.
--Adam
------=_NextPart_000_063D_01C5445F.92CA23B0
Content-Type: application/octet-stream;
name="apcupsd-freebsd-usb-lockup-fix-2.patch"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: attachment;
filename="apcupsd-freebsd-usb-lockup-fix-2.patch"
Index: bsd-usb.c=0A=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=0A=
RCS file: /cvsroot/apcupsd/apcupsd/src/drivers/usb/bsd/bsd-usb.c,v=0A=
retrieving revision 1.4=0A=
diff -u -r1.4 bsd-usb.c=0A=
--- bsd-usb.c 9 Nov 2004 01:51:54 -0000 1.4=0A=
+++ bsd-usb.c 18 Apr 2005 02:40:35 -0000=0A=
@@ -48,6 +48,7 @@=0A=
unsigned unit; /* units */=0A=
int data_type; /* data type */=0A=
hid_item_t item; /* HID item */=0A=
+ int report_len; /* Length of containing report */=0A=
} USB_INFO;=0A=
=0A=
=0A=
@@ -394,6 +395,8 @@=0A=
info->unit =3D item.unit;=0A=
info->data_type =3D known_info[i].data_type;=0A=
memcpy(&info->item, &item, sizeof(item));=0A=
+ info->report_len =3D hid_report_size( /* +1 for report =
id */=0A=
+ my_data->rdesc, item.kind, item.report_ID) + 1;=0A=
Dmsg3(200, "Got ci=3D%d, usage=3D0x%x (len=3D%d)\n",ci,=0A=
known_info[i].usage_code, item.report_size);=0A=
}=0A=
@@ -424,11 +427,38 @@=0A=
if (!UPS_HAS_CAP(ci) || !info)=0A=
return false; /* UPS does not have capability */=0A=
=0A=
+ /*=0A=
+ * Clear the destination buffer. In the case of a short transfer =
(see=0A=
+ * below) this will increase the likelihood of extracting the =
correct=0A=
+ * value in spite of the missing data.=0A=
+ */=0A=
+ memset(data, 0, sizeof(data));=0A=
+=0A=
/* Fetch the proper report */=0A=
- len =3D hidu_get_report(my_data->fd, &info->item, data);=0A=
+ len =3D hidu_get_report(my_data->fd, &info->item, data, =
info->report_len);=0A=
if (len =3D=3D -1)=0A=
return false;=0A=
=0A=
+ /*=0A=
+ * Some UPSes seem to have broken firmware that sends a different =
number=0A=
+ * of bytes (usually fewer) than the report descriptor specifies. On=0A=
+ * UHCI controllers under *BSD, this can lead to random lockups. To=0A=
+ * reduce the likelihood of a lockup, we adjust our expected length =
to=0A=
+ * match the actual as soon as a mismatch is detected, so future=0A=
+ * transfers will have the proper lengths from the outset. NOTE that=0A=
+ * the data returned may not be parsed properly (since the parsing =
is=0A=
+ * necessarily based on the report descriptor) but given that HID=0A=
+ * reports are in little endian byte order and we cleared the buffer=0A=
+ * above, chances are good that we will actually extract the right=0A=
+ * value in spite of the UPS's brokenness.=0A=
+ */=0A=
+ if (info->report_len !=3D len) {=0A=
+ Dmsg4(100, "Report length mismatch, fixing "=0A=
+ "(id=3D%d, ci=3D%d, expected=3D%d, actual=3D%d)",=0A=
+ info->item.report_ID, ci, info->report_len, len);=0A=
+ info->report_len =3D len;=0A=
+ }=0A=
+=0A=
/* data+1 skips the report tag byte */=0A=
value =3D hid_get_data(data+1, &info->item);=0A=
=0A=
@@ -798,7 +828,8 @@=0A=
{=0A=
info =3D my_data->info[ci]; /* point to our info =
structure */=0A=
=0A=
- if (hidu_get_report(my_data->fd, &info->item, rpt) < 1)=0A=
+ if (hidu_get_report(=0A=
+ my_data->fd, &info->item, rpt, info->report_len) < 1)=0A=
{=0A=
Dmsg1(000, "get_report for kill power function %s =
failed.\n", =0A=
name);=0A=
@@ -809,14 +840,15 @@=0A=
=0A=
hid_set_data(rpt+1, &info->item, value);=0A=
=0A=
- if (!hidu_set_report(my_data->fd, &info->item, rpt))=0A=
+ if (!hidu_set_report(my_data->fd, &info->item, rpt, =
info->report_len))=0A=
{=0A=
Dmsg1(000, "set_report for kill power function %s =
failed.\n", =0A=
name);=0A=
return false;=0A=
}=0A=
=0A=
- if (hidu_get_report(my_data->fd, &info->item, rpt) < 1)=0A=
+ if (hidu_get_report(=0A=
+ my_data->fd, &info->item, rpt, info->report_len) < 1)=0A=
{=0A=
Dmsg1(000, "get_report for kill power function %s =
failed.\n", =0A=
name);=0A=
Index: hidutils.c=0A=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=0A=
RCS file: /cvsroot/apcupsd/apcupsd/src/drivers/usb/bsd/hidutils.c,v=0A=
retrieving revision 1.2=0A=
diff -u -r1.2 hidutils.c=0A=
--- hidutils.c 9 Nov 2004 02:03:49 -0000 1.2=0A=
+++ hidutils.c 18 Apr 2005 02:40:35 -0000=0A=
@@ -259,43 +259,22 @@=0A=
=0A=
/*=0A=
* Fetch a report from a device given an fd for the device's control=0A=
- * endpoint, the populated item structure describing the report, and=0A=
- * a data buffer in which to store the result. Returns report length=0A=
- * (in bytes) on success and -1 on failure.=0A=
+ * endpoint, the populated item structure describing the report, a=0A=
+ * data buffer in which to store the result, and the report length.=0A=
+ * Returns actual report length (in bytes) on success and -1 on failure.=0A=
*/=0A=
-int hidu_get_report(int fd, hid_item_t* item, unsigned char* data)=0A=
+int hidu_get_report(int fd, hid_item_t* item, unsigned char* data, int =
len)=0A=
{=0A=
- int rc, len;=0A=
+ int rc;=0A=
struct usb_ctl_request req;=0A=
- unsigned char buf[100];=0A=
=0A=
Dmsg4(200, "get_report: id=3D0x%02x, kind=3D%d, length=3D%d =
pos=3D%d\n",=0A=
- item->report_ID, item->kind, item->report_size, item->pos);=0A=
-=0A=
-#if 0=0A=
- /*=0A=
- * Length is report size (in bits) rounded up to nearest=0A=
- * byte, plus one additional byte for the report tag.=0A=
- */=0A=
- len =3D (item->report_size+7)/8+1;=0A=
-#else=0A=
- /*=0A=
- * Some reports seem to be longer than the above calculation says.=0A=
- * Either APC has bogus length fields in their descriptors or=0A=
- * libusbhid is buggy. The FreeBSD kernel corrupts its memory when=0A=
- * this happens, resulting in a panic shortly thereafter. Work =
around=0A=
- * the problem by using a plenty large buffer and letting the =
transfer=0A=
- * return less than was requested.=0A=
- */=0A=
- len =3D sizeof(buf);=0A=
-#endif=0A=
-=0A=
- memset(buf, 0, sizeof(buf));=0A=
+ item->report_ID, item->kind, len, item->pos);=0A=
=0A=
req.ucr_flags =3D USBD_SHORT_XFER_OK;=0A=
req.ucr_actlen =3D 0;=0A=
req.ucr_addr =3D 0;=0A=
- req.ucr_data =3D buf;=0A=
+ req.ucr_data =3D data;=0A=
req.ucr_request.bmRequestType =3D UT_READ_CLASS_INTERFACE;=0A=
req.ucr_request.bRequest =3D UR_GET_REPORT;=0A=
USETW(req.ucr_request.wValue, ((item->kind+1) << 8) | =
item->report_ID);=0A=
@@ -308,7 +287,7 @@=0A=
rc =3D ioctl(fd, USB_DO_REQUEST, &req);=0A=
if (rc)=0A=
{=0A=
- Dmsg1(100, "Error getting report: %s\n", strerror(-rc));=0A=
+ Dmsg1(100, "Error getting report: %s\n", strerror(errno));=0A=
return -1;=0A=
}=0A=
=0A=
@@ -316,32 +295,25 @@=0A=
{=0A=
printf( "%02x: ", item->report_ID);=0A=
for (rc=3D0; rc<req.ucr_actlen; rc++)=0A=
- printf("%02x,", buf[rc]);=0A=
+ printf("%02x,", data[rc]);=0A=
printf("\n");=0A=
}=0A=
=0A=
- memcpy(data, buf, req.ucr_actlen);=0A=
return req.ucr_actlen;=0A=
}=0A=
=0A=
/*=0A=
* Send a report to the device given an fd for the device's control=0A=
- * endpoint, the populated item structure, and the data to send. =0A=
- * Returns true on success, false on failure.=0A=
+ * endpoint, the populated item structure, the data to send, and the=0A=
+ * report length. Returns true on success, false on failure.=0A=
*/=0A=
-int hidu_set_report(int fd, hid_item_t* item, unsigned char* data)=0A=
+int hidu_set_report(int fd, hid_item_t* item, unsigned char* data, int =
len)=0A=
{=0A=
- int rc, len;=0A=
+ int rc;=0A=
struct usb_ctl_request req;=0A=
=0A=
Dmsg4(200, "set_report: id=3D0x%02x, kind=3D%d, length=3D%d =
pos=3D%d\n",=0A=
- item->report_ID, item->kind, item->report_size, item->pos);=0A=
-=0A=
- /*=0A=
- * Length is report size (in bits) rounded up to nearest=0A=
- * byte, plus one additional byte for the report tag.=0A=
- */=0A=
- len =3D (item->report_size+7)/8+1;=0A=
+ item->report_ID, item->kind, len, item->pos);=0A=
=0A=
if (debug_level >=3D 300)=0A=
{=0A=
@@ -367,7 +339,7 @@=0A=
rc =3D ioctl(fd, USB_DO_REQUEST, &req);=0A=
if (rc)=0A=
{=0A=
- Dmsg2(100, "Error setting report: (%d) %s\n", rc, =
strerror(-rc));=0A=
+ Dmsg2(100, "Error setting report: (%d) %s\n", errno, =
strerror(errno));=0A=
return 0;=0A=
}=0A=
=0A=
@@ -391,7 +363,7 @@=0A=
rc =3D ioctl(fd, USB_GET_STRING_DESC, &sd);=0A=
if (rc)=0A=
{=0A=
- Dmsg1(100, "Error fetching string descriptor: %s\n", =
strerror(-rc));=0A=
+ Dmsg1(100, "Error fetching string descriptor: %s\n", =
strerror(errno));=0A=
return NULL;=0A=
}=0A=
=0A=
Index: hidutils.h=0A=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=0A=
RCS file: /cvsroot/apcupsd/apcupsd/src/drivers/usb/bsd/hidutils.h,v=0A=
retrieving revision 1.1=0A=
diff -u -r1.1 hidutils.h=0A=
--- hidutils.h 7 Nov 2004 23:46:55 -0000 1.1=0A=
+++ hidutils.h 18 Apr 2005 02:40:35 -0000=0A=
@@ -34,18 +34,18 @@=0A=
=0A=
/*=0A=
* Fetch a report from a device given an fd for the device's control=0A=
- * endpoint, the populated item structure describing the report, and=0A=
- * a data buffer in which to store the result. Returns report length=0A=
- * (in bytes) on success and -1 on failure.=0A=
+ * endpoint, the populated item structure describing the report, a=0A=
+ * data buffer in which to store the result, and the report length.=0A=
+ * Returns actual report length (in bytes) on success and -1 on failure.=0A=
*/=0A=
-int hidu_get_report(int fd, hid_item_t* item, unsigned char* data);=0A=
+int hidu_get_report(int fd, hid_item_t* item, unsigned char* data, int =
len);=0A=
=0A=
/*=0A=
* Send a report to the device given an fd for the device's control=0A=
- * endpoint, the populated item structure, and the data to send. =0A=
- * Returns true on success, false on failure.=0A=
+ * endpoint, the populated item structure, the data to send, and the=0A=
+ * report length. Returns true on success, false on failure.=0A=
*/=0A=
-int hidu_set_report(int fd, hid_item_t* item, unsigned char* data);=0A=
+int hidu_set_report(int fd, hid_item_t* item, unsigned char* data, int =
len);=0A=
=0A=
/*=0A=
* Fetch a string descriptor from the device given an fd for the=0A=
------=_NextPart_000_063D_01C5445F.92CA23B0--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?064001c54481$19ea1b90$03c8a8c0>