Hi Dave,
> There's logic in __rfcomm_dlc_close:
> rfcomm_dlc_lock(d);
> d->state = BT_CLOSED;
> d->state_changed(d, err);
> rfcomm_dlc_unlock(d);
>
> In rfcomm_dev_state_change, it's possible that rfcomm_dev_put try to
> take the
> dlc lock, then we will deadlock.
>
> Here fixed it by unlock dlc before rfcomm_dev_get in
> rfcomm_dev_state_change.
>
> why not unlock just before rfcomm_dev_put? it's because there's
> another problem.
> rfcomm_dev_get/rfcomm_dev_del will take rfcomm_dev_lock, but in
> rfcomm_dev_add
> the lock order is : rfcomm_dev_lock --> dlc lock
>
> so I unlock dlc before the taken of rfcomm_dev_lock.
>
> Actually it's a regression caused by commit
> 1905f6c736cb618e07eca0c96e60e3c024023428, the dlc state_change could
> be two
> callbacks : rfcomm_sk_state_change and rfcomm_dev_state_change. I
> missed the rfcomm_sk_state_change that time.
>
> Thanks Arjan van de Ven <[email protected]> for the effort in
> commit
> 4c8411f8c115def968820a4df6658ccfd55d7f1a
> but he missed the rfcomm_dev_state_change lock issue.
>
> Signed-off-by: Dave Young <[email protected]>
>
> ---
> net/bluetooth/rfcomm/tty.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff -upr linux/net/bluetooth/rfcomm/tty.c linux.new/net/bluetooth/
> rfcomm/tty.c
> --- linux/net/bluetooth/rfcomm/tty.c 2008-05-30 15:46:33.000000000
> +0800
> +++ linux.new/net/bluetooth/rfcomm/tty.c 2008-05-30
> 17:08:30.000000000 +0800
> @@ -566,11 +566,22 @@ static void rfcomm_dev_state_change(stru
> if (dlc->state == BT_CLOSED) {
> if (!dev->tty) {
> if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags)) {
> - if (rfcomm_dev_get(dev->id) == NULL)
> + /* Drop DLC lock here to avoid deadlock
> + * 1. rfcomm_dev_get will take rfcomm_dev_lock
> + * but in rfcomm_dev_add there's lock order:
> + * rfcomm_dev_lock -> dlc lock
> + * 2. rfcomm_dev_put will deaklock if it's
> + * the last reference
I meant this one :) s/deaklock/deadlock/
Regards
Marcel
On Sun, Jun 1, 2008 at 3:06 PM, Marcel Holtmann <[email protected]> wrote:
> Hi Dave,
>
>> There's logic in __rfcomm_dlc_close:
>> rfcomm_dlc_lock(d);
>> d->state = BT_CLOSED;
>> d->state_changed(d, err);
>> rfcomm_dlc_unlock(d);
>>
>> In rfcomm_dev_state_change, it's possible that rfcomm_dev_put try to take
>> the
>> dlc lock, then we will deadlock.
>>
>> Here fixed it by unlock dlc before rfcomm_dev_get in
>> rfcomm_dev_state_change.
>>
>> why not unlock just before rfcomm_dev_put? it's because there's another
>> problem.
>> rfcomm_dev_get/rfcomm_dev_del will take rfcomm_dev_lock, but in
>> rfcomm_dev_add
>> the lock order is : rfcomm_dev_lock --> dlc lock
>>
>> so I unlock dlc before the taken of rfcomm_dev_lock.
>>
>> Actually it's a regression caused by commit
>> 1905f6c736cb618e07eca0c96e60e3c024023428, the dlc state_change could be
>> two
>> callbacks : rfcomm_sk_state_change and rfcomm_dev_state_change. I missed
>> the rfcomm_sk_state_change that time.
>>
>> Thanks Arjan van de Ven <[email protected]> for the effort in commit
>> 4c8411f8c115def968820a4df6658ccfd55d7f1a
>> but he missed the rfcomm_dev_state_change lock issue.
>>
>> Signed-off-by: Dave Young <[email protected]>
>>
>> ---
>> net/bluetooth/rfcomm/tty.c | 13 ++++++++++++-
>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff -upr linux/net/bluetooth/rfcomm/tty.c
>> linux.new/net/bluetooth/rfcomm/tty.c
>> --- linux/net/bluetooth/rfcomm/tty.c 2008-05-30 15:46:33.000000000
>> +0800
>> +++ linux.new/net/bluetooth/rfcomm/tty.c 2008-05-30
>> 17:08:30.000000000 +0800
>> @@ -566,11 +566,22 @@ static void rfcomm_dev_state_change(stru
>> if (dlc->state == BT_CLOSED) {
>> if (!dev->tty) {
>> if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags)) {
>> - if (rfcomm_dev_get(dev->id) == NULL)
>> + /* Drop DLC lock here to avoid deadlock
>> + * 1. rfcomm_dev_get will take
>> rfcomm_dev_lock
>> + * but in rfcomm_dev_add there's lock
>> order:
>> + * rfcomm_dev_lock -> dlc lock
>> + * 2. rfcomm_dev_put will deaklock if it's
>> + * the last reference
>
> I meant this one :) s/deaklock/deadlock/
Fixed, thanks.