staging: greybus: Convert timers to use timer_setup()

PATCH In review Kees Cook 3 participants 10 messages Oct 24, 2017

Subsystems (3)

Staging Greybus Greybus loopback driver

lore

Share & tools

lore

Kees Cook

author

2017-10-24 08:25 UTC #0 lore raw

PATCH 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.

Cc Greg Kroah-Hartman <[email protected]>

Cc "Bryan O'Donoghue" <[email protected]>

Cc Johan Hovold <[email protected]>

Cc Alex Elder <[email protected]>

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
 drivers/staging/greybus/loopback.c  | 14 ++++----------
 drivers/staging/greybus/operation.c |  7 +++----
 2 files changed, 7 insertions(+), 14 deletions(-)

drivers/staging/greybus/loopback.c +4 ~~-10~~

index 08e255884206..045aaf81113a 100644
@@ -572,16 +572,11 @@ 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);
 }

@@ -631,8 +626,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

index 3023012808d9..ee4ba3f23bef 100644
@@ -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

Bryan O'Donoghue

maintainer Reviewed-by on v1 1/1

2017-10-24 09:35 UTC #1 lore raw

Quoted 27 lines · Kees Cook

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 <[email protected]>
Cc "Bryan O'Donoghue" <[email protected]>
Cc Johan Hovold <[email protected]>
Cc Alex Elder <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
---
  drivers/staging/greybus/loopback.c  | 14 ++++----------
  drivers/staging/greybus/operation.c |  7 +++----
  2 files changed, 7 insertions(+), 14 deletions(-)
drivers/staging/greybus/loopback.c +1 -1
index 08e255884206..045aaf81113a 100644
@@ -572,16 +572,11 @@ 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 584

Quoted 10 lines

drivers/staging/greybus/loopback.c +2 -7

-	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;
-	}

Hi Kees, you need to add

gb_loopback_async_operation_get(op_async); when dropping the
gb_loopback_operation_find() call here.

Quoted 31 lines

drivers/staging/greybus/loopback.c +1 -2

  	schedule_work(&op_async->work);
  }

@@ -631,8 +626,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 +2 -2

index 3023012808d9..ee4ba3f23bef 100644
@@ -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)) {
  		/*

drivers/staging/greybus/operation.c gb_operation_create_common(struct gb_connection *connection, u8 type, · line 547

Quoted 9 lines

drivers/staging/greybus/operation.c +1 -2

@@ -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;

Other than that, the rest looks good to me and you can add my

Reviewed-by Bryan O'Donoghue <[email protected]>

Dan Carpenter

2017-10-24 10:07 UTC #9 lore raw

Actually, on further reflection, I don't care. However you write it is fine.

regards,
dan carpenter

Kees Cook

author

2017-10-24 12:48 UTC #3 lore raw

In reply to Bryan O'Donoghue

Quoted 51 lines · Bryan O'Donoghue

Quoted 46 lines · Bryan O'Donoghue
Quoted 30 lines · Kees Cook
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 <[email protected]>
Cc "Bryan O'Donoghue" <[email protected]>
Cc Johan Hovold <[email protected]>
Cc Alex Elder <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
---
  drivers/staging/greybus/loopback.c  | 14 ++++----------
  drivers/staging/greybus/operation.c |  7 +++----
  2 files changed, 7 insertions(+), 14 deletions(-)
drivers/staging/greybus/loopback.c +1 -1
index 08e255884206..045aaf81113a 100644
@@ -572,16 +572,11 @@ 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 · line 583
Quoted 10 lines
drivers/staging/greybus/loopback.c +2 -7
  {
-    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;
-    }
Hi Kees, you need to add
Quoted 2 lines
gb_loopback_async_operation_get(op_async); when dropping the
gb_loopback_operation_find() call here.

Quoted 5 lines

Actually:

        spin_lock_irqsave(&gb_dev.lock, flags);
        gb_loopback_async_operation_get(op_async);
        spin_unlock_irqrestore(&gb_dev.lock, flags);

Shouldn't the get/put follow the lifetime of the timer running instead? It shouldnt' be possible to free the op_async while the timer is still pending/running.

-Kees

Signature · 2 lines

Kees Cook
Pixel Security

Bryan O'Donoghue

maintainer

2017-10-24 12:52 UTC #4 lore raw

Quoted 59 lines · Kees Cook

Quoted 51 lines · Bryan O'Donoghue
Quoted 46 lines · Bryan O'Donoghue
Quoted 30 lines · Kees Cook
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 <[email protected]>
Cc "Bryan O'Donoghue" <[email protected]>
Cc Johan Hovold <[email protected]>
Cc Alex Elder <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
---
   drivers/staging/greybus/loopback.c  | 14 ++++----------
   drivers/staging/greybus/operation.c |  7 +++----
   2 files changed, 7 insertions(+), 14 deletions(-)
drivers/staging/greybus/loopback.c +1 -1
index 08e255884206..045aaf81113a 100644
@@ -572,16 +572,11 @@ 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 · line 583
Quoted 10 lines
drivers/staging/greybus/loopback.c +2 -7
   {
-    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;
-    }
Hi Kees, you need to add
Quoted 2 lines
gb_loopback_async_operation_get(op_async); when dropping the
gb_loopback_operation_find() call here.
Quoted 5 lines
Actually:
         spin_lock_irqsave(&gb_dev.lock, flags);
         gb_loopback_async_operation_get(op_async);
         spin_unlock_irqrestore(&gb_dev.lock, flags);

Quoted 6 lines

Shouldn't the get/put follow the lifetime of the timer running instead? It shouldnt' be possible to free the op_async while the timer is still pending/running.
-Kees

The timeout timer runs for an operation that never completed but on the regular "everything is good" path you end up doing del_timer_sync(&op_async->timer); so the timer doesn't run.

Kees Cook

author

2017-10-24 13:27 UTC #5 lore raw

Quoted 68 lines · Bryan O'Donoghue

Quoted 67 lines · Kees Cook
Quoted 63 lines · Bryan O'Donoghue
Quoted 51 lines · Bryan O'Donoghue
Quoted 33 lines · Kees Cook
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 <[email protected]>
Cc "Bryan O'Donoghue" <[email protected]>
Cc Johan Hovold <[email protected]>
Cc Alex Elder <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
---
   drivers/staging/greybus/loopback.c  | 14 ++++----------
   drivers/staging/greybus/operation.c |  7 +++----
   2 files changed, 7 insertions(+), 14 deletions(-)
drivers/staging/greybus/loopback.c +0 -0
index 08e255884206..045aaf81113a 100644
@@ -572,16 +572,11 @@ static void
gb_loopback_async_operation_work(struct
work_struct *work)
       gb_loopback_async_operation_put(op_async);
}
drivers/staging/greybus/loopback.c +1 -1
-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 584
Quoted 10 lines
drivers/staging/greybus/loopback.c +2 -7
   {
-    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;
-    }
Hi Kees, you need to add
Quoted 2 lines
gb_loopback_async_operation_get(op_async); when dropping the
gb_loopback_operation_find() call here.
Actually:
         spin_lock_irqsave(&gb_dev.lock, flags);
         gb_loopback_async_operation_get(op_async);
         spin_unlock_irqrestore(&gb_dev.lock, flags);

Quoted 7 lines

Quoted 3 lines
Shouldn't the get/put follow the lifetime of the timer running instead? It shouldnt' be possible to free the op_async while the timer is still pending/running.
The timeout timer runs for an operation that never completed but on the regular "everything is good" path you end up doing del_timer_sync(&op_async->timer); so the timer doesn't run.

As far as I can tell, the get/put is already happening external to the timer:

gb_loopback_async_operation():
...

        op_async = kzalloc(sizeof(*op_async), GFP_KERNEL);
...
        INIT_WORK(&op_async->work, gb_loopback_async_operation_work);
        kref_init(&op_async->kref);
...
        op_async->pending = true;
...
        ret = gb_operation_request_send(operation,
                                        gb_loopback_async_operation_callback,
                                        0,
                                        GFP_KERNEL);
...
        timer_setup(&op_async->timer, gb_loopback_async_operation_timeout, 0);
        op_async->timer.expires = jiffies + gb->jiffy_timeout;
        add_timer(&op_async->timer);

gb_loopback_async_operation_callback(): (run by operation)
...

                op_async->pending = false;
                del_timer_sync(&op_async->timer);
                gb_loopback_async_operation_put(op_async);

gb_loopback_async_operation_work(): (scheduled by timer)
...

                op_async->pending = false;
                gb_loopback_async_operation_put(op_async);

(Doing a get/put in the timer callback itself shouldn't be happening:
it must already be pinned in memory for the callback to run.)

-Kees

Signature · 2 lines

Kees Cook
Pixel Security

Bryan O'Donoghue

maintainer

2017-10-24 13:30 UTC #6 lore raw

Quoted 108 lines · Kees Cook

Quoted 68 lines · Bryan O'Donoghue
Quoted 67 lines · Kees Cook
Quoted 63 lines · Bryan O'Donoghue
Quoted 51 lines · Bryan O'Donoghue
Quoted 33 lines · Kees Cook
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 <[email protected]>
Cc "Bryan O'Donoghue" <[email protected]>
Cc Johan Hovold <[email protected]>
Cc Alex Elder <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
---
    drivers/staging/greybus/loopback.c  | 14 ++++----------
    drivers/staging/greybus/operation.c |  7 +++----
    2 files changed, 7 insertions(+), 14 deletions(-)
drivers/staging/greybus/loopback.c +0 -0
index 08e255884206..045aaf81113a 100644
@@ -572,16 +572,11 @@ static void
gb_loopback_async_operation_work(struct
work_struct *work)
        gb_loopback_async_operation_put(op_async);
    }
drivers/staging/greybus/loopback.c +1 -1
-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 584
Quoted 10 lines
drivers/staging/greybus/loopback.c +2 -7
    {
-    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;
-    }
Hi Kees, you need to add
Quoted 2 lines
gb_loopback_async_operation_get(op_async); when dropping the
gb_loopback_operation_find() call here.
Actually:
          spin_lock_irqsave(&gb_dev.lock, flags);
          gb_loopback_async_operation_get(op_async);
          spin_unlock_irqrestore(&gb_dev.lock, flags);
Quoted 7 lines
Quoted 3 lines
Shouldn't the get/put follow the lifetime of the timer running instead? It shouldnt' be possible to free the op_async while the timer is still pending/running.
The timeout timer runs for an operation that never completed but on the regular "everything is good" path you end up doing del_timer_sync(&op_async->timer); so the timer doesn't run.
As far as I can tell, the get/put is already happening external to the timer:
gb_loopback_async_operation():
...
         op_async = kzalloc(sizeof(*op_async), GFP_KERNEL);
...
         INIT_WORK(&op_async->work, gb_loopback_async_operation_work);
         kref_init(&op_async->kref);
...
         op_async->pending = true;
...
         ret = gb_operation_request_send(operation,
                                         gb_loopback_async_operation_callback,
                                         0,
                                         GFP_KERNEL);
...
         timer_setup(&op_async->timer, gb_loopback_async_operation_timeout, 0);
         op_async->timer.expires = jiffies + gb->jiffy_timeout;
         add_timer(&op_async->timer);
gb_loopback_async_operation_callback(): (run by operation)
...
                 op_async->pending = false;
                 del_timer_sync(&op_async->timer);
                 gb_loopback_async_operation_put(op_async);
gb_loopback_async_operation_work(): (scheduled by timer)
...

Quoted 7 lines

                 op_async->pending = false;
                 gb_loopback_async_operation_put(op_async);
(Doing a get/put in the timer callback itself shouldn't be happening:
it must already be pinned in memory for the callback to run.)

Both gb_loopback_async_operation_callback() and (gb_loopback_async_operation_timeout || gb_loopback_async_operation_work()) can run in parallel so the get in the timer callback means we can let the timer stuff free-run w/r to gb_loopback_async_operation_callback() - take a reference indicating we still want that object and then let gb_loopback_async_operation_work() run.

---
bod

Kees Cook

author

2017-10-24 14:36 UTC #7 lore raw

Quoted 133 lines · Bryan O'Donoghue

Quoted 123 lines · Kees Cook
Quoted 77 lines · Bryan O'Donoghue
Quoted 75 lines · Kees Cook
Quoted 69 lines · Bryan O'Donoghue
Quoted 55 lines · Bryan O'Donoghue
Quoted 36 lines · Kees Cook
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 <[email protected]>
Cc "Bryan O'Donoghue" <[email protected]>
Cc Johan Hovold <[email protected]>
Cc Alex Elder <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
---
    drivers/staging/greybus/loopback.c  | 14 ++++----------
    drivers/staging/greybus/operation.c |  7 +++----
    2 files changed, 7 insertions(+), 14 deletions(-)
drivers/staging/greybus/loopback.c +0 -0
index 08e255884206..045aaf81113a 100644
@@ -572,16 +572,11 @@ static void
gb_loopback_async_operation_work(struct
work_struct *work)
        gb_loopback_async_operation_put(op_async);
    }
drivers/staging/greybus/loopback.c +1 -1
-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
Quoted 9 lines
drivers/staging/greybus/loopback.c +2 -7
-    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;
-    }
Hi Kees, you need to add
Quoted 2 lines
gb_loopback_async_operation_get(op_async); when dropping the
gb_loopback_operation_find() call here.
Actually:
          spin_lock_irqsave(&gb_dev.lock, flags);
          gb_loopback_async_operation_get(op_async);
          spin_unlock_irqrestore(&gb_dev.lock, flags);
Quoted 8 lines
Quoted 3 lines
Shouldn't the get/put follow the lifetime of the timer running instead? It shouldnt' be possible to free the op_async while the timer is still pending/running.
The timeout timer runs for an operation that never completed but on the regular "everything is good" path you end up doing del_timer_sync(&op_async->timer); so the timer doesn't run.
As far as I can tell, the get/put is already happening external to the timer:
gb_loopback_async_operation():
...
         op_async = kzalloc(sizeof(*op_async), GFP_KERNEL);
...
         INIT_WORK(&op_async->work, gb_loopback_async_operation_work);
         kref_init(&op_async->kref);
...
         op_async->pending = true;
...
ret = gb_operation_request_send(operation,
gb_loopback_async_operation_callback,
                                         0,
                                         GFP_KERNEL);
...
         timer_setup(&op_async->timer,
gb_loopback_async_operation_timeout, 0);
         op_async->timer.expires = jiffies + gb->jiffy_timeout;
         add_timer(&op_async->timer);
gb_loopback_async_operation_callback(): (run by operation)
...
                 op_async->pending = false;
                 del_timer_sync(&op_async->timer);
                 gb_loopback_async_operation_put(op_async);
gb_loopback_async_operation_work(): (scheduled by timer)
...
Quoted 7 lines
                 op_async->pending = false;
                 gb_loopback_async_operation_put(op_async);
(Doing a get/put in the timer callback itself shouldn't be happening:
it must already be pinned in memory for the callback to run.)

Quoted 6 lines

Both gb_loopback_async_operation_callback() and (gb_loopback_async_operation_timeout || gb_loopback_async_operation_work()) can run in parallel so the get in the timer callback means we can let the timer stuff free-run w/r to gb_loopback_async_operation_callback() - take a reference indicating we still want that object and then let gb_loopback_async_operation_work() run.

Ah right, we're protecting the scheduled worker thread. I'll send a v2, thanks!

-Kees

Signature · 2 lines

Kees Cook
Pixel Security