In preparation for unconditionally passing the struct timer_list pointer to all timer callbacks, switch to using the new timer_setup() and from_timer() to pass the timer pointer explicitly.
Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: "Bryan O'Donoghue" pure.logic@nexus-software.ie Cc: Johan Hovold johan@kernel.org Cc: Alex Elder elder@kernel.org Cc: greybus-dev@lists.linaro.org Cc: devel@driverdev.osuosl.org Signed-off-by: Kees Cook keescook@chromium.org --- v2: Added back "get" in timer code, thanks to Bryan. :) --- drivers/staging/greybus/loopback.c | 19 +++++++++---------- drivers/staging/greybus/operation.c | 7 +++---- 2 files changed, 12 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c index 08e255884206..1c0bafeb7ea5 100644 --- a/drivers/staging/greybus/loopback.c +++ b/drivers/staging/greybus/loopback.c @@ -572,16 +572,16 @@ static void gb_loopback_async_operation_work(struct work_struct *work) gb_loopback_async_operation_put(op_async); }
-static void gb_loopback_async_operation_timeout(unsigned long data) +static void gb_loopback_async_operation_timeout(struct timer_list *t) { - struct gb_loopback_async_operation *op_async; - u16 id = data; + struct gb_loopback_async_operation *op_async = + from_timer(op_async, t, timer); + unsigned long flags; + + spin_lock_irqsave(&gb_dev.lock, flags); + gb_loopback_async_operation_get(op_async); + spin_unlock_irqrestore(&gb_dev.lock, flags);
- op_async = gb_loopback_operation_find(id); - if (!op_async) { - pr_err("operation %d not found - time out ?\n", id); - return; - } schedule_work(&op_async->work); }
@@ -631,8 +631,7 @@ static int gb_loopback_async_operation(struct gb_loopback *gb, int type, if (ret) goto error;
- setup_timer(&op_async->timer, gb_loopback_async_operation_timeout, - (unsigned long)operation->id); + timer_setup(&op_async->timer, gb_loopback_async_operation_timeout, 0); op_async->timer.expires = jiffies + gb->jiffy_timeout; add_timer(&op_async->timer);
diff --git a/drivers/staging/greybus/operation.c b/drivers/staging/greybus/operation.c index 3023012808d9..ee4ba3f23bef 100644 --- a/drivers/staging/greybus/operation.c +++ b/drivers/staging/greybus/operation.c @@ -294,9 +294,9 @@ static void gb_operation_work(struct work_struct *work) gb_operation_put(operation); }
-static void gb_operation_timeout(unsigned long arg) +static void gb_operation_timeout(struct timer_list *t) { - struct gb_operation *operation = (void *)arg; + struct gb_operation *operation = from_timer(operation, t, timer);
if (gb_operation_result_set(operation, -ETIMEDOUT)) { /* @@ -541,8 +541,7 @@ gb_operation_create_common(struct gb_connection *connection, u8 type, goto err_request; }
- setup_timer(&operation->timer, gb_operation_timeout, - (unsigned long)operation); + timer_setup(&operation->timer, gb_operation_timeout, 0); }
operation->flags = op_flags;
On 24/10/17 15:49, Kees Cook wrote:
In preparation for unconditionally passing the struct timer_list pointer to all timer callbacks, switch to using the new timer_setup() and from_timer() to pass the timer pointer explicitly.
Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: "Bryan O'Donoghue" pure.logic@nexus-software.ie Cc: Johan Hovold johan@kernel.org Cc: Alex Elder elder@kernel.org Cc: greybus-dev@lists.linaro.org Cc: devel@driverdev.osuosl.org Signed-off-by: Kees Cook keescook@chromium.org
v2: Added back "get" in timer code, thanks to Bryan. :)
drivers/staging/greybus/loopback.c | 19 +++++++++---------- drivers/staging/greybus/operation.c | 7 +++---- 2 files changed, 12 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c index 08e255884206..1c0bafeb7ea5 100644 --- a/drivers/staging/greybus/loopback.c +++ b/drivers/staging/greybus/loopback.c @@ -572,16 +572,16 @@ static void gb_loopback_async_operation_work(struct work_struct *work) gb_loopback_async_operation_put(op_async); } -static void gb_loopback_async_operation_timeout(unsigned long data) +static void gb_loopback_async_operation_timeout(struct timer_list *t) {
- struct gb_loopback_async_operation *op_async;
- u16 id = data;
- struct gb_loopback_async_operation *op_async =
from_timer(op_async, t, timer);- unsigned long flags;
- spin_lock_irqsave(&gb_dev.lock, flags);
- gb_loopback_async_operation_get(op_async);
- spin_unlock_irqrestore(&gb_dev.lock, flags);
Damnit I'm just wrong (I hate that).
The pointer can already have gone away by the time the timer runs - my bad...
see attached for update - with my Signed-off added.
--- bod
On Tue, Oct 24, 2017 at 04:54:59PM +0100, Bryan O'Donoghue wrote:
On 24/10/17 15:49, Kees Cook wrote:
In preparation for unconditionally passing the struct timer_list pointer to all timer callbacks, switch to using the new timer_setup() and from_timer() to pass the timer pointer explicitly.
Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: "Bryan O'Donoghue" pure.logic@nexus-software.ie Cc: Johan Hovold johan@kernel.org Cc: Alex Elder elder@kernel.org Cc: greybus-dev@lists.linaro.org Cc: devel@driverdev.osuosl.org Signed-off-by: Kees Cook keescook@chromium.org
v2: Added back "get" in timer code, thanks to Bryan. :)
drivers/staging/greybus/loopback.c | 19 +++++++++---------- drivers/staging/greybus/operation.c | 7 +++---- 2 files changed, 12 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c index 08e255884206..1c0bafeb7ea5 100644 --- a/drivers/staging/greybus/loopback.c +++ b/drivers/staging/greybus/loopback.c @@ -572,16 +572,16 @@ static void gb_loopback_async_operation_work(struct work_struct *work) gb_loopback_async_operation_put(op_async); } -static void gb_loopback_async_operation_timeout(unsigned long data) +static void gb_loopback_async_operation_timeout(struct timer_list *t) {
- struct gb_loopback_async_operation *op_async;
- u16 id = data;
- struct gb_loopback_async_operation *op_async =
from_timer(op_async, t, timer);- unsigned long flags;
- spin_lock_irqsave(&gb_dev.lock, flags);
- gb_loopback_async_operation_get(op_async);
- spin_unlock_irqrestore(&gb_dev.lock, flags);
Damnit I'm just wrong (I hate that).
The pointer can already have gone away by the time the timer runs - my bad...
Hmm. Then something is really broken in this driver, you obviously must never free the async operation which contains the timer while the timer is active.
see attached for update - with my Signed-off added.
The right thing to do here is to respin your patch from last year which converts the loopback driver to use the timeout handling in greybus core. Otherwise, I'm afraid you're not addressing the underlying bug.
Either way, Kees, please submit the operation.c conversion separately from the loopback one, as the latter is non-trivial.
Thanks, Johan
On 30/10/17 11:32, Johan Hovold wrote:
The right thing to do here is to respin your patch from last year which converts the loopback driver to use the timeout handling in greybus core.
Actually I wasn't clear if you wanted to to that yourself aswell as the rest if it.
But sure I can do that conversion, it's on my list.
On Mon, Oct 30, 2017 at 11:35:50AM +0000, Bryan O'Donoghue wrote:
On 30/10/17 11:32, Johan Hovold wrote:
The right thing to do here is to respin your patch from last year which converts the loopback driver to use the timeout handling in greybus core.
Actually I wasn't clear if you wanted to to that yourself aswell as the rest if it.
But sure I can do that conversion, it's on my list.
IIRC it was basically done. Just some odd locking that could now also be removed.
Thanks, Johan
On 30/10/17 11:38, Johan Hovold wrote:
On Mon, Oct 30, 2017 at 11:35:50AM +0000, Bryan O'Donoghue wrote:
On 30/10/17 11:32, Johan Hovold wrote:
The right thing to do here is to respin your patch from last year which converts the loopback driver to use the timeout handling in greybus core.
Actually I wasn't clear if you wanted to to that yourself aswell as the rest if it.
But sure I can do that conversion, it's on my list.
IIRC it was basically done. Just some odd locking that could now also be removed.
Thanks, Johan
I think once Kees' change is applied to operation.c and we convert the async stuff to operation.c's callbacks there ought to be no use of timers, linked lists of operations.
I'll probably need at least a day to look at that, so it'll be the weekend before I can really allocate time.
--- bod
On Mon, Oct 30, 2017 at 11:44:22AM +0000, Bryan O'Donoghue wrote:
On 30/10/17 11:38, Johan Hovold wrote:
On Mon, Oct 30, 2017 at 11:35:50AM +0000, Bryan O'Donoghue wrote:
On 30/10/17 11:32, Johan Hovold wrote:
The right thing to do here is to respin your patch from last year which converts the loopback driver to use the timeout handling in greybus core.
Actually I wasn't clear if you wanted to to that yourself aswell as the rest if it.
But sure I can do that conversion, it's on my list.
IIRC it was basically done. Just some odd locking that could now also be removed.
Thanks, Johan
I think once Kees' change is applied to operation.c and we convert the async stuff to operation.c's callbacks there ought to be no use of timers, linked lists of operations.
That's correct.
I'll probably need at least a day to look at that, so it'll be the weekend before I can really allocate time.
Cool. I'm quite sure I just rebased your loopback conversion patch on my core timeout handling and used that to test the core implementation, so it should be straight forward.
Thanks, Johan
On Mon, Oct 30, 2017 at 4:48 AM, Johan Hovold johan@kernel.org wrote:
On Mon, Oct 30, 2017 at 11:44:22AM +0000, Bryan O'Donoghue wrote:
On 30/10/17 11:38, Johan Hovold wrote:
On Mon, Oct 30, 2017 at 11:35:50AM +0000, Bryan O'Donoghue wrote:
On 30/10/17 11:32, Johan Hovold wrote:
The right thing to do here is to respin your patch from last year which converts the loopback driver to use the timeout handling in greybus core.
Actually I wasn't clear if you wanted to to that yourself aswell as the rest if it.
But sure I can do that conversion, it's on my list.
IIRC it was basically done. Just some odd locking that could now also be removed.
Thanks, Johan
I think once Kees' change is applied to operation.c and we convert the async stuff to operation.c's callbacks there ought to be no use of timers, linked lists of operations.
That's correct.
I'll probably need at least a day to look at that, so it'll be the weekend before I can really allocate time.
Cool. I'm quite sure I just rebased your loopback conversion patch on my core timeout handling and used that to test the core implementation, so it should be straight forward.
Hi,
I seem to have lost the thread of conversation a bit. What exactly remains that I should be doing here for timer conversions? (It sounded like it was already partially handled already?)
-Kees
Just pair the patch down to operation.c.
There's a separate change to loopback.c an old patch ARAIR that will subtract use of the timer from loopback.c so you can skip that bit.
On 30 October 2017 9:37:37 p.m. GMT+00:00, Kees Cook keescook@chromium.org wrote:
On Mon, Oct 30, 2017 at 4:48 AM, Johan Hovold johan@kernel.org wrote:
On Mon, Oct 30, 2017 at 11:44:22AM +0000, Bryan O'Donoghue wrote:
On 30/10/17 11:38, Johan Hovold wrote:
On Mon, Oct 30, 2017 at 11:35:50AM +0000, Bryan O'Donoghue wrote:
On 30/10/17 11:32, Johan Hovold wrote:
The right thing to do here is to respin your patch from last
year which
converts the loopback driver to use the timeout handling in
greybus
core.
Actually I wasn't clear if you wanted to to that yourself aswell
as the
rest if it.
But sure I can do that conversion, it's on my list.
IIRC it was basically done. Just some odd locking that could now
also be
removed.
Thanks, Johan
I think once Kees' change is applied to operation.c and we convert
the
async stuff to operation.c's callbacks there ought to be no use of timers, linked lists of operations.
That's correct.
I'll probably need at least a day to look at that, so it'll be the weekend before I can really allocate time.
Cool. I'm quite sure I just rebased your loopback conversion patch on
my
core timeout handling and used that to test the core implementation,
so
it should be straight forward.
Hi,
I seem to have lost the thread of conversation a bit. What exactly remains that I should be doing here for timer conversions? (It sounded like it was already partially handled already?)
-Kees
-- Kees Cook Pixel Security
On 30 October 2017 9:37:37 p.m. GMT+00:00, Kees Cook keescook@chromium.org wrote:
On Mon, Oct 30, 2017 at 4:48 AM, Johan Hovold johan@kernel.org wrote:
On Mon, Oct 30, 2017 at 11:44:22AM +0000, Bryan O'Donoghue wrote:
On 30/10/17 11:38, Johan Hovold wrote:
On Mon, Oct 30, 2017 at 11:35:50AM +0000, Bryan O'Donoghue wrote:
On 30/10/17 11:32, Johan Hovold wrote:
The right thing to do here is to respin your patch from last
year which
converts the loopback driver to use the timeout handling in
greybus
core.
Actually I wasn't clear if you wanted to to that yourself aswell
as the
rest if it.
But sure I can do that conversion, it's on my list.
IIRC it was basically done. Just some odd locking that could now
also be
removed.
Thanks, Johan
I think once Kees' change is applied to operation.c and we convert
the
async stuff to operation.c's callbacks there ought to be no use of timers, linked lists of operations.
That's correct.
I'll probably need at least a day to look at that, so it'll be the weekend before I can really allocate time.
Cool. I'm quite sure I just rebased your loopback conversion patch on
my
core timeout handling and used that to test the core implementation,
so
it should be straight forward.
Hi,
I seem to have lost the thread of conversation a bit. What exactly remains that I should be doing here for timer conversions? (It sounded like it was already partially handled already?)
-Kees
Trying again without top posting in html :(
Just pair the patch down to operation.c.
There's a separate change to loopback.c an old patch ARAIR that will subtract use of the timer from loopback.c so you can skip that bit.
On Mon, Oct 30, 2017 at 5:01 PM, pure.logic@nexus-software.ie wrote:
On 30 October 2017 9:37:37 p.m. GMT+00:00, Kees Cook keescook@chromium.org wrote:
On Mon, Oct 30, 2017 at 4:48 AM, Johan Hovold johan@kernel.org wrote:
On Mon, Oct 30, 2017 at 11:44:22AM +0000, Bryan O'Donoghue wrote:
On 30/10/17 11:38, Johan Hovold wrote:
On Mon, Oct 30, 2017 at 11:35:50AM +0000, Bryan O'Donoghue wrote:
On 30/10/17 11:32, Johan Hovold wrote: > The right thing to do here is to respin your patch from last
year which
> converts the loopback driver to use the timeout handling in
greybus
> core.
Actually I wasn't clear if you wanted to to that yourself aswell
as the
rest if it.
But sure I can do that conversion, it's on my list.
IIRC it was basically done. Just some odd locking that could now
also be
removed.
Thanks, Johan
I think once Kees' change is applied to operation.c and we convert
the
async stuff to operation.c's callbacks there ought to be no use of timers, linked lists of operations.
That's correct.
I'll probably need at least a day to look at that, so it'll be the weekend before I can really allocate time.
Cool. I'm quite sure I just rebased your loopback conversion patch on
my
core timeout handling and used that to test the core implementation,
so
it should be straight forward.
Hi,
I seem to have lost the thread of conversation a bit. What exactly remains that I should be doing here for timer conversions? (It sounded like it was already partially handled already?)
-Kees
Trying again without top posting in html :(
Just pair the patch down to operation.c.
There's a separate change to loopback.c an old patch ARAIR that will subtract use of the timer from loopback.c so you can skip that bit.
Okay, cool. Since the operation.c change is trivial, I'll include it in the giant tree-wide patch that will (hopefully) land in -rc1.
Thanks!
-Kees
On Mon, Oct 30, 2017 at 5:05 PM, Kees Cook keescook@chromium.org wrote:
On Mon, Oct 30, 2017 at 5:01 PM, pure.logic@nexus-software.ie wrote:
There's a separate change to loopback.c an old patch ARAIR that will subtract use of the timer from loopback.c so you can skip that bit.
Okay, cool. Since the operation.c change is trivial, I'll include it in the giant tree-wide patch that will (hopefully) land in -rc1.
I forgot to ask: will the patch for loopback.c that removes the timers get posted in the next couple days? I just want to make sure the timer conversions don't get blocked behind this.
Thanks!
-Kees
On 03/11/17 20:21, Kees Cook wrote:
On Mon, Oct 30, 2017 at 5:05 PM, Kees Cook keescook@chromium.org wrote:
On Mon, Oct 30, 2017 at 5:01 PM, pure.logic@nexus-software.ie wrote:
There's a separate change to loopback.c an old patch ARAIR that will subtract use of the timer from loopback.c so you can skip that bit.
Okay, cool. Since the operation.c change is trivial, I'll include it in the giant tree-wide patch that will (hopefully) land in -rc1.
I forgot to ask: will the patch for loopback.c that removes the timers get posted in the next couple days? I just want to make sure the timer conversions don't get blocked behind this.
Yep.
I should get that out tomorrow at some stage.
On Fri, Nov 3, 2017 at 2:49 PM, Bryan O'Donoghue bryan.odonoghue@linaro.org wrote:
On 03/11/17 20:21, Kees Cook wrote:
On Mon, Oct 30, 2017 at 5:05 PM, Kees Cook keescook@chromium.org wrote:
On Mon, Oct 30, 2017 at 5:01 PM, pure.logic@nexus-software.ie wrote:
There's a separate change to loopback.c an old patch ARAIR that will subtract use of the timer from loopback.c so you can skip that bit.
Okay, cool. Since the operation.c change is trivial, I'll include it in the giant tree-wide patch that will (hopefully) land in -rc1.
I forgot to ask: will the patch for loopback.c that removes the timers get posted in the next couple days? I just want to make sure the timer conversions don't get blocked behind this.
Yep.
I should get that out tomorrow at some stage.
Awesome, thanks very much!
-Kees
[ Resend to Bryan's nexus-software address ]
On Tue, Oct 24, 2017 at 04:54:59PM +0100, Bryan O'Donoghue wrote:
On 24/10/17 15:49, Kees Cook wrote:
In preparation for unconditionally passing the struct timer_list pointer to all timer callbacks, switch to using the new timer_setup() and from_timer() to pass the timer pointer explicitly.
Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: "Bryan O'Donoghue" pure.logic@nexus-software.ie Cc: Johan Hovold johan@kernel.org Cc: Alex Elder elder@kernel.org Cc: greybus-dev@lists.linaro.org Cc: devel@driverdev.osuosl.org Signed-off-by: Kees Cook keescook@chromium.org
v2: Added back "get" in timer code, thanks to Bryan. :)
drivers/staging/greybus/loopback.c | 19 +++++++++---------- drivers/staging/greybus/operation.c | 7 +++---- 2 files changed, 12 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c index 08e255884206..1c0bafeb7ea5 100644 --- a/drivers/staging/greybus/loopback.c +++ b/drivers/staging/greybus/loopback.c @@ -572,16 +572,16 @@ static void gb_loopback_async_operation_work(struct work_struct *work) gb_loopback_async_operation_put(op_async); } -static void gb_loopback_async_operation_timeout(unsigned long data) +static void gb_loopback_async_operation_timeout(struct timer_list *t) {
- struct gb_loopback_async_operation *op_async;
- u16 id = data;
- struct gb_loopback_async_operation *op_async =
from_timer(op_async, t, timer);- unsigned long flags;
- spin_lock_irqsave(&gb_dev.lock, flags);
- gb_loopback_async_operation_get(op_async);
- spin_unlock_irqrestore(&gb_dev.lock, flags);
Damnit I'm just wrong (I hate that).
The pointer can already have gone away by the time the timer runs - my bad...
Hmm. Then something is really broken in this driver, you obviously must never free the async operation which contains the timer while the timer is active.
see attached for update - with my Signed-off added.
The right thing to do here is to respin your patch from last year which converts the loopback driver to use the timeout handling in greybus core. Otherwise, I'm afraid you're not addressing the underlying bug.
Either way, Kees, please submit the operation.c conversion separately from the loopback one, as the latter is non-trivial.
Thanks, Johan
-
Bryan O'Donoghue -
Bryan O'Donoghue -
Johan Hovold -
Kees Cook -
pure.logic＠nexus-software.ie