linux-kernel - Re: [PATCH v5] hwmon: (pmbus/tps53679) Add support for TPS53685

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] [day] [month] [year] [list]
Message-ID: <2628dd1f-1b8f-478a-aa89-4c0f78b27962@roeck-us.net>
Date: Fri, 14 Mar 2025 10:44:58 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Chiang Brian <chiang.brian@...entec.com>, Jean Delvare
 <jdelvare@...e.com>, Mickaël Salaün <mic@...ikod.net>,
 Günther Noack <gnoack@...gle.com>,
 Kees Cook <kees@...nel.org>, Tony Luck <tony.luck@...el.com>,
 "Guilherme G. Piccoli" <gpiccoli@...lia.com>
Cc: linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-security-module@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH v5] hwmon: (pmbus/tps53679) Add support for TPS53685

On 3/13/25 20:30, Chiang Brian wrote:
> The TPS53685 is a fully AMD SVI3 compliant step down
> controller with trans-inductor voltage regulator
> (TLVR) topology support, dual channels, built-in
> non-volatile memory (NVM), PMBus interface, and
> full compatible with TI NexFET smart power
> stages.
> Add support for it to the tps53679 driver.
> 
> Signed-off-by: Chiang Brian <chiang.brian@...entec.com>
> ---
> v4 -> V5:
>      1. document the compatible of tps53685 into dt-bindings
> 	2. add the buffer length as argument for %*ph
> 	3. Add Changelog
> v3 -> V4:
>      1. Add length comparison into the comparison of "id",or it may be true when
> 	   the substring of "id" matches device id.
> 	2. Restore `return 0;` in `tps53679_identify_chip()`
> V2 -> V3:
>      1. Remove the length comparsion in the comparison of "id".
> V1 -> V2:
> 	1. Modify subject and description to meet requirements
> 	2. Add "tps53685" into enum chips with numeric order
> 	3. Modify the content of marco "TPS53681_DEVICE_ID" from 0x81 to "\x81"
> 	   Add marco "TPS53685_DEVICE_ID" with content "TIShP"
> 	4. Modify the type of "id" from u16 to char* in `tps53679_identify_chip()`
> 	5. Modify the comparison of "id". It will be true if the string "id" matches
> 	   device ID and compare with type char*,
> 	6. Add the length comparsion into the comparison of "id".
> 	7. Modify "len" as return code in `tps53679_identify_chip()`
> 	8. Output device error log with %*ph, instead of 0x%x\n"
>      9. Use existing tps53679_identify_multiphase() with argument
> 	   "TPS53685_DEVICE_ID" in tps53685_identify() rather than creating one
> 	   tps53685_identify_multiphase()
> 
> boot-log:

This is completely useless noise.

> 	
>   drivers/hwmon/pmbus/tps53679.c | 29 +++++++++++++++++++++++------
>   1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/tps53679.c b/drivers/hwmon/pmbus/tps53679.c
> index 63524dff5e75..091d5eacbee0 100644
> --- a/drivers/hwmon/pmbus/tps53679.c
> +++ b/drivers/hwmon/pmbus/tps53679.c
> @@ -16,7 +16,7 @@
>   #include "pmbus.h"
>   
>   enum chips {
> -	tps53647, tps53667, tps53676, tps53679, tps53681, tps53688
> +	tps53647, tps53667, tps53676, tps53679, tps53681, tps53685, tps53688
>   };
>   
>   #define TPS53647_PAGE_NUM		1
> @@ -31,7 +31,8 @@ enum chips {
>   #define TPS53679_PROT_VR13_5MV		0x07 /* VR13.0 mode, 5-mV DAC */
>   #define TPS53679_PAGE_NUM		2
>   
> -#define TPS53681_DEVICE_ID		0x81
> +#define TPS53681_DEVICE_ID     "\x81"
> +#define TPS53685_DEVICE_ID     "TIShP"
>   
>   #define TPS53681_PMBUS_REVISION		0x33
>   
> @@ -86,7 +87,7 @@ static int tps53679_identify_phases(struct i2c_client *client,
>   }
>   
>   static int tps53679_identify_chip(struct i2c_client *client,
> -				  u8 revision, u16 id)
> +				  u8 revision, char *id)
>   {
>   	u8 buf[I2C_SMBUS_BLOCK_MAX];
>   	int ret;
> @@ -102,8 +103,8 @@ static int tps53679_identify_chip(struct i2c_client *client,
>   	ret = i2c_smbus_read_block_data(client, PMBUS_IC_DEVICE_ID, buf);
>   	if (ret < 0)
>   		return ret;
> -	if (ret != 1 || buf[0] != id) {
> -		dev_err(&client->dev, "Unexpected device ID 0x%x\n", buf[0]);
> +	if (ret != strlen(id) || strncmp(id, buf, ret)) {
> +		dev_err(&client->dev, "Unexpected device ID: %*ph\n", ret, buf);
>   		return -ENODEV;
>   	}
>   	return 0;
> @@ -138,6 +139,14 @@ static int tps53679_identify(struct i2c_client *client,
>   	return tps53679_identify_mode(client, info);
>   }
>   
> +static int tps53685_identify(struct i2c_client *client,
> +				 struct pmbus_driver_info *info)
> +{
> +	info->format[PSC_VOLTAGE_OUT] = linear;
> +	return tps53679_identify_chip(client, TPS53681_PMBUS_REVISION,
> +					   TPS53685_DEVICE_ID);
> +}
> +
>   static int tps53681_identify(struct i2c_client *client,
>   			     struct pmbus_driver_info *info)
>   {
> @@ -215,7 +224,9 @@ static struct pmbus_driver_info tps53679_info = {
>   		PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT |
>   		PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP |
>   		PMBUS_HAVE_POUT,
> -	.func[1] = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
> +	.func[1] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | PMBUS_HAVE_PIN |
> +	    PMBUS_HAVE_STATUS_INPUT |
> +		PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |

This changes the attributes supported for the other chips supported by this
driver. If those chips indeed support the additional attributes, the change
should be submitted as separate patch. Otherwise, please explain how the change
is supposed to work for those chips (and why you don't add the additional flags
after determining that the chip is a tps53685 and after copying the data structure).

Guenter