Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1578914 > unrolled thread
| Started by | Brian Norris <briannorris@chromium.org> |
|---|---|
| First post | 2017-02-11 02:50 +0100 |
| Last post | 2017-02-13 05:20 +0100 |
| Articles | 2 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH 2/3] Bluetooth: cmtp: fix possible might sleep error in cmtp_session Brian Norris <briannorris@chromium.org> - 2017-02-11 02:50 +0100
Re: [PATCH 2/3] Bluetooth: cmtp: fix possible might sleep error in cmtp_session jeffy <jeffy.chen@rock-chips.com> - 2017-02-13 05:20 +0100
| From | Brian Norris <briannorris@chromium.org> |
|---|---|
| Date | 2017-02-11 02:50 +0100 |
| Subject | Re: [PATCH 2/3] Bluetooth: cmtp: fix possible might sleep error in cmtp_session |
| Message-ID | <t9uZ3-2Mc-5@gated-at.bofh.it> |
Hi,
On Tue, Jan 24, 2017 at 12:07:50PM +0800, Jeffy Chen wrote:
> It looks like cmtp_session has same pattern as the issue reported in
> old rfcomm:
>
> while (1) {
> set_current_state(TASK_INTERRUPTIBLE);
> if (condition)
> break;
> // may call might_sleep here
> schedule();
> }
> __set_current_state(TASK_RUNNING);
>
> Which fixed at:
> dfb2fae Bluetooth: Fix nested sleeps
>
> So let's fix it at the same way, also follow the suggestion of:
> https://lwn.net/Articles/628628/
>
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
>
> net/bluetooth/cmtp/core.c | 21 ++++++++++++++-------
> 1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/net/bluetooth/cmtp/core.c b/net/bluetooth/cmtp/core.c
> index 9e59b66..6b03f2b 100644
> --- a/net/bluetooth/cmtp/core.c
> +++ b/net/bluetooth/cmtp/core.c
> @@ -280,16 +280,16 @@ static int cmtp_session(void *arg)
> struct cmtp_session *session = arg;
> struct sock *sk = session->sock->sk;
> struct sk_buff *skb;
> - wait_queue_t wait;
> + DEFINE_WAIT_FUNC(wait, woken_wake_function);
>
> BT_DBG("session %p", session);
>
> set_user_nice(current, -15);
>
> - init_waitqueue_entry(&wait, current);
> add_wait_queue(sk_sleep(sk), &wait);
> while (1) {
> - set_current_state(TASK_INTERRUPTIBLE);
> + /* Ensure session->terminate is updated */
> + smp_mb__before_atomic();
>
> if (atomic_read(&session->terminate))
> break;
> @@ -306,9 +306,8 @@ static int cmtp_session(void *arg)
>
> cmtp_process_transmit(session);
>
> - schedule();
> + wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
> }
> - __set_current_state(TASK_RUNNING);
> remove_wait_queue(sk_sleep(sk), &wait);
>
> down_write(&cmtp_session_sem);
> @@ -393,7 +392,11 @@ int cmtp_add_connection(struct cmtp_connadd_req *req, struct socket *sock)
> err = cmtp_attach_device(session);
> if (err < 0) {
> atomic_inc(&session->terminate);
> - wake_up_process(session->task);
> +
> + /* Ensure session->terminate is updated */
> + smp_mb__after_atomic();
> +
Same comment about the barrier.
> + wake_up_interruptible(sk_sleep(session->sock->sk));
> up_write(&cmtp_session_sem);
> return err;
> }
> @@ -431,7 +434,11 @@ int cmtp_del_connection(struct cmtp_conndel_req *req)
>
> /* Stop session thread */
> atomic_inc(&session->terminate);
> - wake_up_process(session->task);
> +
> + /* Ensure session->terminate is updated */
> + smp_mb__after_atomic();
And again.
But otherwise I think this looks OK, again with the caveat that I don't
know Bluetooth/CMTP that well:
Reviewed-by: Brian Norris <briannorris@chromium.org>
> +
> + wake_up_interruptible(sk_sleep(session->sock->sk));
> } else
> err = -ENOENT;
>
> --
> 2.1.4
>
>
[toc] | [next] | [standalone]
| From | jeffy <jeffy.chen@rock-chips.com> |
|---|---|
| Date | 2017-02-13 05:20 +0100 |
| Message-ID | <taghj-6YW-15@gated-at.bofh.it> |
| In reply to | #1578914 |
Hi brian,
On 02/11/2017 09:43 AM, Brian Norris wrote:
> Hi,
>
> On Tue, Jan 24, 2017 at 12:07:50PM +0800, Jeffy Chen wrote:
>> It looks like cmtp_session has same pattern as the issue reported in
>> old rfcomm:
>>
>> while (1) {
>> set_current_state(TASK_INTERRUPTIBLE);
>> if (condition)
>> break;
>> // may call might_sleep here
>> schedule();
>> }
>> __set_current_state(TASK_RUNNING);
>>
>> Which fixed at:
>> dfb2fae Bluetooth: Fix nested sleeps
>>
>> So let's fix it at the same way, also follow the suggestion of:
>> https://lwn.net/Articles/628628/
>>
>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>> ---
>>
>> net/bluetooth/cmtp/core.c | 21 ++++++++++++++-------
>> 1 file changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/net/bluetooth/cmtp/core.c b/net/bluetooth/cmtp/core.c
>> index 9e59b66..6b03f2b 100644
>> --- a/net/bluetooth/cmtp/core.c
>> +++ b/net/bluetooth/cmtp/core.c
>> @@ -280,16 +280,16 @@ static int cmtp_session(void *arg)
>> struct cmtp_session *session = arg;
>> struct sock *sk = session->sock->sk;
>> struct sk_buff *skb;
>> - wait_queue_t wait;
>> + DEFINE_WAIT_FUNC(wait, woken_wake_function);
>>
>> BT_DBG("session %p", session);
>>
>> set_user_nice(current, -15);
>>
>> - init_waitqueue_entry(&wait, current);
>> add_wait_queue(sk_sleep(sk), &wait);
>> while (1) {
>> - set_current_state(TASK_INTERRUPTIBLE);
>> + /* Ensure session->terminate is updated */
>> + smp_mb__before_atomic();
>>
>> if (atomic_read(&session->terminate))
>> break;
>> @@ -306,9 +306,8 @@ static int cmtp_session(void *arg)
>>
>> cmtp_process_transmit(session);
>>
>> - schedule();
>> + wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
>> }
>> - __set_current_state(TASK_RUNNING);
>> remove_wait_queue(sk_sleep(sk), &wait);
>>
>> down_write(&cmtp_session_sem);
>> @@ -393,7 +392,11 @@ int cmtp_add_connection(struct cmtp_connadd_req *req, struct socket *sock)
>> err = cmtp_attach_device(session);
>> if (err < 0) {
>> atomic_inc(&session->terminate);
>> - wake_up_process(session->task);
>> +
>> + /* Ensure session->terminate is updated */
>> + smp_mb__after_atomic();
>> +
> Same comment about the barrier.
Done, there are barriers in wake functions indeed, thanx!
>
>> + wake_up_interruptible(sk_sleep(session->sock->sk));
>> up_write(&cmtp_session_sem);
>> return err;
>> }
>> @@ -431,7 +434,11 @@ int cmtp_del_connection(struct cmtp_conndel_req *req)
>>
>> /* Stop session thread */
>> atomic_inc(&session->terminate);
>> - wake_up_process(session->task);
>> +
>> + /* Ensure session->terminate is updated */
>> + smp_mb__after_atomic();
> And again.
>
> But otherwise I think this looks OK, again with the caveat that I don't
> know Bluetooth/CMTP that well:
>
> Reviewed-by: Brian Norris <briannorris@chromium.org>
>
>> +
>> + wake_up_interruptible(sk_sleep(session->sock->sk));
>> } else
>> err = -ENOENT;
>>
>> --
>> 2.1.4
>>
>>
>
>
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web