Re: [PATCH v8 1/2] firmware: stratix10-svc: add async HWMON read commands and register socfpga-hwmon device

["NG, TZE YEE" <tze.yee.ng@altera.com>]

On 23/7/2026 2:14 pm, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [Medium] Hardware monitoring functionality is improperly registered as a platform device instead of an auxiliary device.
> 

I'll keep the platform device registration for this series. I’m
happy to look at converting socfpga-hwmon to an auxiliary device in a
follow-up if preferred.

> Pre-existing issues:
> - [High] Pre-existing Use-After-Free race condition during module removal due to improper teardown ordering of child devices.

It is fixed in
https://lore.kernel.org/all/6630a1568f162d9455e0580f7ccadc262db4718e.1784007275.git.adrian.ho.yin.ng@altera.com/

> --
> 
> --- Patch [1]: [PATCH v8 1/2] firmware: stratix10-svc: add async HWMON read commands and register socfpga-hwmon device ---
> commit 5ba2e4fb155f074de568280aeede81883f87924e
> Author: Tze Yee Ng <tze.yee.ng@altera.com>
> 
> firmware: stratix10-svc: add async HWMON read commands and register socfpga-hwmon device
> 
> Add asynchronous Stratix 10 service layer support for hardware monitor
> temperature and voltage read commands. Register a socfpga-hwmon platform
> device from the service layer driver when hardware monitor support is enabled.
> 
>> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
>> index c24ca5823078f..fc38afed5b7fe 100644
>> --- a/drivers/firmware/stratix10-svc.c
>> +++ b/drivers/firmware/stratix10-svc.c
> 
> [ ... ]
> 
>> @@ -2013,16 +2028,38 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev)
>>   	if (ret)
>>   		goto err_put_device;
>>   
>> +	if (IS_ENABLED(CONFIG_SENSORS_ALTERA_SOCFPGA_HWMON)) {
>> +		svc->stratix10_svc_hwmon =
>> +			platform_device_alloc(SOCFPGA_HWMON, 0);
> 
> [Severity: Medium]
> According to the hardware monitoring subsystem guidelines, if the main
> functionality of a chip is not hardware monitoring, its secondary hardware
> monitoring functionality should be implemented as an auxiliary device driver.
> 
> Since the primary purpose of this module is the firmware service layer,
> does registering the hardware monitoring functionality as a platform device
> violate the subsystem API layering? Could this be implemented via the
> auxiliary bus instead of using platform_device_alloc()?
> 
>> +		if (!svc->stratix10_svc_hwmon) {
>> +			dev_err(dev, "failed to allocate %s device\n",
>> +				SOCFPGA_HWMON);
>> +		} else {
>> +			svc->stratix10_svc_hwmon->dev.parent = dev;
>> +
>> +			ret = platform_device_add(svc->stratix10_svc_hwmon);
>> +			if (ret) {
>> +				dev_err(dev, "failed to add %s device: %d\n",
>> +					SOCFPGA_HWMON, ret);
>> +				platform_device_put(svc->stratix10_svc_hwmon);
>> +				svc->stratix10_svc_hwmon = NULL;
>> +			}
>> +		}
>> +	}
>> +
>>   	ret = of_platform_default_populate(dev_of_node(dev), NULL, dev);
> 
> [ ... ]
> 
>> @@ -2046,6 +2083,9 @@ static void stratix10_svc_drv_remove(struct platform_device *pdev)
>>   	struct stratix10_svc_controller *ctrl = platform_get_drvdata(pdev);
>>   	struct stratix10_svc *svc = ctrl->svc;
>>   
>> +	if (svc->stratix10_svc_hwmon)
>> +		platform_device_unregister(svc->stratix10_svc_hwmon);
>> +
>>   	stratix10_svc_async_exit(ctrl);
>>   
>>   	of_platform_depopulate(ctrl->dev);
> 
> [Severity: High]
> This is a pre-existing issue, but does this teardown order create a
> use-after-free race condition during module removal?
> 
> The stratix10_svc_drv_remove() function calls stratix10_svc_async_exit()
> before unregistering the remaining OF child devices and the RSU child device.
> The async exit function destroys the async structures and frees the
> asynchronous channels and handlers.
> 
> If userspace concurrently accesses sysfs attributes for the child devices
> while the module unloads, could it invoke stratix10_svc_async_send()
> or stratix10_svc_async_poll() after the async channels have been freed?
> 
> For example, stratix10_svc_async_send() performs lockless reads of achan:
> 
> stratix10_svc_async_send() {
> 	...
> 	achan = chan->async_chan;
> 	ctrl = chan->ctrl;
> 	actrl = &ctrl->actrl;
> 
> 	if (!actrl->initialized) {
> 	...
> }
> 
> Would this lead to memory corruption if the structures are already deallocated?
> Should the child devices be fully unregistered before calling
> stratix10_svc_async_exit() to ensure no concurrent accesses are possible?
>