libata: Add error handling for pdc20621_i2c_read().

[PATCH] libata: Add error handling for pdc20621_i2c_read().

Posted by Wentao Liang 1 day ago

The pdc20621_prog_dimm0() calls the pdc20621_i2c_read(), but does not
handle the error if the read fails. This could lead to process with
invalid data. A proper inplementation can be found in
pdc20621_prog_dimm_global(). As mentioned in its commit:
bb44e154e25125bef31fa956785e90fccd24610b, the variable spd0 might be
used uninitialized when pdc20621_i2c_read() fails.

Add error handling to the pdc20621_i2c_read(). If a read operation fails,
an error message is logged via dev_err(), and return an under-zero value
to represent error situlation.

Add error handling to pdc20621_prog_dimm0() in pdc20621_dimm_init(), and
return a none-zero value when pdc20621_prog_dimm0() fails.

Fixes: 4447d3515616 ("libata: convert the remaining SATA drivers to new init model")
Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
 drivers/ata/sata_sx4.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/sata_sx4.c b/drivers/ata/sata_sx4.c
index a482741eb181..a4027eb2fb66 100644
--- a/drivers/ata/sata_sx4.c
+++ b/drivers/ata/sata_sx4.c
@@ -1117,9 +1117,14 @@ static int pdc20621_prog_dimm0(struct ata_host *host)
 	mmio += PDC_CHIP0_OFS;
 
 	for (i = 0; i < ARRAY_SIZE(pdc_i2c_read_data); i++)
-		pdc20621_i2c_read(host, PDC_DIMM0_SPD_DEV_ADDRESS,
-				  pdc_i2c_read_data[i].reg,
-				  &spd0[pdc_i2c_read_data[i].ofs]);
+		if (!pdc20621_i2c_read(host, PDC_DIMM0_SPD_DEV_ADDRESS,
+				       pdc_i2c_read_data[i].reg,
+				       &spd0[pdc_i2c_read_data[i].ofs])){
+			dev_err(host->dev,
+				"Failed in i2c read at index %d: device=%#x, reg=%#x\n",
+				i, PDC_DIMM0_SPD_DEV_ADDRESS, pdc_i2c_read_data[i].reg);
+			return -1;
+		}
 
 	data |= (spd0[4] - 8) | ((spd0[21] != 0) << 3) | ((spd0[3]-11) << 4);
 	data |= ((spd0[17] / 4) << 6) | ((spd0[5] / 2) << 7) |
@@ -1284,6 +1289,8 @@ static unsigned int pdc20621_dimm_init(struct ata_host *host)
 
 	/* Programming DIMM0 Module Control Register (index_CID0:80h) */
 	size = pdc20621_prog_dimm0(host);
+	if (size < 0)
+		return 1;
 	dev_dbg(host->dev, "Local DIMM Size = %dMB\n", size);
 
 	/* Programming DIMM Module Global Control Register (index_CID0:88h) */
-- 
2.42.0.windows.2

Re: [PATCH] libata: Add error handling for pdc20621_i2c_read().

Posted by Damien Le Moal 7 hours ago

On 4/3/25 7:39 PM, Wentao Liang wrote:
> The pdc20621_prog_dimm0() calls the pdc20621_i2c_read(), but does not

The function pdc20621_prog_dimm0() calls the function pdc20621_i2c_read() but
does...


> handle the error if the read fails. This could lead to process with
> invalid data. A proper inplementation can be found in

s/inplementation/implementation

> pdc20621_prog_dimm_global(). As mentioned in its commit:
> bb44e154e25125bef31fa956785e90fccd24610b, the variable spd0 might be
> used uninitialized when pdc20621_i2c_read() fails.
> 
> Add error handling to the pdc20621_i2c_read(). If a read operation fails,
> an error message is logged via dev_err(), and return an under-zero value
> to represent error situlation.

and return a negative error code.

> 
> Add error handling to pdc20621_prog_dimm0() in pdc20621_dimm_init(), and
> return a none-zero value when pdc20621_prog_dimm0() fails.

return a negative error code if pdc20621_prog_dimm0() fails.

> 
> Fixes: 4447d3515616 ("libata: convert the remaining SATA drivers to new init model")
> Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
> ---
>  drivers/ata/sata_sx4.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/sata_sx4.c b/drivers/ata/sata_sx4.c
> index a482741eb181..a4027eb2fb66 100644
> --- a/drivers/ata/sata_sx4.c
> +++ b/drivers/ata/sata_sx4.c
> @@ -1117,9 +1117,14 @@ static int pdc20621_prog_dimm0(struct ata_host *host)
>  	mmio += PDC_CHIP0_OFS;
>  
>  	for (i = 0; i < ARRAY_SIZE(pdc_i2c_read_data); i++)
> -		pdc20621_i2c_read(host, PDC_DIMM0_SPD_DEV_ADDRESS,
> -				  pdc_i2c_read_data[i].reg,
> -				  &spd0[pdc_i2c_read_data[i].ofs]);
> +		if (!pdc20621_i2c_read(host, PDC_DIMM0_SPD_DEV_ADDRESS,
> +				       pdc_i2c_read_data[i].reg,
> +				       &spd0[pdc_i2c_read_data[i].ofs])){
> +			dev_err(host->dev,
> +				"Failed in i2c read at index %d: device=%#x, reg=%#x\n",
> +				i, PDC_DIMM0_SPD_DEV_ADDRESS, pdc_i2c_read_data[i].reg);
> +			return -1;

			return -EIO;
> +		}
>  
>  	data |= (spd0[4] - 8) | ((spd0[21] != 0) << 3) | ((spd0[3]-11) << 4);
>  	data |= ((spd0[17] / 4) << 6) | ((spd0[5] / 2) << 7) |
> @@ -1284,6 +1289,8 @@ static unsigned int pdc20621_dimm_init(struct ata_host *host)
>  
>  	/* Programming DIMM0 Module Control Register (index_CID0:80h) */
>  	size = pdc20621_prog_dimm0(host);
> +	if (size < 0)
> +		return 1;

		return size;

>  	dev_dbg(host->dev, "Local DIMM Size = %dMB\n", size);
>  
>  	/* Programming DIMM Module Global Control Register (index_CID0:88h) */


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] libata: Add error handling for pdc20621_i2c_read().

Posted by Damien Le Moal 7 hours ago

On 4/4/25 12:54 PM, Damien Le Moal wrote:
> On 4/3/25 7:39 PM, Wentao Liang wrote:

Also forgot to mention that the patch title should be:

ata: sata_sx4: Add error handling for pdc20621_i2c_read()

And no period at the end of it please.

>> The pdc20621_prog_dimm0() calls the pdc20621_i2c_read(), but does not
> 
> The function pdc20621_prog_dimm0() calls the function pdc20621_i2c_read() but
> does...
> 
> 
>> handle the error if the read fails. This could lead to process with
>> invalid data. A proper inplementation can be found in
> 
> s/inplementation/implementation
> 
>> pdc20621_prog_dimm_global(). As mentioned in its commit:
>> bb44e154e25125bef31fa956785e90fccd24610b, the variable spd0 might be
>> used uninitialized when pdc20621_i2c_read() fails.
>>
>> Add error handling to the pdc20621_i2c_read(). If a read operation fails,
>> an error message is logged via dev_err(), and return an under-zero value
>> to represent error situlation.
> 
> and return a negative error code.
> 
>>
>> Add error handling to pdc20621_prog_dimm0() in pdc20621_dimm_init(), and
>> return a none-zero value when pdc20621_prog_dimm0() fails.
> 
> return a negative error code if pdc20621_prog_dimm0() fails.
> 
>>
>> Fixes: 4447d3515616 ("libata: convert the remaining SATA drivers to new init model")
>> Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
>> ---
>>  drivers/ata/sata_sx4.c | 13 ++++++++++---
>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/ata/sata_sx4.c b/drivers/ata/sata_sx4.c
>> index a482741eb181..a4027eb2fb66 100644
>> --- a/drivers/ata/sata_sx4.c
>> +++ b/drivers/ata/sata_sx4.c
>> @@ -1117,9 +1117,14 @@ static int pdc20621_prog_dimm0(struct ata_host *host)
>>  	mmio += PDC_CHIP0_OFS;
>>  
>>  	for (i = 0; i < ARRAY_SIZE(pdc_i2c_read_data); i++)
>> -		pdc20621_i2c_read(host, PDC_DIMM0_SPD_DEV_ADDRESS,
>> -				  pdc_i2c_read_data[i].reg,
>> -				  &spd0[pdc_i2c_read_data[i].ofs]);
>> +		if (!pdc20621_i2c_read(host, PDC_DIMM0_SPD_DEV_ADDRESS,
>> +				       pdc_i2c_read_data[i].reg,
>> +				       &spd0[pdc_i2c_read_data[i].ofs])){
>> +			dev_err(host->dev,
>> +				"Failed in i2c read at index %d: device=%#x, reg=%#x\n",
>> +				i, PDC_DIMM0_SPD_DEV_ADDRESS, pdc_i2c_read_data[i].reg);
>> +			return -1;
> 
> 			return -EIO;
>> +		}
>>  
>>  	data |= (spd0[4] - 8) | ((spd0[21] != 0) << 3) | ((spd0[3]-11) << 4);
>>  	data |= ((spd0[17] / 4) << 6) | ((spd0[5] / 2) << 7) |
>> @@ -1284,6 +1289,8 @@ static unsigned int pdc20621_dimm_init(struct ata_host *host)
>>  
>>  	/* Programming DIMM0 Module Control Register (index_CID0:80h) */
>>  	size = pdc20621_prog_dimm0(host);
>> +	if (size < 0)
>> +		return 1;
> 
> 		return size;
> 
>>  	dev_dbg(host->dev, "Local DIMM Size = %dMB\n", size);
>>  
>>  	/* Programming DIMM Module Global Control Register (index_CID0:88h) */
> 
> 


-- 
Damien Le Moal
Western Digital Research