This is the forth part of a 5-part series (split for convenience). All 5
parts are:

Part 1: https://lore.kernel.org/lkml/cover.1729864615.git.namcao@linutronix.de
Part 2: https://lore.kernel.org/lkml/cover.1729864823.git.namcao@linutronix.de
Part 3: https://lore.kernel.org/lkml/cover.1729865232.git.namcao@linutronix.de
Part 4: https://lore.kernel.org/lkml/cover.1729865485.git.namcao@linutronix.de
Part 5: https://lore.kernel.org/lkml/cover.1729865740.git.namcao@linutronix.de

To use hrtimer, hrtimer_init() (or one of its variant) must be called, and
also the timer's callfack function must be setup separately.

That can cause misuse of hrtimer. For example, because:
  - The callback function is not setup
  - The callback function is setup while it is not safe to do so

To prevent misuse of hrtimer, this series:
  - Introduce new functions hrtimer_setup*(). These new functions are
    similar to hrtimer_init*(), except that they also sanity-check and
    initialize the callback function.
  - Introduce hrtimer_update_function() which checks that it is safe to
    change the callback function. The 'function' field of hrtimer is then
    made private.
  - Convert all users to use the new functions.
  - Some minor cleanups on the way.

Most conversion patches were created using Coccinelle with the sematic
patch below; except for tricky cases that Coccinelle cannot handle, or for
some cases where a Coccinelle's bug regarding 100 column limit is
triggered. Any patches not mentioning Coccinelle were done manually.

virtual patch
@@ expression timer, clock, mode, func; @@
- hrtimer_init(timer, clock, mode);
  ...
- timer->function = func;
+ hrtimer_setup(timer, func, clock, mode);

@@ expression timer, clock, mode, func; @@
- hrtimer_init(&timer, clock, mode);
  ...
- timer.function = func;
+ hrtimer_setup(&timer, func, clock, mode);

@@ expression timer, clock, mode, func; @@
- hrtimer_init_on_stack(&timer, clock, mode);
  ...
- timer.function = func;
+ hrtimer_setup_on_stack(&timer, func, clock, mode);

@@ expression timer, clock, mode; @@
- hrtimer_init_sleeper_on_stack(timer, clock, mode);
+ hrtimer_setup_sleeper_on_stack(timer, clock, mode);

Signed-off-by: Nam Cao <namcao@linutronix.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Sebastian Reichel <sre@kernel.org>
Cc: Will Deacon <will@kernel.org>
Cc: Jon Mason <jdmason@kudzu.us>
Cc: Jaehoon Chung <jh80.chung@samsung.com>
Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Jassi Brar <jassisinghbrar@gmail.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Jonathan Cameron <jic23@kernel.org>
Cc: Andi Shyti <andi.shyti@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Zack Rusin <zack.rusin@broadcom.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: "Uwe Kleine-König" <ukleinek@kernel.org>
Cc: Takashi Iwai <tiwai@suse.com>

Nam Cao (44):
  USB: chipidea: Switch to use hrtimer_setup()
  usb: dwc2: Switch to use hrtimer_setup()
  usb: fotg210-hcd: Switch to use hrtimer_setup()
  usb: gadget: Switch to use hrtimer_setup()
  usb: ehci: Switch to use hrtimer_setup()
  usb: musb: cppi41: Switch to use hrtimer_setup()
  usb: typec: tcpm: Switch to use hrtimer_setup()
  serial: 8250: Switch to use hrtimer_setup()
  serial: imx: Switch to use hrtimer_setup()
  serial: sh-sci: Switch to use hrtimer_setup()
  serial: xilinx_uartps: Switch to use hrtimer_setup()
  scsi: Switch to use hrtimer_setup()
  rtc: class: Switch to use hrtimer_setup()
  pps: generators: pps_gen_parport: Switch to use hrtimer_setup()
  powercap: Switch to use hrtimer_setup()
  power: supply: ab8500_chargalg: Switch to use hrtimer_setup()
  power: reset: ltc2952-poweroff: Switch to use hrtimer_setup()
  drivers: perf: Switch to use hrtimer_setup()
  ntb: ntb_pingpong: Switch to use hrtimer_setup()
  mmc: dw_mmc: Switch to use hrtimer_setup()
  misc: vcpu_stall_detector: Switch to use hrtimer_setup()
  media: Switch to use hrtimer_setup()
  mailbox: Switch to use hrtimer_setup()
  leds: trigger: pattern: Switch to use hrtimer_setup()
  Input: Switch to use hrtimer_setup()
  iio: Switch to use hrtimer_setup()
  i2c: Switch to use hrtimer_setup()
  stm class: heartbeat: Switch to use hrtimer_setup()
  drm/amdgpu: Switch to use hrtimer_setup()
  drm/i915/huc: Switch to use hrtimer_setup()
  drm/i915/gvt: Switch to use hrtimer_setup()
  drm/i915/perf: Switch to use hrtimer_setup()
  drm/i915/pmu: Switch to use hrtimer_setup()
  drm/i915/uncore: Switch to use hrtimer_setup()
  drm/i915/request: Switch to use hrtimer_setup()
  drm/msm: Switch to use hrtimer_setup()
  drm/vkms: Switch to use hrtimer_setup()
  drm/xe/oa: Switch to use hrtimer_setup()
  drm/vmwgfx: Switch to use hrtimer_setup()
  virtio: mem: Switch to use hrtimer_setup()
  RDMA: Switch to use hrtimer_setup()
  pwm: gpio: Switch to use hrtimer_setup()
  ASoC: fsl: imx-pcm-fiq: Switch to use hrtimer_setup()
  ALSA: Switch to use hrtimer_setup()

 drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c         |  4 ++--
 drivers/gpu/drm/i915/gt/uc/intel_huc.c           |  4 ++--
 drivers/gpu/drm/i915/gvt/display.c               |  3 +--
 drivers/gpu/drm/i915/gvt/sched_policy.c          |  3 +--
 drivers/gpu/drm/i915/i915_perf.c                 |  5 ++---
 drivers/gpu/drm/i915/i915_pmu.c                  |  3 +--
 drivers/gpu/drm/i915/i915_request.c              |  3 +--
 drivers/gpu/drm/i915/intel_uncore.c              |  3 +--
 drivers/gpu/drm/msm/msm_fence.c                  |  3 +--
 drivers/gpu/drm/msm/msm_io_utils.c               |  3 +--
 drivers/gpu/drm/vkms/vkms_crtc.c                 |  4 ++--
 drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c             |  4 ++--
 drivers/gpu/drm/xe/xe_oa.c                       |  4 ++--
 drivers/hwtracing/stm/heartbeat.c                |  6 ++----
 drivers/i2c/busses/i2c-imx.c                     |  4 ++--
 drivers/iio/adc/ti-tsc2046.c                     |  4 +---
 drivers/iio/trigger/iio-trig-hrtimer.c           |  4 ++--
 drivers/infiniband/hw/hfi1/init.c                |  5 ++---
 drivers/infiniband/sw/rdmavt/qp.c                |  5 ++---
 drivers/input/joystick/walkera0701.c             |  3 +--
 drivers/input/keyboard/gpio_keys.c               | 10 ++++------
 drivers/leds/trigger/ledtrig-pattern.c           |  4 ++--
 drivers/mailbox/mailbox.c                        |  4 +---
 drivers/media/cec/core/cec-pin.c                 |  3 +--
 drivers/media/pci/cx88/cx88-input.c              |  3 +--
 .../media/platform/chips-media/wave5/wave5-vpu.c |  4 ++--
 drivers/media/rc/pwm-ir-tx.c                     |  3 +--
 drivers/misc/vcpu_stall_detector.c               |  3 +--
 drivers/mmc/host/dw_mmc.c                        |  3 +--
 drivers/ntb/test/ntb_pingpong.c                  |  3 +--
 drivers/perf/arm-ccn.c                           |  5 ++---
 drivers/perf/marvell_cn10k_ddr_pmu.c             |  4 ++--
 drivers/perf/thunderx2_pmu.c                     |  5 ++---
 drivers/power/reset/ltc2952-poweroff.c           |  8 ++++----
 drivers/power/supply/ab8500_chargalg.c           |  9 ++++-----
 drivers/powercap/idle_inject.c                   |  3 +--
 drivers/powercap/intel_rapl_common.c             |  3 +--
 drivers/pps/generators/pps_gen_parport.c         |  3 +--
 drivers/pwm/pwm-gpio.c                           |  5 ++---
 drivers/rtc/class.c                              |  3 +--
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c         |  4 +---
 drivers/scsi/lpfc/lpfc_init.c                    |  7 +++----
 drivers/scsi/scsi_debug.c                        |  4 ++--
 drivers/tty/serial/8250/8250_bcm7271.c           |  3 +--
 drivers/tty/serial/8250/8250_port.c              | 10 ++++------
 drivers/tty/serial/imx.c                         |  8 ++++----
 drivers/tty/serial/sh-sci.c                      |  3 +--
 drivers/tty/serial/xilinx_uartps.c               |  4 ++--
 drivers/usb/chipidea/otg_fsm.c                   |  3 +--
 drivers/usb/dwc2/hcd_queue.c                     |  3 +--
 drivers/usb/fotg210/fotg210-hcd.c                |  3 +--
 drivers/usb/gadget/function/f_ncm.c              |  3 +--
 drivers/usb/gadget/udc/dummy_hcd.c               |  6 ++----
 drivers/usb/host/ehci-hcd.c                      |  3 +--
 drivers/usb/musb/musb_cppi41.c                   |  4 ++--
 drivers/usb/typec/tcpm/tcpm.c                    | 16 ++++++++--------
 drivers/virtio/virtio_mem.c                      |  4 ++--
 sound/core/hrtimer.c                             |  3 +--
 sound/drivers/dummy.c                            |  3 +--
 sound/drivers/pcsp/pcsp.c                        |  3 +--
 sound/sh/sh_dac_audio.c                          |  3 +--
 sound/soc/fsl/imx-pcm-fiq.c                      |  3 +--
 62 files changed, 108 insertions(+), 160 deletions(-)

-- 
2.39.5

Nam Cao <namcao@linutronix.de> writes:

> This is the forth part of a 5-part series (split for convenience). All 5
> parts are:
>
> Part 1: https://lore.kernel.org/lkml/cover.1729864615.git.namcao@linutronix.de
> Part 2: https://lore.kernel.org/lkml/cover.1729864823.git.namcao@linutronix.de
> Part 3: https://lore.kernel.org/lkml/cover.1729865232.git.namcao@linutronix.de
> Part 4: https://lore.kernel.org/lkml/cover.1729865485.git.namcao@linutronix.de
> Part 5: https://lore.kernel.org/lkml/cover.1729865740.git.namcao@linutronix.de

Which one do I need to click on to see the actual hrtimer_setup*()
implementations? Why is it even a separate series? Please, don't make
people click on things.

> To use hrtimer, hrtimer_init() (or one of its variant) must be called, and
> also the timer's callfack function must be setup separately.

"callback", right?

> That can cause misuse of hrtimer. For example, because:
>   - The callback function is not setup
>   - The callback function is setup while it is not safe to do so

These are not examples, these are hypotheticals. Do either of these
things actually happen in the codebase?

> To prevent misuse of hrtimer, this series:
>   - Introduce new functions hrtimer_setup*(). These new functions are
>     similar to hrtimer_init*(), except that they also sanity-check and
>     initialize the callback function.

No, it doesn't. This series only converts some drivers. I'd like to see
the sanity-checking in question, since it's the big selling point.
But IMO, "switching to a cleaner hrtimer API" or some words to that
effect is a better justification than sanity checking.

I'm not objecting to the idea, it's just carried out weirdly.

>   - Introduce hrtimer_update_function() which checks that it is safe to
>     change the callback function. The 'function' field of hrtimer is then
>     made private.

Also not in this series.

>   - Convert all users to use the new functions.
>   - Some minor cleanups on the way.

Thanks,
--
Alex

On Mon, Oct 28 2024 at 10:58, Alexander Shishkin wrote:
> Nam Cao <namcao@linutronix.de> writes:
>
>> This is the forth part of a 5-part series (split for convenience). All 5
>> parts are:
>>
>> Part 1: https://lore.kernel.org/lkml/cover.1729864615.git.namcao@linutronix.de
>> Part 2: https://lore.kernel.org/lkml/cover.1729864823.git.namcao@linutronix.de
>> Part 3: https://lore.kernel.org/lkml/cover.1729865232.git.namcao@linutronix.de
>> Part 4: https://lore.kernel.org/lkml/cover.1729865485.git.namcao@linutronix.de
>> Part 5: https://lore.kernel.org/lkml/cover.1729865740.git.namcao@linutronix.de
>
> Which one do I need to click on to see the actual hrtimer_setup*()
> implementations? Why is it even a separate series? Please, don't make
> people click on things.

Obviously part 1. The splitup was dont to avoid 500+ people which is
generally frowned upon.

>> That can cause misuse of hrtimer. For example, because:
>>   - The callback function is not setup
>>   - The callback function is setup while it is not safe to do so
>
> These are not examples, these are hypotheticals. Do either of these
> things actually happen in the codebase?

Right now not, but we had this in the past.

>> To prevent misuse of hrtimer, this series:
>>   - Introduce new functions hrtimer_setup*(). These new functions are
>>     similar to hrtimer_init*(), except that they also sanity-check and
>>     initialize the callback function.

> I'm not objecting to the idea, it's just carried out weirdly.

Right. There should have been a sectioning to explain what which part of
the series does. My bad.

Thanks,

        tglx

On Mon, 28 Oct 2024, Nam Cao <namcao@linutronix.de> wrote:
>   drm/i915/huc: Switch to use hrtimer_setup()
>   drm/i915/gvt: Switch to use hrtimer_setup()
>   drm/i915/perf: Switch to use hrtimer_setup()
>   drm/i915/pmu: Switch to use hrtimer_setup()
>   drm/i915/uncore: Switch to use hrtimer_setup()
>   drm/i915/request: Switch to use hrtimer_setup()

Acked-by: Jani Nikula <jani.nikula@intel.com>

for merging the above via whichever tree you find convenient.

-- 
Jani Nikula, Intel

On Mon, Oct 28, 2024 at 3:35 AM Nam Cao <namcao@linutronix.de> wrote:
>
> This is the forth part of a 5-part series (split for convenience). All 5
> parts are:
>
> Part 1: https://lore.kernel.org/lkml/cover.1729864615.git.namcao@linutronix.de
> Part 2: https://lore.kernel.org/lkml/cover.1729864823.git.namcao@linutronix.de
> Part 3: https://lore.kernel.org/lkml/cover.1729865232.git.namcao@linutronix.de
> Part 4: https://lore.kernel.org/lkml/cover.1729865485.git.namcao@linutronix.de
> Part 5: https://lore.kernel.org/lkml/cover.1729865740.git.namcao@linutronix.de
>
> To use hrtimer, hrtimer_init() (or one of its variant) must be called, and
> also the timer's callfack function must be setup separately.
>
> That can cause misuse of hrtimer. For example, because:
>   - The callback function is not setup
>   - The callback function is setup while it is not safe to do so
>
> To prevent misuse of hrtimer, this series:
>   - Introduce new functions hrtimer_setup*(). These new functions are
>     similar to hrtimer_init*(), except that they also sanity-check and
>     initialize the callback function.
>   - Introduce hrtimer_update_function() which checks that it is safe to
>     change the callback function. The 'function' field of hrtimer is then
>     made private.
>   - Convert all users to use the new functions.
>   - Some minor cleanups on the way.
>
> Most conversion patches were created using Coccinelle with the sematic
> patch below; except for tricky cases that Coccinelle cannot handle, or for
> some cases where a Coccinelle's bug regarding 100 column limit is
> triggered. Any patches not mentioning Coccinelle were done manually.
>
> virtual patch
> @@ expression timer, clock, mode, func; @@
> - hrtimer_init(timer, clock, mode);
>   ...
> - timer->function = func;
> + hrtimer_setup(timer, func, clock, mode);
>
> @@ expression timer, clock, mode, func; @@
> - hrtimer_init(&timer, clock, mode);
>   ...
> - timer.function = func;
> + hrtimer_setup(&timer, func, clock, mode);
>
> @@ expression timer, clock, mode, func; @@
> - hrtimer_init_on_stack(&timer, clock, mode);
>   ...
> - timer.function = func;
> + hrtimer_setup_on_stack(&timer, func, clock, mode);
>
> @@ expression timer, clock, mode; @@
> - hrtimer_init_sleeper_on_stack(timer, clock, mode);
> + hrtimer_setup_sleeper_on_stack(timer, clock, mode);
>
> Signed-off-by: Nam Cao <namcao@linutronix.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Sebastian Reichel <sre@kernel.org>
> Cc: Will Deacon <will@kernel.org>
> Cc: Jon Mason <jdmason@kudzu.us>
> Cc: Jaehoon Chung <jh80.chung@samsung.com>
> Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Cc: Jassi Brar <jassisinghbrar@gmail.com>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Cc: Andi Shyti <andi.shyti@kernel.org>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Zack Rusin <zack.rusin@broadcom.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: "Uwe Kleine-König" <ukleinek@kernel.org>
> Cc: Takashi Iwai <tiwai@suse.com>
>
> Nam Cao (44):
>   USB: chipidea: Switch to use hrtimer_setup()
>   usb: dwc2: Switch to use hrtimer_setup()
>   usb: fotg210-hcd: Switch to use hrtimer_setup()
>   usb: gadget: Switch to use hrtimer_setup()
>   usb: ehci: Switch to use hrtimer_setup()
>   usb: musb: cppi41: Switch to use hrtimer_setup()
>   usb: typec: tcpm: Switch to use hrtimer_setup()
>   serial: 8250: Switch to use hrtimer_setup()
>   serial: imx: Switch to use hrtimer_setup()
>   serial: sh-sci: Switch to use hrtimer_setup()
>   serial: xilinx_uartps: Switch to use hrtimer_setup()
>   scsi: Switch to use hrtimer_setup()
>   rtc: class: Switch to use hrtimer_setup()
>   pps: generators: pps_gen_parport: Switch to use hrtimer_setup()
>   powercap: Switch to use hrtimer_setup()
>   power: supply: ab8500_chargalg: Switch to use hrtimer_setup()
>   power: reset: ltc2952-poweroff: Switch to use hrtimer_setup()
>   drivers: perf: Switch to use hrtimer_setup()
>   ntb: ntb_pingpong: Switch to use hrtimer_setup()
>   mmc: dw_mmc: Switch to use hrtimer_setup()
>   misc: vcpu_stall_detector: Switch to use hrtimer_setup()
>   media: Switch to use hrtimer_setup()
>   mailbox: Switch to use hrtimer_setup()
>   leds: trigger: pattern: Switch to use hrtimer_setup()
>   Input: Switch to use hrtimer_setup()
>   iio: Switch to use hrtimer_setup()
>   i2c: Switch to use hrtimer_setup()
>   stm class: heartbeat: Switch to use hrtimer_setup()
>   drm/amdgpu: Switch to use hrtimer_setup()
>   drm/i915/huc: Switch to use hrtimer_setup()
>   drm/i915/gvt: Switch to use hrtimer_setup()
>   drm/i915/perf: Switch to use hrtimer_setup()
>   drm/i915/pmu: Switch to use hrtimer_setup()
>   drm/i915/uncore: Switch to use hrtimer_setup()
>   drm/i915/request: Switch to use hrtimer_setup()
>   drm/msm: Switch to use hrtimer_setup()
>   drm/vkms: Switch to use hrtimer_setup()
>   drm/xe/oa: Switch to use hrtimer_setup()
>   drm/vmwgfx: Switch to use hrtimer_setup()

Acked-by: Zack Rusin <zack.rusin@broadcom.com>

FWIW, even if there's no current errors in those paths, that's a nice
change. I remember looking at the hrtimer initialization code the
first time and thinking that setting the function member of the struct
after initialization must be a bug or some hack... Now it will
instinctively make sense to anyone reading it for the first time.

z