linux-kernel - Re: [PATCH v4 55/58] mtd: nand: add helpers to access ->priv

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: <20151219040124.7ffe4274@bbrezillon>
Date:	Sat, 19 Dec 2015 04:01:24 +0100
From:	Boris Brezillon <boris.brezillon@...e-electrons.com>
To:	Brian Norris <computersforpeace@...il.com>
Cc:	David Woodhouse <dwmw2@...radead.org>,
	linux-mtd@...ts.infradead.org,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 55/58] mtd: nand: add helpers to access ->priv

Hi Brian,

On Fri, 18 Dec 2015 14:17:58 -0800
Brian Norris <computersforpeace@...il.com> wrote:

> Hi Boris,
> 
> On Thu, Dec 10, 2015 at 09:00:39AM +0100, Boris Brezillon wrote:
> > Add two helpers to access the field reserved for private controller data.
> > This makes it clearer what this field is reserved for and ease future
> > refactoring.
> 
> I agree with the refactoring part, but I'm not sure about the name. Is
> it really "controller" data? That sounds like something that has 1
> instance per controller. But the way this is sometimes used, we get 1
> instance per NAND chip, and there may be more than one NAND chip per
> controller.
> 
> So at the moment, this is more like opaque "driver data", like
> dev_{get,set}_drvdata(), which doesn't really have a prescribed use --
> it's up to the driver.
> 
> Notably, we already have a (sort of) 1-per-controler-instance field:
> struct nand_hw_control (I notice we have both the 'controller' and
> 'hwcontrol' fields in nand_chip; that's pretty ugly too...).

Will be addressed soon ;-).

> Those don't
> have private data fields, but we could of course extend that if we
> really want "controller" data.

Actually the nand_{get,set}_controller_data() helpers are not about
assigning NAND controller private data (as you pointed those can
already be retrieved thanks to the ->controller field using the
container_of() trick), but per-chip private data instantiated by the
NAND controller and attached to a specific chip. For example, some
controllers pre-compute some register values or a clk rate to set when
a specific chip is selected. This is what per-chip controller data is
meant for.

Now, the reason I explicitly specified the data usage instead of using
a generic name like nand_{get,set}_data() is because I plan to define
other helpers to allow NAND manufacturer code to manipulate its own
private data. This is required if we want to support read-retry on some
chips who are requiring a read OTP area step to retrieve some register
values which will later be used to change from one read-retry mode to
another.
The plan was to define the nand_{set,get}_manufacturer_data() helpers,
and create or reuse an existing priv field (mtd->priv?) to store this
private data.

Also note that the spi framework provides the same kind of helpers [1].

This being said, I'm perfectly fine changing the function names, but
I'd like to replace it by something explicitly telling the user that
this field should only be set by NAND controller drivers. 

[1]http://lxr.free-electrons.com/source/include/linux/spi/spi.h#L189

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
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/