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: <20160106231323.GM109450@google.com>
Date:	Wed, 6 Jan 2016 15:13:23 -0800
From:	Brian Norris <computersforpeace@...il.com>
To:	Boris Brezillon <boris.brezillon@...e-electrons.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

On Sat, Dec 19, 2015 at 04:01:24AM +0100, Boris Brezillon wrote:
> 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.

Sure. Really, it's just anything the controller driver needs to store on
a per-chip basis. All I'm suggesting is picking a name that doesn't
imply it's a per-controller instance, when it's actually a
per-flash-chip instance.

> 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

I never suggested just "_data"; I said "_drvdata".

> 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.

That's interesting. Sounds like an OK idea. (Personally, I wouldn't
try to use mtd->priv for this, but otherwise looks OK.)

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

Hmm, OK. FWIW, they have both "driver data" and "controller state". It's
not perfectly clear to me why both exist.

> 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. 

Sure. I though a "driver data"-based name did this. But I'll leave it to
you. I could even be OK with "controller data", if you still think this
fits your overall controller refactoring plan, and communicates its
purpose best.

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

Regards,
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/