linux-kernel - RE: [PATCH v1 1/3] mfd: Add new mfd device TPS68470

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]
Date:   Fri, 9 Jun 2017 22:09:27 +0000
From:   "Mani, Rajmohan" <rajmohan.mani@...el.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
        Lee Jones <lee.jones@...aro.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        "Alexandre Courbot" <gnurou@...il.com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        "Len Brown" <lenb@...nel.org>
Subject: RE: [PATCH v1 1/3] mfd: Add new mfd device TPS68470

Hi Andy,

Thanks for the reviews and patience.

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@...il.com]
> Sent: Tuesday, June 06, 2017 6:00 AM
> To: Mani, Rajmohan <rajmohan.mani@...el.com>
> Cc: linux-kernel@...r.kernel.org; linux-gpio@...r.kernel.org; linux-
> acpi@...r.kernel.org; Lee Jones <lee.jones@...aro.org>; Linus Walleij
> <linus.walleij@...aro.org>; Alexandre Courbot <gnurou@...il.com>; Rafael J.
> Wysocki <rjw@...ysocki.net>; Len Brown <lenb@...nel.org>
> Subject: Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470
> 
> On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani <rajmohan.mani@...el.com>
> wrote:
> > The TPS68470 device is an advanced power management unit that powers a
> > Compact Camera Module (CCM), generates clocks for image sensors,
> > drives a dual LED for Flash and incorporates two LED drivers for
> > general purpose indicators.
> >
> > This patch adds support for TPS68470 mfd device.
> 
> I dunno why you decide to send this out now, see my comments below.
> 

We decided to go with the submission of these drivers for upstream review sooner rather than later.

> > +static int tps68470_chip_init(struct tps68470 *tps) {
> > +       unsigned int version;
> > +       int ret;
> 
> > +       /* FIXME: configure these dynamically */
> 
> So, what prevents you to fix this?
> 

I will respond on top of Sakari's response.

> > +       /* Enable Daisy Chain LDO and configure relevant GPIOs as
> > + output */
> 
> > +}
> 
> > +static int tps68470_probe(struct i2c_client *client) {
> > +       struct tps68470 *tps;
> > +       int ret;
> > +
> > +       tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
> > +       if (!tps)
> > +               return -ENOMEM;
> > +
> > +       mutex_init(&tps->lock);
> > +       i2c_set_clientdata(client, tps);
> > +       tps->dev = &client->dev;
> > +
> > +       tps->regmap = devm_regmap_init_i2c(client,
> &tps68470_regmap_config);
> > +       if (IS_ERR(tps->regmap)) {
> > +               dev_err(tps->dev, "devm_regmap_init_i2c Error %d\n", ret);
> > +               return PTR_ERR(tps->regmap);
> > +       }
> > +
> 
> > +       ret = mfd_add_devices(tps->dev, -1, tps68470s,
> > +                             ARRAY_SIZE(tps68470s), NULL, 0, NULL);
> 
> devm_?
> 

Ack

> > +       if (ret < 0) {
> > +               dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       ret = tps68470_chip_init(tps);
> > +       if (ret < 0) {
> > +               dev_err(tps->dev, "TPS68470 Init Error %d\n", ret);
> > +               goto fail;
> > +       }
> > +
> > +       return 0;
> 
> > +fail:
> > +       mutex_lock(&tps->lock);
> 
> I'm not sure you need this mutex to be held here.
> Otherwise your code has a bug with locking.
> 

Repeating the response to Heikki here

I had this following question from Alan Cox on the original code without these wrappers.

"What is the model for insuring that no interrupt or thread of a driver is not in parallel issuing a tps68470_ operation when the device goes away (eg if I down the i2c controller) ?"

To address the above concerns, I got extra cautious and implemented locks around the regmap_* calls.
Now, I have been asked from more than one reviewer about the necessity of the same.
With the use of devm_* calls, tps68470_remove() goes away and leaves the driver just with regmap_* calls.
Unless I hear from Alan or other reviewers otherwise, I will drop these wrappers around regmap_* calls.

> > +       mfd_remove_devices(tps->dev);
> > +       mutex_unlock(&tps->lock);
> > +
> > +       return ret;
> 
> Taking above into consideration I suggest to clarify your locking scheme.
> 

Same as above.

> > +}
> > +
> > +static int tps68470_remove(struct i2c_client *client) {
> > +       struct tps68470 *tps = i2c_get_clientdata(client);
> > +
> 
> > +       mutex_lock(&tps->lock);
> > +       mfd_remove_devices(tps->dev);
> > +       mutex_unlock(&tps->lock);
> 
> Ditto.
> 

Same as above

> > +       return 0;
> > +}
> 
> > +/**
> > + * struct tps68470 - tps68470 sub-driver chip access routines
> > + *
> 
> kbuild bot will be unhappy. You need to file a description per field.
> 

Ack
It looks like this structure will go away, once I implement the feedback from Heikki.

> > + * Device data may be used to access the TPS68470 chip */
> > +
> > +struct tps68470 {
> > +       struct device *dev;
> > +       struct regmap *regmap;
> 
> > +       /*
> > +        * Used to synchronize access to tps68470_ operations
> > +        * and addition and removal of mfd devices
> > +        */
> 
> Move it to kernel-doc above.
> 

Same as above

> > +       struct mutex lock;
> > +};
> 
> --
> With Best Regards,
> Andy Shevchenko