[PATCH V3] mtd: basic (read only) driver for BCMA serial flash

[Jonas Gorski]

I know I'm a bit late, but ...

On 17 September 2012 11:50, Rafał Miłecki <zajec5 at gmail.com> wrote:
> (...)
> diff --git a/drivers/mtd/devices/Makefile b/drivers/mtd/devices/Makefile
> index a4dd1d8..395733a 100644
> --- a/drivers/mtd/devices/Makefile
> +++ b/drivers/mtd/devices/Makefile
> @@ -19,5 +19,6 @@ obj-$(CONFIG_MTD_DATAFLASH)   += mtd_dataflash.o
>  obj-$(CONFIG_MTD_M25P80)       += m25p80.o
>  obj-$(CONFIG_MTD_SPEAR_SMI)    += spear_smi.o
>  obj-$(CONFIG_MTD_SST25L)       += sst25l.o
> +obj-$(CONFIG_MTD_BCM47XXSFLASH)        += bcm47xxsflash.o
>
>  CFLAGS_docg3.o                 += -I$(src)
> \ No newline at end of file
> diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
> new file mode 100644
> index 0000000..f711a51
> --- /dev/null
> +++ b/drivers/mtd/devices/bcm47xxsflash.c
> @@ -0,0 +1,131 @@
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/platform_device.h>
> +#include <linux/bcma/bcma.h>
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Serial flash driver for BCMA bus");
> +
> +static const char *probes[] = { "bcm47xxpart", NULL };
> +
> +static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
> +                             size_t *retlen, u_char *buf)
> +{
> +       struct bcma_sflash *sflash = mtd->priv;
> +       size_t bytes_read = 0;
> +       u8 *src = (u8 *)KSEG0ADDR(sflash->window + from);
> +       int i;
> +       size_t unaligned_before, unaligned_after;
> +
> +       /* Check address range */
> +       if ((from + len) > mtd->size)
> +               return -EINVAL;
> +
> +       unaligned_before = from & 0x3;

I think this should be 4 - (from & 0x3), else you will read too much
in the unaligned_before loop, also being at an unaligned address then.

E.g. assuming I want to read four bytes from offset 3, you will read
three bytes from from, and will actually then read three bytes from 3,
landing at offset 6, and then reading another three in the
unaligned_after loop; overwriting memory with the last two.

> +       unaligned_after = (from + len) & 0x3;
> +
> +       for (i = 0; i < unaligned_before; i++) {
> +               *buf = readb(src);
> +               buf++;
> +               src++;
> +               bytes_read++;
> +       }
> +       for (i = from - unaligned_before; i < from + len - unaligned_after;

This one looks suspicious, aren't you assuming you have read
unaligned_before bytes, so shouldn't i be from /+/ unaligned_before?
Currently this will result in the example above that it will loop once
(since i = 3 - 3 = 0, and 0 < 3 + 4 - 3), resulting in additional 4
bytes read.

> +            i += 4) {
> +               *(u32 *)buf = readl((u32 *)src);
> +               buf += 4;
> +               src += 4;
> +               bytes_read += 4;
> +       }
> +       for (i = 0; i < unaligned_after; i++) {
> +               *buf = readb(src);
> +               buf++;
> +               src++;
> +               bytes_read++;
> +       }

It will also fail for unaligned reads within the same 4 byte block -
they will be read twice, once in _before, once in _after.

Also in the before and after loops, you are reading with native
endianess (since you don't swab the addresses), but in the aligned
case you are assuming little endianess. If you intend to do native
endianess reads (we only support little endian bcm47xx), you should be
fine by just replacing this whole logic with a memcpy(_fromio?), if
not you should probably swab the addresses when reading the unaligned
parts when big endian. Unfortunately I don't have any big endian
bcm47xx devices at hand to test ;-) (but I do know they exist - AFAICT
from netgear's gpl sources the bcm53xxx switch chips are big endian
bcm47xx).


Regards
Jonas