Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1724645 > unrolled thread

[PATCH 00/31] struct timer_list callback argument conversion, step 1

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/31] struct timer_list callback argument conversion, step 1 Kees Cook <keescook@chromium.org> - 2017-09-01 01:40 +0200
    [PATCH 01/31] coccinelle: Improve setup_timer.cocci matching Kees Cook <keescook@chromium.org> - 2017-09-01 01:40 +0200
    [PATCH 16/31] usb/phy-isp1301-omap: Remove .data assignment Kees Cook <keescook@chromium.org> - 2017-09-01 01:40 +0200
    [PATCH 10/31] timer: Remove users of expire and data arguments to DEFINE_TIMER Kees Cook <keescook@chromium.org> - 2017-09-01 01:40 +0200
    [PATCH 04/31] timer: Remove init_timer_on_stack() in favor of setup_timer_on_stack() Kees Cook <keescook@chromium.org> - 2017-09-01 01:40 +0200
    [PATCH 15/31] timer: Additional init_timer() -> setup_timer() conversions Kees Cook <keescook@chromium.org> - 2017-09-01 01:40 +0200
    [PATCH 09/31] timer: Remove unused static initializer macros Kees Cook <keescook@chromium.org> - 2017-09-01 01:40 +0200
    [PATCH 08/31] timer: Remove users of TIMER_INITIALIZER Kees Cook <keescook@chromium.org> - 2017-09-01 01:40 +0200
    [PATCH 03/31] timer: Remove init_timer_pinned_deferrable() in favor of setup_pinned_deferrable_timer() Kees Cook <keescook@chromium.org> - 2017-09-01 01:40 +0200
    [PATCH 07/31] timer: Remove users of TIMER_DEFERRED_INITIALIZER Kees Cook <keescook@chromium.org> - 2017-09-01 01:40 +0200
    [PATCH 12/31] timer: Remove expires argument from __TIMER_INITIALIZER() Kees Cook <keescook@chromium.org> - 2017-09-01 01:40 +0200
    [PATCH 18/31] scsi/aic7xxx: Clean up timer usage Kees Cook <keescook@chromium.org> - 2017-09-01 01:40 +0200
    [PATCH 30/31] appletalk: Remove unneeded synchronization Kees Cook <keescook@chromium.org> - 2017-09-01 01:40 +0200
    [PATCH 06/31] timer: Remove init_timer_deferrable() in favor of setup_deferrable_timer() Kees Cook <keescook@chromium.org> - 2017-09-01 01:40 +0200
    [PATCH 11/31] timer: Remove expires and data arguments from DEFINE_TIMER Kees Cook <keescook@chromium.org> - 2017-09-01 01:40 +0200
      Re: [PATCH 11/31] timer: Remove expires and data arguments from DEFINE_TIMER Geert Uytterhoeven <geert@linux-m68k.org> - 2017-09-01 09:30 +0200
    [PATCH 05/31] timer: Remove init_timer_pinned() in favor of setup_pinned_timer() Kees Cook <keescook@chromium.org> - 2017-09-01 01:40 +0200
    [PATCH 17/31] media/i2c/tc358743: Initialize timer Kees Cook <keescook@chromium.org> - 2017-09-01 01:40 +0200
    [PATCH 24/31] mips/sgi-ip22: Use separate static data field with with static timer Kees Cook <keescook@chromium.org> - 2017-09-01 01:40 +0200
      Re: [PATCH 24/31] mips/sgi-ip22: Use separate static data field with  with static timer Ralf Baechle <ralf@linux-mips.org> - 2017-09-01 09:10 +0200
        Re: [PATCH 24/31] mips/sgi-ip22: Use separate static data field with  with static timer Kees Cook <keescook@chromium.org> - 2017-09-01 19:00 +0200
      Re: [PATCH 24/31] mips/sgi-ip22: Use separate static data field with  with static timer Ralf Baechle <ralf@linux-mips.org> - 2017-09-02 01:00 +0200
    [PATCH 13/31] timer: Remove meaningless .data/.function assignments Kees Cook <keescook@chromium.org> - 2017-09-01 01:40 +0200
      Re: [PATCH 13/31] timer: Remove meaningless .data/.function  assignments Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-09-01 07:10 +0200
      Re: [PATCH 13/31] timer: Remove meaningless .data/.function assignments Krzysztof Halasa <khc@pm.waw.pl> - 2017-09-01 20:10 +0200
      Re: [PATCH 13/31] timer: Remove meaningless .data/.function  assignments Jens Axboe <axboe@kernel.dk> - 2017-09-01 22:10 +0200
    [PATCH 28/31] infiniband/rdmavt: Remove redundant timer initialization Kees Cook <keescook@chromium.org> - 2017-09-01 01:50 +0200
    [PATCH 22/31] sparc/led: Use separate static data field with with static timer Kees Cook <keescook@chromium.org> - 2017-09-01 01:50 +0200
    Re: [PATCH 31/31] timer: Switch to testing for .function instead of .data Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-09-01 01:50 +0200
      Re: [PATCH 31/31] timer: Switch to testing for .function instead of .data Kees Cook <keescook@chromium.org> - 2017-09-01 02:00 +0200
        Re: [PATCH 31/31] timer: Switch to testing for .function instead of .data Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-09-01 03:10 +0200
    [PATCH 23/31] mips/sgi-ip32: Use separate static data field with with static timer Kees Cook <keescook@chromium.org> - 2017-09-01 01:50 +0200
      Re: [PATCH 23/31] mips/sgi-ip32: Use separate static data field with  with static timer Ralf Baechle <ralf@linux-mips.org> - 2017-09-01 09:10 +0200
      Re: [PATCH 23/31] mips/sgi-ip32: Use separate static data field with  with static timer Ralf Baechle <ralf@linux-mips.org> - 2017-09-02 01:00 +0200
    [PATCH 20/31] net/core: Collapse redundant sk_timer callback data assignments Kees Cook <keescook@chromium.org> - 2017-09-01 01:50 +0200
    [PATCH 27/31] usb/gadget/snps_udc_core: Move timer initialization earlier Kees Cook <keescook@chromium.org> - 2017-09-01 01:50 +0200
      Re: [PATCH 27/31] usb/gadget/snps_udc_core: Move timer initialization earlier Michal Nazarewicz <mina86@mina86.com> - 2017-09-03 23:20 +0200
        Re: [PATCH 27/31] usb/gadget/snps_udc_core: Move timer initialization earlier Kees Cook <keescook@chromium.org> - 2017-09-07 01:30 +0200
    [PATCH 21/31] s390/char/sclp: Use separate static data field with with static timer Kees Cook <keescook@chromium.org> - 2017-09-01 01:50 +0200
    [PATCH 29/31] scsi/bnx2i: Initialize timer Kees Cook <keescook@chromium.org> - 2017-09-01 01:50 +0200
    [PATCH 19/31] timer: Remove open-coded casts for .data and .function Kees Cook <keescook@chromium.org> - 2017-09-01 01:50 +0200
      Re: [PATCH 19/31] timer: Remove open-coded casts for .data and  .function Tyrel Datwyler <tyreld@linux.vnet.ibm.com> - 2017-09-01 02:30 +0200
      Re: [PATCH 19/31] timer: Remove open-coded casts for .data and  .function Tyrel Datwyler <turtle.in.the.kernel@gmail.com> - 2017-09-01 02:30 +0200
    [PATCH 25/31] net/atm/mpc: Use separate static data field with with static timer Kees Cook <keescook@chromium.org> - 2017-09-01 01:50 +0200
    [PATCH 31/31] timer: Switch to testing for .function instead of .data Kees Cook <keescook@chromium.org> - 2017-09-01 01:50 +0200
      Re: [PATCH 31/31] timer: Switch to testing for .function instead of  .data Jeff Kirsher <jeffrey.t.kirsher@intel.com> - 2017-09-01 23:40 +0200
      Re: [PATCH 31/31] timer: Switch to testing for .function instead of .data "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-09-02 16:00 +0200

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

#1725229 — Re: [PATCH 24/31] mips/sgi-ip22: Use separate static data field with with static timer

On Thu, Aug 31, 2017 at 11:59 PM, Ralf Baechle <ralf@linux-mips.org> wrote:
> On Thu, Aug 31, 2017 at 04:29:36PM -0700, Kees Cook wrote:
>
>> In preparation for changing the timer callback argument to the timer
>> pointer, move to a separate static data variable.
>>
>> Cc: Ralf Baechle <ralf@linux-mips.org>
>> Cc: James Hogan <james.hogan@imgtec.com>
>> Cc: Ingo Molnar <mingo@kernel.org>
>> Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
>> Cc: linux-mips@linux-mips.org
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>>  arch/mips/sgi-ip22/ip22-reset.c | 16 ++++++++--------
>>  1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/mips/sgi-ip22/ip22-reset.c b/arch/mips/sgi-ip22/ip22-reset.c
>> index 196b041866ac..5cc32610e6d3 100644
>> --- a/arch/mips/sgi-ip22/ip22-reset.c
>> +++ b/arch/mips/sgi-ip22/ip22-reset.c
>> @@ -38,6 +38,7 @@
>>  #define PANIC_FREQ           (HZ / 8)
>>
>>  static struct timer_list power_timer, blink_timer, debounce_timer;
>> +static unsigned long blink_timer_timeout;
>
> You're removing power_timer and debounce_timer ...

Nope, I think you misread: this only adds blink_timer_timeout;

>
>>  #define MACHINE_PANICED              1
>>  #define MACHINE_SHUTTING_DOWN        2
>> @@ -86,13 +87,13 @@ static void power_timeout(unsigned long data)
>>       sgi_machine_power_off();
>>  }
>>
>> -static void blink_timeout(unsigned long data)
>> +static void blink_timeout(unsigned long unused)
>>  {
>>       /* XXX fix this for fullhouse  */
>>       sgi_ioc_reset ^= (SGIOC_RESET_LC0OFF|SGIOC_RESET_LC1OFF);
>>       sgioc->reset = sgi_ioc_reset;
>>
>> -     mod_timer(&blink_timer, jiffies + data);
>> +     mod_timer(&blink_timer, jiffies + blink_timer_timeout);
>>  }
>>
>>  static void debounce(unsigned long data)
>> @@ -128,8 +129,8 @@ static inline void power_button(void)
>>       }
>>
>>       machine_state |= MACHINE_SHUTTING_DOWN;
>> -     blink_timer.data = POWERDOWN_FREQ;
>> -     blink_timeout(POWERDOWN_FREQ);
>> +     blink_timer_timeout = POWERDOWN_FREQ;
>> +     blink_timeout(0);
>>
>>       setup_timer(&power_timer, power_timeout, 0UL);
>
> ... but don't remove the reference to power_timer nor use of debounce_timer.
>
>>       power_timer.expires = jiffies + POWERDOWN_TIMEOUT * HZ;
>> @@ -169,8 +170,8 @@ static int panic_event(struct notifier_block *this, unsigned long event,
>>               return NOTIFY_DONE;
>>       machine_state |= MACHINE_PANICED;
>>
>> -     blink_timer.data = PANIC_FREQ;
>> -     blink_timeout(PANIC_FREQ);
>> +     blink_timer_timeout = PANIC_FREQ;
>> +     blink_timeout(0);
>>
>>       return NOTIFY_DONE;
>>  }
>> @@ -193,8 +194,7 @@ static int __init reboot_setup(void)
>>               return res;
>>       }
>>
>> -     init_timer(&blink_timer);
>> -     blink_timer.function = blink_timeout;
>> +     setup_timer(&blink_timer, blink_timeout, 0);
>>       atomic_notifier_chain_register(&panic_notifier_list, &panic_block);
>>
>>       return 0;
>
>   Ralf

-Kees


-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1725396 — Re: [PATCH 24/31] mips/sgi-ip22: Use separate static data field with with static timer

Acked-by: Ralf Baechle <ralf@linux-mips.org>

  Ralf

[toc] | [prev] | [next] | [standalone]

#1724666 — [PATCH 13/31] timer: Remove meaningless .data/.function assignments

Several timer users needlessly reset their .function/.data fields during
their timer callback, but nothing else changes them. Some users do not
use their .data field at all. Each instance is removed here.

Cc: Krzysztof Halasa <khc@pm.waw.pl>
Cc: Aditya Shankar <aditya.shankar@microchip.com>
Cc: Ganesh Krishna <ganesh.krishna@microchip.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jens Axboe <axboe@fb.com>
Cc: netdev@vger.kernel.org
Cc: linux-wireless@vger.kernel.org
Cc: devel@driverdev.osuosl.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/block/amiflop.c                           | 3 +--
 drivers/net/wan/hdlc_cisco.c                      | 2 --
 drivers/net/wan/hdlc_fr.c                         | 2 --
 drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 4 +---
 4 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/block/amiflop.c b/drivers/block/amiflop.c
index c4b1cba27178..6680d75bc857 100644
--- a/drivers/block/amiflop.c
+++ b/drivers/block/amiflop.c
@@ -323,7 +323,7 @@ static void fd_deselect (int drive)
 
 }
 
-static void motor_on_callback(unsigned long nr)
+static void motor_on_callback(unsigned long ignored)
 {
 	if (!(ciaa.pra & DSKRDY) || --on_attempts == 0) {
 		complete_all(&motor_on_completion);
@@ -344,7 +344,6 @@ static int fd_motor_on(int nr)
 		fd_select(nr);
 
 		reinit_completion(&motor_on_completion);
-		motor_on_timer.data = nr;
 		mod_timer(&motor_on_timer, jiffies + HZ/2);
 
 		on_attempts = 10;
diff --git a/drivers/net/wan/hdlc_cisco.c b/drivers/net/wan/hdlc_cisco.c
index c696d42f4502..6c98d85f2773 100644
--- a/drivers/net/wan/hdlc_cisco.c
+++ b/drivers/net/wan/hdlc_cisco.c
@@ -276,8 +276,6 @@ static void cisco_timer(unsigned long arg)
 	spin_unlock(&st->lock);
 
 	st->timer.expires = jiffies + st->settings.interval * HZ;
-	st->timer.function = cisco_timer;
-	st->timer.data = arg;
 	add_timer(&st->timer);
 }
 
diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
index de42faca076a..7da2424c28a4 100644
--- a/drivers/net/wan/hdlc_fr.c
+++ b/drivers/net/wan/hdlc_fr.c
@@ -644,8 +644,6 @@ static void fr_timer(unsigned long arg)
 			state(hdlc)->settings.t391 * HZ;
 	}
 
-	state(hdlc)->timer.function = fr_timer;
-	state(hdlc)->timer.data = arg;
 	add_timer(&state(hdlc)->timer);
 }
 
diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index 68fd5b3b8b2d..2fca2b017093 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -275,7 +275,7 @@ static void update_scan_time(void)
 		last_scanned_shadow[i].time_scan = jiffies;
 }
 
-static void remove_network_from_shadow(unsigned long arg)
+static void remove_network_from_shadow(unsigned long unused)
 {
 	unsigned long now = jiffies;
 	int i, j;
@@ -296,7 +296,6 @@ static void remove_network_from_shadow(unsigned long arg)
 	}
 
 	if (last_scanned_cnt != 0) {
-		hAgingTimer.data = arg;
 		mod_timer(&hAgingTimer, jiffies + msecs_to_jiffies(AGING_TIME));
 	}
 }
@@ -313,7 +312,6 @@ static int is_network_in_shadow(struct network_info *pstrNetworkInfo,
 	int i;
 
 	if (last_scanned_cnt == 0) {
-		hAgingTimer.data = (unsigned long)user_void;
 		mod_timer(&hAgingTimer, jiffies + msecs_to_jiffies(AGING_TIME));
 		state = -1;
 	} else {
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1724787 — Re: [PATCH 13/31] timer: Remove meaningless .data/.function assignments

On Thu, Aug 31, 2017 at 04:29:25PM -0700, Kees Cook wrote:
> Several timer users needlessly reset their .function/.data fields during
> their timer callback, but nothing else changes them. Some users do not
> use their .data field at all. Each instance is removed here.
> 
> Cc: Krzysztof Halasa <khc@pm.waw.pl>
> Cc: Aditya Shankar <aditya.shankar@microchip.com>
> Cc: Ganesh Krishna <ganesh.krishna@microchip.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jens Axboe <axboe@fb.com>
> Cc: netdev@vger.kernel.org
> Cc: linux-wireless@vger.kernel.org
> Cc: devel@driverdev.osuosl.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  drivers/block/amiflop.c                           | 3 +--
>  drivers/net/wan/hdlc_cisco.c                      | 2 --
>  drivers/net/wan/hdlc_fr.c                         | 2 --
>  drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 4 +---
>  4 files changed, 2 insertions(+), 9 deletions(-)

For the staging driver:

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[toc] | [prev] | [next] | [standalone]

#1725270 — Re: [PATCH 13/31] timer: Remove meaningless .data/.function assignments

Kees Cook <keescook@chromium.org> writes:

> Several timer users needlessly reset their .function/.data fields during
> their timer callback, but nothing else changes them. Some users do not
> use their .data field at all. Each instance is removed here.

For *wan/hdlc*
Acked-by: Krzysztof Halasa <khc@pm.waw.pl>

> --- a/drivers/net/wan/hdlc_cisco.c
> +++ b/drivers/net/wan/hdlc_cisco.c
> @@ -276,8 +276,6 @@ static void cisco_timer(unsigned long arg)
>  	spin_unlock(&st->lock);
>  
>  	st->timer.expires = jiffies + st->settings.interval * HZ;
> -	st->timer.function = cisco_timer;
> -	st->timer.data = arg;
>  	add_timer(&st->timer);
>  }
>  
> diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
> index de42faca076a..7da2424c28a4 100644
> --- a/drivers/net/wan/hdlc_fr.c
> +++ b/drivers/net/wan/hdlc_fr.c
> @@ -644,8 +644,6 @@ static void fr_timer(unsigned long arg)
>  			state(hdlc)->settings.t391 * HZ;
>  	}
>  
> -	state(hdlc)->timer.function = fr_timer;
> -	state(hdlc)->timer.data = arg;
>  	add_timer(&state(hdlc)->timer);
>  }

-- 
Krzysztof Halasa

[toc] | [prev] | [next] | [standalone]

#1725327 — Re: [PATCH 13/31] timer: Remove meaningless .data/.function assignments

On 08/31/2017 05:29 PM, Kees Cook wrote:
> Several timer users needlessly reset their .function/.data fields during
> their timer callback, but nothing else changes them. Some users do not
> use their .data field at all. Each instance is removed here.

For amiflop:

Acked-by: Jens Axboe <axboe@kernel.dk>

-- 
Jens Axboe

[toc] | [prev] | [next] | [standalone]

#1724671 — [PATCH 28/31] infiniband/rdmavt: Remove redundant timer initialization

setup_timer() is already called before the open-coded init_timer() and .data
assignment. Remove it.

Cc: Dennis Dalessandro <dennis.dalessandro@intel.com>
Cc: Doug Ledford <dledford@redhat.com>
Cc: Sean Hefty <sean.hefty@intel.com>
Cc: Hal Rosenstock <hal.rosenstock@gmail.com>
Cc: linux-rdma@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/infiniband/sw/rdmavt/qp.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/infiniband/sw/rdmavt/qp.c b/drivers/infiniband/sw/rdmavt/qp.c
index 8876ee7bc326..6b95cdb6c517 100644
--- a/drivers/infiniband/sw/rdmavt/qp.c
+++ b/drivers/infiniband/sw/rdmavt/qp.c
@@ -789,8 +789,6 @@ struct ib_qp *rvt_create_qp(struct ib_pd *ibpd,
 		atomic_set(&qp->refcount, 0);
 		atomic_set(&qp->local_ops_pending, 0);
 		init_waitqueue_head(&qp->wait);
-		init_timer(&qp->s_timer);
-		qp->s_timer.data = (unsigned long)qp;
 		INIT_LIST_HEAD(&qp->rspwait);
 		qp->state = IB_QPS_RESET;
 		qp->s_wq = swq;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1724673 — [PATCH 22/31] sparc/led: Use separate static data field with with static timer

In preparation for changing the timer callback argument to the timer
pointer, move to a separate static data variable.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Geliang Tang <geliangtang@gmail.com>
Cc: sparclinux@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/sparc/kernel/led.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/sparc/kernel/led.c b/arch/sparc/kernel/led.c
index e278bf52963b..382860e482e0 100644
--- a/arch/sparc/kernel/led.c
+++ b/arch/sparc/kernel/led.c
@@ -31,19 +31,20 @@ static inline void led_toggle(void)
 }
 
 static struct timer_list led_blink_timer;
+static unsigned long led_blink_timer_timeout;
 
-static void led_blink(unsigned long timeout)
+static void led_blink(unsigned long unused)
 {
+	unsigned long timeout = led_blink_timer_timeout;
+
 	led_toggle();
 
 	/* reschedule */
 	if (!timeout) { /* blink according to load */
 		led_blink_timer.expires = jiffies +
 			((1 + (avenrun[0] >> FSHIFT)) * HZ);
-		led_blink_timer.data = 0;
 	} else { /* blink at user specified interval */
 		led_blink_timer.expires = jiffies + (timeout * HZ);
-		led_blink_timer.data = timeout;
 	}
 	add_timer(&led_blink_timer);
 }
@@ -88,8 +89,10 @@ static ssize_t led_proc_write(struct file *file, const char __user *buffer,
 	} else if (!strcmp(buf, "toggle")) {
 		led_toggle();
 	} else if ((*buf > '0') && (*buf <= '9')) {
-		led_blink(simple_strtoul(buf, NULL, 10));
+		led_blink_timer_timeout = simple_strtoul(buf, NULL, 10);
+		led_blink(0);
 	} else if (!strcmp(buf, "load")) {
+		led_blink_timer_timeout = 0;
 		led_blink(0);
 	} else {
 		auxio_set_led(AUXIO_LED_OFF);
@@ -115,8 +118,7 @@ static struct proc_dir_entry *led;
 
 static int __init led_init(void)
 {
-	init_timer(&led_blink_timer);
-	led_blink_timer.function = led_blink;
+	setup_timer(&led_blink_timer, led_blink, 0);
 
 	led = proc_create("led", 0, NULL, &led_proc_fops);
 	if (!led)
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1724674 — Re: [PATCH 31/31] timer: Switch to testing for .function instead of .data

On Thu, Aug 31, 2017 at 4:29 PM, Kees Cook <keescook@chromium.org> wrote:
> In several places, .data is checked for initialization to gate early
> calls to del_timer_sync(). Checking for .function is equally valid, so
> switch to this in all callers.

Not seeing the rest of patches it is unclear from the patch
description why this is needed/wanted.

Thanks.

-- 
Dmitry

[toc] | [prev] | [next] | [standalone]

#1724685 — Re: [PATCH 31/31] timer: Switch to testing for .function instead of .data

On Thu, Aug 31, 2017 at 4:45 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Thu, Aug 31, 2017 at 4:29 PM, Kees Cook <keescook@chromium.org> wrote:
>> In several places, .data is checked for initialization to gate early
>> calls to del_timer_sync(). Checking for .function is equally valid, so
>> switch to this in all callers.
>
> Not seeing the rest of patches it is unclear from the patch
> description why this is needed/wanted.

The CC list would have been really giant, but here is the first patch
and the earlier series list:

https://lkml.org/lkml/2017/8/31/904
https://lkml.org/lkml/2017/8/30/760

tl;dr: We're going to switch all struct timer_list callbacks to get
the timer pointer as the argument instead of from the .data field.
This patch is one step in removing open-coded users of the .data
field.

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1724711 — Re: [PATCH 31/31] timer: Switch to testing for .function instead of .data

On Thu, Aug 31, 2017 at 4:59 PM, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Aug 31, 2017 at 4:45 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> On Thu, Aug 31, 2017 at 4:29 PM, Kees Cook <keescook@chromium.org> wrote:
>>> In several places, .data is checked for initialization to gate early
>>> calls to del_timer_sync(). Checking for .function is equally valid, so
>>> switch to this in all callers.
>>
>> Not seeing the rest of patches it is unclear from the patch
>> description why this is needed/wanted.
>
> The CC list would have been really giant, but here is the first patch
> and the earlier series list:
>
> https://lkml.org/lkml/2017/8/31/904
> https://lkml.org/lkml/2017/8/30/760
>
> tl;dr: We're going to switch all struct timer_list callbacks to get
> the timer pointer as the argument instead of from the .data field.
> This patch is one step in removing open-coded users of the .data
> field.
>

And that is exactly what should have been in the patch description.

FWIW for input bits:

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Thanks.

-- 
Dmitry

[toc] | [prev] | [next] | [standalone]

#1724675 — [PATCH 23/31] mips/sgi-ip32: Use separate static data field with with static timer

In preparation for changing the timer callback argument to the timer
pointer, move to a separate static data variable.

Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: linux-mips@linux-mips.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/mips/sgi-ip32/ip32-reset.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/mips/sgi-ip32/ip32-reset.c b/arch/mips/sgi-ip32/ip32-reset.c
index 4e263fd4deff..6636a9c686cd 100644
--- a/arch/mips/sgi-ip32/ip32-reset.c
+++ b/arch/mips/sgi-ip32/ip32-reset.c
@@ -38,6 +38,7 @@
 extern struct platform_device ip32_rtc_device;
 
 static struct timer_list power_timer, blink_timer;
+static unsigned long blink_timer_timeout;
 static int has_panicked, shutting_down;
 
 static __noreturn void ip32_poweroff(void *data)
@@ -71,11 +72,11 @@ static void ip32_machine_restart(char *cmd)
 	unreachable();
 }
 
-static void blink_timeout(unsigned long data)
+static void blink_timeout(unsigned long unused)
 {
 	unsigned long led = mace->perif.ctrl.misc ^ MACEISA_LED_RED;
 	mace->perif.ctrl.misc = led;
-	mod_timer(&blink_timer, jiffies + data);
+	mod_timer(&blink_timer, jiffies + blink_timer_timeout);
 }
 
 static void ip32_machine_halt(void)
@@ -99,8 +100,8 @@ void ip32_prepare_poweroff(void)
 	}
 
 	shutting_down = 1;
-	blink_timer.data = POWERDOWN_FREQ;
-	blink_timeout(POWERDOWN_FREQ);
+	blink_timer_timeout = POWERDOWN_FREQ;
+	blink_timeout(0);
 
 	setup_timer(&power_timer, power_timeout, 0UL);
 	power_timer.expires = jiffies + POWERDOWN_TIMEOUT * HZ;
@@ -120,8 +121,8 @@ static int panic_event(struct notifier_block *this, unsigned long event,
 	led = mace->perif.ctrl.misc | MACEISA_LED_GREEN;
 	mace->perif.ctrl.misc = led;
 
-	blink_timer.data = PANIC_FREQ;
-	blink_timeout(PANIC_FREQ);
+	blink_timer_timeout = PANIC_FREQ;
+	blink_timeout(0);
 
 	return NOTIFY_DONE;
 }
@@ -142,8 +143,7 @@ static __init int ip32_reboot_setup(void)
 	_machine_halt = ip32_machine_halt;
 	pm_power_off = ip32_machine_halt;
 
-	init_timer(&blink_timer);
-	blink_timer.function = blink_timeout;
+	setup_timer(&blink_timer, blink_timeout, 0);
 	atomic_notifier_chain_register(&panic_notifier_list, &panic_block);
 
 	return 0;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1724821 — Re: [PATCH 23/31] mips/sgi-ip32: Use separate static data field with with static timer

On Thu, Aug 31, 2017 at 04:29:35PM -0700, Kees Cook wrote:

> In preparation for changing the timer callback argument to the timer
> pointer, move to a separate static data variable.
> 
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: linux-mips@linux-mips.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  arch/mips/sgi-ip32/ip32-reset.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/mips/sgi-ip32/ip32-reset.c b/arch/mips/sgi-ip32/ip32-reset.c
> index 4e263fd4deff..6636a9c686cd 100644
> --- a/arch/mips/sgi-ip32/ip32-reset.c
> +++ b/arch/mips/sgi-ip32/ip32-reset.c
> @@ -38,6 +38,7 @@
>  extern struct platform_device ip32_rtc_device;
>  
>  static struct timer_list power_timer, blink_timer;
> +static unsigned long blink_timer_timeout;

Similar to the IP22 patch this patch removes power_timer ...

>  static int has_panicked, shutting_down;
>  
>  static __noreturn void ip32_poweroff(void *data)
> @@ -71,11 +72,11 @@ static void ip32_machine_restart(char *cmd)
>  	unreachable();
>  }
>  
> -static void blink_timeout(unsigned long data)
> +static void blink_timeout(unsigned long unused)
>  {
>  	unsigned long led = mace->perif.ctrl.misc ^ MACEISA_LED_RED;
>  	mace->perif.ctrl.misc = led;
> -	mod_timer(&blink_timer, jiffies + data);
> +	mod_timer(&blink_timer, jiffies + blink_timer_timeout);
>  }
>  
>  static void ip32_machine_halt(void)
> @@ -99,8 +100,8 @@ void ip32_prepare_poweroff(void)
>  	}
>  
>  	shutting_down = 1;
> -	blink_timer.data = POWERDOWN_FREQ;
> -	blink_timeout(POWERDOWN_FREQ);
> +	blink_timer_timeout = POWERDOWN_FREQ;
> +	blink_timeout(0);
>  
>  	setup_timer(&power_timer, power_timeout, 0UL);

... but doesn't fix the users.

>  	power_timer.expires = jiffies + POWERDOWN_TIMEOUT * HZ;
> @@ -120,8 +121,8 @@ static int panic_event(struct notifier_block *this, unsigned long event,
>  	led = mace->perif.ctrl.misc | MACEISA_LED_GREEN;
>  	mace->perif.ctrl.misc = led;
>  
> -	blink_timer.data = PANIC_FREQ;
> -	blink_timeout(PANIC_FREQ);
> +	blink_timer_timeout = PANIC_FREQ;
> +	blink_timeout(0);
>  
>  	return NOTIFY_DONE;
>  }
> @@ -142,8 +143,7 @@ static __init int ip32_reboot_setup(void)
>  	_machine_halt = ip32_machine_halt;
>  	pm_power_off = ip32_machine_halt;
>  
> -	init_timer(&blink_timer);
> -	blink_timer.function = blink_timeout;
> +	setup_timer(&blink_timer, blink_timeout, 0);
>  	atomic_notifier_chain_register(&panic_notifier_list, &panic_block);
>  
>  	return 0;

  Ralf

[toc] | [prev] | [next] | [standalone]

#1725394 — Re: [PATCH 23/31] mips/sgi-ip32: Use separate static data field with with static timer

Acked-by: Ralf Baechle <ralf@linux-mips.org>

  Ralf

[toc] | [prev] | [next] | [standalone]

#1724676 — [PATCH 20/31] net/core: Collapse redundant sk_timer callback data assignments

The core sk_timer initializer can provide the common .data assignment
instead of it being set separately in users.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Andrew Hendry <andrew.hendry@gmail.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Colin Ian King <colin.king@canonical.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: linzhang <xiaolou4617@gmail.com>
Cc: netdev@vger.kernel.org
Cc: linux-hams@vger.kernel.org
Cc: linux-x25@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 net/core/sock.c       | 2 +-
 net/netrom/nr_timer.c | 1 -
 net/rose/rose_timer.c | 1 -
 net/x25/af_x25.c      | 1 -
 net/x25/x25_timer.c   | 1 -
 5 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/net/core/sock.c b/net/core/sock.c
index ac2a404c73eb..5281a998ab32 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2623,7 +2623,7 @@ void sock_init_data(struct socket *sock, struct sock *sk)
 	sk_init_common(sk);
 	sk->sk_send_head	=	NULL;
 
-	init_timer(&sk->sk_timer);
+	setup_timer(&sk->sk_timer, NULL, (unsigned long)sk);
 
 	sk->sk_allocation	=	GFP_KERNEL;
 	sk->sk_rcvbuf		=	sysctl_rmem_default;
diff --git a/net/netrom/nr_timer.c b/net/netrom/nr_timer.c
index 94d05806a9a2..f84ce71f1f5f 100644
--- a/net/netrom/nr_timer.c
+++ b/net/netrom/nr_timer.c
@@ -45,7 +45,6 @@ void nr_init_timers(struct sock *sk)
 	setup_timer(&nr->idletimer, nr_idletimer_expiry, (unsigned long)sk);
 
 	/* initialized by sock_init_data */
-	sk->sk_timer.data     = (unsigned long)sk;
 	sk->sk_timer.function = &nr_heartbeat_expiry;
 }
 
diff --git a/net/rose/rose_timer.c b/net/rose/rose_timer.c
index bc5469d6d9cb..6baa415b199a 100644
--- a/net/rose/rose_timer.c
+++ b/net/rose/rose_timer.c
@@ -36,7 +36,6 @@ void rose_start_heartbeat(struct sock *sk)
 {
 	del_timer(&sk->sk_timer);
 
-	sk->sk_timer.data     = (unsigned long)sk;
 	sk->sk_timer.function = &rose_heartbeat_expiry;
 	sk->sk_timer.expires  = jiffies + 5 * HZ;
 
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index 5a1a98df3499..a5ac385b9120 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -414,7 +414,6 @@ static void __x25_destroy_socket(struct sock *sk)
 		/* Defer: outstanding buffers */
 		sk->sk_timer.expires  = jiffies + 10 * HZ;
 		sk->sk_timer.function = x25_destroy_timer;
-		sk->sk_timer.data = (unsigned long)sk;
 		add_timer(&sk->sk_timer);
 	} else {
 		/* drop last reference so sock_put will free */
diff --git a/net/x25/x25_timer.c b/net/x25/x25_timer.c
index 5c5db1a36399..de5cec41d100 100644
--- a/net/x25/x25_timer.c
+++ b/net/x25/x25_timer.c
@@ -36,7 +36,6 @@ void x25_init_timers(struct sock *sk)
 	setup_timer(&x25->timer, x25_timer_expiry, (unsigned long)sk);
 
 	/* initialized by sock_init_data */
-	sk->sk_timer.data     = (unsigned long)sk;
 	sk->sk_timer.function = &x25_heartbeat_expiry;
 }
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1724677 — [PATCH 27/31] usb/gadget/snps_udc_core: Move timer initialization earlier

With timer initialization made earlier at the start, there is no reason
to make del_timer_sync() calls conditionally, there by removing the
assignments and tests of the .data field.

Cc: Felipe Balbi <balbi@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Raviteja Garimella <raviteja.garimella@broadcom.com>
Cc: Michal Nazarewicz <mina86@mina86.com>
Cc: "Gustavo A. R. Silva" <garsilva@embeddedor.com>
Cc: linux-usb@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/usb/gadget/udc/snps_udc_core.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/gadget/udc/snps_udc_core.c b/drivers/usb/gadget/udc/snps_udc_core.c
index 5460e5ba1c3c..1607e901e16b 100644
--- a/drivers/usb/gadget/udc/snps_udc_core.c
+++ b/drivers/usb/gadget/udc/snps_udc_core.c
@@ -3067,14 +3067,12 @@ void udc_remove(struct udc *dev)
 	stop_timer++;
 	if (timer_pending(&udc_timer))
 		wait_for_completion(&on_exit);
-	if (udc_timer.data)
-		del_timer_sync(&udc_timer);
+	del_timer_sync(&udc_timer);
 	/* remove pollstall timer */
 	stop_pollstall_timer++;
 	if (timer_pending(&udc_pollstall_timer))
 		wait_for_completion(&on_pollstall_exit);
-	if (udc_pollstall_timer.data)
-		del_timer_sync(&udc_pollstall_timer);
+	del_timer_sync(&udc_pollstall_timer);
 	udc = NULL;
 }
 EXPORT_SYMBOL_GPL(udc_remove);
@@ -3164,9 +3162,9 @@ int udc_probe(struct udc *dev)
 	u32		reg;
 	int		retval;
 
-	/* mark timer as not initialized */
-	udc_timer.data = 0;
-	udc_pollstall_timer.data = 0;
+	/* timer init */
+	setup_timer(&udc_timer, udc_timer_function, 0);
+	setup_timer(&udc_pollstall_timer, udc_pollstall_timer_function, 0);
 
 	/* device struct setup */
 	dev->gadget.ops = &udc_ops;
@@ -3206,10 +3204,6 @@ int udc_probe(struct udc *dev)
 	if (retval)
 		goto finished;
 
-	/* timer init */
-	setup_timer(&udc_timer, udc_timer_function, 1);
-	setup_timer(&udc_pollstall_timer, udc_pollstall_timer_function, 1);
-
 	/* set SD */
 	reg = readl(&dev->regs->ctl);
 	reg |= AMD_BIT(UDC_DEVCTL_SD);
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1725761 — Re: [PATCH 27/31] usb/gadget/snps_udc_core: Move timer initialization earlier

On Thu, Aug 31 2017, Kees Cook wrote:
> With timer initialization made earlier at the start, there is no reason
> to make del_timer_sync() calls conditionally, there by removing the
> assignments and tests of the .data field.
>
> Cc: Felipe Balbi <balbi@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Raviteja Garimella <raviteja.garimella@broadcom.com>
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Cc: "Gustavo A. R. Silva" <garsilva@embeddedor.com>
> Cc: linux-usb@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  drivers/usb/gadget/udc/snps_udc_core.c | 16 +++++-----------
>  1 file changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/snps_udc_core.c b/drivers/usb/gadget/udc/snps_udc_core.c
> index 5460e5ba1c3c..1607e901e16b 100644
> --- a/drivers/usb/gadget/udc/snps_udc_core.c
> +++ b/drivers/usb/gadget/udc/snps_udc_core.c
> @@ -3067,14 +3067,12 @@ void udc_remove(struct udc *dev)
>  	stop_timer++;
>  	if (timer_pending(&udc_timer))
>  		wait_for_completion(&on_exit);
> -	if (udc_timer.data)
> -		del_timer_sync(&udc_timer);
> +	del_timer_sync(&udc_timer);
>  	/* remove pollstall timer */
>  	stop_pollstall_timer++;
>  	if (timer_pending(&udc_pollstall_timer))
>  		wait_for_completion(&on_pollstall_exit);
> -	if (udc_pollstall_timer.data)
> -		del_timer_sync(&udc_pollstall_timer);
> +	del_timer_sync(&udc_pollstall_timer);
>  	udc = NULL;
>  }
>  EXPORT_SYMBOL_GPL(udc_remove);
> @@ -3164,9 +3162,9 @@ int udc_probe(struct udc *dev)
>  	u32		reg;
>  	int		retval;
>  
> -	/* mark timer as not initialized */
> -	udc_timer.data = 0;
> -	udc_pollstall_timer.data = 0;
> +	/* timer init */
> +	setup_timer(&udc_timer, udc_timer_function, 0);
> +	setup_timer(&udc_pollstall_timer, udc_pollstall_timer_function, 0);
>  
>  	/* device struct setup */
>  	dev->gadget.ops = &udc_ops;
> @@ -3206,10 +3204,6 @@ int udc_probe(struct udc *dev)
>  	if (retval)
>  		goto finished;
>  
> -	/* timer init */
> -	setup_timer(&udc_timer, udc_timer_function, 1);
> -	setup_timer(&udc_pollstall_timer, udc_pollstall_timer_function, 1);
> -
>  	/* set SD */
>  	reg = readl(&dev->regs->ctl);
>  	reg |= AMD_BIT(UDC_DEVCTL_SD);


Stupid question, is the check in udc_remove even necessary?

udc_probe is called from udc_plat_probe:

	if (udc_probe(udc)) {
		ret = -ENODEV;
		goto exit_dma;
	}

If the call fails, udc_plat_probe cleans up after itself and noticeably
*does not* call udc_remove.  As far as I understand, if probe callback
fails, remove callback is *not* called.  Meanwhile, udc_remove is called
from the remove callback which is udc_plat_remove.  So, udc_remove can
be called only if udc_probe succeeds.

It seems to me that a better patch is:

--- a/drivers/usb/gadget/udc/snps_udc_core.c
+++ b/drivers/usb/gadget/udc/snps_udc_core.c
@@ -3067,14 +3067,12 @@ void udc_remove(struct udc *dev)
 	stop_timer++;
 	if (timer_pending(&udc_timer))
 		wait_for_completion(&on_exit);
-	if (udc_timer.data)
-		del_timer_sync(&udc_timer);
+	del_timer_sync(&udc_timer);
 	/* remove pollstall timer */
 	stop_pollstall_timer++;
 	if (timer_pending(&udc_pollstall_timer))
 		wait_for_completion(&on_pollstall_exit);
-	if (udc_pollstall_timer.data)
-		del_timer_sync(&udc_pollstall_timer);
+	del_timer_sync(&udc_pollstall_timer);
 	udc = NULL;
 }
 EXPORT_SYMBOL_GPL(udc_remove);
@@ -3164,9 +3162,9 @@ int udc_probe(struct udc *dev)
 	u32		reg;
 	int		retval;
 
-	/* mark timer as not initialized */
-	udc_timer.data = 0;
-	udc_pollstall_timer.data = 0;
-
 	/* device struct setup */
 	dev->gadget.ops = &udc_ops;

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

[toc] | [prev] | [next] | [standalone]

#1727819 — Re: [PATCH 27/31] usb/gadget/snps_udc_core: Move timer initialization earlier

On Sun, Sep 3, 2017 at 2:12 PM, Michal Nazarewicz <mina86@mina86.com> wrote:
> On Thu, Aug 31 2017, Kees Cook wrote:
>> With timer initialization made earlier at the start, there is no reason
>> to make del_timer_sync() calls conditionally, there by removing the
>> assignments and tests of the .data field.
>>
>> Cc: Felipe Balbi <balbi@kernel.org>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Raviteja Garimella <raviteja.garimella@broadcom.com>
>> Cc: Michal Nazarewicz <mina86@mina86.com>
>> Cc: "Gustavo A. R. Silva" <garsilva@embeddedor.com>
>> Cc: linux-usb@vger.kernel.org
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>>  drivers/usb/gadget/udc/snps_udc_core.c | 16 +++++-----------
>>  1 file changed, 5 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/udc/snps_udc_core.c b/drivers/usb/gadget/udc/snps_udc_core.c
>> index 5460e5ba1c3c..1607e901e16b 100644
>> --- a/drivers/usb/gadget/udc/snps_udc_core.c
>> +++ b/drivers/usb/gadget/udc/snps_udc_core.c
>> @@ -3067,14 +3067,12 @@ void udc_remove(struct udc *dev)
>>       stop_timer++;
>>       if (timer_pending(&udc_timer))
>>               wait_for_completion(&on_exit);
>> -     if (udc_timer.data)
>> -             del_timer_sync(&udc_timer);
>> +     del_timer_sync(&udc_timer);
>>       /* remove pollstall timer */
>>       stop_pollstall_timer++;
>>       if (timer_pending(&udc_pollstall_timer))
>>               wait_for_completion(&on_pollstall_exit);
>> -     if (udc_pollstall_timer.data)
>> -             del_timer_sync(&udc_pollstall_timer);
>> +     del_timer_sync(&udc_pollstall_timer);
>>       udc = NULL;
>>  }
>>  EXPORT_SYMBOL_GPL(udc_remove);
>> @@ -3164,9 +3162,9 @@ int udc_probe(struct udc *dev)
>>       u32             reg;
>>       int             retval;
>>
>> -     /* mark timer as not initialized */
>> -     udc_timer.data = 0;
>> -     udc_pollstall_timer.data = 0;
>> +     /* timer init */
>> +     setup_timer(&udc_timer, udc_timer_function, 0);
>> +     setup_timer(&udc_pollstall_timer, udc_pollstall_timer_function, 0);
>>
>>       /* device struct setup */
>>       dev->gadget.ops = &udc_ops;
>> @@ -3206,10 +3204,6 @@ int udc_probe(struct udc *dev)
>>       if (retval)
>>               goto finished;
>>
>> -     /* timer init */
>> -     setup_timer(&udc_timer, udc_timer_function, 1);
>> -     setup_timer(&udc_pollstall_timer, udc_pollstall_timer_function, 1);
>> -
>>       /* set SD */
>>       reg = readl(&dev->regs->ctl);
>>       reg |= AMD_BIT(UDC_DEVCTL_SD);
>
>
> Stupid question, is the check in udc_remove even necessary?
>
> udc_probe is called from udc_plat_probe:
>
>         if (udc_probe(udc)) {
>                 ret = -ENODEV;
>                 goto exit_dma;
>         }
>
> If the call fails, udc_plat_probe cleans up after itself and noticeably
> *does not* call udc_remove.  As far as I understand, if probe callback
> fails, remove callback is *not* called.  Meanwhile, udc_remove is called
> from the remove callback which is udc_plat_remove.  So, udc_remove can
> be called only if udc_probe succeeds.
>
> It seems to me that a better patch is:
>
> --- a/drivers/usb/gadget/udc/snps_udc_core.c
> +++ b/drivers/usb/gadget/udc/snps_udc_core.c
> @@ -3067,14 +3067,12 @@ void udc_remove(struct udc *dev)
>         stop_timer++;
>         if (timer_pending(&udc_timer))
>                 wait_for_completion(&on_exit);
> -       if (udc_timer.data)
> -               del_timer_sync(&udc_timer);
> +       del_timer_sync(&udc_timer);
>         /* remove pollstall timer */
>         stop_pollstall_timer++;
>         if (timer_pending(&udc_pollstall_timer))
>                 wait_for_completion(&on_pollstall_exit);
> -       if (udc_pollstall_timer.data)
> -               del_timer_sync(&udc_pollstall_timer);
> +       del_timer_sync(&udc_pollstall_timer);
>         udc = NULL;
>  }
>  EXPORT_SYMBOL_GPL(udc_remove);
> @@ -3164,9 +3162,9 @@ int udc_probe(struct udc *dev)
>         u32             reg;
>         int             retval;
>
> -       /* mark timer as not initialized */
> -       udc_timer.data = 0;
> -       udc_pollstall_timer.data = 0;
> -
>         /* device struct setup */
>         dev->gadget.ops = &udc_ops;
>

Okay, sounds good. I'll adjust the patch. Thanks!

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1724679 — [PATCH 21/31] s390/char/sclp: Use separate static data field with with static timer

In preparation for changing the timer callback argument to the timer
pointer, move to a separate static data variable.

Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Peter Oberparleiter <oberpar@linux.vnet.ibm.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-s390@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/s390/char/sclp.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/s390/char/sclp.c b/drivers/s390/char/sclp.c
index d0157067a80a..40794f9f1598 100644
--- a/drivers/s390/char/sclp.c
+++ b/drivers/s390/char/sclp.c
@@ -90,6 +90,7 @@ static struct sclp_req sclp_suspend_req;
 
 /* Timer for request retries. */
 static struct timer_list sclp_request_timer;
+static unsigned long sclp_request_timer_data;
 
 /* Timer for queued requests. */
 static struct timer_list sclp_queue_timer;
@@ -158,7 +159,7 @@ __sclp_set_request_timer(unsigned long time, void (*function)(unsigned long),
 {
 	del_timer(&sclp_request_timer);
 	sclp_request_timer.function = function;
-	sclp_request_timer.data = data;
+	sclp_request_timer_data = data;
 	sclp_request_timer.expires = jiffies + time;
 	add_timer(&sclp_request_timer);
 }
@@ -166,12 +167,12 @@ __sclp_set_request_timer(unsigned long time, void (*function)(unsigned long),
 /* Request timeout handler. Restart the request queue. If DATA is non-zero,
  * force restart of running request. */
 static void
-sclp_request_timeout(unsigned long data)
+sclp_request_timeout(unsigned long unused)
 {
 	unsigned long flags;
 
 	spin_lock_irqsave(&sclp_lock, flags);
-	if (data) {
+	if (sclp_request_timer_data) {
 		if (sclp_running_state == sclp_running_state_running) {
 			/* Break running state and queue NOP read event request
 			 * to get a defined interface state. */
@@ -557,7 +558,7 @@ sclp_sync_wait(void)
 		if (timer_pending(&sclp_request_timer) &&
 		    get_tod_clock_fast() > timeout &&
 		    del_timer(&sclp_request_timer))
-			sclp_request_timer.function(sclp_request_timer.data);
+			sclp_request_timer.function(0);
 		cpu_relax();
 	}
 	local_irq_disable();
@@ -914,7 +915,7 @@ static void sclp_check_handler(struct ext_code ext_code,
 
 /* Initial init mask request timed out. Modify request state to failed. */
 static void
-sclp_check_timeout(unsigned long data)
+sclp_check_timeout(unsigned long unused)
 {
 	unsigned long flags;
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1724680 — [PATCH 29/31] scsi/bnx2i: Initialize timer

There was a seemingly missing call to setup_timer() in one handler,
so add setup_timer() here to remove the open-coded initialization.

Cc: QLogic-Storage-Upstream@qlogic.com
Cc: "James E.J. Bottomley" <jejb@linux.vnet.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/scsi/bnx2i/bnx2i_iscsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c b/drivers/scsi/bnx2i/bnx2i_iscsi.c
index bffc7e91b7e5..337139dadad0 100644
--- a/drivers/scsi/bnx2i/bnx2i_iscsi.c
+++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c
@@ -1611,9 +1611,9 @@ static int bnx2i_conn_start(struct iscsi_cls_conn *cls_conn)
 	 * this should normally not sleep for a long time so it should
 	 * not disrupt the caller.
 	 */
+	setup_timer(&bnx2i_conn->ep->ofld_timer, bnx2i_ep_ofld_timer,
+		    (unsigned long) bnx2i_conn->ep);
 	bnx2i_conn->ep->ofld_timer.expires = 1 * HZ + jiffies;
-	bnx2i_conn->ep->ofld_timer.function = bnx2i_ep_ofld_timer;
-	bnx2i_conn->ep->ofld_timer.data = (unsigned long) bnx2i_conn->ep;
 	add_timer(&bnx2i_conn->ep->ofld_timer);
 	/* update iSCSI context for this conn, wait for CNIC to complete */
 	wait_event_interruptible(bnx2i_conn->ep->ofld_wait,
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web