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

[Chris Blake]

Hey Yousong,

Thanks for the clarification, will get this all cleaned up for the
next patch revision. Keep the feedback coming! :)

Regards,
Chris Blake

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