linux-kernel - Re: [PATCH 07/12] memory: davinci-aemif: introduce AEMIF 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: <20131127003707.GQ9468@ld-irv-0074.broadcom.com>
Date:	Tue, 26 Nov 2013 16:37:07 -0800
From:	Brian Norris <computersforpeace@...il.com>
To:	Santosh Shilimkar <santosh.shilimkar@...com>
Cc:	Sekhar Nori <nsekhar@...com>, Mark Rutland <mark.rutland@....com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"Strashko, Grygorii" <grygorii.strashko@...com>,
	Russell King <linux@....linux.org.uk>,
	Pawel Moll <pawel.moll@....com>,
	Stephen Warren <swarren@...dotorg.org>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...nel.crashing.org>,
	Rob Herring <rob.herring@...xeda.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
	Rob Landley <rob@...dley.net>,
	"Khoronzhuk, Ivan" <ivan.khoronzhuk@...com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 07/12] memory: davinci-aemif: introduce AEMIF driver

On Tue, Nov 26, 2013 at 01:26:44PM -0500, Santosh Shilimkar wrote:
> On Tuesday 26 November 2013 12:21 PM, Sekhar Nori wrote:
> > On 11/26/2013 8:35 PM, Santosh Shilimkar wrote:
> >> On Tuesday 26 November 2013 02:20 AM, Sekhar Nori wrote:
> >>> On Monday 11 November 2013 10:36 PM, Khoronzhuk, Ivan wrote:
> >>>> +static int davinci_aemif_probe(struct platform_device *pdev)
> >>>> +{
> >>>> +       int ret  = -ENODEV, i;
> >>>> +       struct resource *res;
> >>>> +       struct device *dev = &pdev->dev;
> >>>> +       struct device_node *np = dev->of_node;
> >>>> +
> >>>> +       if (np == NULL)
> >>>> +               return 0;
> >>>> +
> >>>> +       if (aemif) {
> >>>> +               dev_err(dev, "davinci_aemif driver is in use currently\n");
> >>>> +               return -EBUSY;
> >>>> +       }
> >>>
> >>> Why expressly prevent multiple AEMIF devices? Its entirely conceivable
> >>> to have two memories like NAND and NOR flash connect to two different
> >>> AEMIF interfaces.
> >>>
> >> Ivan wanted me to clarify the Keystone hardware which supports
> >> 1 instance of controller with 4 CS. That allows already four
> >> devices to be connected. Currently NAND and NOR are connected on it
> >> and two more slots are free.
> >>
> >> Since driver support what hardware is, lets not build a driver for
> >> hardware which don't exist. And if at all such a support would be
> >> needed in future, we can always add it.
> > 
> > I understand the lack of hardware part, but its common to write the
> > driver such that it can handle multiple instances. Is there any gain on
> > current hardware because of this check? From what I am hearing, the code
> > in question wont be exercised at all. So why go all the way and add it
> > in first place?
> > 
> Fair enough. The check can be dropped.

Hmm, while the sentiment expressed by Sekhar is noble (to avoid
unnecessarily constraining the driver), removing the check is not
enough. You're still using a global static variable 'aemif', and it is
important not to accidentally re-use this struct if a second device ever
becomes available. So for the current implementation, the check is
necessary IMO. If instead, you were to make 'aemif' a per-device struct
(like with platform_set_drvdata()), then you would not have this issue.

Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/