Re: [PATCH v4] media: tuner: check return value of I2C transfers in set_type()

[Hans Verkuil]

On 3/29/26 12:51, Wenyuan Li wrote:
> In set_type(), several I2C transfers are performed to initialize
> specific tuners (e.g. FMD1216ME, FMD1216MEX, TD1316), but the return
> value of i2c_master_send() is not checked.
> 
> If an I2C transfer fails, the initialization sequence may be
> incomplete, potentially leaving the tuner in an inconsistent state
> without any error being reported.
> 
> Check the return value of i2c_master_send() and propagate failures
> to the attach_failed path. A small helper is introduced to reduce
> duplication and provide consistent error reporting.
> 
> This ensures that I2C communication errors during tuner
> initialization are properly detected and handled.
> 
> Fixes: 93df3413f1b4 ("[PATCH] v4l: 655: added support for the philips td1316 tuner")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Wenyuan Li <2063309626@xxxxxx>
> 
> ---
> v5:
> - Reword commit message to better explain rationale
> - Clarify error handling approach
> - No functional changes
> 
> v4:
> - Added Cc: stable@xxxxxxxxxxxxxxx
> - Updated Fixes tag
> ---
>  drivers/media/v4l2-core/tuner-core.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/tuner-core.c b/drivers/media/v4l2-core/tuner-core.c
> index 004ec4d7beea..01f28436a1f8 100644
> --- a/drivers/media/v4l2-core/tuner-core.c
> +++ b/drivers/media/v4l2-core/tuner-core.c
> @@ -280,6 +280,19 @@ static const struct analog_demod_ops tuner_analog_ops = {
>   * Functions to select between radio and TV and tuner probe/remove functions
>   */
>  
> +static int tuner_i2c_send(struct i2c_client *c, u8 *buf, int len)
> +{
> +	int ret = i2c_master_send(c, buf, len);
> +
> +	if (ret != len) {
> +		int err = ret < 0 ? ret : -EIO;
> +
> +		dev_err(&c->dev, "I2C send failed: %pe\n", ERR_PTR(err));
> +		return err;
> +	}
> +	return 0;
> +}
> +

I think this is overkill, and I also like to keep changes to this old code
to a minimum.

>  /**
>   * set_type - Sets the tuner type for a given device
>   *
> @@ -351,11 +364,13 @@ static void set_type(struct i2c_client *c, unsigned int type,
>  		buffer[1] = 0xdc;
>  		buffer[2] = 0x9c;
>  		buffer[3] = 0x60;
> -		i2c_master_send(c, buffer, 4);
> +		if (tuner_i2c_send(c, buffer, 4))
> +			goto attach_failed;

Just keep it simple:

		if (i2c_master_send(c, buffer, 4) != 4)
			goto attach_failed;

Ditto below.

>  		mdelay(1);
>  		buffer[2] = 0x86;
>  		buffer[3] = 0x54;
> -		i2c_master_send(c, buffer, 4);
> +		if (tuner_i2c_send(c, buffer, 4))
> +			goto attach_failed;
>  		if (!dvb_attach(simple_tuner_attach, &t->fe,
>  				t->i2c->adapter, t->i2c->addr, t->type))
>  			goto attach_failed;
> @@ -365,7 +380,8 @@ static void set_type(struct i2c_client *c, unsigned int type,
>  		buffer[1] = 0xdc;
>  		buffer[2] = 0x86;
>  		buffer[3] = 0xa4;
> -		i2c_master_send(c, buffer, 4);
> +		if (tuner_i2c_send(c, buffer, 4))
> +			goto attach_failed;
>  		if (!dvb_attach(simple_tuner_attach, &t->fe,
>  				t->i2c->adapter, t->i2c->addr, t->type))
>  			goto attach_failed;

Regards,

	Hans