isdn/gigaset: Convert timers to use timer_setup()

PATCH Superseded Kees Cook 3 participants 5 messages Oct 5, 2017 – Oct 9, 2017

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Kees Cook

author

2017-10-05 19:31 UTC #0 lore raw

PATCH v2 1/1 isdn/gigaset: 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 Paul Bolle <[email protected]>

Cc Karsten Keil <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc Johan Hovold <[email protected]>

Cc [email protected]

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

---
This requires commit 686fef928bba ("timer: Prepare to change timer callback argument type") in v4.14-rc3, but should be otherwise stand-alone.

v2:
- split kzalloc() into a separate patch; pebolle.

---
 drivers/isdn/gigaset/bas-gigaset.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

drivers/isdn/gigaset/bas-gigaset.c +20 ~~-16~~

index 33151f05e744..c990c6bbffc2 100644
@@ -433,10 +433,11 @@ static void check_pending(struct bas_cardstate *ucs)
  * argument:
  *	controller state structure
  */
-static void cmd_in_timeout(unsigned long data)
+static void cmd_in_timeout(struct timer_list *t)
 {
-	struct cardstate *cs = (struct cardstate *) data;
-	struct bas_cardstate *ucs = cs->hw.bas;
+	struct bas_cardstate *ucs = from_timer(ucs, t, timer_cmd_in);
+	struct urb *urb = ucs->urb_int_in;
+	struct cardstate *cs = urb->context;
 	int rc;
 
 	if (!ucs->rcvbuf_size) {

@@ -639,10 +640,11 @@ static void int_in_work(struct work_struct *work)
  * argument:
  *	controller state structure
  */
-static void int_in_resubmit(unsigned long data)
+static void int_in_resubmit(struct timer_list *t)
 {
-	struct cardstate *cs = (struct cardstate *) data;
-	struct bas_cardstate *ucs = cs->hw.bas;
+	struct bas_cardstate *ucs = from_timer(ucs, t, timer_int_in);
+	struct urb *urb = ucs->urb_int_in;
+	struct cardstate *cs = urb->context;
 	int rc;
 
 	if (ucs->retry_int_in++ >= BAS_RETRY) {

@@ -1441,10 +1443,11 @@ static void read_iso_tasklet(unsigned long data)
  * argument:
  *	controller state structure
  */
-static void req_timeout(unsigned long data)
+static void req_timeout(struct timer_list *t)
 {
-	struct cardstate *cs = (struct cardstate *) data;
-	struct bas_cardstate *ucs = cs->hw.bas;
+	struct bas_cardstate *ucs = from_timer(ucs, t, timer_ctrl);
+	struct urb *urb = ucs->urb_int_in;
+	struct cardstate *cs = urb->context;
 	int pending;
 	unsigned long flags;

@@ -1837,10 +1840,11 @@ static void write_command_callback(struct urb *urb)
  * argument:
  *	controller state structure
  */
-static void atrdy_timeout(unsigned long data)
+static void atrdy_timeout(struct timer_list *t)
 {
-	struct cardstate *cs = (struct cardstate *) data;
-	struct bas_cardstate *ucs = cs->hw.bas;
+	struct bas_cardstate *ucs = from_timer(ucs, t, timer_atrdy);
+	struct urb *urb = ucs->urb_int_in;
+	struct cardstate *cs = urb->context;
 
 	dev_warn(cs->dev, "timeout waiting for HD_READY_SEND_ATDATA\n");

@@ -2213,10 +2217,10 @@ static int gigaset_initcshw(struct cardstate *cs)
 	}
 
 	spin_lock_init(&ucs->lock);
-	setup_timer(&ucs->timer_ctrl, req_timeout, (unsigned long) cs);
-	setup_timer(&ucs->timer_atrdy, atrdy_timeout, (unsigned long) cs);
-	setup_timer(&ucs->timer_cmd_in, cmd_in_timeout, (unsigned long) cs);
-	setup_timer(&ucs->timer_int_in, int_in_resubmit, (unsigned long) cs);
+	timer_setup(&ucs->timer_ctrl, req_timeout, 0);
+	timer_setup(&ucs->timer_atrdy, atrdy_timeout, 0);
+	timer_setup(&ucs->timer_cmd_in, cmd_in_timeout, 0);
+	timer_setup(&ucs->timer_int_in, int_in_resubmit, 0);
 	init_waitqueue_head(&ucs->waitqueue);
 	INIT_WORK(&ucs->int_in_wq, int_in_work);

Signature · 6 lines

2.7.4


-- 
Kees Cook
Pixel Security

Paul Bolle

2017-10-06 19:00 UTC #1 lore raw

drivers/isdn/gigaset/bas-gigaset.c static void check_pending(struct bas_cardstate *ucs) · line 440

Quoted 11 lines · Kees Cook

drivers/isdn/gigaset/bas-gigaset.c +4 -3

 
-static void cmd_in_timeout(unsigned long data)
+static void cmd_in_timeout(struct timer_list *t)
 {
-	struct cardstate *cs = (struct cardstate *) data;
-	struct bas_cardstate *ucs = cs->hw.bas;
+	struct bas_cardstate *ucs = from_timer(ucs, t, timer_cmd_in);
+	struct urb *urb = ucs->urb_int_in;
+	struct cardstate *cs = urb->context;

This makes me nervous. Are you sure urb->context points to a struct cardstate here and in the other two places this patch changes?

Anyhow, I'd like to have some time to do my review. So what's your timeframe here? I do hope I have at least a few weeks. (In other words: I hope gigaset isn't the only driver where the ability to use random pointers in these timer callbacks is removed.)

Thanks,

Paul Bolle

Kees Cook

author

2017-10-06 19:39 UTC #2 lore raw

Quoted 16 lines · Paul Bolle

drivers/isdn/gigaset/bas-gigaset.c static void check_pending(struct bas_cardstate *ucs) · line 440
Quoted 11 lines · Kees Cook
drivers/isdn/gigaset/bas-gigaset.c +4 -3
 
-static void cmd_in_timeout(unsigned long data)
+static void cmd_in_timeout(struct timer_list *t)
 {
-     struct cardstate *cs = (struct cardstate *) data;
-     struct bas_cardstate *ucs = cs->hw.bas;
+     struct bas_cardstate *ucs = from_timer(ucs, t, timer_cmd_in);
+     struct urb *urb = ucs->urb_int_in;
+     struct cardstate *cs = urb->context;
This makes me nervous. Are you sure urb->context points to a struct cardstate here and in the other two places this patch changes?

Quoted 4 lines

Anyhow, I'd like to have some time to do my review. So what's your timeframe here? I do hope I have at least a few weeks. (In other words: I hope gigaset isn't the only driver where the ability to use random pointers in these timer callbacks is removed.)

Hi!

I'm in no rush for any specific change. There are about 900 call sites I'm making my way through, about 2/3rd are pretty trivial, and the less obvious is what I've started sending out now, since I expect some will need some more careful review.

Thanks for taking a look at it!

-Kees

Signature · 2 lines

Kees Cook
Pixel Security

David Laight

2017-10-09 09:15 UTC #3 lore raw

From: Kees Cook

Quoted 1 line

Sent: 06 October 2017 20:40

...

Quoted 4 lines

I'm in no rush for any specific change. There are about 900 call sites I'm making my way through, about 2/3rd are pretty trivial, and the less obvious is what I've started sending out now, since I expect some will need some more careful review.

Is it worth adding a structure that contains a timer and an extra 'long' than can be used to maintain the existing API logic for the 'difficult' cases?

David

Kees Cook

author

2017-10-09 17:36 UTC #4 lore raw

Quoted 11 lines · David Laight

From: Kees Cook
Quoted 1 line
Sent: 06 October 2017 20:40
...
Quoted 4 lines
I'm in no rush for any specific change. There are about 900 call sites I'm making my way through, about 2/3rd are pretty trivial, and the less obvious is what I've started sending out now, since I expect some will need some more careful review.
Is it worth adding a structure that contains a timer and an extra 'long' than can be used to maintain the existing API logic for the 'difficult' cases?

I didn't want to have this available in the general case, since I'd like to get all the conversions actually finished. There are a couple very special cases that need this, and they have one-off structs that do this.

-Kees

Signature · 2 lines

Kees Cook
Pixel Security