Pass -EUCLEN to userspace?

Pass -EUCLEN to userspace?

[Sascha Hauer]

Hi All,

I am currently working on a program similar to ubihealthd, just for raw
mtd pages, not UBI. Basically I want to find out in userspace if my Nand needs
scrubbing. Is it possible somehow to get this information in userspace?

So far I can count the number of bitflips corrected using the
ECCGETSTATS ioctl. Also I can read the bitflip_threshold from
/sys/class/mtdx/bitflip_threshold. The problem is now that I can only
read full pages, but the bitflip threshold is per ecc_step_size. Simply
dividing by the number of ecc_steps is not accurate. Any way to solve
this?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: Pass -EUCLEN to userspace?

[Boris Brezillon]

Hi Sascha,

On Wed, 20 Apr 2016 15:25:16 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> Hi All,
> 
> I am currently working on a program similar to ubihealthd, just for raw
> mtd pages, not UBI. Basically I want to find out in userspace if my Nand needs
> scrubbing. Is it possible somehow to get this information in userspace?

Actually we discussed that a year ago with Richard. I told him that we
should put the read/write/erase statistics at the MTD level so that
other MTD users (including userspace programs) could use the same infra
for non-UBI partitions (I need that for the UBOOT and SPL partitions).

My suggestion was to store those information at the MTD level, and let
UBI implement its own scrubbing layer on top of that, but Richard
decided to go for a simpler approach for its first implementation.

> 
> So far I can count the number of bitflips corrected using the
> ECCGETSTATS ioctl. Also I can read the bitflip_threshold from
> /sys/class/mtdx/bitflip_threshold. The problem is now that I can only
> read full pages, but the bitflip threshold is per ecc_step_size. Simply
> dividing by the number of ecc_steps is not accurate. Any way to solve
> this?

Hm, you're right, then we'll need to expose those information through a
different ioctl.

Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: Pass -EUCLEN to userspace?

[Boris Brezillon]

+ Richard, Daniel and Brian,

On Fri, 22 Apr 2016 17:24:56 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> Hi Sascha,
> 
> On Wed, 20 Apr 2016 15:25:16 +0200
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > Hi All,
> > 
> > I am currently working on a program similar to ubihealthd, just for raw
> > mtd pages, not UBI. Basically I want to find out in userspace if my Nand needs
> > scrubbing. Is it possible somehow to get this information in userspace?
> 
> Actually we discussed that a year ago with Richard. I told him that we
> should put the read/write/erase statistics at the MTD level so that
> other MTD users (including userspace programs) could use the same infra
> for non-UBI partitions (I need that for the UBOOT and SPL partitions).
> 
> My suggestion was to store those information at the MTD level, and let
> UBI implement its own scrubbing layer on top of that, but Richard
> decided to go for a simpler approach for its first implementation.
> 
> > 
> > So far I can count the number of bitflips corrected using the
> > ECCGETSTATS ioctl. Also I can read the bitflip_threshold from
> > /sys/class/mtdx/bitflip_threshold. The problem is now that I can only
> > read full pages, but the bitflip threshold is per ecc_step_size. Simply
> > dividing by the number of ecc_steps is not accurate. Any way to solve
> > this?
> 
> Hm, you're right, then we'll need to expose those information through a
> different ioctl.
> 
> Regards,
> 
> Boris
> 



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: Pass -EUCLEN to userspace?

[Richard Weinberger]

Sascha, Boris,

Am 22.04.2016 um 17:28 schrieb Boris Brezillon:
>>> I am currently working on a program similar to ubihealthd, just for raw
>>> mtd pages, not UBI. Basically I want to find out in userspace if my Nand needs
>>> scrubbing. Is it possible somehow to get this information in userspace?
>>
>> Actually we discussed that a year ago with Richard. I told him that we
>> should put the read/write/erase statistics at the MTD level so that
>> other MTD users (including userspace programs) could use the same infra
>> for non-UBI partitions (I need that for the UBOOT and SPL partitions).
>>
>> My suggestion was to store those information at the MTD level, and let
>> UBI implement its own scrubbing layer on top of that, but Richard
>> decided to go for a simpler approach for its first implementation.

Yeah, I did a first implementation on UBI layer as it had everything we need
and I didn't want to replicate UBI at MTD level.
Another reason is that we were not sure how sophisticated ubihealthd needs to be.

Sasha, what exactly is your use case and why is the UBI approach not sufficient for you?
On Linux MTD access should only happen through UBI and UBOOT/SPL partitions stay untouched.

Thanks,
//richard

Re: Pass -EUCLEN to userspace?

[Boris Brezillon]

On Fri, 22 Apr 2016 17:48:35 +0200
Richard Weinberger <richard@nod.at> wrote:

> Sascha, Boris,
> 
> Am 22.04.2016 um 17:28 schrieb Boris Brezillon:
> >>> I am currently working on a program similar to ubihealthd, just for raw
> >>> mtd pages, not UBI. Basically I want to find out in userspace if my Nand needs
> >>> scrubbing. Is it possible somehow to get this information in userspace?
> >>
> >> Actually we discussed that a year ago with Richard. I told him that we
> >> should put the read/write/erase statistics at the MTD level so that
> >> other MTD users (including userspace programs) could use the same infra
> >> for non-UBI partitions (I need that for the UBOOT and SPL partitions).
> >>
> >> My suggestion was to store those information at the MTD level, and let
> >> UBI implement its own scrubbing layer on top of that, but Richard
> >> decided to go for a simpler approach for its first implementation.
> 
> Yeah, I did a first implementation on UBI layer as it had everything we need
> and I didn't want to replicate UBI at MTD level.
> Another reason is that we were not sure how sophisticated ubihealthd needs to be.
> 
> Sasha, what exactly is your use case and why is the UBI approach not sufficient for you?
> On Linux MTD access should only happen through UBI and UBOOT/SPL partitions stay untouched.

Fair enough. So all we'll need is a way to retrieve the maximum number
of bitflips for a given block, so that the userspace deamon can
periodically read the SPL and bootloader blocks and decide to scrub
those blocks if the number of bitflips exceed the threshold.


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: Pass -EUCLEN to userspace?

[Richard Weinberger]

Am 22.04.2016 um 18:11 schrieb Boris Brezillon:
>> Sasha, what exactly is your use case and why is the UBI approach not sufficient for you?
>> On Linux MTD access should only happen through UBI and UBOOT/SPL partitions stay untouched.
> 
> Fair enough. So all we'll need is a way to retrieve the maximum number
> of bitflips for a given block, so that the userspace deamon can
> periodically read the SPL and bootloader blocks and decide to scrub
> those blocks if the number of bitflips exceed the threshold.

How can we scrub non-UBI blocks in a power cut safe way?
Unless we have a backup SPL/booloader.

Thanks,
//richard

Re: Pass -EUCLEN to userspace?

[Boris Brezillon]

On Fri, 22 Apr 2016 20:20:20 +0200
Richard Weinberger <richard@nod.at> wrote:

> Am 22.04.2016 um 18:11 schrieb Boris Brezillon:
> >> Sasha, what exactly is your use case and why is the UBI approach not sufficient for you?
> >> On Linux MTD access should only happen through UBI and UBOOT/SPL partitions stay untouched.  
> > 
> > Fair enough. So all we'll need is a way to retrieve the maximum number
> > of bitflips for a given block, so that the userspace deamon can
> > periodically read the SPL and bootloader blocks and decide to scrub
> > those blocks if the number of bitflips exceed the threshold.  
> 
> How can we scrub non-UBI blocks in a power cut safe way?
> Unless we have a backup SPL/booloader.

Of course this assumes you have backup partitions.


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: Pass -EUCLEN to userspace?

[Sascha Hauer]

On Fri, Apr 22, 2016 at 05:48:35PM +0200, Richard Weinberger wrote:
> Sascha, Boris,
> 
> Am 22.04.2016 um 17:28 schrieb Boris Brezillon:
> >>> I am currently working on a program similar to ubihealthd, just for raw
> >>> mtd pages, not UBI. Basically I want to find out in userspace if my Nand needs
> >>> scrubbing. Is it possible somehow to get this information in userspace?
> >>
> >> Actually we discussed that a year ago with Richard. I told him that we
> >> should put the read/write/erase statistics at the MTD level so that
> >> other MTD users (including userspace programs) could use the same infra
> >> for non-UBI partitions (I need that for the UBOOT and SPL partitions).
> >>
> >> My suggestion was to store those information at the MTD level, and let
> >> UBI implement its own scrubbing layer on top of that, but Richard
> >> decided to go for a simpler approach for its first implementation.
> 
> Yeah, I did a first implementation on UBI layer as it had everything we need
> and I didn't want to replicate UBI at MTD level.
> Another reason is that we were not sure how sophisticated ubihealthd needs to be.
> 
> Sasha, what exactly is your use case and why is the UBI approach not sufficient for you?
> On Linux MTD access should only happen through UBI and UBOOT/SPL partitions stay untouched.

On i.MX6 the Bootloader in Nand can indeed be redundant, so it's
possible to scrub the pages. This is exactly our usecase, we want to be
able to detect bitflips in the bootloader area.
Note that on i.MX6 the first page in the first n blocks on Nand contains
a structure called FCB (flash control block). This is not encoded with
the standard ECC algorithm used on the other areas in Nand. Reading
these pages will always return -EBABDMSG, they have to be read in raw
mode. That just to say that a "maximum bitflips per block" might not be
sufficient.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: Pass -EUCLEN to userspace?

[Boris Brezillon]

On Mon, 25 Apr 2016 07:28:57 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> On Fri, Apr 22, 2016 at 05:48:35PM +0200, Richard Weinberger wrote:
> > Sascha, Boris,
> > 
> > Am 22.04.2016 um 17:28 schrieb Boris Brezillon:  
> > >>> I am currently working on a program similar to ubihealthd, just for raw
> > >>> mtd pages, not UBI. Basically I want to find out in userspace if my Nand needs
> > >>> scrubbing. Is it possible somehow to get this information in userspace?  
> > >>
> > >> Actually we discussed that a year ago with Richard. I told him that we
> > >> should put the read/write/erase statistics at the MTD level so that
> > >> other MTD users (including userspace programs) could use the same infra
> > >> for non-UBI partitions (I need that for the UBOOT and SPL partitions).
> > >>
> > >> My suggestion was to store those information at the MTD level, and let
> > >> UBI implement its own scrubbing layer on top of that, but Richard
> > >> decided to go for a simpler approach for its first implementation.  
> > 
> > Yeah, I did a first implementation on UBI layer as it had everything we need
> > and I didn't want to replicate UBI at MTD level.
> > Another reason is that we were not sure how sophisticated ubihealthd needs to be.
> > 
> > Sasha, what exactly is your use case and why is the UBI approach not sufficient for you?
> > On Linux MTD access should only happen through UBI and UBOOT/SPL partitions stay untouched.  
> 
> On i.MX6 the Bootloader in Nand can indeed be redundant, so it's
> possible to scrub the pages. This is exactly our usecase, we want to be
> able to detect bitflips in the bootloader area.
> Note that on i.MX6 the first page in the first n blocks on Nand contains
> a structure called FCB (flash control block). This is not encoded with
> the standard ECC algorithm used on the other areas in Nand. Reading
> these pages will always return -EBABDMSG, they have to be read in raw
> mode. That just to say that a "maximum bitflips per block" might not be
> sufficient.

Okay, pretty much the same use-case we have on sunxi platforms: the SPL
partition is written in raw mode because the page layout (in-band/ECC
data disposition) is not the one we're using for the rest of the NAND.
For this specific partition, I see 2 solutions that you can implement
in userspace to count the number of bitflips:

1/ read the partition page by page in raw mode and compare each
   page to a reference file. This implies having a reference file stored
   on your FS.
2/ if you know the ECC algorithm (and the platform specific config,
   like the polynomial for a BCH engine) then you can create a tool
   doing the ECC error detection in userspace.


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: Pass -EUCLEN to userspace?

[Sascha Hauer]

On Mon, Apr 25, 2016 at 09:50:34AM +0200, Boris Brezillon wrote:
> On Mon, 25 Apr 2016 07:28:57 +0200
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > On Fri, Apr 22, 2016 at 05:48:35PM +0200, Richard Weinberger wrote:
> > > Sascha, Boris,
> > > 
> > > Am 22.04.2016 um 17:28 schrieb Boris Brezillon:  
> > > >>> I am currently working on a program similar to ubihealthd, just for raw
> > > >>> mtd pages, not UBI. Basically I want to find out in userspace if my Nand needs
> > > >>> scrubbing. Is it possible somehow to get this information in userspace?  
> > > >>
> > > >> Actually we discussed that a year ago with Richard. I told him that we
> > > >> should put the read/write/erase statistics at the MTD level so that
> > > >> other MTD users (including userspace programs) could use the same infra
> > > >> for non-UBI partitions (I need that for the UBOOT and SPL partitions).
> > > >>
> > > >> My suggestion was to store those information at the MTD level, and let
> > > >> UBI implement its own scrubbing layer on top of that, but Richard
> > > >> decided to go for a simpler approach for its first implementation.  
> > > 
> > > Yeah, I did a first implementation on UBI layer as it had everything we need
> > > and I didn't want to replicate UBI at MTD level.
> > > Another reason is that we were not sure how sophisticated ubihealthd needs to be.
> > > 
> > > Sasha, what exactly is your use case and why is the UBI approach not sufficient for you?
> > > On Linux MTD access should only happen through UBI and UBOOT/SPL partitions stay untouched.  
> > 
> > On i.MX6 the Bootloader in Nand can indeed be redundant, so it's
> > possible to scrub the pages. This is exactly our usecase, we want to be
> > able to detect bitflips in the bootloader area.
> > Note that on i.MX6 the first page in the first n blocks on Nand contains
> > a structure called FCB (flash control block). This is not encoded with
> > the standard ECC algorithm used on the other areas in Nand. Reading
> > these pages will always return -EBABDMSG, they have to be read in raw
> > mode. That just to say that a "maximum bitflips per block" might not be
> > sufficient.
> 
> Okay, pretty much the same use-case we have on sunxi platforms: the SPL
> partition is written in raw mode because the page layout (in-band/ECC
> data disposition) is not the one we're using for the rest of the NAND.
> For this specific partition, I see 2 solutions that you can implement
> in userspace to count the number of bitflips:
> 
> 1/ read the partition page by page in raw mode and compare each
>    page to a reference file. This implies having a reference file stored
>    on your FS.
> 2/ if you know the ECC algorithm (and the platform specific config,
>    like the polynomial for a BCH engine) then you can create a tool
>    doing the ECC error detection in userspace.

I can do the ECC calculation for the FBCs in userspace, that's no
problem. I was referring to:

> Fair enough. So all we'll need is a way to retrieve the maximum number
> of bitflips for a given block

When a block contains both FCB and data protected with regular ECC in
other pages, an algorithm retrieving the maximum number of bitlfips for
a given block might stumble over the bad data in the page containing the
FCB. So I think we need the maximum number of bitflips per page, not per
block.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: Pass -EUCLEN to userspace?

[Boris Brezillon]

On Mon, 25 Apr 2016 10:22:11 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> On Mon, Apr 25, 2016 at 09:50:34AM +0200, Boris Brezillon wrote:
> > On Mon, 25 Apr 2016 07:28:57 +0200
> > Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >   
> > > On Fri, Apr 22, 2016 at 05:48:35PM +0200, Richard Weinberger wrote:  
> > > > Sascha, Boris,
> > > > 
> > > > Am 22.04.2016 um 17:28 schrieb Boris Brezillon:    
> > > > >>> I am currently working on a program similar to ubihealthd, just for raw
> > > > >>> mtd pages, not UBI. Basically I want to find out in userspace if my Nand needs
> > > > >>> scrubbing. Is it possible somehow to get this information in userspace?    
> > > > >>
> > > > >> Actually we discussed that a year ago with Richard. I told him that we
> > > > >> should put the read/write/erase statistics at the MTD level so that
> > > > >> other MTD users (including userspace programs) could use the same infra
> > > > >> for non-UBI partitions (I need that for the UBOOT and SPL partitions).
> > > > >>
> > > > >> My suggestion was to store those information at the MTD level, and let
> > > > >> UBI implement its own scrubbing layer on top of that, but Richard
> > > > >> decided to go for a simpler approach for its first implementation.    
> > > > 
> > > > Yeah, I did a first implementation on UBI layer as it had everything we need
> > > > and I didn't want to replicate UBI at MTD level.
> > > > Another reason is that we were not sure how sophisticated ubihealthd needs to be.
> > > > 
> > > > Sasha, what exactly is your use case and why is the UBI approach not sufficient for you?
> > > > On Linux MTD access should only happen through UBI and UBOOT/SPL partitions stay untouched.    
> > > 
> > > On i.MX6 the Bootloader in Nand can indeed be redundant, so it's
> > > possible to scrub the pages. This is exactly our usecase, we want to be
> > > able to detect bitflips in the bootloader area.
> > > Note that on i.MX6 the first page in the first n blocks on Nand contains
> > > a structure called FCB (flash control block). This is not encoded with
> > > the standard ECC algorithm used on the other areas in Nand. Reading
> > > these pages will always return -EBABDMSG, they have to be read in raw
> > > mode. That just to say that a "maximum bitflips per block" might not be
> > > sufficient.  
> > 
> > Okay, pretty much the same use-case we have on sunxi platforms: the SPL
> > partition is written in raw mode because the page layout (in-band/ECC
> > data disposition) is not the one we're using for the rest of the NAND.
> > For this specific partition, I see 2 solutions that you can implement
> > in userspace to count the number of bitflips:
> > 
> > 1/ read the partition page by page in raw mode and compare each
> >    page to a reference file. This implies having a reference file stored
> >    on your FS.
> > 2/ if you know the ECC algorithm (and the platform specific config,
> >    like the polynomial for a BCH engine) then you can create a tool
> >    doing the ECC error detection in userspace.  
> 
> I can do the ECC calculation for the FBCs in userspace, that's no
> problem. I was referring to:
> 
> > Fair enough. So all we'll need is a way to retrieve the maximum number
> > of bitflips for a given block  
> 
> When a block contains both FCB and data protected with regular ECC in
> other pages, an algorithm retrieving the maximum number of bitlfips for
> a given block might stumble over the bad data in the page containing the
> FCB. So I think we need the maximum number of bitflips per page, not per
> block.

Oh, so you mix 2 different page layout in the same partition (we
try to avoid that, even if this means loosing some space) :-/.

Regarding the maximum number of bitflips per chunk, maybe we can make it
part of the ioctl request instead of saving the statistics at the MTD
level.

How about creating a new ioctl taking a pointer to this struct as a
parameter:

struct mtd_extended_read_ops {
	/* Existing params */
	unsigned int mode;
	size_t len;
	size_t retlen;
	size_t ooblen;
	size_t oobretlen;
	uint32_t ooboffs;
	void *datbuf;
	void *oobbuf;

	/*
	 * Param containing the maximum number of bitflips for this
	 * read request.
	 */
	unsigned int max_bitflips;
};

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: Pass -EUCLEN to userspace?

[Sascha Hauer]

On Mon, Apr 25, 2016 at 10:40:41AM +0200, Boris Brezillon wrote:
> On Mon, 25 Apr 2016 10:22:11 +0200
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > On Mon, Apr 25, 2016 at 09:50:34AM +0200, Boris Brezillon wrote:
> > > On Mon, 25 Apr 2016 07:28:57 +0200
> > > Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > >   
> > > > On Fri, Apr 22, 2016 at 05:48:35PM +0200, Richard Weinberger wrote:  
> > > > > Sascha, Boris,
> > > > > 
> > > > > Am 22.04.2016 um 17:28 schrieb Boris Brezillon:    
> > > > > >>> I am currently working on a program similar to ubihealthd, just for raw
> > > > > >>> mtd pages, not UBI. Basically I want to find out in userspace if my Nand needs
> > > > > >>> scrubbing. Is it possible somehow to get this information in userspace?    
> > > > > >>
> > > > > >> Actually we discussed that a year ago with Richard. I told him that we
> > > > > >> should put the read/write/erase statistics at the MTD level so that
> > > > > >> other MTD users (including userspace programs) could use the same infra
> > > > > >> for non-UBI partitions (I need that for the UBOOT and SPL partitions).
> > > > > >>
> > > > > >> My suggestion was to store those information at the MTD level, and let
> > > > > >> UBI implement its own scrubbing layer on top of that, but Richard
> > > > > >> decided to go for a simpler approach for its first implementation.    
> > > > > 
> > > > > Yeah, I did a first implementation on UBI layer as it had everything we need
> > > > > and I didn't want to replicate UBI at MTD level.
> > > > > Another reason is that we were not sure how sophisticated ubihealthd needs to be.
> > > > > 
> > > > > Sasha, what exactly is your use case and why is the UBI approach not sufficient for you?
> > > > > On Linux MTD access should only happen through UBI and UBOOT/SPL partitions stay untouched.    
> > > > 
> > > > On i.MX6 the Bootloader in Nand can indeed be redundant, so it's
> > > > possible to scrub the pages. This is exactly our usecase, we want to be
> > > > able to detect bitflips in the bootloader area.
> > > > Note that on i.MX6 the first page in the first n blocks on Nand contains
> > > > a structure called FCB (flash control block). This is not encoded with
> > > > the standard ECC algorithm used on the other areas in Nand. Reading
> > > > these pages will always return -EBABDMSG, they have to be read in raw
> > > > mode. That just to say that a "maximum bitflips per block" might not be
> > > > sufficient.  
> > > 
> > > Okay, pretty much the same use-case we have on sunxi platforms: the SPL
> > > partition is written in raw mode because the page layout (in-band/ECC
> > > data disposition) is not the one we're using for the rest of the NAND.
> > > For this specific partition, I see 2 solutions that you can implement
> > > in userspace to count the number of bitflips:
> > > 
> > > 1/ read the partition page by page in raw mode and compare each
> > >    page to a reference file. This implies having a reference file stored
> > >    on your FS.
> > > 2/ if you know the ECC algorithm (and the platform specific config,
> > >    like the polynomial for a BCH engine) then you can create a tool
> > >    doing the ECC error detection in userspace.  
> > 
> > I can do the ECC calculation for the FBCs in userspace, that's no
> > problem. I was referring to:
> > 
> > > Fair enough. So all we'll need is a way to retrieve the maximum number
> > > of bitflips for a given block  
> > 
> > When a block contains both FCB and data protected with regular ECC in
> > other pages, an algorithm retrieving the maximum number of bitlfips for
> > a given block might stumble over the bad data in the page containing the
> > FCB. So I think we need the maximum number of bitflips per page, not per
> > block.
> 
> Oh, so you mix 2 different page layout in the same partition (we
> try to avoid that, even if this means loosing some space) :-/.

I wasn't really aware of this. The way we have it now allows us to
create a single partition containing the bootloader. Splitting this up
into multiple partitions wouldn't be as flexible.

> 
> Regarding the maximum number of bitflips per chunk, maybe we can make it
> part of the ioctl request instead of saving the statistics at the MTD
> level.
> 
> How about creating a new ioctl taking a pointer to this struct as a
> parameter:
> 
> struct mtd_extended_read_ops {
> 	/* Existing params */
> 	unsigned int mode;
> 	size_t len;
> 	size_t retlen;
> 	size_t ooblen;
> 	size_t oobretlen;
> 	uint32_t ooboffs;
> 	void *datbuf;
> 	void *oobbuf;
> 
> 	/*
> 	 * Param containing the maximum number of bitflips for this
> 	 * read request.
> 	 */
> 	unsigned int max_bitflips;
> };

Not sure how this ioctl exactly should look like, but this would solve
the problem.

This also solves another problem. Right now calling the ECCGETSTATS
ioctl before and after the read operation assumes that there are no
concurrent readers on the mtd device.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: Pass -EUCLEN to userspace?

[Boris Brezillon]

On Mon, 25 Apr 2016 11:14:16 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> On Mon, Apr 25, 2016 at 10:40:41AM +0200, Boris Brezillon wrote:
> > On Mon, 25 Apr 2016 10:22:11 +0200
> > Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >   
> > > On Mon, Apr 25, 2016 at 09:50:34AM +0200, Boris Brezillon wrote:  
> > > > On Mon, 25 Apr 2016 07:28:57 +0200
> > > > Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > > >     
> > > > > On Fri, Apr 22, 2016 at 05:48:35PM +0200, Richard Weinberger wrote:    
> > > > > > Sascha, Boris,
> > > > > > 
> > > > > > Am 22.04.2016 um 17:28 schrieb Boris Brezillon:      
> > > > > > >>> I am currently working on a program similar to ubihealthd, just for raw
> > > > > > >>> mtd pages, not UBI. Basically I want to find out in userspace if my Nand needs
> > > > > > >>> scrubbing. Is it possible somehow to get this information in userspace?      
> > > > > > >>
> > > > > > >> Actually we discussed that a year ago with Richard. I told him that we
> > > > > > >> should put the read/write/erase statistics at the MTD level so that
> > > > > > >> other MTD users (including userspace programs) could use the same infra
> > > > > > >> for non-UBI partitions (I need that for the UBOOT and SPL partitions).
> > > > > > >>
> > > > > > >> My suggestion was to store those information at the MTD level, and let
> > > > > > >> UBI implement its own scrubbing layer on top of that, but Richard
> > > > > > >> decided to go for a simpler approach for its first implementation.      
> > > > > > 
> > > > > > Yeah, I did a first implementation on UBI layer as it had everything we need
> > > > > > and I didn't want to replicate UBI at MTD level.
> > > > > > Another reason is that we were not sure how sophisticated ubihealthd needs to be.
> > > > > > 
> > > > > > Sasha, what exactly is your use case and why is the UBI approach not sufficient for you?
> > > > > > On Linux MTD access should only happen through UBI and UBOOT/SPL partitions stay untouched.      
> > > > > 
> > > > > On i.MX6 the Bootloader in Nand can indeed be redundant, so it's
> > > > > possible to scrub the pages. This is exactly our usecase, we want to be
> > > > > able to detect bitflips in the bootloader area.
> > > > > Note that on i.MX6 the first page in the first n blocks on Nand contains
> > > > > a structure called FCB (flash control block). This is not encoded with
> > > > > the standard ECC algorithm used on the other areas in Nand. Reading
> > > > > these pages will always return -EBABDMSG, they have to be read in raw
> > > > > mode. That just to say that a "maximum bitflips per block" might not be
> > > > > sufficient.    
> > > > 
> > > > Okay, pretty much the same use-case we have on sunxi platforms: the SPL
> > > > partition is written in raw mode because the page layout (in-band/ECC
> > > > data disposition) is not the one we're using for the rest of the NAND.
> > > > For this specific partition, I see 2 solutions that you can implement
> > > > in userspace to count the number of bitflips:
> > > > 
> > > > 1/ read the partition page by page in raw mode and compare each
> > > >    page to a reference file. This implies having a reference file stored
> > > >    on your FS.
> > > > 2/ if you know the ECC algorithm (and the platform specific config,
> > > >    like the polynomial for a BCH engine) then you can create a tool
> > > >    doing the ECC error detection in userspace.    
> > > 
> > > I can do the ECC calculation for the FBCs in userspace, that's no
> > > problem. I was referring to:
> > >   
> > > > Fair enough. So all we'll need is a way to retrieve the maximum number
> > > > of bitflips for a given block    
> > > 
> > > When a block contains both FCB and data protected with regular ECC in
> > > other pages, an algorithm retrieving the maximum number of bitlfips for
> > > a given block might stumble over the bad data in the page containing the
> > > FCB. So I think we need the maximum number of bitflips per page, not per
> > > block.  
> > 
> > Oh, so you mix 2 different page layout in the same partition (we
> > try to avoid that, even if this means loosing some space) :-/.  
> 
> I wasn't really aware of this.

By "we" I meant on the sunxi platform. This kind of thing has never been
documented, so you're currently not infringing any rule, I just find it
easier when everything is well separated... 

> The way we have it now allows us to
> create a single partition containing the bootloader. Splitting this up
> into multiple partitions wouldn't be as flexible.
> 
> > 
> > Regarding the maximum number of bitflips per chunk, maybe we can make it
> > part of the ioctl request instead of saving the statistics at the MTD
> > level.
> > 
> > How about creating a new ioctl taking a pointer to this struct as a
> > parameter:
> > 
> > struct mtd_extended_read_ops {
> > 	/* Existing params */
> > 	unsigned int mode;
> > 	size_t len;
> > 	size_t retlen;
> > 	size_t ooblen;
> > 	size_t oobretlen;
> > 	uint32_t ooboffs;
> > 	void *datbuf;
> > 	void *oobbuf;
> > 
> > 	/*
> > 	 * Param containing the maximum number of bitflips for this
> > 	 * read request.
> > 	 */
> > 	unsigned int max_bitflips;
> > };  
> 
> Not sure how this ioctl exactly should look like, but this would solve
> the problem.

Let me design a quick prototype, I'll let you follow up with the patch
submission process...

> 
> This also solves another problem. Right now calling the ECCGETSTATS
> ioctl before and after the read operation assumes that there are no
> concurrent readers on the mtd device.

That's true.


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: Pass -EUCLEN to userspace?

[Boris Brezillon]

On Mon, 25 Apr 2016 11:26:16 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> > > 
> > > Regarding the maximum number of bitflips per chunk, maybe we can make it
> > > part of the ioctl request instead of saving the statistics at the MTD
> > > level.
> > > 
> > > How about creating a new ioctl taking a pointer to this struct as a
> > > parameter:
> > > 
> > > struct mtd_extended_read_ops {
> > > 	/* Existing params */
> > > 	unsigned int mode;
> > > 	size_t len;
> > > 	size_t retlen;
> > > 	size_t ooblen;
> > > 	size_t oobretlen;
> > > 	uint32_t ooboffs;
> > > 	void *datbuf;
> > > 	void *oobbuf;
> > > 
> > > 	/*
> > > 	 * Param containing the maximum number of bitflips for this
> > > 	 * read request.
> > > 	 */
> > > 	unsigned int max_bitflips;
> > > };    
> > 
> > Not sure how this ioctl exactly should look like, but this would solve
> > the problem.  
> 
> Let me design a quick prototype, I'll let you follow up with the patch
> submission process...

Below is an untested patch adding a new ioctl returning the ECC/read stats.
Feel free to debug/enhance this implementation and submit it to the MTD
ML.


--->8---
From 2c07a2a015e6c0bdc2f9d91c9a1b971903da0493 Mon Sep 17 00:00:00 2001
From: Boris Brezillon <boris.brezillon@free-electrons.com>
Date: Mon, 25 Apr 2016 16:05:06 +0200
Subject: [PATCH] mtd: add a the MEMREAD ioctl to attach ECC stats to the read
 request

TODO: add proper commit message + split changes into several patches.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/mtd/devices/docg3.c        | 10 +++++
 drivers/mtd/mtdchar.c              | 76 ++++++++++++++++++++++++++++++++++++++
 drivers/mtd/mtdcore.c              |  9 +++++
 drivers/mtd/nand/nand_base.c       | 10 +++++
 drivers/mtd/onenand/onenand_base.c | 11 ++++++
 include/linux/mtd/mtd.h            |  7 ++++
 include/uapi/mtd/mtd-abi.h         | 50 +++++++++++++++++++++++++
 7 files changed, 173 insertions(+)

diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
index b833e6c..965c97a 100644
--- a/drivers/mtd/devices/docg3.c
+++ b/drivers/mtd/devices/docg3.c
@@ -885,6 +885,8 @@ static int doc_read_oob(struct mtd_info *mtd, loff_t from,
 	u8 *buf = ops->datbuf;
 	size_t len, ooblen, nbdata, nboob;
 	u8 hwecc[DOC_ECC_BCH_SIZE], eccconf1;
+	struct mtd_req_stats *reqstats = ops->stats;
+	struct mtd_ecc_stats stats;
 	int max_bitflips = 0;
 
 	if (buf)
@@ -912,6 +914,7 @@ static int doc_read_oob(struct mtd_info *mtd, loff_t from,
 	ret = 0;
 	skip = from % DOC_LAYOUT_PAGE_SIZE;
 	mutex_lock(&docg3->cascade->lock);
+	stats = mtd->ecc_stats;
 	while (ret >= 0 && (len > 0 || ooblen > 0)) {
 		calc_block_sector(from - skip, &block0, &block1, &page, &ofs,
 			docg3->reliable);
@@ -983,6 +986,13 @@ static int doc_read_oob(struct mtd_info *mtd, loff_t from,
 	}
 
 out:
+	if (reqstats) {
+		restats->corrected_bitflips += mtd->ecc_stats.corrected -
+					       stats.corrected;
+		restats->uncorrectable_errors += mtd->ecc_stats.failed -
+						 stats.failed;
+	}
+
 	mutex_unlock(&docg3->cascade->lock);
 	return ret;
 err_in_read:
diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 2a47a3f..708ecee 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -656,6 +656,75 @@ static int mtdchar_write_ioctl(struct mtd_info *mtd,
 	return ret;
 }
 
+static int mtdchar_read_ioctl(struct mtd_info *mtd,
+			      struct mtd_read_req __user *argp)
+{
+	struct mtd_read_req req;
+	struct mtd_req_stats stats;
+	struct mtd_oob_ops ops = { .stats = &stats };
+	void __user *usr_data, *usr_oob;
+	int ret;
+
+	if (copy_from_user(&req, argp, sizeof(req)))
+		return -EFAULT;
+
+	usr_data = (void __user *)(uintptr_t)req.usr_data;
+	usr_oob = (void __user *)(uintptr_t)req.usr_oob;
+	if ((usr_data && !access_ok(VERIFY_WRITE, usr_data, req.len)) ||
+	    (usr_oob && !access_ok(VERIFY_WRITE, usr_oob, req.ooblen)))
+		return -EFAULT;
+
+	if (!mtd->_read_oob)
+		return -EOPNOTSUPP;
+
+	ops.mode = req.mode;
+	ops.len = (size_t)req.len;
+	ops.ooblen = (size_t)req.ooblen;
+	ops.ooboffs = 0;
+
+	if (usr_data) {
+		ops.datbuf = kzalloc(ops.len, GFP_KERNEL);
+		if (!ops.datbuf)
+			return -ENOMEM;
+	}
+
+	if (usr_oob) {
+		ops.oobbuf = kzalloc(ops.ooblen, GFP_KERNEL);
+		if (!ops.oobbuf) {
+			ret = -ENOMEM;
+			goto out;
+		}
+	}
+
+	ret = mtd_read_oob(mtd, (loff_t)req.start, &ops);
+	if (ret)
+		goto out;
+
+	if (usr_data && copy_to_user(ops.datbuf, usr_data, ops.retlen)) {
+		ret = -EFAULT;
+		goto out;
+	}
+
+	if (usr_oob && copy_to_user(ops.oobbuf, usr_oob, ops.oobretlen)) {
+		ret = -EFAULT;
+		goto out;
+	}
+
+	req.len = ops.retlen;
+	req.ooblen = ops.oobretlen;
+	req.stats.max_bitflips = stats.max_bitflips;
+	req.stats.corrected_bitflips = stats.corrected_bitflips;
+	req.stats.uncorrectable_errors = stats.uncorrectable_errors;
+	if (copy_to_user(&req, argp, sizeof(req)))
+		ret = -EFAULT;
+
+out:
+	kfree(ops.datbuf);
+	kfree(ops.oobbuf);
+
+	return ret;
+}
+
 static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
 {
 	struct mtd_file_info *mfi = file->private_data;
@@ -850,6 +919,13 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
 		break;
 	}
 
+	case MEMREAD:
+	{
+		ret = mtdchar_read_ioctl(mtd,
+		      (struct mtd_read_req __user *)arg);
+		break;
+	}
+
 	case MEMLOCK:
 	{
 		struct erase_info_user einfo;
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index e3936b8..ba6488f 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -988,6 +988,11 @@ int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
 		return -EOPNOTSUPP;
 
 	ledtrig_mtd_activity();
+
+	/* Make sure all counters are initialized to zero. */
+	if (ops->stats)
+		memset(ops->stats, 0, sizeof(*ops->stats));
+
 	/*
 	 * In cases where ops->datbuf != NULL, mtd->_read_oob() has semantics
 	 * similar to mtd->_read(), returning a non-negative integer
@@ -999,6 +1004,10 @@ int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
 		return ret_code;
 	if (mtd->ecc_strength == 0)
 		return 0;	/* device lacks ecc */
+
+	if (ops->stats)
+		ops->stats->max_bitflips = ret_code;
+
 	return ret_code >= mtd->bitflip_threshold ? -EUCLEAN : 0;
 }
 EXPORT_SYMBOL_GPL(mtd_read_oob);
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 7bc37b4..25cab31 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2162,6 +2162,8 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
 static int nand_read_oob(struct mtd_info *mtd, loff_t from,
 			 struct mtd_oob_ops *ops)
 {
+	struct mtd_req_stats *reqstats = ops->stats;
+	struct mtd_ecc_stats stats;
 	int ret = -ENOTSUPP;
 
 	ops->retlen = 0;
@@ -2185,11 +2187,19 @@ static int nand_read_oob(struct mtd_info *mtd, loff_t from,
 		goto out;
 	}
 
+	stats = mtd->ecc_stats;
 	if (!ops->datbuf)
 		ret = nand_do_read_oob(mtd, from, ops);
 	else
 		ret = nand_do_read_ops(mtd, from, ops);
 
+	if (reqstats) {
+		reqstats->uncorrectable_errors += mtd->ecc_stats.failed -
+						  stats.failed;
+		reqstats->corrected_bitflips += mtd->ecc_stats.corrected -
+						stats.corrected;
+	}
+
 out:
 	nand_release_device(mtd);
 	return ret;
diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
index a4b029a..2f700eb 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -1491,6 +1491,8 @@ static int onenand_read_oob(struct mtd_info *mtd, loff_t from,
 			    struct mtd_oob_ops *ops)
 {
 	struct onenand_chip *this = mtd->priv;
+	struct mtd_req_stats *reqstats = ops->stats;
+	struct mtd_ecc_stats stats;
 	int ret;
 
 	switch (ops->mode) {
@@ -1504,12 +1506,21 @@ static int onenand_read_oob(struct mtd_info *mtd, loff_t from,
 	}
 
 	onenand_get_device(mtd, FL_READING);
+	stats = mtd->ecc_stats;
+
 	if (ops->datbuf)
 		ret = ONENAND_IS_4KB_PAGE(this) ?
 			onenand_mlc_read_ops_nolock(mtd, from, ops) :
 			onenand_read_ops_nolock(mtd, from, ops);
 	else
 		ret = onenand_read_oob_nolock(mtd, from, ops);
+
+	if (reqstats) {
+		reqstats->corrected_bitflips += mtd->ecc_stats.corrected -
+						stats.corrected;
+		reqstats->uncorrectable_errors += mtd->ecc_stats.failed -
+						  stats.failed;
+	}
 	onenand_release_device(mtd);
 
 	return ret;
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 29a1706..bd5e8a1 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -64,6 +64,12 @@ struct mtd_erase_region_info {
 	unsigned long *lockmap;		/* If keeping bitmap of locks */
 };
 
+struct mtd_req_stats {
+	unsigned int max_bitflips;
+	unsigned int corrected_bitflips;
+	unsigned int uncorrectable_errors;
+};
+
 /**
  * struct mtd_oob_ops - oob operation operands
  * @mode:	operation mode
@@ -92,6 +98,7 @@ struct mtd_oob_ops {
 	uint32_t	ooboffs;
 	uint8_t		*datbuf;
 	uint8_t		*oobbuf;
+	struct mtd_req_stats *stats;
 };
 
 #define MTD_MAX_OOBFREE_ENTRIES_LARGE	32
diff --git a/include/uapi/mtd/mtd-abi.h b/include/uapi/mtd/mtd-abi.h
index 0ec1da2..a61a042 100644
--- a/include/uapi/mtd/mtd-abi.h
+++ b/include/uapi/mtd/mtd-abi.h
@@ -90,6 +90,52 @@ struct mtd_write_req {
 	__u8 padding[7];
 };
 
+/**
+ * struct mtd_read_req_stats - statistics attached to a read request
+ *
+ * @max_bitflips: the maximum number of bitflips found in the read portion.
+ *		  This information can be used to decide when the data stored
+ *		  on a specific portion should be moved somewhere else to
+ *		  avoid data loss.
+ * @corrected_bitflips: the number of bitflips corrected during the read
+ *			operation
+ * @uncorrectable_errors: the number of uncorrectable errors that happened
+ *			  during the read operation
+ */
+struct mtd_read_req_stats {
+	__u32 max_bitflips;
+	__u32 corrected_bitflips;
+	__u32 uncorrectable_errors;
+};
+
+/**
+ * struct mtd_read_req - data structure for requesting a write operation
+ *
+ * @start:	start address
+ * @len:	length of data buffer
+ * @ooblen:	length of OOB buffer
+ * @usr_data:	user-provided data buffer
+ * @usr_oob:	user-provided OOB buffer
+ * @mode:	MTD mode (see "MTD operation modes")
+ * @padding:	reserved, must be set to 0
+ * @stats:	statistics attached to the read request
+ *
+ * This structure supports ioctl(MEMWRITE) operations, allowing data and/or OOB
+ * writes in various modes. To write to OOB-only, set @usr_data == NULL, and to
+ * write data-only, set @usr_oob == NULL. However, setting both @usr_data and
+ * @usr_oob to NULL is not allowed.
+ */
+struct mtd_read_req {
+	__u64 start;
+	__u64 len;
+	__u64 ooblen;
+	__u64 usr_data;
+	__u64 usr_oob;
+	__u8 mode;
+	__u8 padding[7];
+	struct mtd_read_req_stats stats;
+};
+
 #define MTD_ABSENT		0
 #define MTD_RAM			1
 #define MTD_ROM			2
@@ -203,6 +249,10 @@ struct otp_info {
  * without OOB, e.g., NOR flash.
  */
 #define MEMWRITE		_IOWR('M', 24, struct mtd_write_req)
+/*
+ * Same as for MEMWRITE but for read operations.
+ */
+#define MEMREAD			_IOWR('M', 25, struct mtd_read_req)
 
 /*
  * Obsolete legacy interface. Keep it in order not to break userspace

Re: Pass -EUCLEAN to userspace?

[Sascha Hauer]

Fixed typo in subject, maybe this helps finding this thread in the
archives.

On Mon, Apr 25, 2016 at 04:11:44PM +0200, Boris Brezillon wrote:
> On Mon, 25 Apr 2016 11:26:16 +0200
> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> 
> > > > 
> > > > Regarding the maximum number of bitflips per chunk, maybe we can make it
> > > > part of the ioctl request instead of saving the statistics at the MTD
> > > > level.
> > > > 
> > > > How about creating a new ioctl taking a pointer to this struct as a
> > > > parameter:
> > > > 
> > > > struct mtd_extended_read_ops {
> > > > 	/* Existing params */
> > > > 	unsigned int mode;
> > > > 	size_t len;
> > > > 	size_t retlen;
> > > > 	size_t ooblen;
> > > > 	size_t oobretlen;
> > > > 	uint32_t ooboffs;
> > > > 	void *datbuf;
> > > > 	void *oobbuf;
> > > > 
> > > > 	/*
> > > > 	 * Param containing the maximum number of bitflips for this
> > > > 	 * read request.
> > > > 	 */
> > > > 	unsigned int max_bitflips;
> > > > };    
> > > 
> > > Not sure how this ioctl exactly should look like, but this would solve
> > > the problem.  
> > 
> > Let me design a quick prototype, I'll let you follow up with the patch
> > submission process...
> 
> Below is an untested patch adding a new ioctl returning the ECC/read stats.
> Feel free to debug/enhance this implementation and submit it to the MTD
> ML.

Thanks Boris, I'll give it a try.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |