| Message ID | 484f0207cc3f3abe52c2d3ff0e4df1d081dbd3d7.1449350080.git.chunkeey@googlemail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Hello, Christian, a few nitpicks inline :) On 6 December 2015 at 06:48, Christian Lamparter <chunkeey@googlemail.com> wrote: > From: Chris R Blake <chrisrblake93@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@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 > + 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@lists.openwrt.org > https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
On 05/12/2015 23:48:07 CET, Christian Lamparter wrote: > 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 new check will always return true, because $mtd will contain "/dev/" if nand_find_volume cannot find the volume.
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()? I know this would require a new patch, but just want to make sure we are on the same page. Regards, Chris Blake On Sun, Dec 6, 2015 at 10:05 PM, Yousong Zhou <yszhou4tech@gmail.com> wrote: > Hello, Christian, a few nitpicks inline :) > > On 6 December 2015 at 06:48, Christian Lamparter > <chunkeey@googlemail.com> wrote: >> From: Chris R Blake <chrisrblake93@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@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 >> + 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@lists.openwrt.org >> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
Am 07.12.2015 22:12 schrieb "Chris Blake" <chrisrblake93@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@gmail.com> wrote: > > Hello, Christian, a few nitpicks inline :) > > > > On 6 December 2015 at 06:48, Christian Lamparter > > <chunkeey@googlemail.com> wrote: > >> From: Chris R Blake <chrisrblake93@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@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@lists.openwrt.org > >> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel > > _______________________________________________ > > openwrt-devel mailing list > > openwrt-devel@lists.openwrt.org > > https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
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@gmail.com> wrote: > > Am 07.12.2015 22:12 schrieb "Chris Blake" <chrisrblake93@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@gmail.com> >> wrote: >> > Hello, Christian, a few nitpicks inline :) >> > >> > On 6 December 2015 at 06:48, Christian Lamparter >> > <chunkeey@googlemail.com> wrote: >> >> From: Chris R Blake <chrisrblake93@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@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@lists.openwrt.org >> >> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel >> > _______________________________________________ >> > openwrt-devel mailing list >> > openwrt-devel@lists.openwrt.org >> > https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
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@gmail.com> wrote: > > Am 07.12.2015 22:12 schrieb "Chris Blake" <chrisrblake93@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@gmail.com> >> wrote: >> > Hello, Christian, a few nitpicks inline :) >> > >> > On 6 December 2015 at 06:48, Christian Lamparter >> > <chunkeey@googlemail.com> wrote: >> >> From: Chris R Blake <chrisrblake93@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@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@lists.openwrt.org >> >> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel >> > _______________________________________________ >> > openwrt-devel mailing list >> > openwrt-devel@lists.openwrt.org >> > https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
On 7 December 2015 at 23:55, Chris Blake <chrisrblake93@gmail.com> wrote: > 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. > I used the following command before for fetching MAC addresses from mtd devices. Not sure if it also works for ubi devices... hexdump -v -n 6 -s 0x1fc00 -e '5/1 "%02x:" 1/1 "%02x""\n"' /dev/mtd2 P.S. please avoid top posting Cheers, yousong > On Mon, Dec 7, 2015 at 9:42 AM, Yousong Zhou <yszhou4tech@gmail.com> wrote: >> >> Am 07.12.2015 22:12 schrieb "Chris Blake" <chrisrblake93@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@gmail.com> >>> wrote: >>> > Hello, Christian, a few nitpicks inline :) >>> > >>> > On 6 December 2015 at 06:48, Christian Lamparter >>> > <chunkeey@googlemail.com> wrote: >>> >> From: Chris R Blake <chrisrblake93@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@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@lists.openwrt.org >>> >> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel >>> > _______________________________________________ >>> > openwrt-devel mailing list >>> > openwrt-devel@lists.openwrt.org >>> > https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
On Mon, Dec 7, 2015 at 7:48 PM, Yousong Zhou <yszhou4tech@gmail.com> wrote: > On 7 December 2015 at 23:55, Chris Blake <chrisrblake93@gmail.com> wrote: >> 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. >> > > I used the following command before for fetching MAC addresses from > mtd devices. Not sure if it also works for ubi devices... > > hexdump -v -n 6 -s 0x1fc00 -e '5/1 "%02x:" 1/1 "%02x""\n"' /dev/mtd2 > > P.S. please avoid top posting > > Cheers, > yousong > > Will do, and thanks for the advice. As for UBI I was able to get it working, so expect all requested changes to be in the next patch revision. (along with a patch to fix up mtd_get_mac_binary) Thanks again! - Chris Blake >> On Mon, Dec 7, 2015 at 9:42 AM, Yousong Zhou <yszhou4tech@gmail.com> wrote: >>> >>> Am 07.12.2015 22:12 schrieb "Chris Blake" <chrisrblake93@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@gmail.com> >>>> wrote: >>>> > Hello, Christian, a few nitpicks inline :) >>>> > >>>> > On 6 December 2015 at 06:48, Christian Lamparter >>>> > <chunkeey@googlemail.com> wrote: >>>> >> From: Chris R Blake <chrisrblake93@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@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@lists.openwrt.org >>>> >> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel >>>> > _______________________________________________ >>>> > openwrt-devel mailing list >>>> > openwrt-devel@lists.openwrt.org >>>> > https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
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 )" + + if [ -z "$part" ]; then + echo "mtd_get_mac_binary: ubi partition $mtdname not found!" >&2 + 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"' +} + 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" 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 board=$(ar71xx_board_name)