| 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: <20090922141705.b5d31e24.akpm@linux-foundation.org>
Date: Tue, 22 Sep 2009 14:17:05 -0700
From: Andrew Morton <akpm@...ux-foundation.org>
To: David Brownell <david-b@...bell.net>
Cc: avorontsov@...mvista.com, ben@...ff.org, dwmw2@...radead.org,
grant.likely@...retlab.ca, benh@...nel.crashing.org,
khali@...ux-fr.org, linux-mtd@...ts.infradead.org,
linux-kernel@...r.kernel.org, lm-sensors@...sensors.org,
linuxppc-dev@...abs.org
Subject: Re: [PATCH 2/6] mtd: m25p80: Convert to device table matching
On Mon, 3 Aug 2009 19:54:50 -0700
David Brownell <david-b@...bell.net> wrote:
> On Thursday 30 July 2009, Anton Vorontsov wrote:
> > This patch converts the m25p80 driver so that now it uses .id_table
> > for device matching, making it properly detect devices on OpenFirmware
> > platforms (prior to this patch the driver misdetected non-JEDEC chips,
> > seeing all chips as "m25p80").
>
> I suspect "detect" is a misnomer there. It only "detects" JEDEC chips.
> All others got explicit declarations ... so if there's misbehavior for
> other chips, it's because those declarations were poorly handled. Maybe
> they were not properly flagged as non-JDEC
>
>
> > Also, now jedec_probe() only does jedec probing, nothing else. If it
> > is not able to detect a chip, NULL is returned and the driver fall
> > backs to the information specified by the platform (platform_data, or
> > exact ID).
>
> I'd rather keep the warning, so there's a clue about what's really
> going on: JEDEC chip found, but its ID is not handled.
>
afaik there was no response to David's review comments, so this patch
is in the "stuck" state.
> > Signed-off-by: Anton Vorontsov <avorontsov@...mvista.com>
> > ---
> > drivers/mtd/devices/m25p80.c | 146 +++++++++++++++++++++++-------------------
> > 1 files changed, 80 insertions(+), 66 deletions(-)
> >
> > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> > index 10ed195..0d74b38 100644
> > --- a/drivers/mtd/devices/m25p80.c
> > +++ b/drivers/mtd/devices/m25p80.c
> > ... deletia ...
>
> > @@ -481,74 +480,83 @@ struct flash_info {
> > #define SECT_4K 0x01 /* OPCODE_BE_4K works uniformly */
> > };
> >
> > +#define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \
> > + ((kernel_ulong_t)&(struct flash_info) { \
> > + .jedec_id = (_jedec_id), \
> > + .ext_id = (_ext_id), \
> > + .sector_size = (_sector_size), \
> > + .n_sectors = (_n_sectors), \
> > + .flags = (_flags), \
> > + })
>
> Anonymous inlined structures ... kind of ugly, but I can
> understand why you might not want to declare and name a
> few dozen single-use structures.
>
>
> >
> > /* NOTE: double check command sets and memory organization when you add
> > * more flash chips. This current list focusses on newer chips, which
> > * have been converging on command sets which including JEDEC ID.
> > */
> > -static struct flash_info __devinitdata m25p_data [] = {
> > -
> > +static const struct spi_device_id m25p_ids[] = {
> > /* Atmel -- some are (confusingly) marketed as "DataFlash" */
> > - { "at25fs010", 0x1f6601, 0, 32 * 1024, 4, SECT_4K, },
> > - { "at25fs040", 0x1f6604, 0, 64 * 1024, 8, SECT_4K, },
> > + { "at25fs010", INFO(0x1f6601, 0, 32 * 1024, 4, SECT_4K) },
> > + { "at25fs040", INFO(0x1f6604, 0, 64 * 1024, 8, SECT_4K) },
> >
> > ... deletia ...
> >
>
> > @@ -596,6 +602,7 @@ static struct flash_info *__devinit jedec_probe(struct spi_device *spi)
> > */
> > static int __devinit m25p_probe(struct spi_device *spi)
> > {
> > + const struct spi_device_id *id;
> > struct flash_platform_data *data;
> > struct m25p *flash;
> > struct flash_info *info;
> > @@ -608,32 +615,38 @@ static int __devinit m25p_probe(struct spi_device *spi)
> > */
> > data = spi->dev.platform_data;
> > if (data && data->type) {
>
> At this point I wonder why you're not changing the probe sequence
> more. Get "id" and then "id" here. If it's for "m25p80" assume
> it's an old-style board init and do the current logic. Else just
> verify "info".
>
> There's a new error case of course: new-style but data->type
> doesn't match id->name.
>
>
> > - for (i = 0, info = m25p_data;
> > - i < ARRAY_SIZE(m25p_data);
> > - i++, info++) {
> > - if (strcmp(data->type, info->name) == 0)
> > - break;
> > + for (i = 0; i < ARRAY_SIZE(m25p_ids) - 1; i++) {
> > + id = &m25p_ids[i];
> > + info = (void *)m25p_ids[i].driver_data;
> > + if (strcmp(data->type, id->name))
> > + continue;
> > + break;
> > }
> >
> > /* unrecognized chip? */
> > - if (i == ARRAY_SIZE(m25p_data)) {
> > + if (i == ARRAY_SIZE(m25p_ids) - 1) {
>
> Better: "if (info == NULL) ..." You've got all the pointers
> in hand; don't use indices.
>
> > DEBUG(MTD_DEBUG_LEVEL0, "%s: unrecognized id %s\n",
> > dev_name(&spi->dev), data->type);
> > info = NULL;
> >
> > /* recognized; is that chip really what's there? */
> > } else if (info->jedec_id) {
> > - struct flash_info *chip = jedec_probe(spi);
> > + id = jedec_probe(spi);
> >
> > - if (!chip || chip != info) {
> > + if (id != &m25p_ids[i]) {
>
> Again, don't use indices except during the lookup.
>
> > dev_warn(&spi->dev, "found %s, expected %s\n",
> > - chip ? chip->name : "UNKNOWN",
> > - info->name);
> > + id ? id->name : "UNKNOWN",
> > + m25p_ids[i].name);
> > info = NULL;
> > }
> > }
> > - } else
> > - info = jedec_probe(spi);
> > + } else {
> > + id = jedec_probe(spi);
> > + if (!id)
> > + id = spi_get_device_id(spi);
> > +
> > + info = (void *)id->driver_data;
> > + }
> >
> > if (!info)
> > return -ENODEV;
>
--
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