| 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
| ||
|
Date: Wed, 3 Aug 2022 19:26:55 +0000
From: Dmitry Rokosov <DDRokosov@...rdevices.ru>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>
CC: "andy.shevchenko@...il.com" <andy.shevchenko@...il.com>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"jic23@...nel.org" <jic23@...nel.org>,
kernel <kernel@...rdevices.ru>,
"lars@...afoo.de" <lars@...afoo.de>,
"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"shawnguo@...nel.org" <shawnguo@...nel.org>,
"stano.jakubek@...il.com" <stano.jakubek@...il.com>,
"stephan@...hold.net" <stephan@...hold.net>
Subject: Re: [PATCH v4 2/3] iio: add MEMSensing MSA311 3-axis accelerometer
driver
On Wed, Aug 03, 2022 at 09:18:26PM +0200, Christophe JAILLET wrote:
> Le 03/08/2022 à 20:39, Dmitry Rokosov a écrit :
> > Hello Christophe,
> >
> > Thank you for quick review
> >
> > On Wed, Aug 03, 2022 at 08:11:05PM +0200, Christophe JAILLET wrote:
> > > Le 03/08/2022 à 15:11, Dmitry Rokosov a écrit :
> > > > MSA311 is a tri-axial, low-g accelerometer with I2C digital output for
> > > > sensitivity consumer applications. It has dynamic user-selectable full
> > > > scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements
> > > > with output data rates from 1Hz to 1000Hz.
> > > >
> > > > Spec: https://cdn-shop.adafruit.com/product-files/5309/MSA311-V1.1-ENG.pdf
> > > >
> > > > This driver supports following MSA311 features:
> > > > - IIO interface
> > > > - Different power modes: NORMAL and SUSPEND (using pm_runtime)
> > > > - ODR (Output Data Rate) selection
> > > > - Scale and samp_freq selection
> > > > - IIO triggered buffer, IIO reg access
> > > > - NEW_DATA interrupt + trigger
> > > >
> > > > Below features to be done:
> > > > - Motion Events: ACTIVE, TAP, ORIENT, FREEFALL
> > > > - Low Power mode
> > > >
> > > > Signed-off-by: Dmitry Rokosov <ddrokosov-i4r8oA+eLlH99rHkP+FxIw@...lic.gmane.org>
> > > > ---
> > > > MAINTAINERS | 6 +
> > > > drivers/iio/accel/Kconfig | 13 +
> > > > drivers/iio/accel/Makefile | 2 +
> > > > drivers/iio/accel/msa311.c | 1323 ++++++++++++++++++++++++++++++++++++
> > > > 4 files changed, 1344 insertions(+)
> > > > create mode 100644 drivers/iio/accel/msa311.c
> > > >
> > >
> > > Hi,
> > > a few nits below.
> > >
> > > [...]
> > >
> > > > +static int msa311_check_partid(struct msa311_priv *msa311)
> > > > +{
> > > > + struct device *dev = msa311->dev;
> > > > + unsigned int partid;
> > > > + int err;
> > > > +
> > > > + err = regmap_read(msa311->regs, MSA311_PARTID_REG, &partid);
> > > > + if (err)
> > > > + return dev_err_probe(dev, err,
> > > > + "failed to read partid (%d)\n", err);
> > >
> > > No need for "(%d)" and err.
> > >
> >
> > Do you mean for all dev_err() calls? I think sometimes it's helpful to
> > know the actual error value got from external API, isn't? Could you please
> > explain your point if possible?
> >
>
> No, my comment is only for dev_err_probe() function.
> Having ret for dev_err() calls is fine.
>
> See: https://elixir.bootlin.com/linux/v5.19/source/drivers/base/core.c#L4732
>
> dev_err_probe() already has a "error %pe:..., ERR_PTR(err)"
> This means that if ret = -ENOMEM:
> "(%d)" --> "(-12)"
> "error %pe:" --> "error -ENOMEM:"
>
> So there is no real need to duplicate the error code in the message itself,
> it is already displayed in a human readable manner.
>
> What your code does would result in:
> "error -ENOMEM: failed to read partid (-12)\n"
https://elixir.bootlin.com/linux/v5.19/source/drivers/base/core.c#L4732>
Hmm, very interesting. Thank you for good point. I will fix it in the
v5, and will use %pe for all dev_err() calls, I suppose it's helpful.
[...]
> > > > + if (msa311->vdd) {
> > > > + err = regulator_enable(msa311->vdd);
> > > > + if (err)
> > > > + return dev_err_probe(dev, err,
> > > > + "cannot enable vdd supply\n");
> > > > +
> > > > + err = devm_add_action_or_reset(dev, msa311_vdd_disable,
> > > > + msa311->vdd);
> > > > + if (err) {
> > > > + regulator_disable(msa311->vdd);
> > >
> > > Double regulator_disable(), because of the _or_reset()?
> > >
> >
> > Yep. If devm_add_action_or_reset() returns an error, we will not
> > call regulator_disable() by devm subsystem. It means, we have to
> > call it directly.
>
> No.
>
> See
https://elixir.bootlin.com/linux/v5.19/source/include/linux/device.h#L249
>
> If devm_add_action_or_reset() fails, "action" is called. This is the meaning
> of the _or_reset suffix.
>
> So here, msa311_vdd_disable() would be called and this function is:
>
> +static void msa311_vdd_disable(void *vdd)
> +{
> + regulator_disable(vdd);
> +}
>
> and "vdd" will be the value of "msa311->vdd"
>
> So, unless I missed something obvious, your code will call twice
> regulator_disable(msa311->vdd).
>
> One in devm_add_action_or_reset() and one explicitly after the "if (err)"
>
You are totally right, thanks a lot for the explanation. I will fix that
in the v5.
>
> Hoping I'm clear and that I didn't miss something obvious.
>
> CJ
>
> >
> > > > + return dev_err_probe(dev, err,
> > > > + "cannot add vdd disable action\n");
> > > > + }
> > > > + }
> > > > +
> > > > + err = msa311_check_partid(msa311);
> > > > + if (err)
> > > > + return err;
> > > > +
> > > > + err = msa311_soft_reset(msa311);
> > > > + if (err)
> > > > + return err;
> > > > +
> > > > + err = msa311_set_pwr_mode(msa311, MSA311_PWR_MODE_NORMAL);
> > > > + if (err)
> > > > + return dev_err_probe(dev, err,
> > > > + "failed to power on device (%d)\n", err);
> > >
> > > No need for "(%d)" and err
> >
> > Asked for the clarification above.
> >
> > >
> > > CJ
> >
>
--
Thank you,
Dmitry
Powered by blists - more mailing lists