linux-kernel - Re: [PATCH v7 3/7] drivers/i2c: Add port structure to FSI algorithm

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:   Thu, 31 May 2018 08:34:16 +1000
From:   Benjamin Herrenschmidt <benh@...nel.crashing.org>
To:     Andy Shevchenko <andy.shevchenko@...il.com>,
        Eddie James <eajames@...ux.vnet.ibm.com>
Cc:     linux-i2c <linux-i2c@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Wolfram Sang <wsa@...-dreams.de>,
        Rob Herring <robh+dt@...nel.org>,
        Joel Stanley <joel@....id.au>,
        Mark Rutland <mark.rutland@....com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Edward A. James" <eajames@...ibm.com>
Subject: Re: [PATCH v7 3/7] drivers/i2c: Add port structure to FSI algorithm

On Thu, 2018-05-31 at 00:27 +0300, Andy Shevchenko wrote:
> On Wed, May 30, 2018 at 6:47 PM, Eddie James <eajames@...ux.vnet.ibm.com> wrote:
> > On 05/29/2018 06:19 PM, Andy Shevchenko wrote:
> > > On Wed, May 30, 2018 at 1:24 AM, Eddie James <eajames@...ux.vnet.ibm.com>
> > > wrote:
> > > >   static int fsi_i2c_probe(struct device *dev)
> > > >   {
> > > 
> > > Isn't below somehow repeats of_i2c_register_devices() ?
> > > Why not to use it?
> > 
> > 
> > Because I need to assign all these port structure fields. Also looks like
> > of_i2c_register_devices creates new devices; I just want an adapter for each
> > port.
> 
> Hmm... Wolfram, what is your opinion on this design?

Andy, I don't understand your issue.

of_i2c_register_devices() is about discovering the i2c devices below a
given bus. This is not what is happening here.

This is a driver for a master that supports multiple busses, so it the
above loop creates all the busses.

> > > > +                       devm_kfree(dev, port);
> > > 
> > > This hurts my eyes. Why?!
> > What would you suggest instead?
> 
> You even didn't wait for answer, why to ask then?

Please stop being so rude.

> Moreover, you didn't answer to my question. Why are you doing that
> call implicitly?

"implicitly" ? What's implicit here ? This is just pretty standard
cleanup after failure, you are being very cryptic here.

Please state precisely what it is you dislike with that code instead of
expecting us to guess and being nasty about it. Eddie was a genuine
question, he doesn't see what you think is "hurtful to the eyes" in the
code you quoted.

> > > > +       if (!list_empty(&i2c->ports)) {
> > > 
> > > My gosh, this is done already in list_for_each*()
> > No, list_for_each_entry does NOT check if the list is empty or if the first
> > entry is NULL.
> 
> Please, read the macro source code again.

Cheers,
Ben.