On Thu, Apr 19, 2012 at 07:35:00PM +0200, Oliver Neukum wrote:
> Am Donnerstag, 19. April 2012, 17:29:48 schrieb Greg KH:
> > On Thu, Apr 19, 2012 at 01:11:44PM +0200, Oliver Neukum wrote:
> > > Hi,
> > >
> > > I've noticed quite some uses of del_timer_sync() in interrupt
> > > in the uwb code. The driver combines reference counting with
> > > timers, but doesn't grab a reference for timers it starts.
> > > I'd like to fix this by introducing references for timers.
> > > Any objections?
> >
> > What do you mean by "references for timers"? Are the timers that are
> > started cleaned up when needed? Or are you finding problems with it?
>
> It does things like this:
>
> /**
> * The entity that reads from the device notification/event channel has
> * detected an error.
> *
> * @rc: UWB Radio Controller
> * @error: Errno error code
> *
> */
> void uwb_rc_neh_error(struct uwb_rc *rc, int error)
> {
> struct uwb_rc_neh *neh;
> unsigned long flags;
>
> for (;;) {
> spin_lock_irqsave(&rc->neh_lock, flags);
> if (list_empty(&rc->neh_list)) {
> spin_unlock_irqrestore(&rc->neh_lock, flags);
> break;
> }
> neh = list_first_entry(&rc->neh_list, struct uwb_rc_neh, list_node);
> __uwb_rc_neh_rm(rc, neh);
> spin_unlock_irqrestore(&rc->neh_lock, flags);
>
> del_timer_sync(&neh->timer);
> uwb_rc_neh_cb(neh, NULL, error);
> }
> }
>
>
> It is called in interrupt, yet uses del_timer_sync()
> The timer drops a reference:
> static void uwb_rc_neh_cb(struct uwb_rc_neh *neh, struct uwb_rceb *rceb, size_t size)
> {
> (*neh->cb)(neh->rc, neh->arg, rceb, size);
> uwb_rc_neh_put(neh);
> }
>
> static void uwb_rc_neh_timer(unsigned long arg)
> {
> struct uwb_rc_neh *neh = (struct uwb_rc_neh *)arg;
> struct uwb_rc *rc = neh->rc;
> unsigned long flags;
>
> spin_lock_irqsave(&rc->neh_lock, flags);
> if (neh->context)
> __uwb_rc_neh_rm(rc, neh);
> else
> neh = NULL;
> spin_unlock_irqrestore(&rc->neh_lock, flags);
>
> if (neh)
> uwb_rc_neh_cb(neh, NULL, -ETIMEDOUT);
> }
>
> This looks quite fishy.
Ick, it does. Have you tried asking the authors of this code?
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
