linux-kernel - Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

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: <20120903094448.GB11780@pengutronix.de>
Date:	Mon, 3 Sep 2012 11:44:48 +0200
From:	Wolfram Sang <w.sang@...gutronix.de>
To:	Linus Walleij <linus.walleij@...aro.org>
Cc:	Lee Jones <lee.jones@...aro.org>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	STEricsson_nomadik_linux@...t.st.com, linus.walleij@...ricsson.com,
	arnd@...db.de, linux-i2c@...r.kernel.org
Subject: Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik
 I2C driver

On Mon, Sep 03, 2012 at 11:22:28AM +0200, Linus Walleij wrote:
> On Fri, Aug 31, 2012 at 2:23 PM, Lee Jones <lee.jones@...aro.org> wrote:
> 
> (...)
> >  static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
> >  {
> >         int ret = 0;
> >         struct nmk_i2c_controller *pdata = adev->dev.platform_data;
> > +       struct device_node *np = adev->dev.of_node;
> >         struct nmk_i2c_dev      *dev;
> >         struct i2c_adapter *adap;
> >
> > +       if (np) {
> > +               if (!pdata) {
> 
> So, if no pdata is provided, we go on to allocate some ...
> 
> > +                       pdata = devm_kzalloc(&adev->dev, sizeof(*pdata), GFP_KERNEL);
> > +                       if (!pdata) {
> > +                               ret = -ENOMEM;
> > +                               goto err_no_mem;
> > +                       }
> > +               }
> > +               /* Provide the default configuration as a base. */
> > +               pdata = &u8500_i2c;
> 
> Then you just override that pointer with a pointer to the local config.
> 
> > +               nmk_i2c_of_probe(np, pdata);
> > +       }
> > +
> >         if (!pdata)
> >                 /* No i2c configuration found, using the default. */
> >                 pdata = &u8500_i2c;
> 
> This in it's entirety does not look sound. I *think* this is what you
> want to do,
> replace all of the above codde (including the last if (!pdata) clause) with:
> 
> if (!pdata) {
>     /* If no platform data passed in, use the default configuration as
> a base. */
>     pdata = &u8500_i2c;
>     if (np)
>         /* Further, if we have a DT node, override the default with this */
>         nmk_i2c_of_probe(np, pdata);
> }
> 
> This makes any passed pdata take precedence, else default pdata
> complemented with DT info. Which is what we want.

No. of_probe modifies pdata which in this case the default config which
might already be in use. So, you will get problems if you have two
instances with different configuration. So, we need to allocate memory
but copy the content of the default data. The patch above just copies
the pointer which is bogus.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Download attachment "signature.asc" of type "application/pgp-signature" (199 bytes)