Am Sonntag 02 Dezember 2001 01:14 schrieben Sie:
> > 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...
How so ? With the exception of alpha all architectures should be able to
access bytes. Could you please elaborate.
> The method of waiting that you employed looks totally broken
> for me (I may be blind and missing something in the patch,
You are right, it is. I feel quite embarased.
[..]
> 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.
It is verbatim from the existing driver. Nobody reported a problem.
> > 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 is the existing nonscheme.
> 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.
I did a patch to do this. It is somewhere in the archives.
IIRC module unloading on SMP on 2.4 is inherently racy conventionally.
Greg rejected the patch as too intrusive for a stable kernel. I agreed.
As a new modules scheme is proposed for 2.5 the problem is smaller, yet it is
worth unearthening the old patch.
>
> > + 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.
Why ? You lost me.
>
> > - 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...
This is KDE. I'll upgrade and hope for the best.
I did not test the patch. I have no USB printer. I was motivated by an
irrational hatred for sleep_on and relatives.
Regards
Oliver
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
