staging: greybus: Convert timers to use timer_setup()
Subsystems (3)
PATCH v2 1/1 staging: greybus: Convert timers to use timer_setup()
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.
---
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(-)
drivers/staging/greybus/loopback.c +9 -10
@@ -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);
drivers/staging/greybus/operation.c +3 -4
@@ -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;
Signature · 6 lines
2.7.4 -- Kees Cook Pixel Security
Quoted 28 lines · Kees Cook
In preparation for unconditionally passing the struct
timer_listpointer to all timer callbacks, switch to using the newtimer_setup()andfrom_timer()to pass the timer pointer explicitly.Cc Greg Kroah-Hartman <[email protected]>Cc "Bryan O'Donoghue" <[email protected]>Cc Johan Hovold <[email protected]>Cc Alex Elder <[email protected]>Signed-off-by Kees Cook <[email protected]>---
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(-)drivers/staging/greybus/loopback.c +1
-1@@ -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)
drivers/staging/greybus/loopback.c static void gb_loopback_async_operation_work(struct work_struct *work) · line 583
Quoted 10 lines
drivers/staging/greybus/loopback.c +7
-2{ -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
Quoted 1 line
From
aa9fbf091122c816a47de2aece5412f7fd9225a9Mon Sep 17 00:00:00 2001
From: Kees Cook <[email protected]>
Date: Tue, 24 Oct 2017 07:49:38 -0700
Subject: [PATCH] staging: greybus: Convert timers to use timer_setup()
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.
--- drivers/staging/greybus/loopback.c | 38 ++++++++++++++++++++++++++----------- drivers/staging/greybus/operation.c | 7 +++---- 2 files changed, 30 insertions(+), 15 deletions(-)
drivers/staging/greybus/loopback.c +27 -11
@@ -472,6 +472,26 @@ static void gb_loopback_async_operation_put(struct gb_loopback_async_operationspin_unlock_irqrestore (&gb_dev.lock, flags); } +static struct gb_loopback_async_operation *gb_loopback_async_operation_valid ( +struct gb_loopback_async_operation *op_async) +{ +struct gb_loopback_async_operation *op_async_tmp; +bool found = false; +unsigned long flags; + +spin_lock_irqsave (&gb_dev.lock, flags); +list_for_each_entry (op_async_tmp, &gb_dev.list_op_async, entry) { +if (op_async_tmp == op_async) { +gb_loopback_async_operation_get (op_async); + found = true; +break ; + } + } +spin_unlock_irqrestore (&gb_dev.lock, flags); + +return found ? op_async :NULL ; +} +static struct gb_loopback_async_operation *gb_loopback_operation_find (u16 id) {
@@ -572,17 +592,14 @@ 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); - 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); + op_async =gb_loopback_async_operation_valid (op_async); +if (op_async) +schedule_work (&op_async->work); } static int gb_loopback_async_operation(struct gb_loopback *gb, int type,
@@ -631,8 +648,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);
drivers/staging/greybus/operation.c +3 -4
@@ -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;
Signature · 1 line
2.7.4
Johan Hovold
maintainerQuoted 42 lines · Bryan O'Donoghue
Quoted 28 lines · Kees Cook
In preparation for unconditionally passing the struct
timer_listpointer to all timer callbacks, switch to using the newtimer_setup()andfrom_timer()to pass the timer pointer explicitly.Cc Greg Kroah-Hartman <[email protected]>Cc "Bryan O'Donoghue" <[email protected]>Cc Johan Hovold <[email protected]>Cc Alex Elder <[email protected]>Signed-off-by Kees Cook <[email protected]>---
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(-)drivers/staging/greybus/loopback.c +1
-1@@ -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)drivers/staging/greybus/loopback.c static void gb_loopback_async_operation_work(struct work_struct *work) · line 583
Quoted 10 lines
drivers/staging/greybus/loopback.c +7
-2{ -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).
Quoted 2 lines
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.
Quoted 1 line
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
maintainerQuoted 3 lines · Johan Hovold
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.
Johan Hovold
maintainer[ Resend to Bryan's nexus-software address ]
Quoted 42 lines · Bryan O'Donoghue
Quoted 28 lines · Kees Cook
In preparation for unconditionally passing the struct
timer_listpointer to all timer callbacks, switch to using the newtimer_setup()andfrom_timer()to pass the timer pointer explicitly.Cc Greg Kroah-Hartman <[email protected]>Cc "Bryan O'Donoghue" <[email protected]>Cc Johan Hovold <[email protected]>Cc Alex Elder <[email protected]>Signed-off-by Kees Cook <[email protected]>---
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(-)drivers/staging/greybus/loopback.c +1
-1@@ -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)drivers/staging/greybus/loopback.c static void gb_loopback_async_operation_work(struct work_struct *work) · line 583
Quoted 10 lines
drivers/staging/greybus/loopback.c +7
-2{ -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).
Quoted 2 lines
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.
Quoted 1 line
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
Johan Hovold
maintainerQuoted 9 lines · Bryan O'Donoghue
Quoted 3 lines · Johan Hovold
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
Quoted 9 lines · Johan Hovold
Quoted 8 lines · Bryan O'Donoghue
Quoted 3 lines · Johan Hovold
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.
Quoted 8 lines
Quoted 1 line
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
Johan Hovold
maintainerQuoted 17 lines · Bryan O'Donoghue
Quoted 14 lines · Johan Hovold
Quoted 9 lines · Bryan O'Donoghue
Quoted 3 lines · Johan Hovold
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.
Quoted 7 lines
Quoted 3 lines
Thanks,
JohanI think once Kees' change is applied to
operation.cand 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.
Quoted 2 lines
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
Quoted 26 lines · Johan Hovold
Quoted 17 lines · Bryan O'Donoghue
Quoted 14 lines · Johan Hovold
Quoted 9 lines · Bryan O'Donoghue
Quoted 3 lines · Johan Hovold
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.
Quoted 7 lines
Quoted 3 lines
Thanks,
JohanI think once Kees' change is applied to
operation.cand we convert the async stuff to operation.c's callbacks there ought to be no use of timers, linked lists of operations.
Quoted 8 lines
That's correct.
Quoted 2 lines
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
Signature · 2 lines
Kees Cook Pixel Security
[email protected]
maintainerQuoted 43 lines · Kees Cook
Quoted 7 lines · Johan Hovold
Quoted 6 lines · Bryan O'Donoghue
Quoted 3 lines · Johan Hovold
Quoted 2 lines · Bryan O'Donoghue
Quoted 1 line · Johan Hovold
The right thing to do here is to respin your patch from last
year which
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
converts the loopback driver to use the timeout handling in
greybus
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
core.
Actually I wasn't clear if you wanted to to that yourself aswell
as the
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
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
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
removed.
Thanks,
JohanI think once Kees' change is applied to
operation.cand we convertthe
Quoted 9 lines
Quoted 2 lines
async stuff to operation.c's callbacks there ought to be no use of timers, linked lists of operations.
That's correct.
Quoted 2 lines
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
Quoted 1 line
core timeout handling and used that to test the core implementation,
so
Quoted 1 line
it should be straight forward.
Quoted 7 lines
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.
Signature · 1 line
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Quoted 50 lines · On Mon, Oct 30, 2017 at 5:01 PM, <[email protected]> wrote:
Quoted 49 lines · Kees Cook
Quoted 7 lines · Johan Hovold
Quoted 6 lines · Bryan O'Donoghue
Quoted 3 lines · Johan Hovold
Quoted 2 lines · Bryan O'Donoghue
Quoted 1 line · Johan Hovold
The right thing to do here is to respin your patch from last
year which
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
converts the loopback driver to use the timeout handling in
greybus
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
core.
Actually I wasn't clear if you wanted to to that yourself aswell
as the
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
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
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
removed.
Thanks,
JohanI think once Kees' change is applied to
operation.cand we convertthe
Quoted 9 lines
Quoted 2 lines
async stuff to operation.c's callbacks there ought to be no use of timers, linked lists of operations.
That's correct.
Quoted 2 lines
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
Quoted 1 line
core timeout handling and used that to test the core implementation,
so
Quoted 1 line
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?)
Quoted 7 lines
Quoted 1 line
-Kees
Trying again without top posting in html :(
Just pair the patch down to
operation.c.There's a separate change to
loopback.can old patch ARAIR that will subtract use of the timer fromloopback.cso 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
Signature · 2 lines
Kees Cook Pixel Security
Quoted 5 lines · Kees Cook
Quoted 1 line · On Mon, Oct 30, 2017 at 5:01 PM, <[email protected]> wrote:
There's a separate change to
loopback.can old patch ARAIR that will subtract use of the timer fromloopback.cso you can skip that bit.Okay, cool. Since the
operation.cchange 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
Signature · 2 lines
Kees Cook Pixel Security
Quoted 10 lines · Kees Cook
Quoted 5 lines · Kees Cook
Quoted 1 line · On Mon, Oct 30, 2017 at 5:01 PM, <[email protected]> wrote:
There's a separate change to
loopback.can old patch ARAIR that will subtract use of the timer fromloopback.cso you can skip that bit.Okay, cool. Since the
operation.cchange 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.cthat 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.
Quoted 16 lines · Bryan O'Donoghue
Quoted 13 lines · Kees Cook
Quoted 9 lines · Kees Cook
Quoted 3 lines · On Mon, Oct 30, 2017 at 5:01 PM, <[email protected]> wrote:
There's a separate change to
loopback.can old patch ARAIR that will subtract use of the timer fromloopback.cso you can skip that bit.Okay, cool. Since the
operation.cchange is trivial, I'll include it in the giant tree-wide patch that will (hopefully) land in -rc1.
Quoted 8 lines
Quoted 3 lines
I forgot to ask: will the patch for
loopback.cthat 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
Signature · 2 lines
Kees Cook Pixel Security