> From: [EMAIL PROTECTED] (Oliver Neukum)
> To: <[EMAIL PROTECTED]>
> Date: Sat, 1 Dec 2001 14:33:17 +0100 (CET)
Good work, thanks - now to the mandatory nitpicking... :)
> unsigned char used; /* True if open */
> unsigned char bidir; /* interface is bidirectional
>*/
> + unsigned char readable; /* True if data was read */
> + unsigned char writeable; /* True if data was written
>*/
Why not int? You saved some data space, losts some code space...
For small number of instances, it's probably not warranted
for writeable flags. Drivers that have thousands of instances
use bitops.h for the same, but here I'd just make these ints.
The method of waiting that you employed looks totally broken
for me (I may be blind and missing something in the patch,
please explain me where I am mistaken). This is what I see:
> +static DECLARE_WAIT_QUEUE_HEAD(usblp_waiters); <==== not needed
> +static void usblp_write_callback(struct urb *urb)
> +{
> + struct usblp *usblp = urb->context;
> + usblp->writeable = 1;
> + wake_up_interruptible(&usblp->wait); <==== ok...
> +}
> xxx_write() {
> + DECLARE_WAITQUEUE(wait, current);
> while (writecount < count) {
> + add_wait_queue(&usblp_waiters, &wait); <=== add where?
> + for (;;) {
> + set_current_state(TASK_INTERRUPTIBLE);
> + timeout = schedule_timeout(timeout);
> }
> remove_wait_queue(&usblp_waiters, &wait);
> }
> }
I am surprised it works _at all_, because the callback wakes up
something entirely different from what is waiting for the write.
I suspect you did not try to do "cat > /dev/printer", but used
an application that selects(), and poll() was waken up by callback.
I suggest to remove usblp_waiters and do it just like you do in
the patch, but call add_wait_queue with usblp->wait instead:
printer_write() {
add_wait_queue(&usblp->wait, &wait);
for (;;) {
if (something) break;
schedule();
}
remove_wait_queue(&usblp->wait, &wait);
}
> +static void usblp_write_callback(struct urb *urb)
> +{
> + struct usblp *usblp = urb->context;
> + if (urb->status)
> + warn("usblp%d: nonzero write bulk status received: %d",
> + usblp->minor, urb->status);
> +}
This looks like an instant flood to me. Not only users will
not appreciate overflows in /var, but some hackers hate it too,
because if fills dmesg buffer.
> if (minor < 0 || minor >= USBLP_MINORS)
> return -ENODEV;
>
> + MOD_INC_USE_COUNT;
> lock_kernel();
This is an interesting idea. Aren't it the job of a framework
that manipulates fops to set counts right? The scheme that you
added seems to be oopsable with rmmod (running from cron :).
As I remember it foggily, one CPU does disconnect, INC, work, DEC,
then gets interrupted. Another CPU does rmmod and fills the
former code segment with garbage. First CPU does iret and oopses.
The solution is to do MOD_INC/DEC *outside* of the module,
using fops->owner or something like that.
> + remove_wait_queue(&usblp_waiters, &wait);
> + set_current_state(TASK_RUNNING);
I think this should be backwards for the sake of weirdos who
run preemptible kernel patches. But not a bit deal, and I am
not too sure.
> - if (usblp->readurb.status == -EINPROGRESS) {
> + if (!usblp->readable) {
This is the best part of the patch, it was on my todo list for
some time, since I broke usb_control_msg() :)
> usblp->readcount = 0;
> usblp->readurb.dev = usblp->dev;
> + usblp->readable = 0;
> usb_submit_urb(&usblp->readurb);
Not very tab friendly, eh :) Fine by me, but still...
Keep up the good work!
-- Pete
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
