[PATCH v2] i2c: mediatek: fix potential incorrect use of I2C_MASTER_WRRD

[PATCH v2] i2c: mediatek: fix potential incorrect use of I2C_MASTER_WRRD

[Leilk Liu]

From: "Leilk.Liu" <leilk.liu@mediatek.com>

The old IC does not support the I2C_MASTER_WRRD (write-then-read)
function, but the current code’s handling of i2c->auto_restart may
potentially lead to entering the I2C_MASTER_WRRD software flow,
resulting in unexpected bugs.

Instead of repurposing the auto_restart flag, add a separate flag
to signal I2C_MASTER_WRRD operations.

Also fix handling of msgs. If the operation (i2c->op) is
I2C_MASTER_WRRD, then the msgs pointer is incremented by 2.
For all other operations, msgs is simply incremented by 1.

Fixes: b2ed11e224a2 ("I2C: mediatek: Add driver for MediaTek MT8173 I2C controller")
Signed-off-by: Leilk.Liu <leilk.liu@mediatek.com>
Suggested-by: Chen-Yu Tsai <wenst@chromium.org>
Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>
---
Changes in v2:
 - modify the commit which introduce this issue.
---
 drivers/i2c/busses/i2c-mt65xx.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
index ab456c3717db..dee40704825c 100644
--- a/drivers/i2c/busses/i2c-mt65xx.c
+++ b/drivers/i2c/busses/i2c-mt65xx.c
@@ -1243,6 +1243,7 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
 {
 	int ret;
 	int left_num = num;
+	bool write_then_read_en = false;
 	struct mtk_i2c *i2c = i2c_get_adapdata(adap);
 
 	ret = clk_bulk_enable(I2C_MT65XX_CLK_MAX, i2c->clocks);
@@ -1256,6 +1257,7 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
 		if (!(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
 		    msgs[0].addr == msgs[1].addr) {
 			i2c->auto_restart = 0;
+			write_then_read_en = true;
 		}
 	}
 
@@ -1280,12 +1282,10 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
 		else
 			i2c->op = I2C_MASTER_WR;
 
-		if (!i2c->auto_restart) {
-			if (num > 1) {
-				/* combined two messages into one transaction */
-				i2c->op = I2C_MASTER_WRRD;
-				left_num--;
-			}
+		if (write_then_read_en) {
+			/* combined two messages into one transaction */
+			i2c->op = I2C_MASTER_WRRD;
+			left_num--;
 		}
 
 		/* always use DMA mode. */
@@ -1293,7 +1293,10 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
 		if (ret < 0)
 			goto err_exit;
 
-		msgs++;
+		if (i2c->op == I2C_MASTER_WRRD)
+			msgs += 2;
+		else
+			msgs++;
 	}
 	/* the return value is number of executed messages */
 	ret = num;
-- 
2.46.0

Re: [PATCH v2] i2c: mediatek: fix potential incorrect use of I2C_MASTER_WRRD

[Andi Shyti]

Hi Leilk,

> diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
> index ab456c3717db..dee40704825c 100644
> --- a/drivers/i2c/busses/i2c-mt65xx.c
> +++ b/drivers/i2c/busses/i2c-mt65xx.c
> @@ -1243,6 +1243,7 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
>  {
>  	int ret;
>  	int left_num = num;
> +	bool write_then_read_en = false;
>  	struct mtk_i2c *i2c = i2c_get_adapdata(adap);
>  
>  	ret = clk_bulk_enable(I2C_MT65XX_CLK_MAX, i2c->clocks);
> @@ -1256,6 +1257,7 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
>  		if (!(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
>  		    msgs[0].addr == msgs[1].addr) {
>  			i2c->auto_restart = 0;
> +			write_then_read_en = true;
>  		}
>  	}
>  
> @@ -1280,12 +1282,10 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
>  		else
>  			i2c->op = I2C_MASTER_WR;
>  
> -		if (!i2c->auto_restart) {
> -			if (num > 1) {
> -				/* combined two messages into one transaction */
> -				i2c->op = I2C_MASTER_WRRD;
> -				left_num--;
> -			}
> +		if (write_then_read_en) {
> +			/* combined two messages into one transaction */
> +			i2c->op = I2C_MASTER_WRRD;

i2c doesn't change for the whole loop so that it can be set only
once outside the loop instead of setting it everytime.

Something like this:

	if (i2c->op == I2C_MASTER_WRRD)
		left_num--;
	else if (msgs->flags & I2C_M_RD)
		...
	else

looks cleaner to me and we save the extra flag. Am I missing
anything?

Andi

> +			left_num--;
>  		}
>  
>  		/* always use DMA mode. */
> @@ -1293,7 +1293,10 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
>  		if (ret < 0)
>  			goto err_exit;
>  
> -		msgs++;
> +		if (i2c->op == I2C_MASTER_WRRD)
> +			msgs += 2;
> +		else
> +			msgs++;
>  	}
>  	/* the return value is number of executed messages */
>  	ret = num;
> -- 
> 2.46.0
> 

Re: [PATCH v2] i2c: mediatek: fix potential incorrect use of I2C_MASTER_WRRD

[Chen-Yu Tsai]

On Tue, Sep 9, 2025 at 6:17 AM Andi Shyti <andi.shyti@kernel.org> wrote:
>
> Hi Leilk,
>
> > diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
> > index ab456c3717db..dee40704825c 100644
> > --- a/drivers/i2c/busses/i2c-mt65xx.c
> > +++ b/drivers/i2c/busses/i2c-mt65xx.c
> > @@ -1243,6 +1243,7 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
> >  {
> >       int ret;
> >       int left_num = num;
> > +     bool write_then_read_en = false;
> >       struct mtk_i2c *i2c = i2c_get_adapdata(adap);
> >
> >       ret = clk_bulk_enable(I2C_MT65XX_CLK_MAX, i2c->clocks);
> > @@ -1256,6 +1257,7 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
> >               if (!(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
> >                   msgs[0].addr == msgs[1].addr) {
> >                       i2c->auto_restart = 0;
> > +                     write_then_read_en = true;
> >               }
> >       }
> >
> > @@ -1280,12 +1282,10 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
> >               else
> >                       i2c->op = I2C_MASTER_WR;
> >
> > -             if (!i2c->auto_restart) {
> > -                     if (num > 1) {
> > -                             /* combined two messages into one transaction */
> > -                             i2c->op = I2C_MASTER_WRRD;
> > -                             left_num--;
> > -                     }
> > +             if (write_then_read_en) {
> > +                     /* combined two messages into one transaction */
> > +                     i2c->op = I2C_MASTER_WRRD;
>
> i2c doesn't change for the whole loop so that it can be set only
> once outside the loop instead of setting it everytime.
>
> Something like this:
>
>         if (i2c->op == I2C_MASTER_WRRD)
>                 left_num--;
>         else if (msgs->flags & I2C_M_RD)
>                 ...
>         else
>
> looks cleaner to me and we save the extra flag. Am I missing
> anything?

It looks correct to me, though I think it requires a comment explaining
that "in the WRRD case there are only two messages that get processed
together, and the while loop doesn't actually iterate", and reference
the block where the WRRD op is set.

Otherwise someone is going to look at this snippet and think there's
some corner case where all messages (# of messages > 2) get handled
using the WRRD op.

So maybe it looks cleaner, but it requires more context to understand.
Whereas in the original patch, the extra variable sort of gives that
context. In this case I prefer the context being more visible, since
the original corner case this issue fixes is also from missing context
and assumptions.


ChenYu

> Andi
>
> > +                     left_num--;
> >               }
> >
> >               /* always use DMA mode. */
> > @@ -1293,7 +1293,10 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
> >               if (ret < 0)
> >                       goto err_exit;
> >
> > -             msgs++;
> > +             if (i2c->op == I2C_MASTER_WRRD)
> > +                     msgs += 2;
> > +             else
> > +                     msgs++;
> >       }
> >       /* the return value is number of executed messages */
> >       ret = num;
> > --
> > 2.46.0
> >

Re: [PATCH v2] i2c: mediatek: fix potential incorrect use of I2C_MASTER_WRRD

[Andi Shyti]

On Tue, Sep 09, 2025 at 11:50:15AM +0800, Chen-Yu Tsai wrote:
> On Tue, Sep 9, 2025 at 6:17 AM Andi Shyti <andi.shyti@kernel.org> wrote:
> > > diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
> > > index ab456c3717db..dee40704825c 100644
> > > --- a/drivers/i2c/busses/i2c-mt65xx.c
> > > +++ b/drivers/i2c/busses/i2c-mt65xx.c
> > > @@ -1243,6 +1243,7 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
> > >  {
> > >       int ret;
> > >       int left_num = num;
> > > +     bool write_then_read_en = false;
> > >       struct mtk_i2c *i2c = i2c_get_adapdata(adap);
> > >
> > >       ret = clk_bulk_enable(I2C_MT65XX_CLK_MAX, i2c->clocks);
> > > @@ -1256,6 +1257,7 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
> > >               if (!(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
> > >                   msgs[0].addr == msgs[1].addr) {
> > >                       i2c->auto_restart = 0;
> > > +                     write_then_read_en = true;
> > >               }
> > >       }
> > >
> > > @@ -1280,12 +1282,10 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
> > >               else
> > >                       i2c->op = I2C_MASTER_WR;
> > >
> > > -             if (!i2c->auto_restart) {
> > > -                     if (num > 1) {
> > > -                             /* combined two messages into one transaction */
> > > -                             i2c->op = I2C_MASTER_WRRD;
> > > -                             left_num--;
> > > -                     }
> > > +             if (write_then_read_en) {
> > > +                     /* combined two messages into one transaction */
> > > +                     i2c->op = I2C_MASTER_WRRD;
> >
> > i2c doesn't change for the whole loop so that it can be set only
> > once outside the loop instead of setting it everytime.
> >
> > Something like this:
> >
> >         if (i2c->op == I2C_MASTER_WRRD)
> >                 left_num--;
> >         else if (msgs->flags & I2C_M_RD)
> >                 ...
> >         else
> >
> > looks cleaner to me and we save the extra flag. Am I missing
> > anything?
> 
> It looks correct to me, though I think it requires a comment explaining
> that "in the WRRD case there are only two messages that get processed
> together, and the while loop doesn't actually iterate", and reference
> the block where the WRRD op is set.
> 
> Otherwise someone is going to look at this snippet and think there's
> some corner case where all messages (# of messages > 2) get handled
> using the WRRD op.

Agree, indeed I wanted to write it somewhere that a comment would
have been nice.

I'd appreciate a comment even with the boolean flag. I think
boolean flags are often a forced solution and we always need to
describe their need.

> So maybe it looks cleaner, but it requires more context to understand.
> Whereas in the original patch, the extra variable sort of gives that
> context. In this case I prefer the context being more visible, since
> the original corner case this issue fixes is also from missing context
> and assumptions.

I think both solutions are clear in a different way. Anyway, I'm
not very strong on this comment, I just see that from a code
perspective looks nicer. If you guys insist, then I will let this
go as it is.

Andi

Re: [PATCH v2] i2c: mediatek: fix potential incorrect use of I2C_MASTER_WRRD

[Wolfram Sang]

On Sat, Sep 06, 2025 at 04:24:06PM +0800, Leilk Liu wrote:
> From: "Leilk.Liu" <leilk.liu@mediatek.com>
> 
> The old IC does not support the I2C_MASTER_WRRD (write-then-read)
> function, but the current code’s handling of i2c->auto_restart may
> potentially lead to entering the I2C_MASTER_WRRD software flow,
> resulting in unexpected bugs.
> 
> Instead of repurposing the auto_restart flag, add a separate flag
> to signal I2C_MASTER_WRRD operations.
> 
> Also fix handling of msgs. If the operation (i2c->op) is
> I2C_MASTER_WRRD, then the msgs pointer is incremented by 2.
> For all other operations, msgs is simply incremented by 1.
> 
> Fixes: b2ed11e224a2 ("I2C: mediatek: Add driver for MediaTek MT8173 I2C controller")
> Signed-off-by: Leilk.Liu <leilk.liu@mediatek.com>
> Suggested-by: Chen-Yu Tsai <wenst@chromium.org>
> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>

Applied to for-next, thanks!