On 4/5/25 10:53 PM, Wentao Liang wrote:
You forgot to update the patch title. As I said, it must be:
ata: sata_sx4: Add error handling in pdc20621_i2c_read()
> The function pdc20621_prog_dimm0() calls the function 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
> /source/drivers/ata/sata_sx4.c, 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,
s/to the/to
> an error message is logged via dev_err(), and return a negative error
> code.
>
> Add error handling to pdc20621_prog_dimm0() in pdc20621_dimm_init(), and
> 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@xxxxxxxxxxx>
> ---
> 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..c3042eca6332 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 -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 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
