Re: [OpenWrt-Devel] [PATCH v1 0/7] Add custom TRX header option

2015-02-10 Thread Rafał Miłecki 
On 9 February 2015 at 21:52, Florian Fainelli  wrote:
> On 09/02/15 08:29, Will Sheppard wrote:
>> Patchset to essentially add custom TRX header to all firmware produced.
>>
>> This is most useful for the Belkin routers from my experience. I'm not
>> how other trx based firmwares modify the header for their own purposes.
>>
>> This is applied across the board so that the kernel, trx tools and mtd
>> tools are all compiled with the specified header.
>>
>> I have modified the kernel specifically for the brcm47xx based boards to
>> ensure the mtd parser looks for the correct magic. NOTE: This function will,
>> for other boards, fail.
>
> I don't think this is desirable at all to have, for many reasons:

A quick comment: I agree.

This patchset looks hacky to me with all these magics set in config file.
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel

Re: [OpenWrt-Devel] [PATCH v1 0/7] Add custom TRX header option

2015-02-09 Thread Florian Fainelli 
On 09/02/15 13:12, Will Sheppard wrote:
> Hi - thanks for review; reply inline
> 
> On 9 February 2015 at 20:52, Florian Fainelli  > wrote:
> 
> On 09/02/15 08:29, Will Sheppard wrote:
> > Patchset to essentially add custom TRX header to all firmware produced.
> >
> > This is most useful for the Belkin routers from my experience. I'm not
> > how other trx based firmwares modify the header for their own purposes.
> >
> > This is applied across the board so that the kernel, trx tools and mtd
> > tools are all compiled with the specified header.
> >
> > I have modified the kernel specifically for the brcm47xx based boards to
> > ensure the mtd parser looks for the correct magic. NOTE: This function 
> will,
> > for other boards, fail.
> 
> I don't think this is desirable at all to have, for many reasons:
> 
> - how can an use figure out the proper TRX_MAGIC value he/she should
> use?
> 
> 
> There's simply no hard-and-fast rule for this - just looking at a
> factory binary might not help - it worked for me. 

That was more of a rhetorical question, since this is not easy, someone
has to do it, and preferably in a way that makes the build reliable for
end-users.

> 
> 
> - this is extremely error prone and can result, as you mention in
> failing to work with existing devices with well-defined TRX headers
> 
> 
> Error prone inasmuch as it could be the wrong value and thus brick the
> router? ( I guess I'm always thinking from a "have a serial port"
> perspective ) 

Error prone on multiple levels: what happens if I wipe-out my original
config and can't remember the correct 32-bits magic value? What if the
option is renamed, what I upgrade the kernel but not mtd, etc...

> 
> 
> - how can we get some level of reproducibility for both the kernel and
> firmware tools builds?
> 
> I would really encourage you to extend the existing code to take an
> arbitrary list of TRX magics, such that you can have maybe a single file
> to modify and both "mtd" and the kernel can use that list of magic
> values accordingly.
> 
> 
> I agree. As the mtd utility looks for this value though, it would have
> to know what router it was being used on - i'm not sure that is
> available as a value in user-space is it?

You can certainly extract the board you are running on, but that assumes
you already have a working user-space and kernel, hence the partition
parser needs to work etc... chicken and egg. The alternative, which is
likely what is happening today, is that you need to flash a special
image, with a special magic that only supports a particular set of
boards using a defined TRX magic value, that way you can become (at
least at the MTD level) board agnostic.

> 
> 
> Out of curiosity, how many different TRX magic values should we have to
> support with or without your patches?
> 
> 
> I know that Belkin seem to use different magics i.e. not HDR0, I'm not
> sure about other manufacturers. But presumably these values could be
> added as they become known.

That is the direction I would take, just add them one by one such that
you can control exactly when both user-space and kernel space can
properly support a given model (or family of).

> 
> 
> >
> > Will Sheppard (7):
> >   config: Add option to specify custom TRX header
> >   mtd: Add custom trx magic option to mtd tool
> >   mtd: Fix: Use TRX_MAGIC define not hard-coded number
> >   trx: Add custom TRX option to trx firmware tool
> >   mtd: Add debug showing header magic in use
> >   kernel: bcm47xx: Add custom TRX header option to kernel
> >   mtd: Fix makefile to work with quilt as per wiki
> >
> >  config/Config-images.in| 15 ++
> >  package/system/mtd/Makefile|  9 +-
> >  package/system/mtd/src/trx.c   | 10 +--
> >  .../patches-3.14/162-Belkin_custom_trx_magic.patch | 35
> ++
> >  tools/firmware-utils/Makefile  |  4 +++
> >  tools/firmware-utils/src/trx.c | 10 ++-
> >  6 files changed, 79 insertions(+), 4 deletions(-)
> >  create mode 100644
> target/linux/brcm47xx/patches-3.14/162-Belkin_custom_trx_magic.patch
> >
> 
> 
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel

Re: [OpenWrt-Devel] [PATCH v1 0/7] Add custom TRX header option

2015-02-09 Thread Will Sheppard 
Hi - thanks for review; reply inline

On 9 February 2015 at 20:52, Florian Fainelli  wrote:

> On 09/02/15 08:29, Will Sheppard wrote:
> > Patchset to essentially add custom TRX header to all firmware produced.
> >
> > This is most useful for the Belkin routers from my experience. I'm not
> > how other trx based firmwares modify the header for their own purposes.
> >
> > This is applied across the board so that the kernel, trx tools and mtd
> > tools are all compiled with the specified header.
> >
> > I have modified the kernel specifically for the brcm47xx based boards to
> > ensure the mtd parser looks for the correct magic. NOTE: This function
> will,
> > for other boards, fail.
>
> I don't think this is desirable at all to have, for many reasons:
>
> - how can an use figure out the proper TRX_MAGIC value he/she should use?
>

There's simply no hard-and-fast rule for this - just looking at a factory
binary might not help - it worked for me.

>
> - this is extremely error prone and can result, as you mention in
> failing to work with existing devices with well-defined TRX headers
>

Error prone inasmuch as it could be the wrong value and thus brick the
router? ( I guess I'm always thinking from a "have a serial port"
perspective )

>
> - how can we get some level of reproducibility for both the kernel and
> firmware tools builds?
>
> I would really encourage you to extend the existing code to take an
> arbitrary list of TRX magics, such that you can have maybe a single file
> to modify and both "mtd" and the kernel can use that list of magic
> values accordingly.
>

I agree. As the mtd utility looks for this value though, it would have to
know what router it was being used on - i'm not sure that is available as a
value in user-space is it?

>
> Out of curiosity, how many different TRX magic values should we have to
> support with or without your patches?
>

I know that Belkin seem to use different magics i.e. not HDR0, I'm not sure
about other manufacturers. But presumably these values could be added as
they become known.

>
> >
> > Will Sheppard (7):
> >   config: Add option to specify custom TRX header
> >   mtd: Add custom trx magic option to mtd tool
> >   mtd: Fix: Use TRX_MAGIC define not hard-coded number
> >   trx: Add custom TRX option to trx firmware tool
> >   mtd: Add debug showing header magic in use
> >   kernel: bcm47xx: Add custom TRX header option to kernel
> >   mtd: Fix makefile to work with quilt as per wiki
> >
> >  config/Config-images.in| 15 ++
> >  package/system/mtd/Makefile|  9 +-
> >  package/system/mtd/src/trx.c   | 10 +--
> >  .../patches-3.14/162-Belkin_custom_trx_magic.patch | 35
> ++
> >  tools/firmware-utils/Makefile  |  4 +++
> >  tools/firmware-utils/src/trx.c | 10 ++-
> >  6 files changed, 79 insertions(+), 4 deletions(-)
> >  create mode 100644
> target/linux/brcm47xx/patches-3.14/162-Belkin_custom_trx_magic.patch
> >
>
>
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel

Re: [OpenWrt-Devel] [PATCH v1 0/7] Add custom TRX header option

2015-02-09 Thread Florian Fainelli 
On 09/02/15 08:29, Will Sheppard wrote:
> Patchset to essentially add custom TRX header to all firmware produced.
> 
> This is most useful for the Belkin routers from my experience. I'm not
> how other trx based firmwares modify the header for their own purposes.
> 
> This is applied across the board so that the kernel, trx tools and mtd 
> tools are all compiled with the specified header.
> 
> I have modified the kernel specifically for the brcm47xx based boards to
> ensure the mtd parser looks for the correct magic. NOTE: This function will, 
> for other boards, fail.

I don't think this is desirable at all to have, for many reasons:

- how can an use figure out the proper TRX_MAGIC value he/she should use?

- this is extremely error prone and can result, as you mention in
failing to work with existing devices with well-defined TRX headers

- how can we get some level of reproducibility for both the kernel and
firmware tools builds?

I would really encourage you to extend the existing code to take an
arbitrary list of TRX magics, such that you can have maybe a single file
to modify and both "mtd" and the kernel can use that list of magic
values accordingly.

Out of curiosity, how many different TRX magic values should we have to
support with or without your patches?

> 
> Will Sheppard (7):
>   config: Add option to specify custom TRX header
>   mtd: Add custom trx magic option to mtd tool
>   mtd: Fix: Use TRX_MAGIC define not hard-coded number
>   trx: Add custom TRX option to trx firmware tool
>   mtd: Add debug showing header magic in use
>   kernel: bcm47xx: Add custom TRX header option to kernel
>   mtd: Fix makefile to work with quilt as per wiki
> 
>  config/Config-images.in| 15 ++
>  package/system/mtd/Makefile|  9 +-
>  package/system/mtd/src/trx.c   | 10 +--
>  .../patches-3.14/162-Belkin_custom_trx_magic.patch | 35 
> ++
>  tools/firmware-utils/Makefile  |  4 +++
>  tools/firmware-utils/src/trx.c | 10 ++-
>  6 files changed, 79 insertions(+), 4 deletions(-)
>  create mode 100644 
> target/linux/brcm47xx/patches-3.14/162-Belkin_custom_trx_magic.patch
> 
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel

[OpenWrt-Devel] [PATCH v1 0/7] Add custom TRX header option

2015-02-09 Thread Will Sheppard 
Patchset to essentially add custom TRX header to all firmware produced.

This is most useful for the Belkin routers from my experience. I'm not
how other trx based firmwares modify the header for their own purposes.

This is applied across the board so that the kernel, trx tools and mtd 
tools are all compiled with the specified header.

I have modified the kernel specifically for the brcm47xx based boards to
ensure the mtd parser looks for the correct magic. NOTE: This function will, 
for other boards, fail.

Will Sheppard (7):
  config: Add option to specify custom TRX header
  mtd: Add custom trx magic option to mtd tool
  mtd: Fix: Use TRX_MAGIC define not hard-coded number
  trx: Add custom TRX option to trx firmware tool
  mtd: Add debug showing header magic in use
  kernel: bcm47xx: Add custom TRX header option to kernel
  mtd: Fix makefile to work with quilt as per wiki

 config/Config-images.in| 15 ++
 package/system/mtd/Makefile|  9 +-
 package/system/mtd/src/trx.c   | 10 +--
 .../patches-3.14/162-Belkin_custom_trx_magic.patch | 35 ++
 tools/firmware-utils/Makefile  |  4 +++
 tools/firmware-utils/src/trx.c | 10 ++-
 6 files changed, 79 insertions(+), 4 deletions(-)
 create mode 100644 
target/linux/brcm47xx/patches-3.14/162-Belkin_custom_trx_magic.patch

-- 
1.9.1
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel