On Tue, Sep 15, 2020 at 03:54:45PM +0200, Vincent Whitchurch wrote:

On Sat, Sep 05, 2020 at 06:42:49PM +0200, Uwe Kleine-König wrote:

quoted

On Wed, Sep 02, 2020 at 02:12:34PM +0200, Vincent Whitchurch wrote:

quoted

v2:
 - [..]
 - Stop PWM before unregister

I didn't take the time yet to look at v2, but just spotted this which is
wrong. .remove() is not supposed to modify the output. (If the PWM is
still running in .remove() this is either because it was running at
bootup and was never modified or is a bug in the consumer code.)

If the PWM is not stopped while unregistering, the hrtimer will still
be active and unloading the module will result in a crash when the next
callback hits.  The consumer can be userspace via sysfs.

This definitely outweighs my argument. So please stop the timer and put
a comment above like:

	The PWM should be already off here. Even if it is not we have to
	remove the timer because the timer function is about to go away
	and failing to stop it most probably results in an oops.

# insmod /pwm-gpio.ko
# lsmod
Module                  Size  Used by    Not tainted
pwm_gpio               16384  0
# cd /sys/class/pwm/
# ls
pwmchip0
# cd pwmchip0/
# ls
device     export     npwm       power      subsystem  uevent     unexport
# echo 0 > export
# ls
device     npwm       pwm0       uevent
export     power      subsystem  unexport
# cd pwm0/
# ls
capture     enable      polarity    uevent
duty_cycle  period      power
# echo 100000 > period
# echo 10000 > duty_cycle
# echo 1 > enable
# lsmod
Module                  Size  Used by    Not tainted
pwm_gpio               16384  1
# echo 0 > unexport

I'm a bit torn if I should claim that this is a bug in sysfs.

Thierry, do you have an opinion here?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |