On Fri, 22 Jun 2007 15:25:43 +0200, Oliver Neukum <[EMAIL PROTECTED]> wrote:
> > > > + /* XXX Use usb_setup_bulk_urb when available. Talk to Marcel. */
> > > > + kfree(urb->transfer_buffer);
> > > > + urb->transfer_buffer = NULL; /* Not refcounted, so to be
> > > > safe... */
> > > > + usb_free_urb(urb);
> > >
> > > If you want to be safe, do it without a race.
> >
> > What race?
>
> CPU A CPU B
> kfree(urb->transfer_buffer);
> if (urb->transfer_buffer)
> something();
> urb->transfer_buffer = NULL;
Where would the CPU B get that urb pointer? If the code is not buggy,
it can't. CPU A registers is the only place where it can be found.
> Generally, I would oppose such nullings. There's no way to do
> this cleanly.
We don't rely on this for normal logics. It's only there to catch
something going wrong and to remind about removal of the whole
thing when Marcel's attached buffers make it. I believe there's
even an XXX nearby.
> > > > static unsigned int usblp_poll(struct file *file, struct
> > > > poll_table_struct *wait)
> > > > {
> > > > + int ret;
> > > > + unsigned long flags;
> > >
> > > We are called with interrupts on.
> >
> > We discussed it a few times before. I guess my use of spin_lock in
> > callbacks wasn't enough to appease you, eh?
>
> No, because here it bothers me for another reason.
> It is just a few cycles. But it makes the code harder to maintain.
> The next one to touch this will look at it, scratch his head and
> use GFP_ATOMIC just in case or do some funny stuff just in
> case somebody might call this function with a spinlock held.
I'll have to think about this. The main issue is, the code travels
contexts over time, and thus context-specific code like spin_lock_irq
gets relocated into improper context. In this specific case, the
methods affected are not deeply nested away from a known API and
thus we do know that it will not happen... But it's only here.
I am trying not to create the pattern of coding which you advocate.
We keep fixing bugs where spin_lock_irqsave should've been used but
wasn't. Here's how it all started:
http://www.advogato.org/person/Zaitcev/diary.html?start=153
In RHEL-5 we began to use lockcheck and what cropped out of it is
in linux-2.6-lockdep-fixes.patch. Things like this:
+++ linux-2.6.17-mm6/include/net/sock.h
@@ -1025,9 +1025,10 @@ __sk_dst_reset(struct sock *sk)
static inline void
sk_dst_reset(struct sock *sk)
{
- write_lock(&sk->sk_dst_lock);
+ unsigned long flags;
+ write_lock_irqsave(&sk->sk_dst_lock, flags);
__sk_dst_reset(sk);
- write_unlock(&sk->sk_dst_lock);
+ write_unlock_irqrestore(&sk->sk_dst_lock, flags);
}
+++ a/drivers/net/forcedeth.c
@@ -2508,16 +2509,16 @@ static irqreturn_t nv_nic_irq_tx(int foo
if (!(events & np->irqmask))
break;
- spin_lock_irq(&np->lock);
+ spin_lock_irqsave(&np->lock, flags);
nv_tx_done(dev);
- spin_unlock_irq(&np->lock);
+ spin_unlock_irqrestore(&np->lock, flags);
The lockdep/lockcheck is not a panacea though. I remember approving patches
about exactly the same thing which came through normal debugging (e.g.
a customer actually hit the issues). Just cannot find them quickly now.
It may not be a big deal, but it's annoying how it keeps coming up.
The question is, if the disadvantages you mentioned (e.g. lost cycles)
are worth avoiding the issue. Maybe in the age of lockdep it's not
that big a deal anymore.
> > > > + if (mutex_lock_interruptible(&usblp->wmut)) {
> > > > + rv = -EINTR;
> > >
> > > Why not -ERESTARTSYS ?
> >
> > Hmm. Yes, the syscall is restartable at this point. I guess I inherited
> > it from the old driver, where the loop was unified, so it didn't try
> > to distinguish the cases when something was written or nothing was written,
> > and returned -EINTR always. On the other hand, what's the harm?
>
> No harm. I just wondered whether there's a reason which would have
> meant auditing other drivers for it.
I added this to TODO list, to investigate how this actually works.
I started answering you and understood that my ideas about the signal
paths are quite vague.
-- Pete
-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
