linux-kernel - RE: [RESEND PATCH v2 1/2] mfd: da9063: Fix revision handling to correctly select reg tables

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <AM6PR10MB2263F5CE9B3627A256BD695880D90@AM6PR10MB2263.EURPRD10.PROD.OUTLOOK.COM>
Date:   Fri, 17 Apr 2020 09:51:24 +0000
From:   Adam Thomson <Adam.Thomson.Opensource@...semi.com>
To:     Lee Jones <lee.jones@...aro.org>,
        Adam Thomson <Adam.Thomson.Opensource@...semi.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Support Opensource <Support.Opensource@...semi.com>
Subject: RE: [RESEND PATCH v2 1/2] mfd: da9063: Fix revision handling to
 correctly select reg tables

On 17 April 2020 10:24, Lee Jones wrote:

> > > > +		return -EINVAL;
> > >
> > > Do you want to fail silently here?
> >
> > Well an error message is printed in the calling code, so didn't feel like it
> > was necessary to have additional debug here. Felt like bloat.
> 
> As a user, I would prefer a more specific reason.
> 
> Thus, I would provide an error message here and omit the generic one.

I can update although I'll of course then need to do similar messages for the
other error legs of this function. FWIW, as this is only being called once in
the same file this error leg of code currently can never occur.

> > > > +}
> > > > +
> > > > +enum {
> > > > +	DA9063_DEV_ID_REG = 0,
> > > > +	DA9063_VAR_ID_REG,
> > > > +	DA9063_CHIP_ID_REGS,
> > > > +};
> > > > +
> > > > +static int da9063_get_device_type(struct i2c_client *i2c, struct da9063
> > > *da9063)
> > > > +{
> > > > +	int ret;
> > > > +	u8 buf[DA9063_CHIP_ID_REGS];
> > >
> > > Really small nit: Could you reverse these please.
> >
> > Yep, agreed.
> >
> > >
> > > > +	ret = da9063_i2c_blockreg_read(i2c, DA9063_REG_DEVICE_ID, buf,
> > > > +				       DA9063_CHIP_ID_REGS);
> > > > +	if (ret < 0) {
> > >
> > > if (ret)
> > >
> > > Or better yet, as this is a read function, you could just return
> > > i2c_transfer() and do the appropriate error checking here *instead*.
> >
> > I think given that the function handles all of the I2C specific stuff I'd prefer
> > it be kept there. Logically that to me makes more sense. Can change this to
> > 'if (ret)'
> 
> Yes, not that I understand the message length (3) has more do to with
> the I2C interactions (rather than a derisive of 'count'), it makes
> sense to handle that inside the function.
> 
> However, it does seem odd to handle the return value of a *_read()
> function in this way.  They usually return the number of bytes read,
> which in this case would be DA9063_CHIP_ID_REGS (count), right?

Well regmap_bulk_read and regmap_read return 0 for success and negative for
failure so I'd disagree on this part.