linux-kernel - Re: [PATCH v7] iio: adc: pac1921: Add ACPI support to Microchip pac1921

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: <ZyaFNsuWOOK1HHg0@surfacebook.localdomain>
Date: Sat, 2 Nov 2024 22:01:58 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: victor.duicu@...rochip.com
Cc: matteomartelli3@...il.com, jic23@...nel.org, lars@...afoo.de,
	marius.cristea@...rochip.com, linux-iio@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7] iio: adc: pac1921: Add ACPI support to Microchip
 pac1921

Thu, Oct 31, 2024 at 08:52:05AM +0200, victor.duicu@...rochip.com kirjoitti:
> From: Victor Duicu <victor.duicu@...rochip.com>
> 
> This patch implements ACPI support to Microchip pac1921.
> The driver can read shunt resistor value and label from ACPI table.

More comments below.

...

> +#define PAC1921_ACPI_GET_UOHMS_VALS             0

uOHM ?

#define PAC1921_ACPI_GET_uOHM_VALS             0

...

> +/* The maximum accepted value of shunt_resistor in UOHMS <= INT_MAX */
> +#define PAC1921_MAX_SHUNT_VALUE_OHMS		2147

Instead of the comment do it like this:

#define PAC1921_MAX_SHUNT_VALUE_OHM		(INT_MAX / MICRO)

Need to include limits.h and units.h.

...

> +static inline bool pac1921_shunt_is_invalid(u32 shunt_val)

is_invalid is confusing name, make it rather is_valid

> +{
> +	return shunt_val == 0 || shunt_val > INT_MAX;
> +}

...

> +	/* This check is to ensure val * MICRO won't overflow */
> +	if (val < 0 || val > PAC1921_MAX_SHUNT_VALUE_OHMS)
> +		return -EINVAL;
> +
>  	rshunt_uohm = val * MICRO + val_fract;
> -	if (rshunt_uohm == 0 || rshunt_uohm > INT_MAX)
> +	if (pac1921_shunt_is_invalid(rshunt_uohm))
>  		return -EINVAL;

With the above check how this can't be a dead code?

...

> +	rez = acpi_evaluate_dsm(handle, &guid, 1, PAC1921_ACPI_GET_UOHMS_VALS, NULL);
> +	if (!rez)

Use status instead of rez for the variable name. This is kinda standard to keep
the return of ACPI APIs.

> +		return dev_err_probe(dev, -EINVAL,
> +				     "Could not read shunt from ACPI table\n");

...

> +	label = devm_kstrdup(dev, rez->package.elements->string.pointer, GFP_KERNEL);
> +	if (!label)
> +		return dev_err_probe(dev, -EINVAL, "Label is NULL\n");

We do not print an error for -ENOMEM, which should be here as the error code
(Jonathan already pointed out on this).

So, just

		return -ENOMEM;

-- 
With Best Regards,
Andy Shevchenko