Hi Ulisses,
On Tue, Mar 20, 2012 at 04:56:11PM -0300, Ulisses Furquim wrote:
> Hi Andrei,
>
> On Tue, Mar 20, 2012 at 9:51 AM, Andrei Emeltchenko
> <Andrei.Emeltchenko.news@xxxxxxxxx> wrote:
> > From: Andrei Emeltchenko <andrei.emeltchenko@xxxxxxxxx>
> >
> > Simplify L2CAP timers logic. Previous logic was hard to understand.
> > Now we always hold(chan) when setting up timer and put(chan) only
> > if work pending and we successfully cancel delayed work.
> >
> > If delayed work is executing it will put(chan) itself.
>
> The description is a lot better, thanks. However, I don't see why this
> change is an improvement. The old code could be hard to read but then
> we need probably some comments to clarify it, just that IMHO.
Agree with you here.
After further investigation I think that current code is OK, Gustavo could
you revert the patch.
Best regards
Andrei Emeltchenko
> If you
> are solving a real bug I'd very much like to see an oops with a stack
> trace or a very good description or test case.
>
> > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@xxxxxxxxx>
> > ---
> > include/net/bluetooth/l2cap.h | 27 ++++++++++++++-------------
> > 1 files changed, 14 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> > index 9b242c6..e4b2fe7 100644
> > --- a/include/net/bluetooth/l2cap.h
> > +++ b/include/net/bluetooth/l2cap.h
> > @@ -621,29 +621,30 @@ static inline void l2cap_chan_unlock(struct l2cap_chan *chan)
> > mutex_unlock(&chan->lock);
> > }
> >
> > -static inline void l2cap_set_timer(struct l2cap_chan *chan,
> > - struct delayed_work *work, long timeout)
> > -{
> > - BT_DBG("chan %p state %s timeout %ld", chan,
> > - state_to_string(chan->state), timeout);
> > -
> > - if (!cancel_delayed_work(work))
> > - l2cap_chan_hold(chan);
> > - schedule_delayed_work(work, timeout);
> > -}
>
> Here the old code just checked if we could cancel the delayed work. If
> it returns 0 it means the work wasn't pending at all and then we need
> to hold(chan). If it was pending somehow we don't need to hold(chan)
> once again.
>
> > static inline bool l2cap_clear_timer(struct l2cap_chan *chan,
> > - struct delayed_work *work)
> > + struct delayed_work *work)
> > {
> > bool ret;
> >
> > - ret = cancel_delayed_work(work);
> > + ret = (delayed_work_pending(work) && cancel_delayed_work(work));
> > if (ret)
> > l2cap_chan_put(chan);
> >
> > return ret;
> > }
>
> The semantic here is the same as the old code, if I'm not missing
> anything. If we had a pending work and we could cancel it then we
> put(chan). Otherwise either the work wasn't pending and we don't need
> to put(chan) or the work was running and it'll put(chan) itself later.
> Moreover, just like Mat said we might be introducing a race here, we'd
> better check that.
>
> > +static inline void l2cap_set_timer(struct l2cap_chan *chan,
> > + struct delayed_work *work, long timeout)
> > +{
> > + BT_DBG("chan %p state %s timeout %ld", chan,
> > + state_to_string(chan->state), timeout);
> > +
> > + l2cap_clear_timer(chan, work);
> > +
> > + l2cap_chan_hold(chan);
> > + schedule_delayed_work(work, timeout);
> > +}
> > +
> > #define __set_chan_timer(c, t) l2cap_set_timer(c, &c->chan_timer, (t))
> > #define __clear_chan_timer(c) l2cap_clear_timer(c, &c->chan_timer)
> > #define __set_retrans_timer(c) l2cap_set_timer(c, &c->retrans_timer, \
> > --
> > 1.7.9.1
>
> These are just my thoughts but I don't have the final word if we merge
> this code or not. I just think we need to discuss more this kind of
> change or even document better why we are changing or solving a bug.
> In particular, code that is core to the stack needs more discussion
> and care as it may impact several things we don't anticipate when
> making a change.
>
> Regards,
>
> --
> Ulisses Furquim
> ProFUSION embedded systems
> http://profusion.mobi
> Mobile: +55 19 9250 0942
> Skype: ulissesffs
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
