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
