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: > > On Wed, Sep 02, 2020 at 02:12:34PM +0200, Vincent Whitchurch wrote: > > > 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/ |
Attachment:
signature.asc
Description: PGP signature
