| 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
| ||
|
Message-ID: <20160618134046.6ec19c15@bbrezillon>
Date: Sat, 18 Jun 2016 13:40:46 +0200
From: Boris Brezillon <boris.brezillon@...e-electrons.com>
To: Richard Weinberger <richard@....at>
Cc: David Woodhouse <dwmw2@...radead.org>,
Brian Norris <computersforpeace@...il.com>,
linux-mtd@...ts.infradead.org, Hans de Goede <hdegoede@...hat.com>,
linux-kernel@...r.kernel.org, Aleksei Mamlin <mamlinav@...il.com>
Subject: Re: [PATCH v2 05/15] mtd: nand: add manufacturer specific
initialization/detection steps
On Sat, 18 Jun 2016 13:31:21 +0200
Boris Brezillon <boris.brezillon@...e-electrons.com> wrote:
> On Sat, 18 Jun 2016 11:23:01 +0200
> Richard Weinberger <richard@....at> wrote:
>
> > Boris,
> >
> > Am 08.06.2016 um 15:00 schrieb Boris Brezillon:
> > > A lot of NANDs are implementing generic features in a non-generic way,
> > > or are providing advanced auto-detection logic where the NAND ID bytes
> > > meaning changes with the NAND generation.
> > >
> > > Providing this vendor specific initialization step will allow us to get
> > > rid of the full ids in the nand_ids table or all the vendor specific
> > > cases added over the time in the generic NAND ID decoding logic.
> > >
> > > Signed-off-by: Boris Brezillon <boris.brezillon@...e-electrons.com>
> > > ---
> > > drivers/mtd/nand/nand_base.c | 42 ++++++++++++++++++++++++++++++++----------
> > > include/linux/mtd/nand.h | 36 ++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 68 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > > index 95e9a8e..0a7d1c6 100644
> > > --- a/drivers/mtd/nand/nand_base.c
> > > +++ b/drivers/mtd/nand/nand_base.c
> > > @@ -3580,7 +3580,7 @@ static int nand_get_bits_per_cell(u8 cellinfo)
> > > * chip. The rest of the parameters must be decoded according to generic or
> > > * manufacturer-specific "extended ID" decoding patterns.
> > > */
> > > -static void nand_decode_ext_id(struct nand_chip *chip)
> > > +void nand_decode_ext_id(struct nand_chip *chip)
> > > {
> > > struct mtd_info *mtd = nand_to_mtd(chip);
> > > int extid, id_len = chip->id.len;
> > > @@ -3705,6 +3705,7 @@ static void nand_decode_ext_id(struct nand_chip *chip)
> > >
> > > }
> > > }
> > > +EXPORT_SYMBOL_GPL(nand_decode_ext_id);
> > >
> > > /*
> > > * Old devices have chip data hardcoded in the device ID table. nand_decode_id
> > > @@ -3815,7 +3816,7 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
> > > {
> > > struct mtd_info *mtd = nand_to_mtd(chip);
> > > int busw;
> > > - int i, maf_idx;
> > > + int i, maf_idx, ret;
> > > u8 *id_data = chip->id.data;
> > > u8 maf_id, dev_id;
> > >
> > > @@ -3856,6 +3857,14 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
> > >
> > > chip->id.len = nand_id_len(id_data, 8);
> > >
> > > + /* Try to identify manufacturer */
> > > + for (maf_idx = 0; nand_manuf_ids[maf_idx].id != 0x0; maf_idx++) {
> > > + if (nand_manuf_ids[maf_idx].id == maf_id)
> > > + break;
> > > + }
> > > +
> > > + chip->manufacturer.ops = nand_manuf_ids[maf_idx].ops;
> > > +
> >
> > Instead of checking on every access whether chip->manufacturer.ops is present, just
> > assign a nop field.
> > i.e.
> >
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index 55f3ab8..aadebe7 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -3703,6 +3703,10 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
> >
> > chip->manufacturer.ops = nand_manuf_ids[maf_idx].ops;
> >
> > + if (!chip->manufacturer.ops)
> > + /* assign no operations */
> > + chip->manufacturer.ops = nand_manuf_ids[0].ops;
> > +
> > if (!type)
> > type = nand_flash_ids;
> >
> > diff --git a/drivers/mtd/nand/nand_ids.c b/drivers/mtd/nand/nand_ids.c
> > index f1476ff..cdd26af 100644
> > --- a/drivers/mtd/nand/nand_ids.c
> > +++ b/drivers/mtd/nand/nand_ids.c
> > @@ -173,6 +173,8 @@ extern const struct nand_manufacturer_ops micron_nand_manuf_ops;
> > extern const struct nand_manufacturer_ops amd_nand_manuf_ops;
> > extern const struct nand_manufacturer_ops macronix_nand_manuf_ops;
> >
> > +static const struct nand_manufacturer_ops no_ops;
> > +
> > struct nand_manufacturers nand_manuf_ids[] = {
> > {NAND_MFR_TOSHIBA, "Toshiba", &toshiba_nand_manuf_ops},
> > {NAND_MFR_SAMSUNG, "Samsung", &samsung_nand_manuf_ops},
> > @@ -188,7 +190,7 @@ struct nand_manufacturers nand_manuf_ids[] = {
> > {NAND_MFR_SANDISK, "SanDisk"},
> > {NAND_MFR_INTEL, "Intel"},
> > {NAND_MFR_ATO, "ATO"},
> > - {0x0, "Unknown"}
> > + {0x0, "Unknown", &no_ops}
> > };
> >
> > EXPORT_SYMBOL(nand_manuf_ids);
> >
>
> Sorry, but I don't see any added value in adding this no_ops instance.
> The NULL value is supposed to represent no_ops.
> That's not like this path was critical: it's only call once during NAND
> initialization.
>
>
> How about adding the following helpers instead:
>
> static inline bool nand_has_manufacturer_ops(struct nand_chip * chip)
> {
> return chip->manufacturer.ops != NULL;
> }
I meant
static inline bool nand_has_manufacturer_detect(struct nand_chip * chip)
{
return chip->manufacturer.ops && chip->manufacturer.ops->detect;
}
>
> static inline void nand_manufacturer_detect(struct nand_chip * chip)
> {
> if (!chip->manufacturer.ops || chip->manufacturer.ops->detect)
> return;
>
> chip->manufacturer.ops->detect(chip);
> }
>
> static inline int nand_manufacturer_init(struct nand_chip * chip)
> {
> if (!chip->manufacturer.ops || chip->manufacturer.ops->init)
> return 0;
>
> return chip->manufacturer.ops->init(chip);
> }
>
> >
> > > if (!type)
> > > type = nand_flash_ids;
> > >
> > > @@ -3903,8 +3912,14 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
> > > chip->chipsize = (uint64_t)type->chipsize << 20;
> > >
> > > if (!type->pagesize) {
> > > - /* Decode parameters from extended ID */
> > > - nand_decode_ext_id(chip);
> > > + /*
> > > + * Try manufacturer detection if available and use
> > > + * nand_decode_ext_id() otherwise.
> > > + */
> > > + if (chip->manufacturer.ops->detect)
> >
> > You need to check here for chip->manufacturer.ops first.
>
> Correct.
Powered by blists - more mailing lists