linux-kernel - Re: [PATCH v2 2/2] thermal: add a virtual sensor to aggregate temperatures

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJZ5v0gTJSzPXimxwK6RUZfE-XJAPD0OcpdbchXxZ5hN=C_Euw@mail.gmail.com>
Date:   Sat, 9 Oct 2021 16:46:52 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Alexandre Bailon <abailon@...libre.com>
Cc:     "Zhang, Rui" <rui.zhang@...el.com>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Amit Kucheria <amitk@...nel.org>,
        Linux PM <linux-pm@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        ben.tseng@...iatek.com, Kevin Hilman <khilman@...libre.com>,
        Matthias Kaehlcke <mka@...omium.org>,
        kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v2 2/2] thermal: add a virtual sensor to aggregate temperatures

On Thu, Oct 7, 2021 at 6:38 PM Rafael J. Wysocki <rafael@...nel.org> wrote:

[cut]

> > +static int virtual_thermal_sensor_probe(struct platform_device *pdev)
> > +{
> > +       struct virtual_thermal_sensor *sensor;
> > +       struct device *dev = &pdev->dev;
> > +       struct of_phandle_args args;
> > +       u32 type;
> > +       int ret;
> > +       int i;
> > +
> > +       sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
> > +       if (!sensor)
> > +               return -ENOMEM;
> > +
> > +       sensor->count = of_count_phandle_with_args(dev->of_node,
> > +                                                  "thermal-sensors",
> > +                                                  "#thermal-sensor-cells");
> > +       if (sensor->count <= 0)
> > +               return -EINVAL;
> > +
> > +       sensor->sensors = devm_kmalloc_array(dev, sensor->count,
> > +                                            sizeof(*sensor->sensors),
> > +                                            GFP_KERNEL);
> > +       if (!sensor->sensors)
> > +               return -ENOMEM;
> > +
> > +       for (i = 0; i < sensor->count; i++) {
> > +               ret = of_parse_phandle_with_args(dev->of_node,
> > +                                                "thermal-sensors",
> > +                                                "#thermal-sensor-cells",
> > +                                                i,
> > +                                                &args);
> > +               if (ret)
> > +                       return ret;
> > +
> > +               ret = virtual_sensor_add_sensor(sensor, args, i);
> > +               if (ret)
> > +                       return ret;

One more issue that escaped me earlier is that if the hardware sensor
being looked for is not there in the thermal_sensors list at this
point (say because it has not been added to it yet by whoever is
expected to do that), the above will return -ENODEV and all probe will
fail without a way to retry.

It would be better to return -EPROBE_DEFER from here IIUC.

However, this still wouldn't address a more general issue that if the
hardware sensor gets unregistered and then registered again (eg. by
unloading and reloading a module managing it), it will not be added
back to the corresponding virtual sensor's data set.  This should not
be very hard to address and it would take care of the above
initialization ordering issue automatically.