[PATCH V2 0/4] mtd: bcm47xxpart.c: Extra TRX magics, NVRAM part handling

[PATCH V2 0/4] mtd: bcm47xxpart.c: Extra TRX magics, NVRAM part handling

[Joseph East]

A set of four patches to upstream from the OpenWrt project

Based off of dev.openwrt.org repo r44854

<trunk/target/linux/generic/patches-3.18>
431-mtd-bcm47xx.part-support-for-Xaiomi-specific-board_da.patch
432-mtd-bcm47xx.part-detect-T_Meter_partition_da.patch

<trunk/target/linux/brcm47xx/patches-3.18>
400-mtd-bcm47xxpart-get-nvram.patch

Joseph East (4):
  mtd: bcm47xxpart.c: NVRAM partition handling
  mtd: bcm47xxpart.c: Adding Xaiomi board support
  mtd: bcm47xxpart.c: Adding T_Meter partition detection
  mtd: bcm47xxpart.c: Adding Belkin TRX signatures (Play max series)

 drivers/mtd/bcm47xxpart.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 43 insertions(+), 2 deletions(-)

-- 
2.4.6

[PATCH V2 1/4] mtd: bcm47xxpart.c: NVRAM partition handling

[Joseph East]

---
 drivers/mtd/bcm47xxpart.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c
index c0720c1..55a1462 100644
--- a/drivers/mtd/bcm47xxpart.c
+++ b/drivers/mtd/bcm47xxpart.c
@@ -95,6 +95,7 @@ static int bcm47xxpart_parse(struct mtd_info *master,
 	int trx_part = -1;
 	int last_trx_part = -1;
 	int possible_nvram_sizes[] = { 0x8000, 0xF000, 0x10000, };
+	bool found_nvram = false;
 
 	/*
 	 * Some really old flashes (like AT45DB*) had smaller erasesize-s, but
@@ -288,12 +289,23 @@ static int bcm47xxpart_parse(struct mtd_info *master,
 		if (buf[0] == NVRAM_HEADER) {
 			bcm47xxpart_add_part(&parts[curr_part++], "nvram",
 					     master->size - blocksize, 0);
+			found_nvram = true;
 			break;
 		}
 	}
 
 	kfree(buf);
 
+	if (!found_nvram) {
+		pr_warning("Cannot find a nvram partition, reserving last two blocks\n");
+		bcm47xxpart_add_part(&parts[curr_part++], "nvram_guess",
+				     master->size - blocksize * 2, MTD_WRITEABLE);
+		for (i = 0; i < curr_part; i++) {
+			if (parts[i].size + parts[i].offset == master->size)
+				parts[i].offset -= blocksize * 2;
+		}
+	}
+
 	/*
 	 * Assume that partitions end at the beginning of the one they are
 	 * followed by.
-- 
2.4.6

Re: [PATCH V2 1/4] mtd: bcm47xxpart.c: NVRAM partition handling

[Rafał Miłecki]

On 25 July 2015 at 16:37, Joseph East <eastyjr@gmail.com> wrote:
> ---

You didn't describe your change, you didn't signed off the patch.


>  drivers/mtd/bcm47xxpart.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c
> index c0720c1..55a1462 100644
> --- a/drivers/mtd/bcm47xxpart.c
> +++ b/drivers/mtd/bcm47xxpart.c
> @@ -95,6 +95,7 @@ static int bcm47xxpart_parse(struct mtd_info *master,
>         int trx_part = -1;
>         int last_trx_part = -1;
>         int possible_nvram_sizes[] = { 0x8000, 0xF000, 0x10000, };
> +       bool found_nvram = false;
>
>         /*
>          * Some really old flashes (like AT45DB*) had smaller erasesize-s, but
> @@ -288,12 +289,23 @@ static int bcm47xxpart_parse(struct mtd_info *master,
>                 if (buf[0] == NVRAM_HEADER) {
>                         bcm47xxpart_add_part(&parts[curr_part++], "nvram",
>                                              master->size - blocksize, 0);
> +                       found_nvram = true;
>                         break;
>                 }
>         }
>
>         kfree(buf);
>
> +       if (!found_nvram) {
> +               pr_warning("Cannot find a nvram partition, reserving last two blocks\n");
> +               bcm47xxpart_add_part(&parts[curr_part++], "nvram_guess",
> +                                    master->size - blocksize * 2, MTD_WRITEABLE);
> +               for (i = 0; i < curr_part; i++) {
> +                       if (parts[i].size + parts[i].offset == master->size)
> +                               parts[i].offset -= blocksize * 2;
> +               }
> +       }
> +

I'm not asking "what for", because it should be in a commit message.
Waiting for it.

One problem I can see: it won't work well on devices with multiple
flashes. Is it expected for NVRAM to be located only on a one flash
memory.

-- 
Rafał

Re: [PATCH V2 1/4] mtd: bcm47xxpart.c: NVRAM partition handling

[Rafał Miłecki]

On 25 July 2015 at 16:37, Joseph East <eastyjr@gmail.com> wrote:
> ---
>  drivers/mtd/bcm47xxpart.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)

Wait... this change seems identical to Hauke's patch. So are you
resending somebodys patch pretending its your own?

Nack.

[PATCH V2 2/4] mtd: bcm47xxpart.c: Adding Xaiomi board support

[Joseph East]

---
 drivers/mtd/bcm47xxpart.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c
index 55a1462..a27d185 100644
--- a/drivers/mtd/bcm47xxpart.c
+++ b/drivers/mtd/bcm47xxpart.c
@@ -33,6 +33,7 @@
 /* Magics */
 #define BOARD_DATA_MAGIC		0x5246504D	/* MPFR */
 #define BOARD_DATA_MAGIC2		0xBD0D0BBD
+#define BOARD_DATA_XIAOMI_MAGIC	0x474D4442	/* GMDB */
 #define CFE_MAGIC			0x43464531	/* 1EFC */
 #define FACTORY_MAGIC			0x59544346	/* FCTY */
 #define NVRAM_HEADER			0x48534C46	/* FLSH */
@@ -263,7 +264,8 @@ static int bcm47xxpart_parse(struct mtd_info *master,
 		}
 
 		/* Some devices (ex. WNDR3700v3) don't have a standard 'MPFR' */
-		if (buf[0x000 / 4] == BOARD_DATA_MAGIC2) {
+		if (buf[0x000 / 4] == BOARD_DATA_MAGIC2 ||
+		    le32_to_cpu(buf[0x000 / 4]) == BOARD_DATA_XIAOMI_MAGIC) {
 			bcm47xxpart_add_part(&parts[curr_part++], "board_data",
 					     offset, MTD_WRITEABLE);
 			continue;
-- 
2.4.6

Re: [PATCH V2 2/4] mtd: bcm47xxpart.c: Adding Xaiomi board support

[Rafał Miłecki]

On 25 July 2015 at 16:37, Joseph East <eastyjr@gmail.com> wrote:
> ---

Oh and now you're sending my patch pretending its your. NACK!


>  drivers/mtd/bcm47xxpart.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c
> index 55a1462..a27d185 100644
> --- a/drivers/mtd/bcm47xxpart.c
> +++ b/drivers/mtd/bcm47xxpart.c
> @@ -33,6 +33,7 @@
>  /* Magics */
>  #define BOARD_DATA_MAGIC               0x5246504D      /* MPFR */
>  #define BOARD_DATA_MAGIC2              0xBD0D0BBD
> +#define BOARD_DATA_XIAOMI_MAGIC        0x474D4442      /* GMDB */
>  #define CFE_MAGIC                      0x43464531      /* 1EFC */
>  #define FACTORY_MAGIC                  0x59544346      /* FCTY */
>  #define NVRAM_HEADER                   0x48534C46      /* FLSH */
> @@ -263,7 +264,8 @@ static int bcm47xxpart_parse(struct mtd_info *master,
>                 }
>
>                 /* Some devices (ex. WNDR3700v3) don't have a standard 'MPFR' */
> -               if (buf[0x000 / 4] == BOARD_DATA_MAGIC2) {
> +               if (buf[0x000 / 4] == BOARD_DATA_MAGIC2 ||
> +                   le32_to_cpu(buf[0x000 / 4]) == BOARD_DATA_XIAOMI_MAGIC) {

le32_to_cpu won't work here obviously, so it seems you didn't even
test it on a proper device. Nack.

-- 
Rafał

Re: [PATCH V2 2/4] mtd: bcm47xxpart.c: Adding Xaiomi board support

[Joseph East]

On 26/07/2015 4:35 PM, Rafał Miłecki wrote:
> On 25 July 2015 at 16:37, Joseph East <eastyjr@gmail.com> wrote:
>> ---
> 
> Oh and now you're sending my patch pretending its your. NACK!
> 

The cover letter [PATCH 0/4] clearly states the origin of these patches.

The sign-off indicates that I am compliant with http://developercertificate.org/, in particular clause b)

I do not take credit for the creation for all of these patches, but I am pushing them.

I apologise if it appears anything was untoward, but I could not find any trace of these patches being pushed elsewhere.

>>  drivers/mtd/bcm47xxpart.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c
>> index 55a1462..a27d185 100644
>> --- a/drivers/mtd/bcm47xxpart.c
>> +++ b/drivers/mtd/bcm47xxpart.c
>> @@ -33,6 +33,7 @@
>>  /* Magics */
>>  #define BOARD_DATA_MAGIC               0x5246504D      /* MPFR */
>>  #define BOARD_DATA_MAGIC2              0xBD0D0BBD
>> +#define BOARD_DATA_XIAOMI_MAGIC        0x474D4442      /* GMDB */
>>  #define CFE_MAGIC                      0x43464531      /* 1EFC */
>>  #define FACTORY_MAGIC                  0x59544346      /* FCTY */
>>  #define NVRAM_HEADER                   0x48534C46      /* FLSH */
>> @@ -263,7 +264,8 @@ static int bcm47xxpart_parse(struct mtd_info *master,
>>                 }
>>
>>                 /* Some devices (ex. WNDR3700v3) don't have a standard 'MPFR' */
>> -               if (buf[0x000 / 4] == BOARD_DATA_MAGIC2) {
>> +               if (buf[0x000 / 4] == BOARD_DATA_MAGIC2 ||
>> +                   le32_to_cpu(buf[0x000 / 4]) == BOARD_DATA_XIAOMI_MAGIC) {
> 
> le32_to_cpu won't work here obviously, so it seems you didn't even
> test it on a proper device. Nack.

This patch has been in OpenWrt for 6 months which you apparently signed off.

https://dev.openwrt.org/browser/trunk/target/linux/generic/patches-3.18/431-mtd-bcm47xxpart-support-for-Xiaomi-specific-board_da.patch

While true I have not tested this code path, why has this not been raised earlier?

At the very least it does not prevent compilation for other brcm47xx targets.

That does not justify its inclusion however, and I have no issues with this patch being dropped.

Regards,
Joseph

Re: [PATCH V2 2/4] mtd: bcm47xxpart.c: Adding Xaiomi board support

[Rafał Miłecki]

On 26 July 2015 at 11:45, Joseph East <eastyjr@gmail.com> wrote:
> On 26/07/2015 4:35 PM, Rafał Miłecki wrote:
>> On 25 July 2015 at 16:37, Joseph East <eastyjr@gmail.com> wrote:
>>> ---
>>
>> Oh and now you're sending my patch pretending its your. NACK!
>>
>
> The cover letter [PATCH 0/4] clearly states the origin of these patches.

And it doesn't go into git history at all.


> The sign-off indicates that I am compliant with http://developercertificate.org/, in particular clause b)

There isn't any S-o-b for this patch. Check it yourself.


> I do not take credit for the creation for all of these patches, but I am pushing them.

So why did you drop "From" and Signed-of-by from original patch?


> I apologise if it appears anything was untoward, but I could not find any trace of these patches being pushed elsewhere.

This could be for a good reason, e.g. patch not being tested or clean enough.

So 2 quite important rules:
1) Respect patch author, don't drop him from "From" and his S-o-b
2) Don't send patches you don't understand


>>>  drivers/mtd/bcm47xxpart.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c
>>> index 55a1462..a27d185 100644
>>> --- a/drivers/mtd/bcm47xxpart.c
>>> +++ b/drivers/mtd/bcm47xxpart.c
>>> @@ -33,6 +33,7 @@
>>>  /* Magics */
>>>  #define BOARD_DATA_MAGIC               0x5246504D      /* MPFR */
>>>  #define BOARD_DATA_MAGIC2              0xBD0D0BBD
>>> +#define BOARD_DATA_XIAOMI_MAGIC        0x474D4442      /* GMDB */
>>>  #define CFE_MAGIC                      0x43464531      /* 1EFC */
>>>  #define FACTORY_MAGIC                  0x59544346      /* FCTY */
>>>  #define NVRAM_HEADER                   0x48534C46      /* FLSH */
>>> @@ -263,7 +264,8 @@ static int bcm47xxpart_parse(struct mtd_info *master,
>>>                 }
>>>
>>>                 /* Some devices (ex. WNDR3700v3) don't have a standard 'MPFR' */
>>> -               if (buf[0x000 / 4] == BOARD_DATA_MAGIC2) {
>>> +               if (buf[0x000 / 4] == BOARD_DATA_MAGIC2 ||
>>> +                   le32_to_cpu(buf[0x000 / 4]) == BOARD_DATA_XIAOMI_MAGIC) {
>>
>> le32_to_cpu won't work here obviously, so it seems you didn't even
>> test it on a proper device. Nack.
>
> This patch has been in OpenWrt for 6 months which you apparently signed off.
>
> https://dev.openwrt.org/browser/trunk/target/linux/generic/patches-3.18/431-mtd-bcm47xxpart-support-for-Xiaomi-specific-board_da.patch
>
> While true I have not tested this code path, why has this not been raised earlier?
>
> At the very least it does not prevent compilation for other brcm47xx targets.
>
> That does not justify its inclusion however, and I have no issues with this patch being dropped.

It's common that someone reports a problem to OpenWrt team, we try to
fix it and then wait for testing it for ever. That was the case. After
few months I can see problems with this patch but I guess it should
probably just get dropped since it has no interest at all.

-- 
Rafał

[PATCH V2 3/4] mtd: bcm47xxpart.c: Adding T_Meter partition detection

[Joseph East]

---
 drivers/mtd/bcm47xxpart.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c
index a27d185..ef6db73 100644
--- a/drivers/mtd/bcm47xxpart.c
+++ b/drivers/mtd/bcm47xxpart.c
@@ -39,6 +39,7 @@
 #define NVRAM_HEADER			0x48534C46	/* FLSH */
 #define POT_MAGIC1			0x54544f50	/* POTT */
 #define POT_MAGIC2			0x504f		/* OP */
+#define T_METER_MAGIC			0x4D540000	/* MT */
 #define ML_MAGIC1			0x39685a42
 #define ML_MAGIC2			0x26594131
 #define TRX_MAGIC			0x30524448
@@ -178,6 +179,15 @@ static int bcm47xxpart_parse(struct mtd_info *master,
 			continue;
 		}
 
+		/* T_Meter */
+		if ((le32_to_cpu(buf[0x000 / 4]) & 0xFFFF0000) == T_METER_MAGIC &&
+		    (le32_to_cpu(buf[0x030 / 4]) & 0xFFFF0000) == T_METER_MAGIC &&
+		    (le32_to_cpu(buf[0x060 / 4]) & 0xFFFF0000) == T_METER_MAGIC) {
+			bcm47xxpart_add_part(&parts[curr_part++], "T_Meter", offset,
+					     MTD_WRITEABLE);
+			continue;
+		}
+
 		/* TRX */
 		if (buf[0x000 / 4] == TRX_MAGIC) {
 			if (BCM47XXPART_MAX_PARTS - curr_part < 4) {
-- 
2.4.6

[PATCH V2 4/4] mtd: bcm47xxpart.c: Adding Belkin TRX signatures (Play max series)

[Joseph East]

---
 drivers/mtd/bcm47xxpart.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c
index ef6db73..669e24b 100644
--- a/drivers/mtd/bcm47xxpart.c
+++ b/drivers/mtd/bcm47xxpart.c
@@ -45,6 +45,10 @@
 #define TRX_MAGIC			0x30524448
 #define SHSQ_MAGIC			0x71736873	/* shsq (weird ZTE H218N endianness) */
 #define UBI_EC_MAGIC			0x23494255	/* UBI# */
+#define BELKIN_F7D3301_MAGIC		0x20100322	/* Belkin TRX */
+#define BELKIN_F7D3302_MAGIC		0x20090928
+#define BELKIN_F7D4302_MAGIC		0x20101006
+#define BELKIN_F7D4401_MAGIC		0x00018517
 
 struct trx_header {
 	uint32_t magic;
@@ -55,6 +59,19 @@ struct trx_header {
 	uint32_t offset[3];
 } __packed;
 
+static bool is_trx_magic(uint32_t magic) {
+	switch (magic) {
+	case TRX_MAGIC:
+	case BELKIN_F7D3301_MAGIC:
+	case BELKIN_F7D3302_MAGIC:
+	case BELKIN_F7D4302_MAGIC:
+	case BELKIN_F7D4401_MAGIC:
+		return true;
+	default:
+		return false;
+	}
+}
+
 static void bcm47xxpart_add_part(struct mtd_partition *part, const char *name,
 				 u64 offset, uint32_t mask_flags)
 {
@@ -189,7 +206,7 @@ static int bcm47xxpart_parse(struct mtd_info *master,
 		}
 
 		/* TRX */
-		if (buf[0x000 / 4] == TRX_MAGIC) {
+		if (is_trx_magic(buf[0x000 / 4])) {
 			if (BCM47XXPART_MAX_PARTS - curr_part < 4) {
 				pr_warn("Not enough partitions left to register trx, scanning stopped!\n");
 				break;
-- 
2.4.6