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

From: "Rafael J. Wysocki" <rafael@kernel.org>
To: Alexandre Bailon <abailon@baylibre.com>
Cc: "Zhang, Rui" <rui.zhang@intel.com>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Amit Kucheria <amitk@kernel.org>,
	Linux PM <linux-pm@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	ben.tseng@mediatek.com, Kevin Hilman <khilman@baylibre.com>,
	Matthias Kaehlcke <mka@chromium.org>,
	kernel test robot <lkp@intel.com>
Subject: Re: [PATCH v2 2/2] thermal: add a virtual sensor to aggregate temperatures
Date: Sat, 9 Oct 2021 16:46:52 +0200	[thread overview]
Message-ID: <CAJZ5v0gTJSzPXimxwK6RUZfE-XJAPD0OcpdbchXxZ5hN=C_Euw@mail.gmail.com> (raw)
In-Reply-To: <CAJZ5v0hzS-bquhW_wbsd81EpJYx5RHxeXkamdZ+MBs4oczDw1A@mail.gmail.com>

On Thu, Oct 7, 2021 at 6:38 PM Rafael J. Wysocki <rafael@kernel.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.