On Fri, 20 Dec 2013, David Cohen wrote:
> On Fri, Dec 20, 2013 at 11:55:53AM -0600, Thomas Pugliese wrote:
> > Only acquire rc->uwb_dev.mutex in the error case in
> > uwbd_evt_handle_rc_bp_slot_change. This fixes a bug where establishing
> > a reservation on a new channel will fail if we were unable to establish
> > a reservation on the previous channel due to DRP conflict.
> >
> > If rc->uwb_dev.mutex is acquired in the non-error case when the uwb
> > system is attempting to start beaconing, it will block because the start
> > beaconing code is holding this mutex. This prevents any other
> > notifications from the URC from being processed. In particular, the
> > DRP_AVAILABILITY notification will not be processed during the start
> > beaconing process which can result in a failure to establish a
> > reservation. It is safe to not hold the mutex in the non-error
> > case since the only other place rc->uwb_dev.beacon_slot is accessed is
> > in the same worker thread that uwbd_evt_handle_rc_bp_slot_change
> > executes in.
> >
> > Signed-off-by: Thomas Pugliese <thomas.pugliese@xxxxxxxxx>
> > ---
> > drivers/uwb/beacon.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/uwb/beacon.c b/drivers/uwb/beacon.c
> > index 4190f29..92407e8 100644
> > --- a/drivers/uwb/beacon.c
> > +++ b/drivers/uwb/beacon.c
> > @@ -516,13 +516,13 @@ int uwbd_evt_handle_rc_bp_slot_change(struct uwb_event *evt)
> > }
> > bpsc = container_of(evt->notif.rceb, struct uwb_rc_evt_bp_slot_change, rceb);
> >
> > - mutex_lock(&rc->uwb_dev.mutex);
> > if (uwb_rc_evt_bp_slot_change_no_slot(bpsc)) {
> > + mutex_lock(&rc->uwb_dev.mutex);
> > dev_err(dev, "stopped beaconing: No free slots in BP\n");
>
> It could be just nitpicking, but I think it's a waste of resource to
> unnecessarily print to console while holding a lock.
> If you move this dev_err() to after mutex_unlock() you'd achieve the
> same result with a cheaper lock.
>
That's a fair point. Might as well lock only what is absolutely
necessary.
> > rc->beaconing = -1;
>
> Although I'm not sure how useful it is to protect this single line of
> code.
>
I'm not ready to get rid of the lock altogether in this function. There
are other users of the beaconing flag that don't expect the value to
change out from under them while holding the lock.
> Br, David Cohen
>
> > + mutex_unlock(&rc->uwb_dev.mutex);
> > } else
> > rc->uwb_dev.beacon_slot = uwb_rc_evt_bp_slot_change_slot_num(bpsc);
> > - mutex_unlock(&rc->uwb_dev.mutex);
> >
> > return 0;
> > }
> > --
I'll resend this with the lock surrounding just the variable assignment.
Tom
--
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
