Re: [PATCH] i2c: designware: slave should do WRITE_RECEIVED before SLAVE_STOP

[Andy Shevchenko]

On Mon, Oct 10, 2022 at 11:40:15AM +0800, tianye@xxxxxxxxx wrote:
> From: Tian Ye <tianye@xxxxxxxxx>
> 
> Sometimes when designware slave receive 3byte in high speed mode:

DesignWare
3 bytes

> 0x1 STATUS SLAVE_ACTIVITY=0x1 : RAW_INTR_STAT=0x514 : INTR_STAT=0x4
> I2C_SLAVE_WRITE_REQUESTED
> I2C_SLAVE_WRITE_RECEIVED
> 0x1 STATUS SLAVE_ACTIVITY=0 : RAW_INTR_STAT=0x714 : INTR_STAT=0x204
> I2C_SLAVE_WRITE_RECEIVED
> I2C_SLAVE_STOP
> 0x1 STATUS SLAVE_ACTIVITY=0x1 : RAW_INTR_STAT=0x514 : INTR_STAT=0x4
> I2C_SLAVE_WRITE_REQUESTED
> I2C_SLAVE_WRITE_RECEIVED
> 
> When second slave interrupt occus:slave rx fifo receive two bytes and

occurs: slave Rx FIFO receives 2 bytes

> stop interrupt occus at the same time.

occurs

...

Please, do a spell check / proof reading of the commit messages.

...

> +	u32 rx_valid;

> +		regmap_read(dev, DW_IC_RXFLR, &rx_valid);

If regmap_read() fails, rx_valid will contain garbage...

> +		for (; rx_valid > 0; rx_valid--) {

...and this will go far beyond the expected boundaries.

> +			regmap_read(dev->map, DW_IC_DATA_CMD, &tmp);
> +			val = tmp;
> +			if (!i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_RECEIVED,
> +					     &val))

> +				dev_vdbg(dev->dev, "Byte %X acked!", val);

Why do you need this? regmap has it's own trace event mechanism, isn't it
enough?

> +		}

-- 
With Best Regards,
Andy Shevchenko