| 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
| ||
|
Message-ID: <20240903063329.GA222264@debian>
Date: Tue, 3 Sep 2024 08:33:29 +0200
From: Dimitri Fedrau <dima.fedrau@...il.com>
To: Sebastian Reichel <sebastian.reichel@...labora.com>
Cc: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] power: supply: max1720x: add read support for nvmem
Am Mon, Sep 02, 2024 at 09:55:42PM +0000 schrieb Sebastian Reichel:
> Hi,
>
> On Sat, Aug 31, 2024 at 08:21:45PM GMT, Dimitri Fedrau wrote:
> > ModelGauge m5 and device configuration values are stored in nonvolatile
> > memory to prevent data loss if the IC loses power. Add read support for
> > the nonvolatile memory on MAX1720X devices.
> >
> > Signed-off-by: Dimitri Fedrau <dima.fedrau@...il.com>
> > ---
> >
> > Based on:
> > 479b6d04964b "power: supply: add support for MAX1720x standalone fuel gauge"
> > in branch for-next
> >
> > Changes in V2:
> > - remove function max1720x_remove and use devm_add_action_or_reset() to
> > unregister info->ancillary to avoid race condition during module remove
>
> Thanks, but the transformation is quite incomplete. You probably
> should have a look what device managed resource actually means :)
>
Yes, I noticed shortly after sending V2, that I missed to remove all
i2c_unregister_device calls in the error path. Already prepared V3
handling this and also the missing header "linux/nvmem-provider.h".
Thanks for reviewing so quickly, I just hoped to get away with this by
sending V3 before you notice. :)
> > ---
> > drivers/power/supply/max1720x_battery.c | 220 ++++++++++++++++++++++--
> > 1 file changed, 205 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/power/supply/max1720x_battery.c b/drivers/power/supply/max1720x_battery.c
> > index edc262f0a62f..d27c94bdb835 100644
> > --- a/drivers/power/supply/max1720x_battery.c
> > +++ b/drivers/power/supply/max1720x_battery.c
>
> [...]
>
> > +static int max1720x_probe_nvmem(struct i2c_client *client,
> > + struct max1720x_device_info *info)
> > {
> > struct device *dev = &client->dev;
> > - struct i2c_client *ancillary;
> > + struct nvmem_config nvmem_config = {
>
> As noticed by the build bot: you need to add this include for the
> struct:
>
> #include <linux/nvmem-provider.h>
>
Will fix it.
> > + .dev = dev,
> > + .name = "max1720x_nvmem",
> > + .cells = max1720x_nvmem_cells,
> > + .ncells = ARRAY_SIZE(max1720x_nvmem_cells),
> > + .read_only = true,
> > + .root_only = true,
> > + .reg_read = max1720x_nvmem_reg_read,
> > + .size = ARRAY_SIZE(max1720x_nvmem_cells) * 2,
> > + .word_size = 2,
> > + .stride = 2,
> > + .priv = info,
> > + };
> > + struct nvmem_device *nvmem;
> > + unsigned int val;
> > int ret;
> >
> > - ancillary = i2c_new_ancillary_device(client, "nvmem", 0xb);
> > - if (IS_ERR(ancillary)) {
> > + info->ancillary = i2c_new_ancillary_device(client, "nvmem", 0xb);
> > + if (IS_ERR(info->ancillary)) {
> > dev_err(dev, "Failed to initialize ancillary i2c device\n");
> > - return PTR_ERR(ancillary);
> > + return PTR_ERR(info->ancillary);
> > }
> >
> > - ret = i2c_smbus_read_word_data(ancillary, MAX1720X_NRSENSE);
> > - i2c_unregister_device(ancillary);
> > - if (ret < 0)
> > - return ret;
> > + ret = devm_add_action_or_reset(dev, max1720x_unregister_ancillary, info);
> > + if (ret) {
> > + dev_err(dev, "Failed to add unregister callback\n");
> > + goto err;
> > + }
> >
> > - info->rsense = ret;
> > + info->regmap_nv = devm_regmap_init_i2c(info->ancillary,
> > + &max1720x_nvmem_regmap_cfg);
> > + if (IS_ERR(info->regmap_nv)) {
> > + dev_err(dev, "regmap initialization of nvmem failed\n");
> > + ret = PTR_ERR(info->regmap_nv);
> > + goto err;
> > + }
> > +
> > + ret = regmap_read(info->regmap_nv, MAX1720X_NRSENSE, &val);
> > + if (ret < 0) {
> > + dev_err(dev, "Failed to read sense resistor value\n");
> > + goto err;
> > + }
> > +
> > + info->rsense = val;
> > if (!info->rsense) {
> > dev_warn(dev, "RSense not calibrated, set 10 mOhms!\n");
> > info->rsense = 1000; /* in regs in 10^-5 */
> > }
> >
> > + nvmem = devm_nvmem_register(dev, &nvmem_config);
> > + if (IS_ERR(nvmem)) {
> > + dev_err(dev, "Could not register nvmem!");
> > + ret = PTR_ERR(nvmem);
> > + goto err;
> > + }
> > +
> > return 0;
> > +err:
> > + i2c_unregister_device(info->ancillary);
>
> devm_add_action_or_reset() already unregisters on failure, so this
> results in a double unregister. Please also simplify 'goto err'
> with 'return ret;'.
>
Will fix it.
> > +
> > + return ret;
> > }
> >
> > static const struct power_supply_desc max1720x_bat_desc = {
> > @@ -299,20 +487,22 @@ static int max1720x_probe(struct i2c_client *client)
> >
> > psy_cfg.drv_data = info;
> > psy_cfg.fwnode = dev_fwnode(dev);
> > + i2c_set_clientdata(client, info);
> > info->regmap = devm_regmap_init_i2c(client, &max1720x_regmap_cfg);
> > if (IS_ERR(info->regmap))
> > return dev_err_probe(dev, PTR_ERR(info->regmap),
> > "regmap initialization failed\n");
> >
> > - ret = max1720x_probe_sense_resistor(client, info);
> > + ret = max1720x_probe_nvmem(client, info);
> > if (ret)
> > - return dev_err_probe(dev, ret,
> > - "Failed to read sense resistor value\n");
> > + return dev_err_probe(dev, ret, "Failed to probe nvmem\n");
> >
> > bat = devm_power_supply_register(dev, &max1720x_bat_desc, &psy_cfg);
> > - if (IS_ERR(bat))
> > + if (IS_ERR(bat)) {
> > + i2c_unregister_device(info->ancillary);
>
> This is also already handled by devm and must be removed.
>
Will fix it.
> > return dev_err_probe(dev, PTR_ERR(bat),
> > "Failed to register power supply\n");
> > + }
> >
> > return 0;
> > }
>
> -- Sebastian
Best regards
Dimitri Fedrau
Powered by blists - more mailing lists