Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup()

[Kees Cook]

On Fri, Nov 3, 2017 at 2:49 PM, Bryan O'Donoghue
 wrote:
>
>
> On 03/11/17 20:21, Kees Cook wrote:
>>
>> On Mon, Oct 30, 2017 at 5:05 PM, Kees Cook  wrote:
>>>
>>> On Mon, Oct 30, 2017 at 5:01 PM,   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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup()

[Bryan O'Donoghue]

On 03/11/17 20:21, Kees Cook wrote:

On Mon, Oct 30, 2017 at 5:05 PM, Kees Cook  wrote:

On Mon, Oct 30, 2017 at 5:01 PM,   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.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup()

[Kees Cook]

On Mon, Oct 30, 2017 at 5:05 PM, Kees Cook  wrote:
> On Mon, Oct 30, 2017 at 5:01 PM,   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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup()

[Kees Cook]

On Mon, Oct 30, 2017 at 5:01 PM,   wrote:
> On 30 October 2017 9:37:37 p.m. GMT+00:00, Kees Cook  
> wrote:
>>On Mon, Oct 30, 2017 at 4:48 AM, Johan Hovold  wrote:
>>> On Mon, Oct 30, 2017 at 11:44:22AM +, Bryan O'Donoghue wrote:


 On 30/10/17 11:38, Johan Hovold wrote:
 > On Mon, Oct 30, 2017 at 11:35:50AM +, 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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup()

[pure . logic]

On 30 October 2017 9:37:37 p.m. GMT+00:00, Kees Cook  
wrote:
>On Mon, Oct 30, 2017 at 4:48 AM, Johan Hovold  wrote:
>> On Mon, Oct 30, 2017 at 11:44:22AM +, Bryan O'Donoghue wrote:
>>>
>>>
>>> On 30/10/17 11:38, Johan Hovold wrote:
>>> > On Mon, Oct 30, 2017 at 11:35:50AM +, 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.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup()

[Kees Cook]

On Mon, Oct 30, 2017 at 4:48 AM, Johan Hovold  wrote:
> On Mon, Oct 30, 2017 at 11:44:22AM +, Bryan O'Donoghue wrote:
>>
>>
>> On 30/10/17 11:38, Johan Hovold wrote:
>> > On Mon, Oct 30, 2017 at 11:35:50AM +, 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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup()

[Johan Hovold]

On Mon, Oct 30, 2017 at 11:44:22AM +, Bryan O'Donoghue wrote:
> 
> 
> On 30/10/17 11:38, Johan Hovold wrote:
> > On Mon, Oct 30, 2017 at 11:35:50AM +, 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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup()

[Bryan O'Donoghue]

On 30/10/17 11:38, Johan Hovold wrote:

On Mon, Oct 30, 2017 at 11:35:50AM +, 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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup()

[Johan Hovold]

On Mon, Oct 30, 2017 at 11:35:50AM +, 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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup()

[Bryan O'Donoghue]

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.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup()

[Johan Hovold]

[ 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 
> > Cc: "Bryan O'Donoghue" 
> > Cc: Johan Hovold 
> > Cc: Alex Elder 
> > Cc: greybus-...@lists.linaro.org
> > Cc: de...@driverdev.osuosl.org
> > Signed-off-by: Kees Cook 
> > ---
> > 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(_dev.lock, flags);
> > +   gb_loopback_async_operation_get(op_async);
> > +   spin_unlock_irqrestore(_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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup()

[Johan Hovold]

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 
> > Cc: "Bryan O'Donoghue" 
> > Cc: Johan Hovold 
> > Cc: Alex Elder 
> > Cc: greybus-...@lists.linaro.org
> > Cc: de...@driverdev.osuosl.org
> > Signed-off-by: Kees Cook 
> > ---
> > 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(_dev.lock, flags);
> > +   gb_loopback_async_operation_get(op_async);
> > +   spin_unlock_irqrestore(_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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup()

[Bryan O'Donoghue]

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 
Cc: "Bryan O'Donoghue" 
Cc: Johan Hovold 
Cc: Alex Elder 
Cc: greybus-...@lists.linaro.org
Cc: de...@driverdev.osuosl.org
Signed-off-by: Kees Cook 
---
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(_dev.lock, flags);
+   gb_loopback_async_operation_get(op_async);
+   spin_unlock_irqrestore(_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
>From aa9fbf091122c816a47de2aece5412f7fd9225a9 Mon Sep 17 00:00:00 2001
From: Kees Cook 
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 
Cc: "Bryan O'Donoghue" 
Cc: Johan Hovold 
Cc: Alex Elder 
Cc: greybus-...@lists.linaro.org
Cc: de...@driverdev.osuosl.org
Signed-off-by: Kees Cook 
Signed-off-by: Bryan O'Donoghue 
---
 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(_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(_dev.lock, flags);
+	list_for_each_entry(op_async_tmp, _dev.list_op_async, entry) {
+		if (op_async_tmp == op_async) {
+			gb_loopback_async_operation_get(op_async);
+			found = true;
+			break;
+		}
+	}
+	spin_unlock_irqrestore(_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(_async->work);
+	op_async = gb_loopback_async_operation_valid(op_async);
+	if (op_async)
+		schedule_work(_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(_async->timer, gb_loopback_async_operation_timeout,
-			(unsigned long)operation->id);
+	timer_setup(_async->timer, gb_loopback_async_operation_timeout, 0);
 	op_async->timer.expires = jiffies + gb->jiffy_timeout;
 	add_timer(_async->timer);
 
diff --git a/drivers/staging/greybus/operation.c b/drivers/staging/greybus/operation.c
index 3023012..ee4ba3f 100644