Re: [PATCH][next] media: i2c: ds90ub960: Fix missing return check on ub960_rxport_read call

[Sakari Ailus]

Moi,

On Thu, Oct 03, 2024 at 04:33:54PM +0300, Tomi Valkeinen wrote:
> On 03/10/2024 16:19, Sakari Ailus wrote:
> > Moi,
> > 
> > On Thu, Oct 03, 2024 at 03:52:17PM +0300, Tomi Valkeinen wrote:
> > > Hi,
> > > 
> > > On 03/10/2024 09:05, Sakari Ailus wrote:
> > > > Hi Colin,
> > > > 
> > > > On Wed, Oct 02, 2024 at 05:53:29PM +0100, Colin Ian King wrote:
> > > > > The function ub960_rxport_read is being called and afterwards ret is
> > > > > being checked for any failures, however ret is not being assigned to
> > > > > the return of the function call. Fix this by assigning ret to the
> > > > > return of the call which appears to be missing.
> > > > > 
> > > > > Fixes: afe267f2d368 ("media: i2c: add DS90UB960 driver")
> > > > > Signed-off-by: Colin Ian King <colin.i.king@xxxxxxxxx>
> > > > > ---
> > > > >    drivers/media/i2c/ds90ub960.c | 2 +-
> > > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
> > > > > index ffe5f25f8647..58424d8f72af 100644
> > > > > --- a/drivers/media/i2c/ds90ub960.c
> > > > > +++ b/drivers/media/i2c/ds90ub960.c
> > > > > @@ -1286,7 +1286,7 @@ static int ub960_rxport_get_strobe_pos(struct ub960_data *priv,
> > > > >    	clk_delay += v & UB960_IR_RX_ANA_STROBE_SET_CLK_DELAY_MASK;
> > > > > -	ub960_rxport_read(priv, nport, UB960_RR_SFILTER_STS_1, &v);
> > > > > +	ret = ub960_rxport_read(priv, nport, UB960_RR_SFILTER_STS_1, &v);
> > > > >    	if (ret)
> > > > >    		return ret;
> > > > 
> > > > There seems to be a similar issues all around the driver. It'd be good to
> > > > fix them at the same time.
> > > 
> > > With similar issues, do you mean the code not checking the return value at
> > > all for i2c reads and writes?
> > > 
> > > In this particular case the code already checks the return value, but
> > > setting the return value was missing. With a quick browse, I didn't see
> > > other like this.
> > 
> > See e.g. ub960_clear_rx_errors(), ub960_log_status(),
> > ub960_rxport_set_strobe_pos() and ub960_rxport_set_strobe_range.
> 
> Right, those don't check the return value. So they're not the same as the
> one fixed in this patch.
> 
> I'm not arguing against adding error checks, but that's a big work and I
> think this patch is a different kind of fix which should be applied whether
> the additional error checks are added or not.

How much of work that is really? It seems trivial, albeit there are a
number of locations that need to be fixed.

I'm fine with applying this but it'd be nice to add at least a FIXME:
comment to the code while at it.

> 
> Also, while still not arguing against adding the checks, it looks quite
> common to not check the returns values. E.g. it's not just a few errors I
> see if I add __must_check to cci functions.

I haven't seen this being as widespread in pretty much any other (upstream)
driver.

-- 
Terveisin,

Sakari Ailus