Re: [PATCH 1/2] mtd: m25p80: Rework probing/JEDEC code

[Barry Song]

On Wed, Aug 19, 2009 at 5:46 AM, Anton Vorontsov
<avorontsov@xxxxxxxxxxxxx> wrote:
>
> Previosly the driver always tried JEDEC probing, assuming that non-JEDEC
> chips will return '0'. But truly non-JEDEC chips (like CAT25) won't do
> that, their behaviour on RDID command is undefined, so the driver should
> not call jedec_probe() for these chips.
>
> Also, be less strict on error conditions, don't fail to probe if JEDEC
> found a chip that is different from what platform code told, instead
> just print some warnings and use an information obtained via JEDEC. In
This patch caused a problem:
even though the external flash doesn't exist, it will still pass the
probe() and be registerred into kernel and given the partition table.
You may refer to this bug report:
http://blackfin.uclinux.org/gf/project/uclinux-dist/tracker/?action=TrackerItemEdit&tracker_item_id=5975&start=0

> that case we should not trust partitions any longer, but they might be
> still useful (i.e. they could protect some parts of the chip).
>
> Signed-off-by: Anton Vorontsov <avorontsov@xxxxxxxxxxxxx>
> ---
> Âdrivers/mtd/devices/m25p80.c | Â 69 ++++++++++++++++++++++++-----------------
> Â1 files changed, 40 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 0d74b38..b75e319 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -581,6 +581,14 @@ static const struct spi_device_id *__devinit jedec_probe(struct spi_device *spi)
> Â Â Â Âjedec = jedec << 8;
> Â Â Â Âjedec |= id[2];
>
> + Â Â Â /*
> + Â Â Â Â* Some chips (like Numonyx M25P80) have JEDEC and non-JEDEC variants,
> + Â Â Â Â* which depend on technology process. Officially RDID command doesn't
> + Â Â Â Â* exist for non-JEDEC chips, but for compatibility they return ID 0.
> + Â Â Â Â*/
> + Â Â Â if (jedec == 0)
> + Â Â Â Â Â Â Â return NULL;
> +
> Â Â Â Âext_jedec = id[3] << 8 | id[4];
>
> Â Â Â Âfor (tmp = 0; tmp < ARRAY_SIZE(m25p_ids) - 1; tmp++) {
> @@ -602,7 +610,7 @@ static const struct spi_device_id *__devinit jedec_probe(struct spi_device *spi)
> Â*/
> Âstatic int __devinit m25p_probe(struct spi_device *spi)
> Â{
> -    const struct spi_device_id   Â*id;
> +    const struct spi_device_id   Â*id = spi_get_device_id(spi);
>    Âstruct flash_platform_data   Â*data;
>    Âstruct m25p           *flash;
>    Âstruct flash_info        *info;
> @@ -615,41 +623,44 @@ static int __devinit m25p_probe(struct spi_device *spi)
> Â Â Â Â */
> Â Â Â Âdata = spi->dev.platform_data;
> Â Â Â Âif (data && data->type) {
> + Â Â Â Â Â Â Â const struct spi_device_id *plat_id;
> +
> Â Â Â Â Â Â Â Â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))
> + Â Â Â Â Â Â Â Â Â Â Â plat_id = &m25p_ids[i];
> + Â Â Â Â Â Â Â Â Â Â Â if (strcmp(data->type, plat_id->name))
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âcontinue;
> Â Â Â Â Â Â Â Â Â Â Â Âbreak;
> Â Â Â Â Â Â Â Â}
>
> - Â Â Â Â Â Â Â /* unrecognized chip? */
> - Â Â Â Â Â Â Â if (i == ARRAY_SIZE(m25p_ids) - 1) {
> - Â Â Â Â Â Â Â Â Â Â Â 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) {
> - Â Â Â Â Â Â Â Â Â Â Â id = jedec_probe(spi);
> -
> - Â Â Â Â Â Â Â Â Â Â Â if (id != &m25p_ids[i]) {
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â dev_warn(&spi->dev, "found %s, expected %s\n",
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â id ? id->name : "UNKNOWN",
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â m25p_ids[i].name);
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â info = NULL;
> - Â Â Â Â Â Â Â Â Â Â Â }
> - Â Â Â Â Â Â Â }
> - Â Â Â } else {
> - Â Â Â Â Â Â Â id = jedec_probe(spi);
> - Â Â Â Â Â Â Â if (!id)
> - Â Â Â Â Â Â Â Â Â Â Â id = spi_get_device_id(spi);
> -
> - Â Â Â Â Â Â Â info = (void *)id->driver_data;
> + Â Â Â Â Â Â Â if (plat_id)
> + Â Â Â Â Â Â Â Â Â Â Â id = plat_id;
> + Â Â Â Â Â Â Â else
> + Â Â Â Â Â Â Â Â Â Â Â dev_warn(&spi->dev, "unrecognized id %s\n", data->type);
> Â Â Â Â}
>
> - Â Â Â if (!info)
> - Â Â Â Â Â Â Â return -ENODEV;
> + Â Â Â info = (void *)id->driver_data;
> +
> + Â Â Â if (info->jedec_id) {
> + Â Â Â Â Â Â Â const struct spi_device_id *jid;
> +
> + Â Â Â Â Â Â Â jid = jedec_probe(spi);
> + Â Â Â Â Â Â Â if (!jid) {
> + Â Â Â Â Â Â Â Â Â Â Â dev_info(&spi->dev, "non-JEDEC variant of %s\n",
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âid->name);
> + Â Â Â Â Â Â Â } else if (jid != id) {
> + Â Â Â Â Â Â Â Â Â Â Â /*
> + Â Â Â Â Â Â Â Â Â Â Â Â* JEDEC knows better, so overwrite platform ID. We
> + Â Â Â Â Â Â Â Â Â Â Â Â* can't trust partitions any longer, but we'll let
> + Â Â Â Â Â Â Â Â Â Â Â Â* mtd apply them anyway, since some partitions may be
> + Â Â Â Â Â Â Â Â Â Â Â Â* marked read-only, and we don't want to lose that
> + Â Â Â Â Â Â Â Â Â Â Â Â* information, even if it's not 100% accurate.
> + Â Â Â Â Â Â Â Â Â Â Â Â*/
> + Â Â Â Â Â Â Â Â Â Â Â dev_warn(&spi->dev, "found %s, expected %s\n",
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âjid->name, id->name);
> + Â Â Â Â Â Â Â Â Â Â Â id = jid;
> + Â Â Â Â Â Â Â Â Â Â Â info = (void *)jid->driver_data;
> + Â Â Â Â Â Â Â }
> + Â Â Â }
>
> Â Â Â Âflash = kzalloc(sizeof *flash, GFP_KERNEL);
> Â Â Â Âif (!flash)
> --
> 1.6.3.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at Âhttp://vger.kernel.org/majordomo-info.html
> Please read the FAQ at Âhttp://www.tux.org/lkml/
¢éì®&Þ~º&¶¬–+-±éÝ¥Šw®žË±Êâmébžìdz¹Þ)í…æèw*jg¬±¨¶‰šŽŠÝj/êäz¹ÞŠà2ŠÞ¨è­Ú&¢)ß«a¶Úþø®G«éh®æj:+v‰¨Šwè†Ù>Wš±êÞiÛaxPjØm¶ŸÿÃ-»+ƒùdš_