dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [RFC][PATCH v3 00/33] timers: Use timer_shutdown*() before freeing timers
@ 2022-11-04  5:40 Steven Rostedt
  2022-11-04  5:41 ` [RFC][PATCH v3 12/33] timers: dma-buf: Use timer_shutdown_sync() before freeing timer Steven Rostedt
                   ` (4 more replies)
  0 siblings, 5 replies; 18+ messages in thread
From: Steven Rostedt @ 2022-11-04  5:40 UTC (permalink / raw)
  To: linux-kernel
  Cc: alsa-devel, linux-staging, linux-doc, dri-devel, linaro-mm-sig,
	Thomas Gleixner, linux-leds, drbd-dev, linux-s390, linux-nilfs,
	linux-scsi, linux-sh, linux-atm-general, linux-afs, lvs-devel,
	linux-acpi, coreteam, intel-wired-lan, linux-input,
	tipc-discussion, linux-ext4, Guenter Roeck, linux-media, bridge,
	linux-pm, intel-gfx, rcu, cgroups, openipmi-developer,
	Anna-Maria Gleixner, linux-edac, linux-block, linux-nfs,
	linux-parisc, Stephen Boyd, netdev, linux-usb, linux-wireless,
	linux-kernel, linux-bluetooth, netfilter-devel, Andrew Morton,
	Linus Torvalds


Back in April, I posted an RFC patch set to help mitigate a common issue
where a timer gets armed just before it is freed, and when the timer
goes off, it crashes in the timer code without any evidence of who the
culprit was. I got side tracked and never finished up on that patch set.
Since this type of crash is still our #1 crash we are seeing in the field,
it has become a priority again to finish it.

This is v3 of that patch set. Thomas Gleixner posted an untested version
that makes timer->function NULL as the flag that it is shutdown. I took that
code, tested it (fixed it up), added more comments, and changed the
name to timer_shutdown_sync(). I also converted it to use WARN_ON_ONCE()
instead of just WARN_ON() as Linus asked for.

I then created a trivial coccinelle script to find where del_timer*()
is called before being freed, and converted them all to timer_shutdown*()
(There was a couple that still used del_timer() instead of del_timer_sync()).

I also updated DEBUG_OBJECTS_TIMERS to check from where the timer is ever
armed, to calling of timer_shutdown_sync(), and it will trigger if a timer
is freed in between. The current way is to only check if the timer is armed,
but that means it only triggers if the race condition is hit, and with
experience, it's not run on enough machines to catch all of them. By triggering
it from the time the timer is armed to the time it is shutdown, it catches
all potential cases even if the race condition is not hit.

I went though the result of the cocinelle script, and updated the locations.
Some locations were caught by DEBUG_OBJECTS_TIMERS as the coccinelle script
only checked for timers being freed in the same function as the del_timer*().

Ideally, I would have the first patch go into this rc cycle, which is mostly
non functional as it will allow the other patches to come in via the respective
subsystems in the next merge window.

Changes since v2: https://lore.kernel.org/all/20221027150525.753064657@goodmis.org/

 - Talking with Thomas Gleixner, he wanted a better name space and to remove
   the "del_" portion of the API.

 - Since there's now a shutdown interface that does not synchronize, to keep
   it closer to del_timer() and del_timer_sync(), the API is now:

    timer_shutdown() - same as del_timer() but deactivates the timer.

    timer_shutdown_sync() - same as del_timer_sync() but deactivates the timer.

 - Added a few more locations that got converted.

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
trace/timers

Head SHA1: 25106f0bb7968b3e8c746a7853f44b51840746c3


Steven Rostedt (Google) (33):
      timers: Add timer_shutdown_sync() and timer_shutdown() to be called before freeing timers
      timers: s390/cmm: Use timer_shutdown_sync() before freeing timer
      timers: sh: Use timer_shutdown_sync() before freeing timer
      timers: block: Use timer_shutdown_sync() before freeing timer
      timers: ACPI: Use timer_shutdown_sync() before freeing timer
      timers: atm: Use timer_shutdown_sync() before freeing timer
      timers: PM: Use timer_shutdown_sync()
      timers: Bluetooth: Use timer_shutdown_sync() before freeing timer
      timers: hangcheck: Use timer_shutdown_sync() before freeing timer
      timers: ipmi: Use timer_shutdown_sync() before freeing timer
      random: use timer_shutdown_sync() before freeing timer
      timers: dma-buf: Use timer_shutdown_sync() before freeing timer
      timers: drm: Use timer_shutdown_sync() before freeing timer
      timers: HID: Use timer_shutdown_sync() before freeing timer
      timers: Input: Use timer_shutdown_sync() before freeing timer
      timers: mISDN: Use timer_shutdown_sync() before freeing timer
      timers: leds: Use timer_shutdown_sync() before freeing timer
      timers: media: Use timer_shutdown_sync() before freeing timer
      timers: net: Use timer_shutdown_sync() before freeing timer
      timers: usb: Use timer_shutdown_sync() before freeing timer
      timers: cgroup: Use timer_shutdown_sync() before freeing timer
      timers: workqueue: Use timer_shutdown_sync() before freeing timer
      timers: nfc: pn533: Use timer_shutdown_sync() before freeing timer
      timers: pcmcia: Use timer_shutdown_sync() before freeing timer
      timers: scsi: Use timer_shutdown_sync() and timer_shutdown() before freeing timer
      timers: tty: Use timer_shutdown_sync() before freeing timer
      timers: ext4: Use timer_shutdown_sync() before freeing timer
      timers: fs/nilfs2: Use timer_shutdown_sync() before freeing timer
      timers: ALSA: Use timer_shutdown_sync() before freeing timer
      timers: jbd2: Use timer_shutdown() before freeing timer
      timers: sched/psi: Use timer_shutdown_sync() before freeing timer
      timers: x86/mce: Use __init_timer() for resetting timers
      timers: Expand DEBUG_OBJECTS_TIMER to check if it ever was used

----
 .../RCU/Design/Requirements/Requirements.rst       |   2 +-
 Documentation/core-api/local_ops.rst               |   2 +-
 Documentation/kernel-hacking/locking.rst           |   5 +
 arch/s390/mm/cmm.c                                 |   4 +-
 arch/sh/drivers/push-switch.c                      |   2 +-
 arch/x86/kernel/cpu/mce/core.c                     |  14 ++-
 block/blk-iocost.c                                 |   2 +-
 block/blk-iolatency.c                              |   2 +-
 block/blk-stat.c                                   |   2 +-
 block/blk-throttle.c                               |   2 +-
 block/kyber-iosched.c                              |   2 +-
 drivers/acpi/apei/ghes.c                           |   2 +-
 drivers/atm/idt77105.c                             |   4 +-
 drivers/atm/idt77252.c                             |   4 +-
 drivers/atm/iphase.c                               |   2 +-
 drivers/base/power/wakeup.c                        |   7 +-
 drivers/block/drbd/drbd_main.c                     |   2 +-
 drivers/block/loop.c                               |   2 +-
 drivers/block/sunvdc.c                             |   2 +-
 drivers/bluetooth/hci_bcsp.c                       |   2 +-
 drivers/bluetooth/hci_h5.c                         |   2 +-
 drivers/bluetooth/hci_qca.c                        |   4 +-
 drivers/char/hangcheck-timer.c                     |   4 +-
 drivers/char/ipmi/ipmi_msghandler.c                |   2 +-
 drivers/char/ipmi/ipmi_ssif.c                      |   4 +-
 drivers/char/random.c                              |   2 +-
 drivers/dma-buf/st-dma-fence.c                     |   2 +-
 drivers/gpu/drm/gud/gud_pipe.c                     |   2 +-
 drivers/gpu/drm/i915/i915_sw_fence.c               |   2 +-
 drivers/hid/hid-wiimote-core.c                     |   2 +-
 drivers/input/keyboard/locomokbd.c                 |   2 +-
 drivers/input/keyboard/omap-keypad.c               |   2 +-
 drivers/input/mouse/alps.c                         |   2 +-
 drivers/input/serio/hil_mlc.c                      |   2 +-
 drivers/input/serio/hp_sdc.c                       |   2 +-
 drivers/isdn/hardware/mISDN/hfcmulti.c             |   6 +-
 drivers/isdn/mISDN/l1oip_core.c                    |   4 +-
 drivers/isdn/mISDN/timerdev.c                      |   4 +-
 drivers/leds/trigger/ledtrig-activity.c            |   2 +-
 drivers/leds/trigger/ledtrig-heartbeat.c           |   2 +-
 drivers/leds/trigger/ledtrig-pattern.c             |   2 +-
 drivers/leds/trigger/ledtrig-transient.c           |   2 +-
 drivers/media/pci/ivtv/ivtv-driver.c               |   2 +-
 drivers/media/usb/pvrusb2/pvrusb2-hdw.c            |  18 ++--
 drivers/media/usb/s2255/s2255drv.c                 |   4 +-
 drivers/net/ethernet/intel/i40e/i40e_main.c        |   6 +-
 drivers/net/ethernet/marvell/sky2.c                |   2 +-
 drivers/net/ethernet/sun/sunvnet.c                 |   2 +-
 drivers/net/usb/sierra_net.c                       |   2 +-
 drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c   |   2 +-
 drivers/net/wireless/intersil/hostap/hostap_ap.c   |   2 +-
 drivers/net/wireless/marvell/mwifiex/main.c        |   2 +-
 drivers/net/wireless/microchip/wilc1000/hif.c      |   8 +-
 drivers/nfc/pn533/pn533.c                          |   2 +-
 drivers/nfc/pn533/uart.c                           |   2 +-
 drivers/pcmcia/bcm63xx_pcmcia.c                    |   2 +-
 drivers/pcmcia/electra_cf.c                        |   2 +-
 drivers/pcmcia/omap_cf.c                           |   2 +-
 drivers/pcmcia/pd6729.c                            |   4 +-
 drivers/pcmcia/yenta_socket.c                      |   4 +-
 drivers/scsi/qla2xxx/qla_edif.c                    |   4 +-
 drivers/scsi/scsi_lib.c                            |   1 +
 drivers/staging/media/atomisp/i2c/atomisp-lm3554.c |   2 +-
 drivers/tty/n_gsm.c                                |   2 +-
 drivers/tty/sysrq.c                                |   2 +-
 drivers/usb/gadget/udc/m66592-udc.c                |   2 +-
 drivers/usb/serial/garmin_gps.c                    |   2 +-
 drivers/usb/serial/mos7840.c                       |   2 +-
 fs/ext4/super.c                                    |   2 +-
 fs/jbd2/journal.c                                  |   2 +
 fs/nilfs2/segment.c                                |   2 +-
 include/linux/timer.h                              | 100 +++++++++++++++++--
 include/linux/workqueue.h                          |   4 +-
 kernel/cgroup/cgroup.c                             |   2 +-
 kernel/sched/psi.c                                 |   1 +
 kernel/time/timer.c                                | 106 ++++++++++++++-------
 kernel/workqueue.c                                 |   4 +-
 net/802/garp.c                                     |   2 +-
 net/802/mrp.c                                      |   2 +-
 net/bridge/br_multicast.c                          |   6 +-
 net/bridge/br_multicast_eht.c                      |   4 +-
 net/core/gen_estimator.c                           |   2 +-
 net/core/neighbour.c                               |   2 +
 net/ipv4/inet_connection_sock.c                    |   2 +-
 net/ipv4/inet_timewait_sock.c                      |   3 +-
 net/ipv4/ipmr.c                                    |   2 +-
 net/ipv6/ip6mr.c                                   |   2 +-
 net/mac80211/mesh_pathtbl.c                        |   2 +-
 net/netfilter/ipset/ip_set_list_set.c              |   2 +-
 net/netfilter/ipvs/ip_vs_lblc.c                    |   2 +-
 net/netfilter/ipvs/ip_vs_lblcr.c                   |   2 +-
 net/netfilter/xt_LED.c                             |   2 +-
 net/rxrpc/conn_object.c                            |   2 +-
 net/sched/cls_flow.c                               |   2 +-
 net/sunrpc/svc.c                                   |   2 +-
 net/sunrpc/xprt.c                                  |   2 +-
 net/tipc/discover.c                                |   2 +-
 net/tipc/monitor.c                                 |   2 +-
 sound/i2c/other/ak4117.c                           |   2 +-
 sound/synth/emux/emux.c                            |   2 +-
 100 files changed, 310 insertions(+), 175 deletions(-)

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [RFC][PATCH v3 12/33] timers: dma-buf: Use timer_shutdown_sync() before freeing timer
  2022-11-04  5:40 [RFC][PATCH v3 00/33] timers: Use timer_shutdown*() before freeing timers Steven Rostedt
@ 2022-11-04  5:41 ` Steven Rostedt
  2022-11-04  5:54   ` Steven Rostedt
  2022-11-04  5:41 ` [RFC][PATCH v3 13/33] timers: drm: " Steven Rostedt
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 18+ messages in thread
From: Steven Rostedt @ 2022-11-04  5:41 UTC (permalink / raw)
  To: linux-kernel
  Cc: Christian König, Stephen Boyd, Linus Torvalds, dri-devel,
	Sumit Semwal, linaro-mm-sig, Thomas Gleixner,
	Anna-Maria Gleixner, Andrew Morton, Guenter Roeck, linux-media

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

Before a timer is freed, timer_shutdown_sync() must be called.

Link: https://lore.kernel.org/all/20220407161745.7d6754b3@gandalf.local.home/

Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: "Christian König" <christian.koenig@amd.com>
Cc: linux-media@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linaro-mm-sig@lists.linaro.org
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 drivers/dma-buf/st-dma-fence.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma-buf/st-dma-fence.c b/drivers/dma-buf/st-dma-fence.c
index fb6e0a6ae2c9..5d3e7b503501 100644
--- a/drivers/dma-buf/st-dma-fence.c
+++ b/drivers/dma-buf/st-dma-fence.c
@@ -412,7 +412,7 @@ static int test_wait_timeout(void *arg)
 
 	err = 0;
 err_free:
-	del_timer_sync(&wt.timer);
+	timer_shutdown_sync(&wt.timer);
 	destroy_timer_on_stack(&wt.timer);
 	dma_fence_signal(wt.f);
 	dma_fence_put(wt.f);
-- 
2.35.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [RFC][PATCH v3 13/33] timers: drm: Use timer_shutdown_sync() before freeing timer
  2022-11-04  5:40 [RFC][PATCH v3 00/33] timers: Use timer_shutdown*() before freeing timers Steven Rostedt
  2022-11-04  5:41 ` [RFC][PATCH v3 12/33] timers: dma-buf: Use timer_shutdown_sync() before freeing timer Steven Rostedt
@ 2022-11-04  5:41 ` Steven Rostedt
  2022-11-04  5:55   ` Steven Rostedt
  2022-11-04  8:48   ` Tvrtko Ursulin
  2022-11-04 17:00 ` [RFC][PATCH v3 00/33] timers: Use timer_shutdown*() before freeing timers Linus Torvalds
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 18+ messages in thread
From: Steven Rostedt @ 2022-11-04  5:41 UTC (permalink / raw)
  To: linux-kernel
  Cc: Tvrtko Ursulin, Stephen Boyd, Linus Torvalds, intel-gfx,
	Noralf Trønnes, dri-devel, Rodrigo Vivi, Thomas Gleixner,
	Anna-Maria Gleixner, Andrew Morton, Guenter Roeck

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

Before a timer is freed, timer_shutdown_sync() must be called.

Link: https://lore.kernel.org/all/20220407161745.7d6754b3@gandalf.local.home/

Cc: "Noralf Trønnes" <noralf@tronnes.org>
Cc: David Airlie <airlied@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org
Cc: intel-gfx@lists.freedesktop.org
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 drivers/gpu/drm/gud/gud_pipe.c       | 2 +-
 drivers/gpu/drm/i915/i915_sw_fence.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
index 7c6dc2bcd14a..08429bdd57cf 100644
--- a/drivers/gpu/drm/gud/gud_pipe.c
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -272,7 +272,7 @@ static int gud_usb_bulk(struct gud_device *gdrm, size_t len)
 
 	usb_sg_wait(&ctx.sgr);
 
-	if (!del_timer_sync(&ctx.timer))
+	if (!timer_shutdown_sync(&ctx.timer))
 		ret = -ETIMEDOUT;
 	else if (ctx.sgr.status < 0)
 		ret = ctx.sgr.status;
diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
index 6fc0d1b89690..bfaa9a67dc35 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence.c
+++ b/drivers/gpu/drm/i915/i915_sw_fence.c
@@ -465,7 +465,7 @@ static void irq_i915_sw_fence_work(struct irq_work *wrk)
 	struct i915_sw_dma_fence_cb_timer *cb =
 		container_of(wrk, typeof(*cb), work);
 
-	del_timer_sync(&cb->timer);
+	timer_shutdown_sync(&cb->timer);
 	dma_fence_put(cb->dma);
 
 	kfree_rcu(cb, rcu);
-- 
2.35.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [RFC][PATCH v3 12/33] timers: dma-buf: Use timer_shutdown_sync() before freeing timer
  2022-11-04  5:41 ` [RFC][PATCH v3 12/33] timers: dma-buf: Use timer_shutdown_sync() before freeing timer Steven Rostedt
@ 2022-11-04  5:54   ` Steven Rostedt
  2022-11-04  7:15     ` Christian König
  0 siblings, 1 reply; 18+ messages in thread
From: Steven Rostedt @ 2022-11-04  5:54 UTC (permalink / raw)
  To: linux-kernel
  Cc: Christian König, Stephen Boyd, Linus Torvalds, dri-devel,
	Sumit Semwal, linaro-mm-sig, Thomas Gleixner,
	Anna-Maria Gleixner, Andrew Morton, Guenter Roeck, linux-media

[ Once again, quilt fails the MIME coding ]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

Before a timer is freed, timer_shutdown_sync() must be called.

Link: https://lore.kernel.org/all/20220407161745.7d6754b3@gandalf.local.home/

Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: "Christian König" <christian.koenig@amd.com>
Cc: linux-media@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linaro-mm-sig@lists.linaro.org
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 drivers/dma-buf/st-dma-fence.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma-buf/st-dma-fence.c b/drivers/dma-buf/st-dma-fence.c
index fb6e0a6ae2c9..5d3e7b503501 100644
--- a/drivers/dma-buf/st-dma-fence.c
+++ b/drivers/dma-buf/st-dma-fence.c
@@ -412,7 +412,7 @@ static int test_wait_timeout(void *arg)
 
 	err = 0;
 err_free:
-	del_timer_sync(&wt.timer);
+	timer_shutdown_sync(&wt.timer);
 	destroy_timer_on_stack(&wt.timer);
 	dma_fence_signal(wt.f);
 	dma_fence_put(wt.f);
-- 
2.35.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [RFC][PATCH v3 13/33] timers: drm: Use timer_shutdown_sync() before freeing timer
  2022-11-04  5:41 ` [RFC][PATCH v3 13/33] timers: drm: " Steven Rostedt
@ 2022-11-04  5:55   ` Steven Rostedt
  2022-11-04  8:48   ` Tvrtko Ursulin
  1 sibling, 0 replies; 18+ messages in thread
From: Steven Rostedt @ 2022-11-04  5:55 UTC (permalink / raw)
  To: linux-kernel
  Cc: Tvrtko Ursulin, Stephen Boyd, Linus Torvalds, intel-gfx,
	Noralf Trønnes, dri-devel, Rodrigo Vivi, Thomas Gleixner,
	Anna-Maria Gleixner, Andrew Morton, Guenter Roeck

[ Once again, quilt fails the MIME coding ]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

Before a timer is freed, timer_shutdown_sync() must be called.

Link: https://lore.kernel.org/all/20220407161745.7d6754b3@gandalf.local.home/

Cc: "Noralf Trønnes" <noralf@tronnes.org>
Cc: David Airlie <airlied@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org
Cc: intel-gfx@lists.freedesktop.org
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 drivers/gpu/drm/gud/gud_pipe.c       | 2 +-
 drivers/gpu/drm/i915/i915_sw_fence.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
index 7c6dc2bcd14a..08429bdd57cf 100644
--- a/drivers/gpu/drm/gud/gud_pipe.c
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -272,7 +272,7 @@ static int gud_usb_bulk(struct gud_device *gdrm, size_t len)
 
 	usb_sg_wait(&ctx.sgr);
 
-	if (!del_timer_sync(&ctx.timer))
+	if (!timer_shutdown_sync(&ctx.timer))
 		ret = -ETIMEDOUT;
 	else if (ctx.sgr.status < 0)
 		ret = ctx.sgr.status;
diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
index 6fc0d1b89690..bfaa9a67dc35 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence.c
+++ b/drivers/gpu/drm/i915/i915_sw_fence.c
@@ -465,7 +465,7 @@ static void irq_i915_sw_fence_work(struct irq_work *wrk)
 	struct i915_sw_dma_fence_cb_timer *cb =
 		container_of(wrk, typeof(*cb), work);
 
-	del_timer_sync(&cb->timer);
+	timer_shutdown_sync(&cb->timer);
 	dma_fence_put(cb->dma);
 
 	kfree_rcu(cb, rcu);
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [RFC][PATCH v3 12/33] timers: dma-buf: Use timer_shutdown_sync() before freeing timer
  2022-11-04  5:54   ` Steven Rostedt
@ 2022-11-04  7:15     ` Christian König
  2022-11-04 18:58       ` Steven Rostedt
  0 siblings, 1 reply; 18+ messages in thread
From: Christian König @ 2022-11-04  7:15 UTC (permalink / raw)
  To: Steven Rostedt, linux-kernel
  Cc: Stephen Boyd, Linus Torvalds, dri-devel, Sumit Semwal,
	linaro-mm-sig, Thomas Gleixner, Anna-Maria Gleixner,
	Andrew Morton, Guenter Roeck, linux-media

Am 04.11.22 um 06:54 schrieb Steven Rostedt:
> [ Once again, quilt fails the MIME coding ]
>
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
>
> Before a timer is freed, timer_shutdown_sync() must be called.
>
> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20220407161745.7d6754b3%40gandalf.local.home%2F&amp;data=05%7C01%7Cchristian.koenig%40amd.com%7Ca18ff1d0a7e442a1283808dabe29148d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638031380931371691%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=XZgwOy0u20L1AxOjhUpWICodbSn2VYhh6YGSykjUegQ%3D&amp;reserved=0
>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: "Christian König" <christian.koenig@amd.com>
> Cc: linux-media@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linaro-mm-sig@lists.linaro.org
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>   drivers/dma-buf/st-dma-fence.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/dma-buf/st-dma-fence.c b/drivers/dma-buf/st-dma-fence.c
> index fb6e0a6ae2c9..5d3e7b503501 100644
> --- a/drivers/dma-buf/st-dma-fence.c
> +++ b/drivers/dma-buf/st-dma-fence.c
> @@ -412,7 +412,7 @@ static int test_wait_timeout(void *arg)
>   
>   	err = 0;
>   err_free:
> -	del_timer_sync(&wt.timer);
> +	timer_shutdown_sync(&wt.timer);

Mhm, what exactly is the benefit of renaming the function?

Not that I'm against the change, but my thinking is more if there are 
more functions which don't re-arm the time than those which do that then 
why not forbid it in general?

Regards,
Christian.

>   	destroy_timer_on_stack(&wt.timer);
>   	dma_fence_signal(wt.f);
>   	dma_fence_put(wt.f);


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC][PATCH v3 13/33] timers: drm: Use timer_shutdown_sync() before freeing timer
  2022-11-04  5:41 ` [RFC][PATCH v3 13/33] timers: drm: " Steven Rostedt
  2022-11-04  5:55   ` Steven Rostedt
@ 2022-11-04  8:48   ` Tvrtko Ursulin
  2022-11-04 19:02     ` Steven Rostedt
  1 sibling, 1 reply; 18+ messages in thread
From: Tvrtko Ursulin @ 2022-11-04  8:48 UTC (permalink / raw)
  To: Steven Rostedt, linux-kernel
  Cc: Stephen Boyd, Linus Torvalds, intel-gfx, Noralf Trønnes,
	dri-devel, Rodrigo Vivi, Thomas Gleixner, Anna-Maria Gleixner,
	Andrew Morton, Guenter Roeck


Hi,

On 04/11/2022 05:41, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Before a timer is freed, timer_shutdown_sync() must be called.
> 
> Link: https://lore.kernel.org/all/20220407161745.7d6754b3@gandalf.local.home/
> 
> Cc: "Noralf Trønnes" <noralf@tronnes.org>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: intel-gfx@lists.freedesktop.org
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>   drivers/gpu/drm/gud/gud_pipe.c       | 2 +-
>   drivers/gpu/drm/i915/i915_sw_fence.c | 2 +-

If it stays all DRM drivers in one patch then I guess it needs to go via 
drm-misc, which for i915 would be okay I think in this case since patch 
is extremely unlikely to clash with anything. Or split it up per driver 
and then we can handle it in drm-intel-next once core functionality is in.

We do however have some more calls to del_timer_sync, where freeing is 
perhaps not immediately next to the site in code, but things definitely 
get freed like on module unload. Would we need to convert all of them to 
avoid some, presumably new, warnings?

Regards,

Tvrtko

>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
> index 7c6dc2bcd14a..08429bdd57cf 100644
> --- a/drivers/gpu/drm/gud/gud_pipe.c
> +++ b/drivers/gpu/drm/gud/gud_pipe.c
> @@ -272,7 +272,7 @@ static int gud_usb_bulk(struct gud_device *gdrm, size_t len)
>   
>   	usb_sg_wait(&ctx.sgr);
>   
> -	if (!del_timer_sync(&ctx.timer))
> +	if (!timer_shutdown_sync(&ctx.timer))
>   		ret = -ETIMEDOUT;
>   	else if (ctx.sgr.status < 0)
>   		ret = ctx.sgr.status;
> diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
> index 6fc0d1b89690..bfaa9a67dc35 100644
> --- a/drivers/gpu/drm/i915/i915_sw_fence.c
> +++ b/drivers/gpu/drm/i915/i915_sw_fence.c
> @@ -465,7 +465,7 @@ static void irq_i915_sw_fence_work(struct irq_work *wrk)
>   	struct i915_sw_dma_fence_cb_timer *cb =
>   		container_of(wrk, typeof(*cb), work);
>   
> -	del_timer_sync(&cb->timer);
> +	timer_shutdown_sync(&cb->timer);
>   	dma_fence_put(cb->dma);
>   
>   	kfree_rcu(cb, rcu);

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC][PATCH v3 00/33] timers: Use timer_shutdown*() before freeing timers
  2022-11-04  5:40 [RFC][PATCH v3 00/33] timers: Use timer_shutdown*() before freeing timers Steven Rostedt
  2022-11-04  5:41 ` [RFC][PATCH v3 12/33] timers: dma-buf: Use timer_shutdown_sync() before freeing timer Steven Rostedt
  2022-11-04  5:41 ` [RFC][PATCH v3 13/33] timers: drm: " Steven Rostedt
@ 2022-11-04 17:00 ` Linus Torvalds
  2022-11-04 19:22 ` Guenter Roeck
  2022-11-04 23:34 ` Guenter Roeck
  4 siblings, 0 replies; 18+ messages in thread
From: Linus Torvalds @ 2022-11-04 17:00 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: alsa-devel, linux-staging, linux-doc, dri-devel, Thomas Gleixner,
	linux-leds, drbd-dev, linux-s390, linux-nilfs, linux-scsi,
	linux-sh, linux-atm-general, linux-afs, lvs-devel, linux-acpi,
	coreteam, intel-wired-lan, linux-input, tipc-discussion,
	linux-ext4, Guenter Roeck, linux-media, bridge, linux-pm,
	intel-gfx, rcu, cgroups, openipmi-developer, Anna-Maria Gleixner,
	linux-edac, linux-block, linux-nfs, linux-parisc, Stephen Boyd,
	netdev, linux-usb, linux-wireless, linux-kernel, linux-bluetooth,
	netfilter-devel, Andrew Morton, linaro-mm-sig

On Thu, Nov 3, 2022 at 10:48 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> Ideally, I would have the first patch go into this rc cycle, which is mostly
> non functional as it will allow the other patches to come in via the respective
> subsystems in the next merge window.

Ack.

I also wonder if we could do the completely trivially correct
conversions immediately.

I'm talking about the scripted ones where it's currently a
"del_timer_sync()", and the very next action is freeing whatever data
structure the timer is in (possibly with something like free_irq() in
between - my point is that there's an unconditional free that is very
clear and unambiguous), so that there is absolutely no question about
whether they should use "timer_shutdown_sync()" or not.

IOW, things like patches 03, 17 and 31, and at least parts others in
this series.

This series clearly has several much more complex cases that need
actual real code review, and I think it would help to have the
completely unambiguous cases out of the way, just to get rid of noise.

So I'd take that first patch, and a scripted set of "this cannot
change any semantics" patches early.

                Linus

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC][PATCH v3 12/33] timers: dma-buf: Use timer_shutdown_sync() before freeing timer
  2022-11-04  7:15     ` Christian König
@ 2022-11-04 18:58       ` Steven Rostedt
  2022-11-05  8:12         ` [Linaro-mm-sig] " Christian König
  0 siblings, 1 reply; 18+ messages in thread
From: Steven Rostedt @ 2022-11-04 18:58 UTC (permalink / raw)
  To: Christian König
  Cc: Stephen Boyd, Linus Torvalds, linux-kernel, dri-devel,
	Sumit Semwal, linaro-mm-sig, Thomas Gleixner,
	Anna-Maria Gleixner, Andrew Morton, Guenter Roeck, linux-media

On Fri, 4 Nov 2022 08:15:53 +0100
Christian König <christian.koenig@amd.com> wrote:

> > index fb6e0a6ae2c9..5d3e7b503501 100644
> > --- a/drivers/dma-buf/st-dma-fence.c
> > +++ b/drivers/dma-buf/st-dma-fence.c
> > @@ -412,7 +412,7 @@ static int test_wait_timeout(void *arg)
> >   
> >   	err = 0;
> >   err_free:
> > -	del_timer_sync(&wt.timer);
> > +	timer_shutdown_sync(&wt.timer);  
> 
> Mhm, what exactly is the benefit of renaming the function?
> 
> Not that I'm against the change, but my thinking is more if there are 
> more functions which don't re-arm the time than those which do that then 
> why not forbid it in general?

Timers are more often re-armed then not. I had to look for the
locations where del_timer*() was called just before freeing, and other
locations where they are freed later.

I didn't rename del_timer_sync() to timer_shutdown_sync(), this version
renamed the new "del_timer_shutdown()" to "timer_shutdown_sync()".

Maybe I'm just confused at what you are asking.

-- Steve

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC][PATCH v3 13/33] timers: drm: Use timer_shutdown_sync() before freeing timer
  2022-11-04  8:48   ` Tvrtko Ursulin
@ 2022-11-04 19:02     ` Steven Rostedt
  0 siblings, 0 replies; 18+ messages in thread
From: Steven Rostedt @ 2022-11-04 19:02 UTC (permalink / raw)
  To: Tvrtko Ursulin
  Cc: Stephen Boyd, Linus Torvalds, intel-gfx, linux-kernel,
	Noralf Trønnes, dri-devel, Rodrigo Vivi, Thomas Gleixner,
	Anna-Maria Gleixner, Andrew Morton, Guenter Roeck

On Fri, 4 Nov 2022 08:48:28 +0000
Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:

> If it stays all DRM drivers in one patch then I guess it needs to go via 
> drm-misc, which for i915 would be okay I think in this case since patch 
> is extremely unlikely to clash with anything. Or split it up per driver 
> and then we can handle it in drm-intel-next once core functionality is in.
> 
> We do however have some more calls to del_timer_sync, where freeing is 
> perhaps not immediately next to the site in code, but things definitely 
> get freed like on module unload. Would we need to convert all of them to 
> avoid some, presumably new, warnings?


I'm happy to split this patch up. I just got a bit lazy and started
just grouping via entire subsystems. You should see the networking
patch ;-)

-- Steve

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC][PATCH v3 00/33] timers: Use timer_shutdown*() before freeing timers
  2022-11-04  5:40 [RFC][PATCH v3 00/33] timers: Use timer_shutdown*() before freeing timers Steven Rostedt
                   ` (2 preceding siblings ...)
  2022-11-04 17:00 ` [RFC][PATCH v3 00/33] timers: Use timer_shutdown*() before freeing timers Linus Torvalds
@ 2022-11-04 19:22 ` Guenter Roeck
  2022-11-04 19:42   ` Steven Rostedt
  2022-11-04 23:34 ` Guenter Roeck
  4 siblings, 1 reply; 18+ messages in thread
From: Guenter Roeck @ 2022-11-04 19:22 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: alsa-devel, linux-staging, linux-doc, dri-devel, linaro-mm-sig,
	Thomas Gleixner, linux-leds, drbd-dev, linux-s390, linux-nilfs,
	linux-scsi, linux-sh, linux-atm-general, linux-afs, lvs-devel,
	linux-acpi, coreteam, intel-wired-lan, linux-input,
	tipc-discussion, linux-ext4, linux-media, bridge, linux-pm,
	intel-gfx, rcu, cgroups, openipmi-developer, Anna-Maria Gleixner,
	linux-edac, linux-block, linux-nfs, linux-parisc, Stephen Boyd,
	netdev, linux-usb, linux-wireless, linux-kernel, linux-bluetooth,
	netfilter-devel, Andrew Morton, Linus Torvalds

On Fri, Nov 04, 2022 at 01:40:53AM -0400, Steven Rostedt wrote:
> 
> Back in April, I posted an RFC patch set to help mitigate a common issue
> where a timer gets armed just before it is freed, and when the timer
> goes off, it crashes in the timer code without any evidence of who the
> culprit was. I got side tracked and never finished up on that patch set.
> Since this type of crash is still our #1 crash we are seeing in the field,
> it has become a priority again to finish it.
> 
> This is v3 of that patch set. Thomas Gleixner posted an untested version
> that makes timer->function NULL as the flag that it is shutdown. I took that
> code, tested it (fixed it up), added more comments, and changed the
> name to timer_shutdown_sync(). I also converted it to use WARN_ON_ONCE()
> instead of just WARN_ON() as Linus asked for.
> 

Unfortunately the renaming caused some symbol conflicts.

Global definition: timer_shutdown

  File             Line
0 time.c            93 static inline void timer_shutdown(struct clock_event_device *evt)
1 arm_arch_timer.c 690 static __always_inline int timer_shutdown(const int access,
2 timer-fttmr010.c 105 int (*timer_shutdown)(struct clock_event_device *evt);
3 timer-sp804.c    158 static inline void timer_shutdown(struct clock_event_device *evt)
4 timer.h          239 static inline int timer_shutdown(struct timer_list *timer)

Guenter

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC][PATCH v3 00/33] timers: Use timer_shutdown*() before freeing timers
  2022-11-04 19:22 ` Guenter Roeck
@ 2022-11-04 19:42   ` Steven Rostedt
  2022-11-04 19:50     ` Linus Torvalds
                       ` (2 more replies)
  0 siblings, 3 replies; 18+ messages in thread
From: Steven Rostedt @ 2022-11-04 19:42 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: alsa-devel, linux-staging, linux-doc, dri-devel, linaro-mm-sig,
	Thomas Gleixner, linux-leds, drbd-dev, linux-s390, linux-nilfs,
	linux-scsi, linux-sh, linux-atm-general, linux-afs, lvs-devel,
	linux-acpi, coreteam, intel-wired-lan, linux-input,
	tipc-discussion, linux-ext4, linux-media, bridge, linux-pm,
	intel-gfx, rcu, cgroups, openipmi-developer, Anna-Maria Gleixner,
	linux-edac, linux-block, linux-nfs, linux-parisc, Stephen Boyd,
	netdev, linux-usb, linux-wireless, linux-kernel, linux-bluetooth,
	netfilter-devel, Andrew Morton, Linus Torvalds

On Fri, 4 Nov 2022 12:22:32 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> Unfortunately the renaming caused some symbol conflicts.
> 
> Global definition: timer_shutdown
> 
>   File             Line
> 0 time.c            93 static inline void timer_shutdown(struct clock_event_device *evt)
> 1 arm_arch_timer.c 690 static __always_inline int timer_shutdown(const int access,
> 2 timer-fttmr010.c 105 int (*timer_shutdown)(struct clock_event_device *evt);
> 3 timer-sp804.c    158 static inline void timer_shutdown(struct clock_event_device *evt)
> 4 timer.h          239 static inline int timer_shutdown(struct timer_list *timer)

$ git grep '\btimer_shutdown'
arch/arm/mach-spear/time.c:static inline void timer_shutdown(struct clock_event_device *evt)
arch/arm/mach-spear/time.c:     timer_shutdown(evt);
arch/arm/mach-spear/time.c:     timer_shutdown(evt);
arch/arm/mach-spear/time.c:     timer_shutdown(evt);
drivers/clocksource/arm_arch_timer.c:static __always_inline int timer_shutdown(const int access,
drivers/clocksource/arm_arch_timer.c:   return timer_shutdown(ARCH_TIMER_VIRT_ACCESS, clk);
drivers/clocksource/arm_arch_timer.c:   return timer_shutdown(ARCH_TIMER_PHYS_ACCESS, clk);
drivers/clocksource/arm_arch_timer.c:   return timer_shutdown(ARCH_TIMER_MEM_VIRT_ACCESS, clk);
drivers/clocksource/arm_arch_timer.c:   return timer_shutdown(ARCH_TIMER_MEM_PHYS_ACCESS, clk);
drivers/clocksource/timer-fttmr010.c:   int (*timer_shutdown)(struct clock_event_device *evt);
drivers/clocksource/timer-fttmr010.c:   fttmr010->timer_shutdown(evt);
drivers/clocksource/timer-fttmr010.c:   fttmr010->timer_shutdown(evt);
drivers/clocksource/timer-fttmr010.c:   fttmr010->timer_shutdown(evt);
drivers/clocksource/timer-fttmr010.c:           fttmr010->timer_shutdown = ast2600_timer_shutdown;
drivers/clocksource/timer-fttmr010.c:           fttmr010->timer_shutdown = fttmr010_timer_shutdown;
drivers/clocksource/timer-fttmr010.c:   fttmr010->clkevt.set_state_shutdown = fttmr010->timer_shutdown;
drivers/clocksource/timer-fttmr010.c:   fttmr010->clkevt.tick_resume = fttmr010->timer_shutdown;
drivers/clocksource/timer-sp804.c:static inline void timer_shutdown(struct clock_event_device *evt)
drivers/clocksource/timer-sp804.c:      timer_shutdown(evt);
drivers/clocksource/timer-sp804.c:      timer_shutdown(evt);

Honestly, I think these need to be renamed, as "timer_shutdown()"
should be specific to the timer code, and not individual timers.

I'll start making a patch set that starts by renaming these timers,
then adds the timer_shutdown() API, and finished with the trivial
updates, and that will be a real "PATCH" (non RFC).

Linus, should I also add any patches that has already been acked by the
respective maintainer?

-- Steve

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC][PATCH v3 00/33] timers: Use timer_shutdown*() before freeing timers
  2022-11-04 19:42   ` Steven Rostedt
@ 2022-11-04 19:50     ` Linus Torvalds
  2022-11-04 20:38     ` Steven Rostedt
  2022-11-04 20:41     ` Guenter Roeck
  2 siblings, 0 replies; 18+ messages in thread
From: Linus Torvalds @ 2022-11-04 19:50 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: alsa-devel, linux-staging, linux-doc, dri-devel, Thomas Gleixner,
	linux-leds, drbd-dev, linux-s390, linux-nilfs, linux-scsi,
	linux-sh, linux-atm-general, linux-afs, lvs-devel, linux-acpi,
	coreteam, intel-wired-lan, linux-input, tipc-discussion,
	linux-ext4, Guenter Roeck, linux-media, bridge, linux-pm,
	intel-gfx, rcu, cgroups, openipmi-developer, Anna-Maria Gleixner,
	linux-edac, linux-block, linux-nfs, linux-parisc, Stephen Boyd,
	netdev, linux-usb, linux-wireless, linux-kernel, linux-bluetooth,
	netfilter-devel, Andrew Morton, linaro-mm-sig

On Fri, Nov 4, 2022 at 12:42 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> Linus, should I also add any patches that has already been acked by the
> respective maintainer?

No, I'd prefer to keep only the ones that are 100% unambiguously not
changing any semantics.

              Linus

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC][PATCH v3 00/33] timers: Use timer_shutdown*() before freeing timers
  2022-11-04 19:42   ` Steven Rostedt
  2022-11-04 19:50     ` Linus Torvalds
@ 2022-11-04 20:38     ` Steven Rostedt
  2022-11-04 20:42       ` Guenter Roeck
  2022-11-04 20:41     ` Guenter Roeck
  2 siblings, 1 reply; 18+ messages in thread
From: Steven Rostedt @ 2022-11-04 20:38 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: alsa-devel, linux-staging, linux-doc, dri-devel, linaro-mm-sig,
	Thomas Gleixner, linux-leds, drbd-dev, linux-s390, linux-nilfs,
	linux-scsi, linux-sh, linux-atm-general, linux-afs, lvs-devel,
	linux-acpi, coreteam, intel-wired-lan, linux-input,
	tipc-discussion, linux-ext4, linux-media, bridge, linux-pm,
	intel-gfx, rcu, cgroups, openipmi-developer, Anna-Maria Gleixner,
	linux-edac, linux-block, linux-nfs, linux-parisc, Stephen Boyd,
	netdev, linux-usb, linux-wireless, linux-kernel, linux-bluetooth,
	netfilter-devel, Andrew Morton, Linus Torvalds

On Fri, 4 Nov 2022 15:42:09 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> $ git grep '\btimer_shutdown'
> arch/arm/mach-spear/time.c:static inline void timer_shutdown(struct clock_event_device *evt)
> arch/arm/mach-spear/time.c:     timer_shutdown(evt);
> arch/arm/mach-spear/time.c:     timer_shutdown(evt);
> arch/arm/mach-spear/time.c:     timer_shutdown(evt);
> drivers/clocksource/arm_arch_timer.c:static __always_inline int timer_shutdown(const int access,
> drivers/clocksource/arm_arch_timer.c:   return timer_shutdown(ARCH_TIMER_VIRT_ACCESS, clk);
> drivers/clocksource/arm_arch_timer.c:   return timer_shutdown(ARCH_TIMER_PHYS_ACCESS, clk);
> drivers/clocksource/arm_arch_timer.c:   return timer_shutdown(ARCH_TIMER_MEM_VIRT_ACCESS, clk);
> drivers/clocksource/arm_arch_timer.c:   return timer_shutdown(ARCH_TIMER_MEM_PHYS_ACCESS, clk);
> drivers/clocksource/timer-fttmr010.c:   int (*timer_shutdown)(struct clock_event_device *evt);



> drivers/clocksource/timer-fttmr010.c:   fttmr010->timer_shutdown(evt);
> drivers/clocksource/timer-fttmr010.c:   fttmr010->timer_shutdown(evt);
> drivers/clocksource/timer-fttmr010.c:   fttmr010->timer_shutdown(evt);
> drivers/clocksource/timer-fttmr010.c:           fttmr010->timer_shutdown = ast2600_timer_shutdown;
> drivers/clocksource/timer-fttmr010.c:           fttmr010->timer_shutdown = fttmr010_timer_shutdown;
> drivers/clocksource/timer-fttmr010.c:   fttmr010->clkevt.set_state_shutdown = fttmr010->timer_shutdown;
> drivers/clocksource/timer-fttmr010.c:   fttmr010->clkevt.tick_resume = fttmr010->timer_shutdown;

I won't touch structure fields though.

-- Steve


> drivers/clocksource/timer-sp804.c:static inline void timer_shutdown(struct clock_event_device *evt)
> drivers/clocksource/timer-sp804.c:      timer_shutdown(evt);
> drivers/clocksource/timer-sp804.c:      timer_shutdown(evt);

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC][PATCH v3 00/33] timers: Use timer_shutdown*() before freeing timers
  2022-11-04 19:42   ` Steven Rostedt
  2022-11-04 19:50     ` Linus Torvalds
  2022-11-04 20:38     ` Steven Rostedt
@ 2022-11-04 20:41     ` Guenter Roeck
  2 siblings, 0 replies; 18+ messages in thread
From: Guenter Roeck @ 2022-11-04 20:41 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: alsa-devel, linux-staging, linux-doc, dri-devel, linaro-mm-sig,
	Thomas Gleixner, linux-leds, drbd-dev, linux-s390, linux-nilfs,
	linux-scsi, linux-sh, linux-atm-general, linux-afs, lvs-devel,
	linux-acpi, coreteam, intel-wired-lan, linux-input,
	tipc-discussion, linux-ext4, linux-media, bridge, linux-pm,
	intel-gfx, rcu, cgroups, openipmi-developer, Anna-Maria Gleixner,
	linux-edac, linux-block, linux-nfs, linux-parisc, Stephen Boyd,
	netdev, linux-usb, linux-wireless, linux-kernel, linux-bluetooth,
	netfilter-devel, Andrew Morton, Linus Torvalds

On Fri, Nov 04, 2022 at 03:42:09PM -0400, Steven Rostedt wrote:
> On Fri, 4 Nov 2022 12:22:32 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
> > Unfortunately the renaming caused some symbol conflicts.
> > 
> > Global definition: timer_shutdown
> > 
> >   File             Line
> > 0 time.c            93 static inline void timer_shutdown(struct clock_event_device *evt)
> > 1 arm_arch_timer.c 690 static __always_inline int timer_shutdown(const int access,
> > 2 timer-fttmr010.c 105 int (*timer_shutdown)(struct clock_event_device *evt);
> > 3 timer-sp804.c    158 static inline void timer_shutdown(struct clock_event_device *evt)
> > 4 timer.h          239 static inline int timer_shutdown(struct timer_list *timer)
> 
> $ git grep '\btimer_shutdown'
> arch/arm/mach-spear/time.c:static inline void timer_shutdown(struct clock_event_device *evt)
> arch/arm/mach-spear/time.c:     timer_shutdown(evt);
> arch/arm/mach-spear/time.c:     timer_shutdown(evt);
> arch/arm/mach-spear/time.c:     timer_shutdown(evt);
> drivers/clocksource/arm_arch_timer.c:static __always_inline int timer_shutdown(const int access,
> drivers/clocksource/arm_arch_timer.c:   return timer_shutdown(ARCH_TIMER_VIRT_ACCESS, clk);
> drivers/clocksource/arm_arch_timer.c:   return timer_shutdown(ARCH_TIMER_PHYS_ACCESS, clk);
> drivers/clocksource/arm_arch_timer.c:   return timer_shutdown(ARCH_TIMER_MEM_VIRT_ACCESS, clk);
> drivers/clocksource/arm_arch_timer.c:   return timer_shutdown(ARCH_TIMER_MEM_PHYS_ACCESS, clk);
> drivers/clocksource/timer-fttmr010.c:   int (*timer_shutdown)(struct clock_event_device *evt);
> drivers/clocksource/timer-fttmr010.c:   fttmr010->timer_shutdown(evt);
> drivers/clocksource/timer-fttmr010.c:   fttmr010->timer_shutdown(evt);
> drivers/clocksource/timer-fttmr010.c:   fttmr010->timer_shutdown(evt);
> drivers/clocksource/timer-fttmr010.c:           fttmr010->timer_shutdown = ast2600_timer_shutdown;
> drivers/clocksource/timer-fttmr010.c:           fttmr010->timer_shutdown = fttmr010_timer_shutdown;
> drivers/clocksource/timer-fttmr010.c:   fttmr010->clkevt.set_state_shutdown = fttmr010->timer_shutdown;
> drivers/clocksource/timer-fttmr010.c:   fttmr010->clkevt.tick_resume = fttmr010->timer_shutdown;
> drivers/clocksource/timer-sp804.c:static inline void timer_shutdown(struct clock_event_device *evt)
> drivers/clocksource/timer-sp804.c:      timer_shutdown(evt);
> drivers/clocksource/timer-sp804.c:      timer_shutdown(evt);
> 
> Honestly, I think these need to be renamed, as "timer_shutdown()"
> should be specific to the timer code, and not individual timers.

Yes, that is what I did locally. I am repeating my test now with that
change made.

Guenter

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC][PATCH v3 00/33] timers: Use timer_shutdown*() before freeing timers
  2022-11-04 20:38     ` Steven Rostedt
@ 2022-11-04 20:42       ` Guenter Roeck
  0 siblings, 0 replies; 18+ messages in thread
From: Guenter Roeck @ 2022-11-04 20:42 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: alsa-devel, linux-staging, linux-doc, dri-devel, linaro-mm-sig,
	Thomas Gleixner, linux-leds, drbd-dev, linux-s390, linux-nilfs,
	linux-scsi, linux-sh, linux-atm-general, linux-afs, lvs-devel,
	linux-acpi, coreteam, intel-wired-lan, linux-input,
	tipc-discussion, linux-ext4, linux-media, bridge, linux-pm,
	intel-gfx, rcu, cgroups, openipmi-developer, Anna-Maria Gleixner,
	linux-edac, linux-block, linux-nfs, linux-parisc, Stephen Boyd,
	netdev, linux-usb, linux-wireless, linux-kernel, linux-bluetooth,
	netfilter-devel, Andrew Morton, Linus Torvalds

On Fri, Nov 04, 2022 at 04:38:34PM -0400, Steven Rostedt wrote:
> On Fri, 4 Nov 2022 15:42:09 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
[ ... ]
> 
> > drivers/clocksource/timer-fttmr010.c:   fttmr010->timer_shutdown(evt);
> > drivers/clocksource/timer-fttmr010.c:   fttmr010->timer_shutdown(evt);
> > drivers/clocksource/timer-fttmr010.c:   fttmr010->timer_shutdown(evt);
> > drivers/clocksource/timer-fttmr010.c:           fttmr010->timer_shutdown = ast2600_timer_shutdown;
> > drivers/clocksource/timer-fttmr010.c:           fttmr010->timer_shutdown = fttmr010_timer_shutdown;
> > drivers/clocksource/timer-fttmr010.c:   fttmr010->clkevt.set_state_shutdown = fttmr010->timer_shutdown;
> > drivers/clocksource/timer-fttmr010.c:   fttmr010->clkevt.tick_resume = fttmr010->timer_shutdown;
> 
> I won't touch structure fields though.
> 

Agreed, same here.

Guenter

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC][PATCH v3 00/33] timers: Use timer_shutdown*() before freeing timers
  2022-11-04  5:40 [RFC][PATCH v3 00/33] timers: Use timer_shutdown*() before freeing timers Steven Rostedt
                   ` (3 preceding siblings ...)
  2022-11-04 19:22 ` Guenter Roeck
@ 2022-11-04 23:34 ` Guenter Roeck
  4 siblings, 0 replies; 18+ messages in thread
From: Guenter Roeck @ 2022-11-04 23:34 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: alsa-devel, linux-staging, linux-doc, dri-devel, linaro-mm-sig,
	Thomas Gleixner, linux-leds, drbd-dev, linux-s390, linux-nilfs,
	linux-scsi, linux-sh, linux-atm-general, linux-afs, lvs-devel,
	linux-acpi, coreteam, intel-wired-lan, linux-input,
	tipc-discussion, linux-ext4, linux-media, bridge, linux-pm,
	intel-gfx, rcu, cgroups, openipmi-developer, Anna-Maria Gleixner,
	linux-edac, linux-block, linux-nfs, linux-parisc, Stephen Boyd,
	netdev, linux-usb, linux-wireless, linux-kernel, linux-bluetooth,
	netfilter-devel, Andrew Morton, Linus Torvalds

On Fri, Nov 04, 2022 at 01:40:53AM -0400, Steven Rostedt wrote:
> 
> Back in April, I posted an RFC patch set to help mitigate a common issue
> where a timer gets armed just before it is freed, and when the timer
> goes off, it crashes in the timer code without any evidence of who the
> culprit was. I got side tracked and never finished up on that patch set.
> Since this type of crash is still our #1 crash we are seeing in the field,
> it has become a priority again to finish it.
> 

After applying the patches attached below, everything compiles for me,
and there are no crashes. There are still various warnings, most in
networking. I know I need to apply some patch(es) to fix the networking
warnings, but I didn't entirely understand what exactly to apply, so
I didn't try.

Complete logs are at https://kerneltests.org/builders, on the bottom half
of the page (qemu tests, in the 'testing' column).

Guenter

---
Warnings:

ODEBUG: free active (active state 0) object type: timer_list hint: tcp_write_timer+0x0/0x1d0
	from tcp_close -> __sk_destruct -> tcp_write_timer

ODEBUG: free active (active state 0) object type: timer_list hint: tcp_keepalive_timer+0x0/0x4c0
	from tcp_close -> __sk_destruct -> tcp_keepalive_timer -> __del_timer_sync

ODEBUG: free active (active state 0) object type: timer_list hint: blk_rq_timed_out_timer+0x0/0x40
	blk_free_queue_rcu -> blk_free_queue_rcu -> blk_rq_timed_out_timer

---
Changes applied on top of patch set to fix build errors:

diff --git a/arch/arm/mach-spear/time.c b/arch/arm/mach-spear/time.c
index e979e2197f8e..5371c824786d 100644
--- a/arch/arm/mach-spear/time.c
+++ b/arch/arm/mach-spear/time.c
@@ -90,7 +90,7 @@ static void __init spear_clocksource_init(void)
 		200, 16, clocksource_mmio_readw_up);
 }
 
-static inline void timer_shutdown(struct clock_event_device *evt)
+static inline void spear_timer_shutdown(struct clock_event_device *evt)
 {
 	u16 val = readw(gpt_base + CR(CLKEVT));
 
@@ -101,7 +101,7 @@ static inline void timer_shutdown(struct clock_event_device *evt)
 
 static int spear_shutdown(struct clock_event_device *evt)
 {
-	timer_shutdown(evt);
+	spear_timer_shutdown(evt);
 
 	return 0;
 }
@@ -111,7 +111,7 @@ static int spear_set_oneshot(struct clock_event_device *evt)
 	u16 val;
 
 	/* stop the timer */
-	timer_shutdown(evt);
+	spear_timer_shutdown(evt);
 
 	val = readw(gpt_base + CR(CLKEVT));
 	val |= CTRL_ONE_SHOT;
@@ -126,7 +126,7 @@ static int spear_set_periodic(struct clock_event_device *evt)
 	u16 val;
 
 	/* stop the timer */
-	timer_shutdown(evt);
+	spear_timer_shutdown(evt);
 
 	period = clk_get_rate(gpt_clk) / HZ;
 	period >>= CTRL_PRESCALER16;
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index a7ff77550e17..9c3420a0d19d 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -687,8 +687,8 @@ static irqreturn_t arch_timer_handler_virt_mem(int irq, void *dev_id)
 	return timer_handler(ARCH_TIMER_MEM_VIRT_ACCESS, evt);
 }
 
-static __always_inline int timer_shutdown(const int access,
-					  struct clock_event_device *clk)
+static __always_inline int arch_timer_shutdown(const int access,
+					       struct clock_event_device *clk)
 {
 	unsigned long ctrl;
 
@@ -701,22 +701,22 @@ static __always_inline int timer_shutdown(const int access,
 
 static int arch_timer_shutdown_virt(struct clock_event_device *clk)
 {
-	return timer_shutdown(ARCH_TIMER_VIRT_ACCESS, clk);
+	return arch_timer_shutdown(ARCH_TIMER_VIRT_ACCESS, clk);
 }
 
 static int arch_timer_shutdown_phys(struct clock_event_device *clk)
 {
-	return timer_shutdown(ARCH_TIMER_PHYS_ACCESS, clk);
+	return arch_timer_shutdown(ARCH_TIMER_PHYS_ACCESS, clk);
 }
 
 static int arch_timer_shutdown_virt_mem(struct clock_event_device *clk)
 {
-	return timer_shutdown(ARCH_TIMER_MEM_VIRT_ACCESS, clk);
+	return arch_timer_shutdown(ARCH_TIMER_MEM_VIRT_ACCESS, clk);
 }
 
 static int arch_timer_shutdown_phys_mem(struct clock_event_device *clk)
 {
-	return timer_shutdown(ARCH_TIMER_MEM_PHYS_ACCESS, clk);
+	return arch_timer_shutdown(ARCH_TIMER_MEM_PHYS_ACCESS, clk);
 }
 
 static __always_inline void set_next_event(const int access, unsigned long evt,
diff --git a/drivers/clocksource/timer-sp804.c b/drivers/clocksource/timer-sp804.c
index e6a87f4af2b5..a3c38e1343f0 100644
--- a/drivers/clocksource/timer-sp804.c
+++ b/drivers/clocksource/timer-sp804.c
@@ -155,14 +155,14 @@ static irqreturn_t sp804_timer_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static inline void timer_shutdown(struct clock_event_device *evt)
+static inline void sp804_timer_shutdown(struct clock_event_device *evt)
 {
 	writel(0, common_clkevt->ctrl);
 }
 
 static int sp804_shutdown(struct clock_event_device *evt)
 {
-	timer_shutdown(evt);
+	sp804_timer_shutdown(evt);
 	return 0;
 }
 
@@ -171,7 +171,7 @@ static int sp804_set_periodic(struct clock_event_device *evt)
 	unsigned long ctrl = TIMER_CTRL_32BIT | TIMER_CTRL_IE |
 			     TIMER_CTRL_PERIODIC | TIMER_CTRL_ENABLE;
 
-	timer_shutdown(evt);
+	sp804_timer_shutdown(evt);
 	writel(common_clkevt->reload, common_clkevt->load);
 	writel(ctrl, common_clkevt->ctrl);
 	return 0;


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [Linaro-mm-sig] Re: [RFC][PATCH v3 12/33] timers: dma-buf: Use timer_shutdown_sync() before freeing timer
  2022-11-04 18:58       ` Steven Rostedt
@ 2022-11-05  8:12         ` Christian König
  0 siblings, 0 replies; 18+ messages in thread
From: Christian König @ 2022-11-05  8:12 UTC (permalink / raw)
  To: Steven Rostedt, Christian König
  Cc: Stephen Boyd, Linus Torvalds, linux-kernel, dri-devel,
	Sumit Semwal, linaro-mm-sig, Thomas Gleixner,
	Anna-Maria Gleixner, Andrew Morton, Guenter Roeck, linux-media

Am 04.11.22 um 19:58 schrieb Steven Rostedt:
> On Fri, 4 Nov 2022 08:15:53 +0100
> Christian König <christian.koenig@amd.com> wrote:
>
>>> index fb6e0a6ae2c9..5d3e7b503501 100644
>>> --- a/drivers/dma-buf/st-dma-fence.c
>>> +++ b/drivers/dma-buf/st-dma-fence.c
>>> @@ -412,7 +412,7 @@ static int test_wait_timeout(void *arg)
>>>    
>>>    	err = 0;
>>>    err_free:
>>> -	del_timer_sync(&wt.timer);
>>> +	timer_shutdown_sync(&wt.timer);
>> Mhm, what exactly is the benefit of renaming the function?
>>
>> Not that I'm against the change, but my thinking is more if there are
>> more functions which don't re-arm the time than those which do that then
>> why not forbid it in general?
> Timers are more often re-armed then not. I had to look for the
> locations where del_timer*() was called just before freeing, and other
> locations where they are freed later.
>
> I didn't rename del_timer_sync() to timer_shutdown_sync(), this version
> renamed the new "del_timer_shutdown()" to "timer_shutdown_sync()".
>
> Maybe I'm just confused at what you are asking.

No, that explains it a bit better. I was just wondering what exactly the 
different to del_timer_sync() is.

Maybe shorten the summary in the cover letter a bit. The history how 
this change came to be is not as interesting as why we are changing 
something.

Regards,
Christian.

>
> -- Steve
> _______________________________________________
> Linaro-mm-sig mailing list -- linaro-mm-sig@lists.linaro.org
> To unsubscribe send an email to linaro-mm-sig-leave@lists.linaro.org


^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2022-11-05  8:12 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-04  5:40 [RFC][PATCH v3 00/33] timers: Use timer_shutdown*() before freeing timers Steven Rostedt
2022-11-04  5:41 ` [RFC][PATCH v3 12/33] timers: dma-buf: Use timer_shutdown_sync() before freeing timer Steven Rostedt
2022-11-04  5:54   ` Steven Rostedt
2022-11-04  7:15     ` Christian König
2022-11-04 18:58       ` Steven Rostedt
2022-11-05  8:12         ` [Linaro-mm-sig] " Christian König
2022-11-04  5:41 ` [RFC][PATCH v3 13/33] timers: drm: " Steven Rostedt
2022-11-04  5:55   ` Steven Rostedt
2022-11-04  8:48   ` Tvrtko Ursulin
2022-11-04 19:02     ` Steven Rostedt
2022-11-04 17:00 ` [RFC][PATCH v3 00/33] timers: Use timer_shutdown*() before freeing timers Linus Torvalds
2022-11-04 19:22 ` Guenter Roeck
2022-11-04 19:42   ` Steven Rostedt
2022-11-04 19:50     ` Linus Torvalds
2022-11-04 20:38     ` Steven Rostedt
2022-11-04 20:42       ` Guenter Roeck
2022-11-04 20:41     ` Guenter Roeck
2022-11-04 23:34 ` Guenter Roeck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).