* [PATCH v2] staging: greybus: Convert timers to use timer_setup() @ 2017-10-24 14:49 Kees Cook 2017-10-24 15:54 ` Bryan O'Donoghue 0 siblings, 1 reply; 14+ messages in thread From: Kees Cook @ 2017-10-24 14:49 UTC (permalink / raw) To: Greg Kroah-Hartman, Bryan O'Donoghue Cc: linux-kernel, Johan Hovold, Alex Elder, greybus-dev, devel 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; -- 2.7.4 -- Kees Cook Pixel Security ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup() 2017-10-24 14:49 [PATCH v2] staging: greybus: Convert timers to use timer_setup() Kees Cook @ 2017-10-24 15:54 ` Bryan O'Donoghue 2017-10-30 11:32 ` Johan Hovold 2017-10-30 11:35 ` Johan Hovold 0 siblings, 2 replies; 14+ messages in thread From: Bryan O'Donoghue @ 2017-10-24 15:54 UTC (permalink / raw) To: Kees Cook, Greg Kroah-Hartman Cc: linux-kernel, Johan Hovold, Alex Elder, greybus-dev, devel [-- Attachment #1: Type: text/plain, Size: 1809 bytes --] 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 [-- Attachment #2: 0001-staging-greybus-Convert-timers-to-use-timer_setup.patch --] [-- Type: text/x-patch, Size: 4038 bytes --] >From aa9fbf091122c816a47de2aece5412f7fd9225a9 Mon Sep 17 00:00:00 2001 From: Kees Cook <keescook@chromium.org> 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. 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> Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie> --- drivers/staging/greybus/loopback.c | 38 ++++++++++++++++++++++++++----------- drivers/staging/greybus/operation.c | 7 +++---- 2 files changed, 30 insertions(+), 15 deletions(-) diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c index 08e2558..81447e3 100644 --- a/drivers/staging/greybus/loopback.c +++ b/drivers/staging/greybus/loopback.c @@ -472,6 +472,26 @@ static void gb_loopback_async_operation_put(struct gb_loopback_async_operation spin_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); diff --git a/drivers/staging/greybus/operation.c b/drivers/staging/greybus/operation.c index 3023012..ee4ba3f 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; -- 2.7.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup() 2017-10-24 15:54 ` Bryan O'Donoghue @ 2017-10-30 11:32 ` Johan Hovold 2017-10-30 11:35 ` Bryan O'Donoghue 2017-10-30 11:35 ` Johan Hovold 1 sibling, 1 reply; 14+ messages in thread From: Johan Hovold @ 2017-10-30 11:32 UTC (permalink / raw) To: Bryan O'Donoghue Cc: Kees Cook, Greg Kroah-Hartman, linux-kernel, Johan Hovold, Alex Elder, greybus-dev, devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup() 2017-10-30 11:32 ` Johan Hovold @ 2017-10-30 11:35 ` Bryan O'Donoghue 2017-10-30 11:38 ` Johan Hovold 0 siblings, 1 reply; 14+ messages in thread From: Bryan O'Donoghue @ 2017-10-30 11:35 UTC (permalink / raw) To: Johan Hovold, Bryan O'Donoghue Cc: Kees Cook, Greg Kroah-Hartman, linux-kernel, Alex Elder, greybus-dev, devel 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup() 2017-10-30 11:35 ` Bryan O'Donoghue @ 2017-10-30 11:38 ` Johan Hovold 2017-10-30 11:44 ` Bryan O'Donoghue 0 siblings, 1 reply; 14+ messages in thread From: Johan Hovold @ 2017-10-30 11:38 UTC (permalink / raw) To: Bryan O'Donoghue Cc: Johan Hovold, Bryan O'Donoghue, Kees Cook, Greg Kroah-Hartman, linux-kernel, Alex Elder, greybus-dev, devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup() 2017-10-30 11:38 ` Johan Hovold @ 2017-10-30 11:44 ` Bryan O'Donoghue 2017-10-30 11:48 ` Johan Hovold 0 siblings, 1 reply; 14+ messages in thread From: Bryan O'Donoghue @ 2017-10-30 11:44 UTC (permalink / raw) To: Johan Hovold, Bryan O'Donoghue Cc: Kees Cook, Greg Kroah-Hartman, linux-kernel, Alex Elder, greybus-dev, devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup() 2017-10-30 11:44 ` Bryan O'Donoghue @ 2017-10-30 11:48 ` Johan Hovold 2017-10-30 21:37 ` Kees Cook 0 siblings, 1 reply; 14+ messages in thread From: Johan Hovold @ 2017-10-30 11:48 UTC (permalink / raw) To: Bryan O'Donoghue Cc: Johan Hovold, Bryan O'Donoghue, Kees Cook, Greg Kroah-Hartman, linux-kernel, Alex Elder, greybus-dev, devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup() 2017-10-30 11:48 ` Johan Hovold @ 2017-10-30 21:37 ` Kees Cook 2017-10-31 0:01 ` pure.logic 0 siblings, 1 reply; 14+ messages in thread From: Kees Cook @ 2017-10-30 21:37 UTC (permalink / raw) To: Johan Hovold Cc: Bryan O'Donoghue, Bryan O'Donoghue, Greg Kroah-Hartman, LKML, Alex Elder, greybus-dev, devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup() 2017-10-30 21:37 ` Kees Cook @ 2017-10-31 0:01 ` pure.logic 2017-10-31 0:05 ` Kees Cook 0 siblings, 1 reply; 14+ messages in thread From: pure.logic @ 2017-10-31 0:01 UTC (permalink / raw) To: Kees Cook, Johan Hovold Cc: Bryan O'Donoghue, Greg Kroah-Hartman, LKML, Alex Elder, greybus-dev, devel 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. -- Sent from my Android device with K-9 Mail. Please excuse my brevity. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup() 2017-10-31 0:01 ` pure.logic @ 2017-10-31 0:05 ` Kees Cook 2017-11-03 20:21 ` Kees Cook 0 siblings, 1 reply; 14+ messages in thread From: Kees Cook @ 2017-10-31 0:05 UTC (permalink / raw) To: Bryan O'Donoghue Cc: Johan Hovold, Bryan O'Donoghue, Greg Kroah-Hartman, LKML, Alex Elder, greybus-dev, devel 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 -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup() 2017-10-31 0:05 ` Kees Cook @ 2017-11-03 20:21 ` Kees Cook 2017-11-03 21:49 ` Bryan O'Donoghue 0 siblings, 1 reply; 14+ messages in thread From: Kees Cook @ 2017-11-03 20:21 UTC (permalink / raw) To: Bryan O'Donoghue Cc: Johan Hovold, Bryan O'Donoghue, Greg Kroah-Hartman, LKML, Alex Elder, greybus-dev, devel 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 -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup() 2017-11-03 20:21 ` Kees Cook @ 2017-11-03 21:49 ` Bryan O'Donoghue 2017-11-03 21:49 ` Kees Cook 0 siblings, 1 reply; 14+ messages in thread From: Bryan O'Donoghue @ 2017-11-03 21:49 UTC (permalink / raw) To: Kees Cook, Bryan O'Donoghue Cc: Johan Hovold, Greg Kroah-Hartman, LKML, Alex Elder, greybus-dev, devel 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup() 2017-11-03 21:49 ` Bryan O'Donoghue @ 2017-11-03 21:49 ` Kees Cook 0 siblings, 0 replies; 14+ messages in thread From: Kees Cook @ 2017-11-03 21:49 UTC (permalink / raw) To: Bryan O'Donoghue Cc: Bryan O'Donoghue, Johan Hovold, Greg Kroah-Hartman, LKML, Alex Elder, greybus-dev, devel 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 -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup() 2017-10-24 15:54 ` Bryan O'Donoghue 2017-10-30 11:32 ` Johan Hovold @ 2017-10-30 11:35 ` Johan Hovold 1 sibling, 0 replies; 14+ messages in thread From: Johan Hovold @ 2017-10-30 11:35 UTC (permalink / raw) To: Bryan O'Donoghue, Bryan O'Donoghue Cc: Kees Cook, Greg Kroah-Hartman, linux-kernel, Johan Hovold, Alex Elder, greybus-dev, devel [ 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-11-03 21:49 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-10-24 14:49 [PATCH v2] staging: greybus: Convert timers to use timer_setup() Kees Cook 2017-10-24 15:54 ` Bryan O'Donoghue 2017-10-30 11:32 ` Johan Hovold 2017-10-30 11:35 ` Bryan O'Donoghue 2017-10-30 11:38 ` Johan Hovold 2017-10-30 11:44 ` Bryan O'Donoghue 2017-10-30 11:48 ` Johan Hovold 2017-10-30 21:37 ` Kees Cook 2017-10-31 0:01 ` pure.logic 2017-10-31 0:05 ` Kees Cook 2017-11-03 20:21 ` Kees Cook 2017-11-03 21:49 ` Bryan O'Donoghue 2017-11-03 21:49 ` Kees Cook 2017-10-30 11:35 ` Johan Hovold
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.