[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d48395d1-627e-48cb-8eae-391cab27a9d9@roeck-us.net>
Date: Tue, 5 Aug 2025 06:20:38 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: "a.shimko" <artyom.shimko@...il.com>, linux-hwmon@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, sudeep.holla@....com,
cristian.marussi@....com, jdelvare@...e.com, guenter.roeck@...ux.com
Subject: Re: [PATCH 3/3] hwmon: scmi: Enhance error reporting with
dev_err_probe
On 8/5/25 05:43, a.shimko wrote:
> From: Artem Shimko <artyom.shimko@...il.com>
>
> Replace error returns with dev_err_probe() throughout driver:
> - Add descriptive error messages for all failure cases
> - Include relevant context (sensor IDs, types etc)
> - Standardize error reporting format
>
> Improved messages include:
> - "No valid sensor info for index %d"
> - "Failed to allocate channel info array"
> - "SCMI protocol ops not initialized"
>
> Signed-off-by: Artem Shimko <artyom.shimko@...il.com>
> ---
> drivers/hwmon/scmi-hwmon.c | 35 ++++++++++++++++++++++++-----------
> 1 file changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c
> index d03174922e65..081502418dfa 100644
> --- a/drivers/hwmon/scmi-hwmon.c
> +++ b/drivers/hwmon/scmi-hwmon.c
> @@ -240,26 +240,36 @@ static int scmi_hwmon_probe(struct scmi_device *sdev)
> struct scmi_protocol_handle *ph;
>
> if (!handle)
> - return -ENODEV;
> + return dev_err_probe(dev, -ENODEV, "SCMI device handle is NULL\n");
-ENODEV is not supposed to log an error message.
>
> sensor_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_SENSOR, &ph);
> - if (IS_ERR(sensor_ops))
> - return PTR_ERR(sensor_ops);
> + if (IS_ERR_OR_NULL(sensor_ops)) {
This never returns NULL, and AFAIS no other driver checks for it.
Why add this unnecessary check ?
> + if (IS_ERR(sensor_ops))
> + return dev_err_probe(dev, PTR_ERR(sensor_ops),
> + "SCMI sensor protocol acquisition failed\n");
> + return dev_err_probe(dev, -EPROTO,
> + "SCMI sensor protocol ops structure unexpectedly NULL\n");
> + }
> +
> + if (!sensor_ops->info_get || !sensor_ops->count_get)
> + return dev_err_probe(dev, -ENOENT,
> + "SCMI sensor protocol operations are not initialized\n");
It is not this driver's responsibility to validate sensor_ops.
>
> nr_sensors = sensor_ops->count_get(ph);
> if (!nr_sensors)
> - return -EIO;
> + return dev_err_probe(dev, -EIO, "No sensors found\n");
>
> scmi_sensors = devm_kzalloc(dev, sizeof(*scmi_sensors), GFP_KERNEL);
> if (!scmi_sensors)
> - return -ENOMEM;
> + return dev_err_probe(dev, -ENOMEM, "Failed to allocate scmi_sensors structure\n");
-ENOMEM is not supposed to log an error message.
>
> scmi_sensors->ph = ph;
>
> for (i = 0; i < nr_sensors; i++) {
> sensor = sensor_ops->info_get(ph, i);
> if (!sensor)
> - return -EINVAL;
> + return dev_err_probe(dev, -EINVAL,
> + "Failed to get sensor info for sensor %d\n", i);
>
> switch (sensor->type) {
> case TEMPERATURE_C:
> @@ -285,12 +295,12 @@ static int scmi_hwmon_probe(struct scmi_device *sdev)
> scmi_hwmon_chan = devm_kcalloc(dev, nr_types, sizeof(*scmi_hwmon_chan),
> GFP_KERNEL);
> if (!scmi_hwmon_chan)
> - return -ENOMEM;
> + return dev_err_probe(dev, -ENOMEM, "Failed to allocate channel info array\n");
Same as above.
>
> ptr_scmi_ci = devm_kcalloc(dev, nr_types + 1, sizeof(*ptr_scmi_ci),
> GFP_KERNEL);
> if (!ptr_scmi_ci)
> - return -ENOMEM;
> + return dev_err_probe(dev, -ENOMEM, "Failed to allocate channel info pointers\n");
Same as above.
>
> scmi_chip_info.info = ptr_scmi_ci;
> chip_info = &scmi_chip_info;
> @@ -307,7 +317,8 @@ static int scmi_hwmon_probe(struct scmi_device *sdev)
> devm_kcalloc(dev, nr_count[type],
> sizeof(*scmi_sensors->info), GFP_KERNEL);
> if (!scmi_sensors->info[type])
> - return -ENOMEM;
> + return dev_err_probe(dev, -ENOMEM,
> + "Failed to allocate sensor info for type %d\n", type);
And again.
> }
>
> for (i = nr_sensors - 1; i >= 0 ; i--) {
> @@ -336,7 +347,7 @@ static int scmi_hwmon_probe(struct scmi_device *sdev)
> scmi_sensors, chip_info,
> NULL);
> if (IS_ERR(hwdev))
> - return PTR_ERR(hwdev);
> + return dev_err_probe(dev, PTR_ERR(hwdev), "Failed to register hwmon device\n");
>
> for (i = 0; i < nr_count_temp; i++) {
> int ret;
> @@ -352,7 +363,9 @@ static int scmi_hwmon_probe(struct scmi_device *sdev)
> ret = scmi_thermal_sensor_register(dev, ph, sensor);
> if (ret) {
> if (ret == -ENOMEM)
> - return ret;
> + return dev_err_probe(dev, ret,
> + "Failed to allocate memory for thermal zone\n");
> +
And again.
> dev_warn(dev,
> "Thermal zone misconfigured for %s. err=%d\n",
> sensor->name, ret);
Powered by blists - more mailing lists