On 4/19/22 01:49, Alexey Kardashevskiy wrote:
On 14/04/2022 02:51, Scott Cheloha wrote:
This series adds a driver for PAPR hypercall-based watchdog timers,
tentatively named "pseries-wdt".
I wanted to get some clarification on a few things before submitting
the series as a patch, hence the RFC. The first patch adding the
hypercall to hvcall.h is straightforward, but I have questions about
the second patch (the driver). In particular:
- In pseries_wdt_probe() we register the watchdog device with
devm_watchdog_register_device(). However, in pseries_wdt_remove(),
calling watchdog_unregister_devce() causes a kernel panic later,
so I assume this is the wrong thing to do.
It should have been devm_watchdog_unregister_device() (no difference though) and what was the backtrace? Most watchdog drivers do it this way :-/
Please make yourself familiar with devm_ functions and their use.
There is no exported devm_watchdog_unregister_device() because it is
not needed.
Do we need to do anything to clean up the watchdog device during
pseries_wdt_remove()? Or does devm_watchdog_register_device()
ensure the cleanup is handled transparently?
- In pseries_wdt_probe(), is it incorrect to devm_kfree() my
allocation in the event that devm_watchdog_register_device()
fails?
I am pretty sure nothing is going to free the memory you allocated in devm_kzalloc() as you do not even pass the allocated pointer to devm_watchdog_register_device(), it is an offset. The only reason devm_kfree(&pw->wd) won't barf1 is @wd is the first member of the pseries_wdt struct.
Again, please make yourself familiar with devm_ functions
and their use.
Guenter
