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: <20140326095156.3e5585b8@ipc1.ka-ro>
Date:	Wed, 26 Mar 2014 09:51:56 +0100
From:	Lothar Waßmann <LW@...O-electronics.de>
To:	Huang Shijie <b32955@...escale.com>
Cc:	Fabio Estevam <fabio.estevam@...escale.com>,
	Mark Rutland <mark.rutland@....com>,
	Shawn Guo <shawn.guo@...aro.org>,
	Russell King <linux@....linux.org.uk>,
	Pawel Moll <pawel.moll@....com>, Arnd Bergmann <arnd@...db.de>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Artem Bityutskiy <artem.bityutskiy@...ux.intel.com>,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
	linux-mtd@...ts.infradead.org,
	linux-arm-kernel@...ts.infradead.org,
	Rob Landley <rob@...dley.net>,
	Kumar Gala <galak@...eaurora.org>,
	Brian Norris <computersforpeace@...il.com>,
	David Woodhouse <dwmw2@...radead.org>,
	Sascha Hauer <kernel@...gutronix.de>,
	Shawn Guo <shawn.guo@...escale.com>
Subject: Re: [PATCHv2 1/1] mtd: gpmi: make blockmark swapping optional

Hi,

Huang Shijie wrote:
> On Fri, Mar 21, 2014 at 11:50:17AM +0100, Lothar Waßmann wrote:
> > With a flash-based BBT there is no reason to move the Factory Bad
> > Block Marker from the data area buffer (to where it is mapped by the
> > GPMI NAND controller) to the OOB buffer. Thus, make this feature
> > configurable via DT. This is required for the Ka-Ro electronics
> > platforms.
> > 
> > Signed-off-by: Lothar Waßmann <LW@...O-electronics.de>
> > ---
> >  Documentation/devicetree/bindings/mtd/gpmi-nand.txt |    3 +++
> >  drivers/mtd/nand/gpmi-nand/gpmi-nand.c              |   10 +++++++---
> >  2 files changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/mtd/gpmi-nand.txt b/Documentation/devicetree/bindings/mtd/gpmi-nand.txt
> > index 458d596..f28949a 100644
> > --- a/Documentation/devicetree/bindings/mtd/gpmi-nand.txt
> > +++ b/Documentation/devicetree/bindings/mtd/gpmi-nand.txt
> > @@ -25,6 +25,9 @@ Optional properties:
> >                         discoverable or this property is not enabled,
> >                         the software may chooses an implementation-defined
> >                         ECC scheme.
> > +  - fsl,no-blockmark-swap: Don't swap the bad block marker from the OOB
> > +                       area with the byte in the data area but rely on the
> > +                       BBT for identifying bad blocks.
> 
> Please add the following:
>        "NOTE: This property is not supported by the imx23 and imx28"
>
I don't see why this should not be supported on i.MX28 (i.MX23 doesn't
do byteswapping anyway, so this wouldn't change anything for i.MX23).
The partitions used by Linux need not necessarily be accessible for the
Boot ROM code (and vice versa).

> >  The device tree may optionally contain sub-nodes describing partitions of the
> >  address space. See partition.txt for more detail.
> > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > index bb77f75..98562eb 100644
> > --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > @@ -1632,9 +1632,6 @@ static int gpmi_init_last(struct gpmi_nand_data *this)
> >  	struct bch_geometry *bch_geo = &this->bch_geometry;
> >  	int ret;
> >  
> > -	/* Set up swap_block_mark, must be set before the gpmi_set_geometry() */
> > -	this->swap_block_mark = !GPMI_IS_MX23(this);
> > -
> >  	/* Set up the medium geometry */
> >  	ret = gpmi_set_geometry(this);
> >  	if (ret)
> > @@ -1701,6 +1698,13 @@ static int gpmi_nand_init(struct gpmi_nand_data *this)
> >  	if (of_get_nand_on_flash_bbt(this->dev->of_node))
> >  		chip->bbt_options |= NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
> >  
> > +	/* Set up swap_block_mark, must be set before the gpmi_set_geometry() */
> > +	if (!of_property_read_bool(this->dev->of_node, "fsl,no-blockmark-swap"))
> > +		this->swap_block_mark = !GPMI_IS_MX23(this);
> 
> the code should like this:
> ----------------------------------------------
> 	this->swap_block_mark = !GPMI_IS_MX23(this);
> 
> 	if (!GPMI_IS_IMX23(this) && !GPMI_IS_MX28(this) &&
>
the !GPMI_IS_MX23() is redundant here as i.MX23 will always have
swap_block_mark == false.
I would rather add a dependency on nand-on-flash-bbt as a
prerequisite for not checking the BB marks in each block.

> > +
> > +	dev_dbg(this->dev, "Blockmark swapping %sabled\n",
> > +		this->swap_block_mark ? "en" : "dis");
> > +
> >  	/*
> >  	 * Allocate a temporary DMA buffer for reading ID in the
> >  	 * nand_scan_ident().
> 
> There are some bugs in the gpmi driver which is caused by the assumption that
> we always enable the swapping except the imx23. 
> 
> So, there are some places should be changed with this patch:
>    [1] the subpage hook,
>         Please also change the gpmi_ecc_read_subpage() too.
> 
>    [2] the OOB hook,
>    	Please also change the gpmi_ecc_read_oob(). 
> 
>    [3] the markbad hook,
>    	Please also change the gpmi_block_markbad() . 
> 
OK, I'll check that.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@...o-electronics.de
___________________________________________________________
--
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/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ