| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7cc0a576.2446.19bbb13ac12.Coremail.hehuan1@eswincomputing.com>
Date: Wed, 14 Jan 2026 13:56:27 +0800 (GMT+08:00)
From: "Huan He" <hehuan1@...incomputing.com>
To: "Krzysztof Kozlowski" <krzk@...nel.org>
Cc: linux@...ck-us.net, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, linux-hwmon@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
p.zabel@...gutronix.de, ningyu@...incomputing.com,
linmin@...incomputing.com, pinkesh.vaghela@...fochips.com,
luyulin@...incomputing.com, weishangjuan@...incomputing.com
Subject: Re: Re: [PATCH v1 2/2] hwmon: Add Eswin EIC7700 PVT sensor driver
> > +static void pvt_clear_data(void *data)
> > +{
> > + struct pvt_hwmon *pvt = data;
> > + int idx;
> > +
> > + for (idx = 0; idx < PVT_SENSORS_NUM; ++idx)
> > + complete_all(&pvt->cache[idx].conversion);
> > +
> > + mutex_destroy(&pvt->iface_mtx);
> > +}
> > +
> > +static struct pvt_hwmon *eic7700_pvt_create_data(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct pvt_hwmon *pvt;
> > + int ret, idx;
> > +
> > + pvt = devm_kzalloc(dev, sizeof(*pvt), GFP_KERNEL);
> > + if (!pvt)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + ret = devm_add_action(dev, pvt_clear_data, pvt);
> > + if (ret) {
> > + dev_err(dev, "Can't add PVT data clear action\n");
> > + return ERR_PTR(ret);
>
> Why do you set up cleaning - mutex destroy, complete - before even
> initializing them?
We corrected the initialization order. Both mutex_init() and
init_completion() are now called before registering the cleanup action
with devm_add_action().
>
> This is very messy or even buggy coding style.
>
> > + }
> > +
> > + pvt->dev = dev;
> > + pvt->sensor = PVT_SENSOR_FIRST;
> > + mutex_init(&pvt->iface_mtx);
> > +
> > + for (idx = 0; idx < PVT_SENSORS_NUM; ++idx)
> > + init_completion(&pvt->cache[idx].conversion);
> > +
> > + return pvt;
> > +}
> > +
> > +static void eic7700_pvt_remove(void *data)
> > +{
> > + int ret;
> > + struct pvt_hwmon *pvt = data;
> > +
> > + pm_runtime_disable(pvt->dev);
> > + pm_runtime_dont_use_autosuspend(pvt->dev);
> > + pm_runtime_get_sync(pvt->dev);
> > +
> > + ret = reset_control_assert(pvt->rst);
> > + if (ret)
> > + dev_err(pvt->dev, "Failed to assert reset: %d\n", ret);
> > +
> > + if (pm_runtime_active(pvt->dev))
> > + clk_disable_unprepare(pvt->clk);
> > +
> > + pm_runtime_put_noidle(pvt->dev);
> > +}
> > +
> > +static int eic7700_pvt_create_hwmon(struct pvt_hwmon *pvt)
> > +{
> > + struct device *dev = pvt->dev;
> > + struct device_node *np = dev->of_node;
> > + const char *node_label;
> > + int type;
> > + const char *names[2] = {"soc_pvt", "ddr_pvt"};
> > +
> > + if (of_property_read_string(np, "label", &node_label)) {
> > + dev_err(dev, "Missing 'label' property in DTS node\n");
> > + return -EINVAL;
> > + }
> > +
> > + if (strcmp(node_label, "pvt0") == 0) {
> > + type = 0;
> > + } else if (strcmp(node_label, "pvt1") == 0) {
> > + type = 1;
> > + } else {
> > + dev_err(pvt->dev, "Unsupported label: %s\n", node_label);
> > + return -EINVAL;
> > + }
> > +
> > + pvt->hwmon = devm_hwmon_device_register_with_info(pvt->dev, names[type],
> > + pvt, &pvt_hwmon_info,
> > + NULL);
> > + if (IS_ERR(pvt->hwmon)) {
> > + dev_err(pvt->dev, "Couldn't create hwmon device\n");
> > + return PTR_ERR(pvt->hwmon);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int eic7700_pvt_probe(struct platform_device *pdev)
> > +{
> > + struct pvt_hwmon *pvt;
> > + int ret;
> > +
> > + pvt = eic7700_pvt_create_data(pdev);
> > + if (IS_ERR(pvt))
> > + return PTR_ERR(pvt);
> > +
> > + platform_set_drvdata(pdev, pvt);
> > +
> > + ret = eic7700_pvt_create_sensor_info(pvt);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Failed to create sensor info\n");
> > + return ret;
> > + }
> > +
> > + pvt->regs = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(pvt->regs))
> > + return PTR_ERR(pvt->regs);
> > +
> > + pvt->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> > + if (IS_ERR(pvt->clk))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(pvt->clk),
> > + "Couldn't get clock\n");
> > +
> > + pvt->rst = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
> > + if (IS_ERR(pvt->rst))
> > + return dev_err_probe(pvt->dev, PTR_ERR(pvt->rst),
> > + "Couldn't to get reset control\n");
> > +
> > + if (pvt->rst) {
> > + ret = reset_control_reset(pvt->rst);
> > + if (ret)
> > + return dev_err_probe(&pdev->dev, ret,
> > + "Couldn't to trigger reset\n");
> > + }
> > +
> > + ret = eic7700_pvt_check_pwr(pvt);
> > + if (ret)
> > + return ret;
> > +
> > + ret = eic7700_pvt_init_iface(pvt);
> > + if (ret)
> > + return ret;
> > +
> > + ret = eic7700_pvt_request_irq(pvt);
> > + if (ret)
> > + return ret;
> > +
> > + pm_runtime_enable(&pdev->dev);
> > + pm_runtime_set_autosuspend_delay(&pdev->dev, 3000);
> > + pm_runtime_use_autosuspend(&pdev->dev);
> > + pm_runtime_get_noresume(&pdev->dev);
> > +
> > + ret = eic7700_pvt_create_hwmon(pvt);
> > + if (ret)
>
> You leak here - missing PM runtime cleanup.
We added devm_add_action_or_reset(dev, eic7700_pvt_disable_pm_runtime, pvt)
immediately after configuring runtime PM. This ensures that the runtime PM
cleanup will be executed on probe failure, probe success, and device
removal, preventing any potential resource leaks.
static void eic7700_pvt_disable_pm_runtime(void *data)
{
struct pvt_hwmon *pvt = data;
pm_runtime_dont_use_autosuspend(pvt->dev);
pm_runtime_disable(pvt->dev);
}
The relevant code changes in eic7700_pvt_probe():
pm_runtime_enable(&pdev->dev);
pm_runtime_set_autosuspend_delay(&pdev->dev, 3000);
pm_runtime_use_autosuspend(&pdev->dev);
pm_runtime_get_noresume(&pdev->dev);
ret = devm_add_action_or_reset(pvt->dev, eic7700_pvt_disable_pm_runtime, pvt);
if (ret)
return dev_err_probe(&pdev->dev, ret,
"Can't register PM cleanup\n");
ret = eic7700_pvt_create_hwmon(pvt);
if (ret)
return ret;
>
> > + return ret;
> > +
> > + ret = devm_add_action_or_reset(pvt->dev, eic7700_pvt_remove, pvt);
> > + if (ret) {
> > + dev_err(pvt->dev, "Can't add PVT clocks disable action\n");
>
> Why PM runtime cleanup should happen only from this moment/error path?
>
> Way you organized your code and stuffed multiple cleanups at once is
> very messy. This is unreadable at best or even buggy code.
We replaced devm_reset_control_get_optional_exclusive() and manual reset
handling with devm_reset_control_get_exclusive_deasserted(), which
automatically manages the reset lifecycle.
>
> > + return ret;
> > + }
> > +
> > + pm_runtime_put_autosuspend(&pdev->dev);
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused eic7700_pvt_runtime_resume(struct device *dev)
> > +{
> > + struct pvt_hwmon *pvt = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + if (!pvt->clk) {
> > + dev_err(dev, "clk not initialized\n");
>
> What? How?
This judgment has been removed.
Best regards,
Huan He
Powered by blists - more mailing lists