Re: [RFC PATCH v3 2/3] mtd: nand: vf610_nfc: make use of ->exec_op()

[Boris Brezillon <boris.brezillon@bootlin.com>]

On Wed, 21 Feb 2018 13:24:14 +0100
Stefan Agner <stefan@agner.ch> wrote:

> On 21.02.2018 09:35, Boris Brezillon wrote:
> > On Wed, 21 Feb 2018 09:28:21 +0100
> > Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> >   
> >> On Wed, 21 Feb 2018 00:15:18 +0100
> >> Stefan Agner <stefan@agner.ch> wrote:
> >>  
> >> > >> +		}
> >> > >> +
> >> > >> +		row = ROW_ADDR(0, instr->ctx.addr.addrs[addr++]);
> >> > >> +		code |= COMMAND_RAR_BYTE1;
> >> > >> +		if (addr < instr->ctx.addr.naddrs) {
> >> > >> +			row |= ROW_ADDR(1, instr->ctx.addr.addrs[addr++]);
> >> > >> +			code |= COMMAND_RAR_BYTE2;
> >> > >> +		}
> >> > >> +		if (addr < instr->ctx.addr.naddrs) {
> >> > >> +			row |= ROW_ADDR(2, instr->ctx.addr.addrs[addr++]);
> >> > >> +			code |= COMMAND_RAR_BYTE3;
> >> > >> +		}
> >> > >> +
> >> > >> +		dev_dbg(nfc->dev, "OP_ADDR: col %d, row %d\n", col, row);
> >> > >> +
> >> > >> +		instr = vf610_get_next_instr(subop, &op_id);
> >> > >> +	}
> >> > >> +
> >> > >> +	if (instr && instr->type == NAND_OP_DATA_OUT_INSTR) {
> >> > >> +		int len = nand_subop_get_data_len(subop, op_id);
> >> > >> +		int offset = nand_subop_get_data_start_off(subop, op_id);
> >> > >> +
> >> > >> +		dev_dbg(nfc->dev, "OP_DATA_OUT: len %d, offset %d\n", len, offset);
> >> > >> +
> >> > >> +		vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0) + offset,
> >> > >> +				 instr->ctx.data.buf.in + offset,
> >> > >> +				 len);  
> >> > >
> >> > > I think you have the same endianness problem you have for the READ
> >> > > path. For example, I doubt SET_FEATURES will work properly if you're
> >> > > in LE. So I repeat my initial suggestion: always do the byte swapping
> >> > > when you're transfering data to/from the SRAM from vf610_nfc_cmd()
> >> > > and use vf610_nfc_memcpy() only in the ->read/write_page()
> >> > > implementations.
> >> > >  
> >> >
> >> > Hm, but doesn't that leads to wrong order of data when using e.g. raw
> >> > read/write page...?  
> >>
> >> Yep you'll have to implement ->{read,write}_{page,oob}[_raw](), but I
> >> prefer that to having an ->exec_op() implementation that tries to guess
> >> what the core is trying to do.
> >>  
> > 
> > One more thing, in its current state the driver may fail to detect
> > blocks marked bad by the manufacturer because of this endianness issue.
> > That should work most of the time because manufacturers tend to fill
> > the whole block with zeros when they want to mark it bad, but they're
> > not required to do that.  
> 
> That sounds scary. I think bad block scan is done through oob reads
> only. I guess if we only implement raw read/write and ecc read/write,
> the stack will actually read OOB with proper ending when implemented as
> you suggested.

Nope. Because BBM can be written by the NAND manufacturer (during
production), and it's usually placed in the first byte (or first 2 bytes
for 16-bit NANDs) of the OOB region. So let's assume you read the OOB
region without taking care of endianness, on little-endian platforms
you'll check the value of oob[4] (and oob[3] in case it's a 16-bit NAND)
instead of oob[0] (and oob[1]).

Of course, this situation will not happen when Linux marks a block
bad, because then, it's in control of what is written, and since
endianness conversion is skipped in both read/write path we're good.

My intention was not to scare you, but this endianness issue is not
something trivial to solve, and it would have been wiser to force
all users of this controller (that includes bootrom, bootloaders, and
and Linux) to do the cpu_to_be32() conversion when writing to/reading
from the FIFO, 'cause know we're screwed.

-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com