Grant Grundler wrote: > On Tue, Oct 17, 2006 at 08:33:06PM -0400, Adam Kropelin wrote: > ... >>> If someone needs either function to handle more than 24-bits, please >>> document why - point at a specification or specific USB hid device >>> - in comments in the code. >> >> Ummm. Ok. How many pages of the HID 1.11 spec shall I paste in? > > Just a URL or other reference to a section in the document.
It's not hard to find. First hit on Google search for "usb hid" yields the USB forum's page of HID documentation, from which is linked the 1.11 HID spec: <http://www.usb.org/developers/devclass_docs/HID1_11.pdf> There's no easy single section to reference. It's collected knowledge from reading the spec and understanding how reports are described. The thing to keep in mind is that there are very few hard limits in HID; it's design-by-committee so everyone must be accomodated. > one or two examples of a device that has sizeof(bit fields) >= 32 > would also be helpful. > Ie something I can test to verify the ops are working correctly. You should be able to unit test it in userspace with about 5 minutes work. >> I'm not sure what makes you think there's some magic 24 bit limit. > > ISTR it's a side effect of the shifts and the size of the register > on a 32-bit architecture. I meant a magic limit imposed by the spec. Your code is definitely limited to < 32 bits. > Maybe I'm not giving gcc enough credit > to handle 64-bit quantities correctly on a 32-bit arch. Hint: (1 << 32) == 0 on 32 bit. > If not, it should be easy enough to make the code work with u64. If you're explicit about the types it ought to work fine. > Care to take a whack at that? Just use 1ULL. > If the bit field is "naturally aligned" on a byte boundary, I hope > (but haven't verified) one could extract/implement a 32 bit field > with the current patch. No. Your masking effectively turns any 32 bit extract into 0. > But it's been over a year since I've looked at this code and > created this patch. So please work through it first before flaming me. If you're concerned about flames you should have put a little less attitude in your changelog. >> I see no limit specified, although in practice I've never seen >> anything larger than 32 bits. The size of a report field (in bits) >> is set by the Report Size global item, the data portion of which can >> be at most a 32 bit integer, thus giving a maximum field width of 4 >> Gbits. Clearly taking it that far would be insane, but limiting it >> to 24 bits arbitrarily is equally nuts. > > It's not arbitrary. It's a limit of the code (vs a spec). By that definition nothing is arbitrary. Of course your limit is arbitrary. You chose to write code that couldn't deal with anything over 24 bits (actually it's anything over 31 bits, but you already said you only targetted 24). That's an arbitrary choice. You could just as well have chosen some other number. >> I can send report descriptors of devices in my possession which have >> 32 bit wide fields. > > That would be helpful. Adding examples of input to the code makes it > _way_ easier to work through changes. I think you underestimate the complexity of report descriptors. The descriptors are not "input" to the extract() code per-se, but data describing the format of the various reports the device may generate. >> I'm the guy who patched the original extract() from >> "& ((1 << n)" to "& ((1ULL << n)" precisely because it didn't work on >> such devices, a bug which you've kindly recreated. > > 2.6.12-rc5 didn't have the "ULL". So what? You're not patching 2.6.12-rc5 here, you're patching 2.6.18-rc2. >> Please, revert this patch or fix it to handle 32 bit fields. > > Can you demonstrate it's broken for the USB devices you have? I can demonstrate it's broken by *reading* it. Yes, I could go sort through my pile of UPSes to find the model that reports LINEV using a 32 bit field, or I could just wait until this patch hits mainline and read the stream of "why does my LINEV read 0?" emails that start arriving. > ie show me the parameters to extract and/or implement that don't > work with the patch and tell me which device it is. extract("abcd", 0, 32) == 0 Write the code in userspace to prove it to yourself if you don't believe me. In fact, here. I did it for you. (x86 32 bit assumed; adjust to taste) > I'll take another look at it with that info. > > As willy mentioned, this patch works for the simple devices I have > on parisc and mips. I expect it to also work for PPC. > The previous code (with or without ULL) did not. And as I said in my other reply, that may well be true but does not justify introducing regressions on other platforms. --Adam -- #include <stdio.h> typedef unsigned long u32; typedef unsigned char u8; typedef unsigned long long __le64; #define get_unaligned(x) (*(x)) #define le32_to_cpu(x) (x) #define le64_to_cpu(x) (x) static u32 working(u8 *report, unsigned offset, unsigned n) { report += (offset >> 5) << 2; offset &= 31; return (le64_to_cpu(get_unaligned((__le64*)report)) >> offset) & ((1ULL << n) - 1); } static u32 broken(u8 *report, unsigned offset, unsigned n) { u32 x; report += offset >> 3; /* adjust byte index */ offset &= 8 - 1; x = get_unaligned((u32 *) report); x = le32_to_cpu(x); x = (x >> offset) & ((1 << n) - 1); return x; } int main(int argc, char* argv) { u32 x = broken("abcd", 0, 32); u32 y = working("abcd", 0, 32); printf("broken=0x%08x, working=0x%08x\n", x,y); return 0; } ------------------------------------------------------------------------- Using Tomcat but need to do more? Need to support web services, security? Get stuff done quickly with pre-integrated technology to make your job easier Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642 _______________________________________________ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
