[OpenWrt-Devel] [PATCH v6 5/6] ar71xx: scan nand ubi partition for ath9k eeprom files

[Chris Blake]

Yousong,

Sorry to come back so fast, but how would moving the skip to hexdump
help the code in any way? For example, we are currently using:

dd bs=1 skip=$offset count=6 if=/dev/$part 2>/dev/null | hexdump -v -n
6 -e '5/1 "%02x:" 1/1 "%02x"'

If we move the skip to hexdump, then we would also need to remove the
count from dd, which means the entire partition is then piped into
hexdump which to me seems to be a lot less efficient than the current
implementation above.

On Mon, Dec 7, 2015 at 9:42 AM, Yousong Zhou <yszhou4tech at gmail.com> wrote:
>
> Am 07.12.2015 22:12 schrieb "Chris Blake" <chrisrblake93 at gmail.com>:
>>
>> Hey Yousong,
>>
>> looking at the hexdump comment, this function was actually based off
>> of the function mtd_get_mac_binary() which uses the same dd offset
>> logic as you can see in mtd_get_mac_binary_ubi(). If this were to be
>> changed, shouldn't the same change apply to  mtd_get_mac_binary()?
>>
>
> Yes, it should
>
>> I know this would require a new patch, but just want to make sure we
>> are on the same page.
>
> Previously, I thought mtd_get_mac_binary_ubi was a new function and not
> aware of its resemblence to mtd_get_mac_binary.  A new patch is good if we
> think it can help improve the code even though it is mostly unrelated in the
> whole series.
>
> And a new nitpick crawls out below...
>
>>
>> Regards,
>> Chris Blake
>>
>> On Sun, Dec 6, 2015 at 10:05 PM, Yousong Zhou <yszhou4tech at gmail.com>
>> wrote:
>> > Hello, Christian, a few nitpicks inline :)
>> >
>> > On 6 December 2015 at 06:48, Christian Lamparter
>> > <chunkeey at googlemail.com> wrote:
>> >> From: Chris R Blake <chrisrblake93 at gmail.com>
>> >>
>> >> The MR18 stores the ath9k eeprom values on the NAND.
>> >> This patch makes it possible to retrieve the images
>> >> from there.
>> >>
>> >> Signed-off-by: Chris R Blake <chrisrblake93 at gmail.com>
>> >> ---
>> >>  package/base-files/files/lib/functions/system.sh        | 17
>> >> +++++++++++++++++
>> >>  .../base-files/etc/hotplug.d/firmware/10-ath9k-eeprom   |  6 +++++-
>> >>  2 files changed, 22 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/package/base-files/files/lib/functions/system.sh
>> >> b/package/base-files/files/lib/functions/system.sh
>> >> index 8d75a5a..928a429 100644
>> >> --- a/package/base-files/files/lib/functions/system.sh
>> >> +++ b/package/base-files/files/lib/functions/system.sh
>> >> @@ -41,6 +41,23 @@ mtd_get_mac_binary() {
>> >>         dd bs=1 skip=$offset count=6 if=$part 2>/dev/null | hexdump -v
>> >> -n 6 -e '5/1 "%02x:" 1/1 "%02x"'
>> >>  }
>> >>
>> >> +mtd_get_mac_binary_ubi() {
>> >> +       local mtdname="$1"
>> >> +       local offset="$2"
>> >> +
>> >> +       . /lib/upgrade/nand.sh
>> >> +
>> >> +       local ubidev=$( nand_find_ubi $CI_UBIPART )
>> >> +       local part="$( nand_find_volume $ubidev $1 )"
>> >
>> > Quotes and padding whitespaces are not need here for consistency of code
>> > style.
>> >
>> >> +
>> >> +       if [ -z "$part" ]; then
>> >> +               echo "mtd_get_mac_binary: ubi partition $mtdname not
>> >> found!" >&2
>
> Here the error message should be reworded.
>
>                 yousong
>
>> >> +               return
>> >> +       fi
>> >> +
>> >> +       dd bs=1 skip=$offset count=6 if=/dev/$part 2>/dev/null |
>> >> hexdump -v -n 6 -e '5/1 "%02x:" 1/1 "%02x"'
>> >
>> > hexdump accepts an argument for offset with -s option
>> >
>> >> +}
>> >> +
>> >>  mtd_get_part_size() {
>> >>         local part_name=$1
>> >>         local first dev size erasesize name
>> >> diff --git
>> >> a/target/linux/ar71xx/base-files/etc/hotplug.d/firmware/10-ath9k-eeprom
>> >> b/target/linux/ar71xx/base-files/etc/hotplug.d/firmware/10-ath9k-eeprom
>> >> index b5f0588..7287809 100644
>> >> ---
>> >> a/target/linux/ar71xx/base-files/etc/hotplug.d/firmware/10-ath9k-eeprom
>> >> +++
>> >> b/target/linux/ar71xx/base-files/etc/hotplug.d/firmware/10-ath9k-eeprom
>> >> @@ -9,11 +9,14 @@ ath9k_eeprom_extract() {
>> >>         local part=$1
>> >>         local offset=$2
>> >>         local count=$3
>> >> +       local ubidev=$( nand_find_ubi $CI_UBIPART )
>> >>         local mtd
>> >>
>> >>         mtd=$(find_mtd_chardev $part)
>> >>         [ -n "$mtd" ] || \
>> >> -               ath9k_eeprom_die "no mtd device found for partition
>> >> $part"
>> >> +               mtd="/dev/$(nand_find_volume $ubidev $part)"
>> >> +               [ -n "$mtd" ] || \
>> >> +                       ath9k_eeprom_die "no mtd device found for
>> >> partition $part"
>> >>
>> >
>> > The indentation here can be misleading
>> >
>> >>         dd if=$mtd of=/lib/firmware/$FIRMWARE bs=1 skip=$offset
>> >> count=$count 2>/dev/null || \
>> >>                 ath9k_eeprom_die "failed to extract from $mtd"
>> >> @@ -32,6 +35,7 @@ ath9k_patch_firmware_mac() {
>> >>  . /lib/ar71xx.sh
>> >>  . /lib/functions.sh
>> >>  . /lib/functions/system.sh
>> >> +. /lib/upgrade/nand.sh
>> >>
>> >
>> > I suggest we move this part including the line "[ -e
>> > /lib/firmware/$FIRMWARE ] && exit 0" to top of this file.
>> >
>> >                 yousong
>> >
>> >
>> >>  board=$(ar71xx_board_name)
>> >>
>> >> --
>> >> 2.6.2
>> >> _______________________________________________
>> >> openwrt-devel mailing list
>> >> openwrt-devel at lists.openwrt.org
>> >> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
>> > _______________________________________________
>> > openwrt-devel mailing list
>> > openwrt-devel at lists.openwrt.org
>> > https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel