[2/2] media: i2c: ov5647: Use bus-locked i2c_transfer()

  The ov5647_read() functions calls i2c_master_send() and
i2c_master_read() in sequence. However this leaves space for other
clients to contend the bus and insert a unrelated transaction in between
the two calls.

Replace the two calls with a single i2c_transfer() one, that locks the
bus in between the transactions.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 drivers/media/i2c/ov5647.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

--
2.39.0
  

Hi Jacopo,

On Mon, Feb 20, 2023 at 01:41:01PM +0100, Jacopo Mondi wrote:
> The ov5647_read() functions calls i2c_master_send() and
> i2c_master_read() in sequence. However this leaves space for other
> clients to contend the bus and insert a unrelated transaction in between
> the two calls.
> 
> Replace the two calls with a single i2c_transfer() one, that locks the
> bus in between the transactions.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  drivers/media/i2c/ov5647.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index 0b88ac6dee41..a423ee8fe20c 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -631,23 +631,29 @@ static int ov5647_write(struct v4l2_subdev *sd, u16 reg, u8 val)
> 
>  static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val)
>  {
> -	unsigned char data_w[2] = { reg >> 8, reg & 0xff };
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	u8 buf[2] = { reg >> 8, reg & 0xff };
> +	struct i2c_msg msg[2];
>  	int ret;
> 
> -	ret = i2c_master_send(client, data_w, 2);
> +	msg[0].addr = client->addr;
> +	msg[0].flags = client->flags;
> +	msg[0].buf = buf;
> +	msg[0].len = sizeof(buf);
> +
> +	msg[1].addr = client->addr;
> +	msg[1].flags = client->flags | I2C_M_RD;
> +	msg[1].buf = buf;
> +	msg[1].len = 1;
> +
> +	ret = i2c_transfer(client->adapter, msg, 2);
>  	if (ret < 0) {
> -		dev_dbg(&client->dev, "%s: i2c write error, reg: %x\n",
> +		dev_err(&client->dev, "%s: i2c read error, reg: %x\n",
>  			__func__, reg);
>  		return ret;
>  	}
> 
> -	ret = i2c_master_recv(client, val, 1);
> -	if (ret < 0) {
> -		dev_dbg(&client->dev, "%s: i2c read error, reg: %x\n",
> -				__func__, reg);
> -		return ret;
> -	}
> +	*val = buf[0];
> 
>  	return 0;
>  }
> --
> 2.39.0
> 

Fully agree.
Reviewed-by: Tommaso Merciai <tomm.merciai@gmail.com>

Thanks,
Tommaso
  

Hi Jacopo

On Mon, 20 Feb 2023 at 12:41, Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> The ov5647_read() functions calls i2c_master_send() and
> i2c_master_read() in sequence. However this leaves space for other
> clients to contend the bus and insert a unrelated transaction in between
> the two calls.
>
> Replace the two calls with a single i2c_transfer() one, that locks the
> bus in between the transactions.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  drivers/media/i2c/ov5647.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index 0b88ac6dee41..a423ee8fe20c 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -631,23 +631,29 @@ static int ov5647_write(struct v4l2_subdev *sd, u16 reg, u8 val)
>
>  static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val)
>  {
> -       unsigned char data_w[2] = { reg >> 8, reg & 0xff };
>         struct i2c_client *client = v4l2_get_subdevdata(sd);
> +       u8 buf[2] = { reg >> 8, reg & 0xff };
> +       struct i2c_msg msg[2];
>         int ret;
>
> -       ret = i2c_master_send(client, data_w, 2);
> +       msg[0].addr = client->addr;
> +       msg[0].flags = client->flags;
> +       msg[0].buf = buf;
> +       msg[0].len = sizeof(buf);
> +
> +       msg[1].addr = client->addr;
> +       msg[1].flags = client->flags | I2C_M_RD;
> +       msg[1].buf = buf;
> +       msg[1].len = 1;
> +
> +       ret = i2c_transfer(client->adapter, msg, 2);
>         if (ret < 0) {

i2c_transfer
* Returns negative errno, else the number of messages executed.

Is there a valid failure case where it returns 1 having done the write
but failed the read? It's deferred to the individual I2C driver, so
could quite easily be iffy.
Personally I'd be tempted to check if (ret != 2), and remap any other
positive value to -EINVAL.

Otherwise:
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> -               dev_dbg(&client->dev, "%s: i2c write error, reg: %x\n",
> +               dev_err(&client->dev, "%s: i2c read error, reg: %x\n",
>                         __func__, reg);
>                 return ret;
>         }
>
> -       ret = i2c_master_recv(client, val, 1);
> -       if (ret < 0) {
> -               dev_dbg(&client->dev, "%s: i2c read error, reg: %x\n",
> -                               __func__, reg);
> -               return ret;
> -       }
> +       *val = buf[0];
>
>         return 0;
>  }
> --
> 2.39.0
>
  

Hi Dave

On Mon, Feb 20, 2023 at 05:46:02PM +0000, Dave Stevenson wrote:
> Hi Jacopo
>
> On Mon, 20 Feb 2023 at 12:41, Jacopo Mondi
> <jacopo.mondi@ideasonboard.com> wrote:
> >
> > The ov5647_read() functions calls i2c_master_send() and
> > i2c_master_read() in sequence. However this leaves space for other
> > clients to contend the bus and insert a unrelated transaction in between
> > the two calls.
> >
> > Replace the two calls with a single i2c_transfer() one, that locks the
> > bus in between the transactions.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  drivers/media/i2c/ov5647.c | 24 +++++++++++++++---------
> >  1 file changed, 15 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > index 0b88ac6dee41..a423ee8fe20c 100644
> > --- a/drivers/media/i2c/ov5647.c
> > +++ b/drivers/media/i2c/ov5647.c
> > @@ -631,23 +631,29 @@ static int ov5647_write(struct v4l2_subdev *sd, u16 reg, u8 val)
> >
> >  static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val)
> >  {
> > -       unsigned char data_w[2] = { reg >> 8, reg & 0xff };
> >         struct i2c_client *client = v4l2_get_subdevdata(sd);
> > +       u8 buf[2] = { reg >> 8, reg & 0xff };
> > +       struct i2c_msg msg[2];
> >         int ret;
> >
> > -       ret = i2c_master_send(client, data_w, 2);
> > +       msg[0].addr = client->addr;
> > +       msg[0].flags = client->flags;
> > +       msg[0].buf = buf;
> > +       msg[0].len = sizeof(buf);
> > +
> > +       msg[1].addr = client->addr;
> > +       msg[1].flags = client->flags | I2C_M_RD;
> > +       msg[1].buf = buf;
> > +       msg[1].len = 1;
> > +
> > +       ret = i2c_transfer(client->adapter, msg, 2);
> >         if (ret < 0) {
>
> i2c_transfer
> * Returns negative errno, else the number of messages executed.
>
> Is there a valid failure case where it returns 1 having done the write
> but failed the read? It's deferred to the individual I2C driver, so
> could quite easily be iffy.
> Personally I'd be tempted to check if (ret != 2), and remap any other
> positive value to -EINVAL.

Seems indeed up to the individual drivers implementation of
master_xfer, whose return value is documented as:

include/linux/i2c.h:     * master_xfer should return the number of messages successfully
include/linux/i2c.h-     * processed, or a negative value on error

I can indeed:

         if (ret != 2) {
                dev_err(.., "i2c write error, reg: %x\n");
                return ret > 0 ? -EINVAL : ret;
         }

         *val = buf[0];

         return 0;

>
> Otherwise:
> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Thanks for checking

>
> > -               dev_dbg(&client->dev, "%s: i2c write error, reg: %x\n",
> > +               dev_err(&client->dev, "%s: i2c read error, reg: %x\n",
> >                         __func__, reg);
> >                 return ret;
> >         }
> >
> > -       ret = i2c_master_recv(client, val, 1);
> > -       if (ret < 0) {
> > -               dev_dbg(&client->dev, "%s: i2c read error, reg: %x\n",
> > -                               __func__, reg);
> > -               return ret;
> > -       }
> > +       *val = buf[0];
> >
> >         return 0;
> >  }
> > --
> > 2.39.0
> >
  

Hi Jacopo

On Tue, Feb 21, 2023 at 09:18:51AM +0100, Jacopo Mondi wrote:
> On Mon, Feb 20, 2023 at 05:46:02PM +0000, Dave Stevenson wrote:
> > On Mon, 20 Feb 2023 at 12:41, Jacopo Mondi wrote:
> > >
> > > The ov5647_read() functions calls i2c_master_send() and
> > > i2c_master_read() in sequence. However this leaves space for other
> > > clients to contend the bus and insert a unrelated transaction in between

s/a unrelated/an unrelated/

> > > the two calls.
> > >
> > > Replace the two calls with a single i2c_transfer() one, that locks the
> > > bus in between the transactions.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > ---
> > >  drivers/media/i2c/ov5647.c | 24 +++++++++++++++---------
> > >  1 file changed, 15 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > > index 0b88ac6dee41..a423ee8fe20c 100644
> > > --- a/drivers/media/i2c/ov5647.c
> > > +++ b/drivers/media/i2c/ov5647.c
> > > @@ -631,23 +631,29 @@ static int ov5647_write(struct v4l2_subdev *sd, u16 reg, u8 val)
> > >
> > >  static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val)
> > >  {
> > > -       unsigned char data_w[2] = { reg >> 8, reg & 0xff };
> > >         struct i2c_client *client = v4l2_get_subdevdata(sd);
> > > +       u8 buf[2] = { reg >> 8, reg & 0xff };
> > > +       struct i2c_msg msg[2];
> > >         int ret;
> > >
> > > -       ret = i2c_master_send(client, data_w, 2);
> > > +       msg[0].addr = client->addr;
> > > +       msg[0].flags = client->flags;
> > > +       msg[0].buf = buf;
> > > +       msg[0].len = sizeof(buf);
> > > +
> > > +       msg[1].addr = client->addr;
> > > +       msg[1].flags = client->flags | I2C_M_RD;
> > > +       msg[1].buf = buf;
> > > +       msg[1].len = 1;
> > > +
> > > +       ret = i2c_transfer(client->adapter, msg, 2);
> > >         if (ret < 0) {
> >
> > i2c_transfer
> > * Returns negative errno, else the number of messages executed.
> >
> > Is there a valid failure case where it returns 1 having done the write
> > but failed the read? It's deferred to the individual I2C driver, so
> > could quite easily be iffy.
> > Personally I'd be tempted to check if (ret != 2), and remap any other
> > positive value to -EINVAL.
> 
> Seems indeed up to the individual drivers implementation of
> master_xfer, whose return value is documented as:
> 
> include/linux/i2c.h:     * master_xfer should return the number of messages successfully
> include/linux/i2c.h-     * processed, or a negative value on error
> 
> I can indeed:
> 
>          if (ret != 2) {
>                 dev_err(.., "i2c write error, reg: %x\n");

I would also print the ret value, that's useful.

>                 return ret > 0 ? -EINVAL : ret;

Make it >= just to be safe.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>          }
> 
>          *val = buf[0];
> 
>          return 0;
> 
> > Otherwise:
> > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> Thanks for checking
> 
> > > -               dev_dbg(&client->dev, "%s: i2c write error, reg: %x\n",
> > > +               dev_err(&client->dev, "%s: i2c read error, reg: %x\n",
> > >                         __func__, reg);
> > >                 return ret;
> > >         }
> > >
> > > -       ret = i2c_master_recv(client, val, 1);
> > > -       if (ret < 0) {
> > > -               dev_dbg(&client->dev, "%s: i2c read error, reg: %x\n",
> > > -                               __func__, reg);
> > > -               return ret;
> > > -       }
> > > +       *val = buf[0];
> > >
> > >         return 0;
> > >  }