Re: [PATCH 4/6] hwmon: (k10temp): Show errors failing to read

[Guenter Roeck <linux@roeck-us.net>]

On 8/26/21 11:40 AM, Mario Limonciello wrote:
> Enabling Yellow Carp in past commit was initially not working "properly"
> because extra IDs were needed, but this wasn't obvious because fail values
> from `amd_smn_read` were ignored.
> If errors are found, show a kernel warning.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/hwmon/k10temp.c | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/k10temp.c b/drivers/hwmon/k10temp.c
> index 5c1aebf7a56d..41d9c0c0a1f1 100644
> --- a/drivers/hwmon/k10temp.c
> +++ b/drivers/hwmon/k10temp.c
> @@ -164,8 +164,10 @@ static void read_tempreg_nb_f15(struct pci_dev *pdev, u32 *regval)
>   
>   static void read_tempreg_nb_zen(struct pci_dev *pdev, u32 *regval)
>   {
> -	amd_smn_read(amd_pci_dev_to_node_id(pdev),
> +	int ret = amd_smn_read(amd_pci_dev_to_node_id(pdev),
>   		     ZEN_REPORTED_TEMP_CTRL_BASE, regval);
> +	if (ret)
> +		dev_warn(&pdev->dev, "failed to read core temperature: %d\n", ret);
>   }

It would be much better to change the code to return the error to the caller
(and thus to userspace) instead of polluting the kernel log and (presumably)
reporting a random value to userspace.

Guenter

>   
>   static long get_raw_temp(struct k10temp_data *data)
> @@ -212,6 +214,7 @@ static int k10temp_read_temp(struct device *dev, u32 attr, int channel,
>   {
>   	struct k10temp_data *data = dev_get_drvdata(dev);
>   	u32 regval;
> +	int ret;
>   
>   	switch (attr) {
>   	case hwmon_temp_input:
> @@ -227,10 +230,13 @@ static int k10temp_read_temp(struct device *dev, u32 attr, int channel,
>   				*val = 0;
>   			break;
>   		case 2 ... 9:		/* Tccd{1-8} */
> -			amd_smn_read(amd_pci_dev_to_node_id(data->pdev),
> +			ret = amd_smn_read(amd_pci_dev_to_node_id(data->pdev),
>   				     ZEN_CCD_TEMP(data->ccd_offset, channel - 2),
>   						  &regval);
> -			*val = (regval & ZEN_CCD_TEMP_MASK) * 125 - 49000;
> +			if (ret)
> +				dev_warn(dev, "failed to read CCD temperature: %d\n", ret);

This is a static error. Just return the error to userspace, please,
and don't pollute the kernel log.

Guenter

> +			else
> +				*val = (regval & ZEN_CCD_TEMP_MASK) * 125 - 49000;
>   			break;
>   		default:
>   			return -EOPNOTSUPP;
>