linux-kernel - Re: [RESEND PATCH 2/5] mtd: rawnand: add NVIDIA Tegra NAND Flash controller 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]

Date:   Thu, 24 May 2018 14:41:34 +0200
From:   Boris Brezillon <boris.brezillon@...tlin.com>
To:     Stefan Agner <stefan@...er.ch>
Cc:     dwmw2@...radead.org, computersforpeace@...il.com,
        marek.vasut@...il.com, robh+dt@...nel.org, mark.rutland@....com,
        thierry.reding@...il.com, mturquette@...libre.com,
        sboyd@...nel.org, dev@...xeye.de, miquel.raynal@...tlin.com,
        richard@....at, marcel@...wiler.com, krzk@...nel.org,
        digetx@...il.com, benjamin.lindqvist@...ian.se,
        jonathanh@...dia.com, pdeschrijver@...dia.com, pgaikwad@...dia.com,
        mirza.krak@...il.com, linux-mtd@...ts.infradead.org,
        linux-tegra@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org
Subject: Re: [RESEND PATCH 2/5] mtd: rawnand: add NVIDIA Tegra NAND Flash
 controller driver

On Thu, 24 May 2018 14:23:56 +0200
Boris Brezillon <boris.brezillon@...tlin.com> wrote:

> On Thu, 24 May 2018 13:09:53 +0200
> Stefan Agner <stefan@...er.ch> wrote:
> 
> > On 24.05.2018 10:56, Boris Brezillon wrote:  
> > > On Thu, 24 May 2018 10:46:27 +0200
> > > Stefan Agner <stefan@...er.ch> wrote:
> > >     
> > >> Hi Boris,
> > >>
> > >> Thanks for the initial review! One small question below:
> > >>
> > >> On 23.05.2018 16:18, Boris Brezillon wrote:    
> > >> > Hi Stefan,
> > >> >
> > >> > On Tue, 22 May 2018 14:07:06 +0200
> > >> > Stefan Agner <stefan@...er.ch> wrote:    
> > >> >> +
> > >> >> +struct tegra_nand {
> > >> >> +	void __iomem *regs;
> > >> >> +	struct clk *clk;
> > >> >> +	struct gpio_desc *wp_gpio;
> > >> >> +
> > >> >> +	struct nand_chip chip;
> > >> >> +	struct device *dev;
> > >> >> +
> > >> >> +	struct completion command_complete;
> > >> >> +	struct completion dma_complete;
> > >> >> +	bool last_read_error;
> > >> >> +
> > >> >> +	dma_addr_t data_dma;
> > >> >> +	void *data_buf;
> > >> >> +	dma_addr_t oob_dma;
> > >> >> +	void *oob_buf;
> > >> >> +
> > >> >> +	int cur_chip;
> > >> >> +};    
> > >> >
> > >> > This struct should be split in 2 structures: one representing the NAND
> > >> > controller and one representing the NAND chip:
> > >> >
> > >> > struct tegra_nand_controller {
> > >> > 	struct nand_hw_control base;
> > >> > 	void __iomem *regs;
> > >> > 	struct clk *clk;
> > >> > 	struct device *dev;
> > >> > 	struct completion command_complete;
> > >> > 	struct completion dma_complete;
> > >> > 	bool last_read_error;
> > >> > 	int cur_chip;
> > >> > };
> > >> >
> > >> > struct tegra_nand {
> > >> > 	struct nand_chip base;
> > >> > 	dma_addr_t data_dma;
> > >> > 	void *data_buf;
> > >> > 	dma_addr_t oob_dma;
> > >> > 	void *oob_buf;
> > >> > };    
> > >>
> > >> Is there a particular reason why you would leave DMA buffers in the chip
> > >> structure? It seems that is more a controller thing...    
> > > 
> > > The size of those buffers is likely to be device dependent, so if you
> > > have several NANDs connected to the controller, you'll either have to
> > > have one buffer at the controller level which is max(all-chip-buf-size)
> > > or a buffer per device.
> > > 
> > > Also, do you really need these buffers? The core already provide some
> > > which are suitable for DMA (chip->oob_poi and chip->data_buf).
> > >     
> > 
> > Good question, I am not sure, that was existing code.
> > 
> > Are you sure data_buf it is DMA capable?
> > 
> > nand_scan_tail allocates with kmalloc:
> > 
> > chip->data_buf = kmalloc(mtd->writesize + mtd->oobsize, GFP_KERNEL);  
> 
> Yes, kmalloc() allocates DMA-able buffers, so those are DMA-safe.

Hm, that's not exactly true. It depends on the dma_mask attached to the
device.