Re: [PATCH v2] watchdog: lenovo_se30g2_se60_wdt: Watchdog for Lenovo SE30G2 and SE60

Hi Guenter,

On Sun, Jun 28, 2026, at 11:12 AM, Guenter Roeck wrote:
> On 6/25/26 10:52, [email protected] wrote:
>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>> - [Medium] Incorrect error cleanup for a registered platform device leaves a zombified sysfs device.
>> --
>> 
>> commit 728400a02a11a5d2e004ae6d02e2731670483271
>> Author: Mark Pearson <[email protected]>
>> 
>> watchdog: lenovo_se30g2_se60_wdt: Watchdog for Lenovo SE30G2 and SE60
>> 
>> This commit adds the watchdog driver implementation for Lenovo SE30G2 and
>> SE60 platforms.
>> 
>>> diff --git a/drivers/watchdog/lenovo_se30g2_se60_wdt.c b/drivers/watchdog/lenovo_se30g2_se60_wdt.c
>>> --- /dev/null
>>> +++ b/drivers/watchdog/lenovo_se30g2_se60_wdt.c
>> [ ... ]
>>> +static int se_30g2_60_create_device(const struct dmi_system_id *id)
>>> +{
>>> +	int err;
>>> +
>>> +	se_30g2_60_pdev = platform_device_alloc("lenovo-se30g2-se60-wdt", -1);
>>> +	if (!se_30g2_60_pdev)
>>> +		return -ENOMEM;
>>> +
>>> +	err = platform_device_add(se_30g2_60_pdev);
>>> +	if (err) {
>>> +		platform_device_put(se_30g2_60_pdev);
>>> +		se_30g2_60_pdev = NULL;
>>> +	}
>>> +
>>> +	return err;
>>> +}
>> [ ... ]
>>> +static int __init se_30g2_60_wdt_init(void)
>>> +{
>>> +	int err;
>>> +
>>> +	if (!dmi_check_system(se_30g2_60))
>>> +		return -ENODEV;
>>> +
>>> +	err = platform_driver_register(&se_30g2_60_wdt_driver);
>>> +	if (err && se_30g2_60_pdev) {
>>> +		platform_device_put(se_30g2_60_pdev);
>> 
>> [Severity: Medium]
>> Since the device was successfully added to the system bus via
>> platform_device_add() in the DMI callback earlier, will calling
>> platform_device_put() here just drop the allocation reference without
>> removing the device from the sysfs hierarchy?
>> 
>> Should this error path use platform_device_unregister() instead to avoid
>> leaving a zombie device on the bus that could cause collisions on a later
>> module load?
>> 
> I don't understand this code either. The parallel platform_device_put()
> on error in se_30g2_60_create_device() seems risky. Each of the Lenovo
> drivers uses a different mechanism to register the driver and the device.
> Why ? That just makes it more difficult to review the code.
>
> Guenter

Originally this was the same as the lenovo_se10 (I thought about combining them but it got messy do didn't). After review by sashiko pointed out the potential lack of free so I did this fix, but looks like this is wrong too. I need to go and revisit.
I'm sick right now (some sort of summer flu...it sucks) but I'll update it hopefully next week and do a new version.

On the differences between lenovo drivers - agreed. A lot of that is down to new issues being pointed out during the review of each driver as they are submitted. I'll happily bring them all up to the same base once I get this one right.

Mark