From: Jeff D. <jd...@ka...> - 2002-08-20 16:37:45
|
jam...@ho... said: > Ok now using list_entry, The list.h file is not includable from > _user.c as it includes prefetch which is very _user unfriendly also Except you're using list_entry on things which have separate next and prev fields instead of a list_head. It looks like _user doesn't care about the list aspect of umusb_device_t, so how about making it look like +typedef struct umusb_device_entry { + struct list_head list; + struct umusb_device dev; +} +struct umusb_device { + struct umusb_hub_device *hubdev; + struct umusb_bus *bus; + unsigned char *desc; + unsigned int desc_len; + int devnum; + int fd; +}; where struct umusb_device is in a separate header which can be included in both kernel and user code. _kern would do the list searching and pass &device->dev to _user. > list_entry is a very messy macro to dereference the start of a > structure, I just put the list entries at the start of the structures > so list_entry devolves to ((type *)(index)-(unsigned long)(0)) It is, but it's also very safe. What you have now has a hidden requirement that the next field not move. Using a list_head there will just eliminate that requirement, so things will just work if the structure is rearranged. > Ok, busses is in Websters though. Yup. > Which of the three versions would you like usb-uhci.h uhci.h ohci.h, Yuck. > I > would just leave it in a private header, like the other host > controllers do. We could submit a change to the usb core to > consolidate this to one file but I was not planning on bothering You might do that and run it by gregkh to see what he thinks. > It would be nice, But > A) I don't know how to get the interrupt (or > even how to describe what the interrupt is) Getting a file descriptor and calling um_request_irq is all you need to do. The underlying driver needs to support SIGIO. > B) I think the uhci > hardware uses a hardware timer and the umusb version uses a software > timer so I thought is was a good mapping. The real hardware polls in > a timer interrupt so why shouldn't the user mode version. Yeah, that's OK to start with, and it's what I generally do, but I don't hold to it religously if it causes unnecessary inefficiency or it can be done better in userspace. > I dont know, unlink_urb_user just returns the ioctl result? Hmmm, umusb_unlink_urb doesn't even check the return value of umusb_unlink_urb_user, which seems bad. usb_operations.unlink_urb seems to return an errno. I looked at uhci_unlink_urb and it's returning -ETHIS, -ETHAT, and -ETHEOTHERTHING, so it looks like unlink_urb_user should return -errno and unlink_urb should check that. My main cleanliness complaint right now is PIPE_CONTROL in umusb_complete_urb. I don't see any great ways of making it elegant, but getting rid of some of the indentation would help. You can change + if (udev->hubdev) { + ... + } to + if(udev->hubdev == NULL) + break; + ... + if (bmRType_bReq == + (RH_GET_STATUS | RH_OTHER | RH_CLASS)) { + ... + } to + if(bmRType_bReq != RH_GET_STATUS | RH_OTHER | RH_CLASS) + break; + ... + if ((portbit & udev->hubdev->cstatus) > 0) { + ... + } to + if((portbit & udev->hubdev->cstatus) <= 0) + break; + ... and get rid of a few levels of indentation. You can also combine those first two tests since the code between them doesn't appear to depend on the first: + devrequest *dr = (devrequest *) urb->setup_packet; + __u16 bmRType_bReq = dr->requesttype | dr->request << 8; + if((udev->hubdev == NULL) || + (bmRType_bReq != (RH_GET_STATUS | RH_OTHER | RH_CLASS)) + break; There's some duplicated code between PIPE_INTERRUPT and the end of the procedure. A flag saying whether urbctx should be freed would let PIPE_INTERRUPT return by falling through the end rather than explicitly returning. Jeff |