usb: usbatm: Convert timers to use timer_setup()

PATCH In review Kees Cook 3 participants 4 messages Oct 24, 2017 – Nov 1, 2017

Subsystems (4)

USB ATM Alcatel speedtouch USB driver Conexant accessrunner USB driver

lore

Share & tools

lore

Kees Cook

author

2017-10-24 10:09 UTC #0 lore raw

PATCH 1/1 usb: usbatm: 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. Additionally corrects and on-stack timer usage.

Cc Greg Kroah-Hartman <[email protected]>

Cc Duncan Sands <[email protected]>

Cc Allen Pais <[email protected]>

Cc "Gustavo A. R. Silva" <[email protected]>

Cc [email protected]

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

---
 drivers/usb/atm/cxacru.c   | 23 ++++++++++++++++-------
 drivers/usb/atm/speedtch.c | 16 ++++++++--------
 drivers/usb/atm/usbatm.c   | 10 +++++-----
 3 files changed, 29 insertions(+), 20 deletions(-)

drivers/usb/atm/cxacru.c +16 -7

index 600a670b4feb..53c00d035e31 100644
@@ -560,21 +560,30 @@ static void cxacru_blocking_completion(struct urb *urb)
 	complete(urb->context);
 }
 
-static void cxacru_timeout_kill(unsigned long data)
+struct cxacru_timer {
+	struct timer_list timer;
+	struct urb *urb;
+};
+
+static void cxacru_timeout_kill(struct timer_list *t)
 {
-	usb_unlink_urb((struct urb *) data);
+	struct cxacru_timer *timer = from_timer(timer, t, timer);
+
+	usb_unlink_urb(timer->urb);
 }
 
 static int cxacru_start_wait_urb(struct urb *urb, struct completion *done,
 				 int *actual_length)
 {
-	struct timer_list timer;
+	struct cxacru_timer timer = {
+		.urb = urb,
+	};
 
-	setup_timer(&timer, cxacru_timeout_kill, (unsigned long)urb);
-	timer.expires = jiffies + msecs_to_jiffies(CMD_TIMEOUT);
-	add_timer(&timer);
+	timer_setup_on_stack(&timer.timer, cxacru_timeout_kill, 0);
+	mod_timer(&timer.timer, jiffies + msecs_to_jiffies(CMD_TIMEOUT));
 	wait_for_completion(done);
-	del_timer_sync(&timer);
+	del_timer_sync(&timer.timer);
+	destroy_timer_on_stack(&timer.timer);
 
 	if (actual_length)
 		*actual_length = urb->actual_length;

drivers/usb/atm/speedtch.c +8 -8

index 091db9b281f5..b7fb4beed2b8 100644
@@ -571,9 +571,10 @@ static void speedtch_check_status(struct work_struct *work)
 	}
 }
 
-static void speedtch_status_poll(unsigned long data)
+static void speedtch_status_poll(struct timer_list *t)
 {
-	struct speedtch_instance_data *instance = (void *)data;
+	struct speedtch_instance_data *instance =
+		from_timer(instance, t, status_check_timer);
 
 	schedule_work(&instance->status_check_work);

@@ -584,9 +585,10 @@ static void speedtch_status_poll(unsigned long data)
 		atm_warn(instance->usbatm, "Too many failures - disabling line status polling\n");
 }
 
-static void speedtch_resubmit_int(unsigned long data)
+static void speedtch_resubmit_int(struct timer_list *t)
 {
-	struct speedtch_instance_data *instance = (void *)data;
+	struct speedtch_instance_data *instance = from_timer(instance, t,
+							     resubmit_timer);
 	struct urb *int_urb = instance->int_urb;
 	int ret;

@@ -874,13 +876,11 @@ static int speedtch_bind(struct usbatm_data *usbatm,
 	usbatm->flags |= (use_isoc ? UDSL_USE_ISOC : 0);
 
 	INIT_WORK(&instance->status_check_work, speedtch_check_status);
-	setup_timer(&instance->status_check_timer, speedtch_status_poll,
-		    (unsigned long)instance);
+	timer_setup(&instance->status_check_timer, speedtch_status_poll, 0);
 	instance->last_status = 0xff;
 	instance->poll_delay = MIN_POLL_DELAY;
 
-	setup_timer(&instance->resubmit_timer, speedtch_resubmit_int,
-		    (unsigned long)instance);
+	timer_setup(&instance->resubmit_timer, speedtch_resubmit_int, 0);
 
 	instance->int_urb = usb_alloc_urb(0, GFP_KERNEL);

drivers/usb/atm/usbatm.c +5 -5

index 8607af758bbd..31a4e4824f23 100644
@@ -1003,18 +1003,18 @@ static int usbatm_heavy_init(struct usbatm_data *instance)
 	return 0;
 }
 
-static void usbatm_tasklet_schedule(unsigned long data)
+static void usbatm_tasklet_schedule(struct timer_list *t)
 {
-	tasklet_schedule((struct tasklet_struct *) data);
+	struct usbatm_channel *channel = from_timer(channel, t, delay);
+
+	tasklet_schedule(&channel->tasklet);
 }
 
 static void usbatm_init_channel(struct usbatm_channel *channel)
 {
 	spin_lock_init(&channel->lock);
 	INIT_LIST_HEAD(&channel->list);
-	channel->delay.function = usbatm_tasklet_schedule;
-	channel->delay.data = (unsigned long) &channel->tasklet;
-	init_timer(&channel->delay);
+	timer_setup(&channel->delay, usbatm_tasklet_schedule, 0);
 }
 
 int usbatm_usb_probe(struct usb_interface *intf, const struct usb_device_id *id,

Signature · 6 lines

2.7.4


-- 
Kees Cook
Pixel Security

Allen

Reviewed-by on v1 1/1

2017-10-25 03:37 UTC #1 lore raw

Quoted 5 lines

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. Additionally corrects and on-stack timer usage.

Quoted 7 lines

Cc Greg Kroah-Hartman <[email protected]>
Cc Duncan Sands <[email protected]>
Cc Allen Pais <[email protected]>
Cc "Gustavo A. R. Silva" <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>

Reviewed-by Allen Pais <[email protected]>

Quoted 129 lines

---
 drivers/usb/atm/cxacru.c   | 23 ++++++++++++++++-------
 drivers/usb/atm/speedtch.c | 16 ++++++++--------
 drivers/usb/atm/usbatm.c   | 10 +++++-----
 3 files changed, 29 insertions(+), 20 deletions(-)

drivers/usb/atm/cxacru.c +16 -7

index 600a670b4feb..53c00d035e31 100644
@@ -560,21 +560,30 @@ static void cxacru_blocking_completion(struct urb *urb)
        complete(urb->context);
 }
 
-static void cxacru_timeout_kill(unsigned long data)
+struct cxacru_timer {
+       struct timer_list timer;
+       struct urb *urb;
+};
+
+static void cxacru_timeout_kill(struct timer_list *t)
 {
-       usb_unlink_urb((struct urb *) data);
+       struct cxacru_timer *timer = from_timer(timer, t, timer);
+
+       usb_unlink_urb(timer->urb);
 }
 
 static int cxacru_start_wait_urb(struct urb *urb, struct completion *done,
                                 int *actual_length)
 {
-       struct timer_list timer;
+       struct cxacru_timer timer = {
+               .urb = urb,
+       };
 
-       setup_timer(&timer, cxacru_timeout_kill, (unsigned long)urb);
-       timer.expires = jiffies + msecs_to_jiffies(CMD_TIMEOUT);
-       add_timer(&timer);
+       timer_setup_on_stack(&timer.timer, cxacru_timeout_kill, 0);
+       mod_timer(&timer.timer, jiffies + msecs_to_jiffies(CMD_TIMEOUT));
        wait_for_completion(done);
-       del_timer_sync(&timer);
+       del_timer_sync(&timer.timer);
+       destroy_timer_on_stack(&timer.timer);
 
        if (actual_length)
                *actual_length = urb->actual_length;

drivers/usb/atm/speedtch.c +8 -8

index 091db9b281f5..b7fb4beed2b8 100644
@@ -571,9 +571,10 @@ static void speedtch_check_status(struct work_struct *work)
        }
 }
 
-static void speedtch_status_poll(unsigned long data)
+static void speedtch_status_poll(struct timer_list *t)
 {
-       struct speedtch_instance_data *instance = (void *)data;
+       struct speedtch_instance_data *instance =
+               from_timer(instance, t, status_check_timer);
 
        schedule_work(&instance->status_check_work);

@@ -584,9 +585,10 @@ static void speedtch_status_poll(unsigned long data)
                atm_warn(instance->usbatm, "Too many failures - disabling line status polling\n");
 }
 
-static void speedtch_resubmit_int(unsigned long data)
+static void speedtch_resubmit_int(struct timer_list *t)
 {
-       struct speedtch_instance_data *instance = (void *)data;
+       struct speedtch_instance_data *instance = from_timer(instance, t,
+                                                            resubmit_timer);
        struct urb *int_urb = instance->int_urb;
        int ret;

@@ -874,13 +876,11 @@ static int speedtch_bind(struct usbatm_data *usbatm,
        usbatm->flags |= (use_isoc ? UDSL_USE_ISOC : 0);
 
        INIT_WORK(&instance->status_check_work, speedtch_check_status);
-       setup_timer(&instance->status_check_timer, speedtch_status_poll,
-                   (unsigned long)instance);
+       timer_setup(&instance->status_check_timer, speedtch_status_poll, 0);
        instance->last_status = 0xff;
        instance->poll_delay = MIN_POLL_DELAY;
 
-       setup_timer(&instance->resubmit_timer, speedtch_resubmit_int,
-                   (unsigned long)instance);
+       timer_setup(&instance->resubmit_timer, speedtch_resubmit_int, 0);
 
        instance->int_urb = usb_alloc_urb(0, GFP_KERNEL);

drivers/usb/atm/usbatm.c +5 -6

index 8607af758bbd..31a4e4824f23 100644
@@ -1003,18 +1003,18 @@ static int usbatm_heavy_init(struct usbatm_data *instance)
        return 0;
 }
 
-static void usbatm_tasklet_schedule(unsigned long data)
+static void usbatm_tasklet_schedule(struct timer_list *t)
 {
-       tasklet_schedule((struct tasklet_struct *) data);
+       struct usbatm_channel *channel = from_timer(channel, t, delay);
+
+       tasklet_schedule(&channel->tasklet);
 }
 
 static void usbatm_init_channel(struct usbatm_channel *channel)
 {
        spin_lock_init(&channel->lock);
        INIT_LIST_HEAD(&channel->list);
-       channel->delay.function = usbatm_tasklet_schedule;
-       channel->delay.data = (unsigned long) &channel->tasklet;
-       init_timer(&channel->delay);
+       timer_setup(&channel->delay, usbatm_tasklet_schedule, 0);
 }
 
 int usbatm_usb_probe(struct usb_interface *intf, const struct usb_device_id *id,
--

2.7.4

--
Kees Cook
Pixel Security

Signature · 1 line

       - Allen

Kees Cook

author

2017-10-25 14:16 UTC #2 lore raw

In reply to Kees Cook

This also uses timer_setup_on_stack() (only in -next). If it's okay, I'll carry it in the timers tree.

Thanks!

-Kees

Quoted 141 lines · Kees Cook

Cc Greg Kroah-Hartman <[email protected]>

Cc Duncan Sands <[email protected]>

Cc Allen Pais <[email protected]>

Cc "Gustavo A. R. Silva" <[email protected]>

Cc [email protected]

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

---
 drivers/usb/atm/cxacru.c   | 23 ++++++++++++++++-------
 drivers/usb/atm/speedtch.c | 16 ++++++++--------
 drivers/usb/atm/usbatm.c   | 10 +++++-----
 3 files changed, 29 insertions(+), 20 deletions(-)

drivers/usb/atm/cxacru.c +16 -7

index 600a670b4feb..53c00d035e31 100644
@@ -560,21 +560,30 @@ static void cxacru_blocking_completion(struct urb *urb)
        complete(urb->context);
 }
 
-static void cxacru_timeout_kill(unsigned long data)
+struct cxacru_timer {
+       struct timer_list timer;
+       struct urb *urb;
+};
+
+static void cxacru_timeout_kill(struct timer_list *t)
 {
-       usb_unlink_urb((struct urb *) data);
+       struct cxacru_timer *timer = from_timer(timer, t, timer);
+
+       usb_unlink_urb(timer->urb);
 }
 
 static int cxacru_start_wait_urb(struct urb *urb, struct completion *done,
                                 int *actual_length)
 {
-       struct timer_list timer;
+       struct cxacru_timer timer = {
+               .urb = urb,
+       };
 
-       setup_timer(&timer, cxacru_timeout_kill, (unsigned long)urb);
-       timer.expires = jiffies + msecs_to_jiffies(CMD_TIMEOUT);
-       add_timer(&timer);
+       timer_setup_on_stack(&timer.timer, cxacru_timeout_kill, 0);
+       mod_timer(&timer.timer, jiffies + msecs_to_jiffies(CMD_TIMEOUT));
        wait_for_completion(done);
-       del_timer_sync(&timer);
+       del_timer_sync(&timer.timer);
+       destroy_timer_on_stack(&timer.timer);
 
        if (actual_length)
                *actual_length = urb->actual_length;

drivers/usb/atm/speedtch.c +8 -8

index 091db9b281f5..b7fb4beed2b8 100644
@@ -571,9 +571,10 @@ static void speedtch_check_status(struct work_struct *work)
        }
 }
 
-static void speedtch_status_poll(unsigned long data)
+static void speedtch_status_poll(struct timer_list *t)
 {
-       struct speedtch_instance_data *instance = (void *)data;
+       struct speedtch_instance_data *instance =
+               from_timer(instance, t, status_check_timer);
 
        schedule_work(&instance->status_check_work);

@@ -584,9 +585,10 @@ static void speedtch_status_poll(unsigned long data)
                atm_warn(instance->usbatm, "Too many failures - disabling line status polling\n");
 }
 
-static void speedtch_resubmit_int(unsigned long data)
+static void speedtch_resubmit_int(struct timer_list *t)
 {
-       struct speedtch_instance_data *instance = (void *)data;
+       struct speedtch_instance_data *instance = from_timer(instance, t,
+                                                            resubmit_timer);
        struct urb *int_urb = instance->int_urb;
        int ret;

@@ -874,13 +876,11 @@ static int speedtch_bind(struct usbatm_data *usbatm,
        usbatm->flags |= (use_isoc ? UDSL_USE_ISOC : 0);
 
        INIT_WORK(&instance->status_check_work, speedtch_check_status);
-       setup_timer(&instance->status_check_timer, speedtch_status_poll,
-                   (unsigned long)instance);
+       timer_setup(&instance->status_check_timer, speedtch_status_poll, 0);
        instance->last_status = 0xff;
        instance->poll_delay = MIN_POLL_DELAY;
 
-       setup_timer(&instance->resubmit_timer, speedtch_resubmit_int,
-                   (unsigned long)instance);
+       timer_setup(&instance->resubmit_timer, speedtch_resubmit_int, 0);
 
        instance->int_urb = usb_alloc_urb(0, GFP_KERNEL);

drivers/usb/atm/usbatm.c +5 -6

index 8607af758bbd..31a4e4824f23 100644
@@ -1003,18 +1003,18 @@ static int usbatm_heavy_init(struct usbatm_data *instance)
        return 0;
 }
 
-static void usbatm_tasklet_schedule(unsigned long data)
+static void usbatm_tasklet_schedule(struct timer_list *t)
 {
-       tasklet_schedule((struct tasklet_struct *) data);
+       struct usbatm_channel *channel = from_timer(channel, t, delay);
+
+       tasklet_schedule(&channel->tasklet);
 }
 
 static void usbatm_init_channel(struct usbatm_channel *channel)
 {
        spin_lock_init(&channel->lock);
        INIT_LIST_HEAD(&channel->list);
-       channel->delay.function = usbatm_tasklet_schedule;
-       channel->delay.data = (unsigned long) &channel->tasklet;
-       init_timer(&channel->delay);
+       timer_setup(&channel->delay, usbatm_tasklet_schedule, 0);
 }
 
 int usbatm_usb_probe(struct usb_interface *intf, const struct usb_device_id *id,
--

2.7.4

--
Kees Cook
Pixel Security

Signature · 2 lines

Kees Cook
Pixel Security

Greg Kroah-Hartman

maintainer Acked-by

2017-11-01 15:55 UTC #3 lore raw

Quoted 2 lines · Kees Cook

This also uses timer_setup_on_stack() (only in -next). If it's okay, I'll carry it in the timers tree.

Fine with me:

Acked-by Greg Kroah-Hartman <[email protected]>