2017-06-09 01:42:47

by Brian Norris

[permalink] [raw]
Subject: Re: [PATCH v3 1/9] mtd: sharpslpart: add sharpslpart MTD partition parser

Oh, one more thing:

On Thu, Jun 01, 2017 at 12:40:50AM +0200, Andrea Adami wrote:
> diff --git a/drivers/mtd/sharpslpart.c b/drivers/mtd/sharpslpart.c
> new file mode 100644
> index 0000000..40aebe9
> --- /dev/null
> +++ b/drivers/mtd/sharpslpart.c
> @@ -0,0 +1,132 @@
> +/*
> + * MTD partition parser for NAND flash on Sharp SL Series
> + *
> + * Copyright (C) 2017 Andrea Adami <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/partitions.h>
> +#include "sharpsl_ftl.h"
> +
> +/* factory defaults */
> +#define SHARPSL_NAND_PARTS 3
> +#define SHARPSL_FTL_PARTITION_SIZE (7 * 1024 * 1024)
> +#define PARAM_BLOCK_PARTITIONINFO1 0x00060000
> +#define PARAM_BLOCK_PARTITIONINFO2 0x00064000
> +
> +#define BOOT_MAGIC be32_to_cpu(0x424f4f54) /* BOOT */
> +#define FSRO_MAGIC be32_to_cpu(0x4653524f) /* FSRO */
> +#define FSRW_MAGIC be32_to_cpu(0x46535257) /* FSRW */

[...]

> +struct sharpsl_nand_partitioninfo {
> + u32 start;
> + u32 end;

I was going to ignore this earlier, but I just noticed that you handle
endianness for the magic values above, so...

shouldn't you handle endianness for the start and end fields too?

> + u32 magic;
> + u32 reserved;
> +};
[...]

Brian


2017-06-20 08:56:38

by Andrea Adami

[permalink] [raw]
Subject: Re: [PATCH v3 1/9] mtd: sharpslpart: add sharpslpart MTD partition parser

On Fri, Jun 9, 2017 at 3:42 AM, Brian Norris
<[email protected]> wrote:
> Oh, one more thing:
>
> On Thu, Jun 01, 2017 at 12:40:50AM +0200, Andrea Adami wrote:
>> diff --git a/drivers/mtd/sharpslpart.c b/drivers/mtd/sharpslpart.c
>> new file mode 100644
>> index 0000000..40aebe9
>> --- /dev/null
>> +++ b/drivers/mtd/sharpslpart.c
>> @@ -0,0 +1,132 @@
>> +/*
>> + * MTD partition parser for NAND flash on Sharp SL Series
>> + *
>> + * Copyright (C) 2017 Andrea Adami <[email protected]>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/slab.h>
>> +#include <linux/module.h>
>> +#include <linux/mtd/mtd.h>
>> +#include <linux/mtd/partitions.h>
>> +#include "sharpsl_ftl.h"
>> +
>> +/* factory defaults */
>> +#define SHARPSL_NAND_PARTS 3
>> +#define SHARPSL_FTL_PARTITION_SIZE (7 * 1024 * 1024)
>> +#define PARAM_BLOCK_PARTITIONINFO1 0x00060000
>> +#define PARAM_BLOCK_PARTITIONINFO2 0x00064000
>> +
>> +#define BOOT_MAGIC be32_to_cpu(0x424f4f54) /* BOOT */
>> +#define FSRO_MAGIC be32_to_cpu(0x4653524f) /* FSRO */
>> +#define FSRW_MAGIC be32_to_cpu(0x46535257) /* FSRW */
>
> [...]
>
>> +struct sharpsl_nand_partitioninfo {
>> + u32 start;
>> + u32 end;
>
> I was going to ignore this earlier, but I just noticed that you handle
> endianness for the magic values above, so...
>
> shouldn't you handle endianness for the start and end fields too?
>
>> + u32 magic;
>> + u32 reserved;
>> +};
> [...]
>
> Brian

Brian,
I have added a macro so to avoid to repeat le32_to_cpu() for each value.
This seems even to work as intended...

Note that the magics are 'texts' so they appear as BE.
Some more tests and I'll send v4 of the patch to your attention.

Thanks in advance.


Andrea