| Message ID | 1406641544-27372-4-git-send-email-l.majewski@samsung.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
On 07/29/2014 07:45 AM, Lukasz Majewski wrote: > This commit adds new test for UMS USB gadget to u-boot mainline tree. > It it similar in operation to the one already available in test/dfu > directory. Patches 1 and 2, Acked-by: Stephen Warren <swarren@nvidia.com> For this patch, I wonder whether: a) Should it do some raw dd tests too, for partitions/devices without a filesystem? Perhaps we should have separate scripts (for each of DFU and UMS) for filesystem and raw testing. So, this could be addressed later. b) Should this script (optionally?) create the filesystem itself, so the test is completely self-contained. Otherwise, the user has to manually run e.g. parted and mk*fs themselves, by which time they've already tested UMS a fair bit without this script. c) Do we really need the "Y" parameters (filesystem type) to the script? "mount" will automatically try all known filesystem types on my Linux host at least, and it would make it simpler to run the script if you simply dropped the "-t" option to "mount". > diff --git a/test/ums/README b/test/ums/README > +Example usage: > +1. On the target: > + create UMS exportable partition with a proper file system created on > + it (e.g. EXT4, FAT). > + ums 0 mmc 0 > +2. On the host: > + sudo test/ums/ums_gadget_test.sh X Y dir [test_file] > + e.g. sudo test/ums/ums_gadget_test.sh 1 vfat /mnt ./dat_14M.img can you s/X/PARTNUM/ s/Y/fstype/ here. That'd make the example command a bit more explanatory even without the paragraph below that explains what X and Y are, and also make it easier to correlate the description with the command. > + > +... where X is the partition number on which UMS operates and the Y is > +the file system. The 'dir' parameter is the mount directory on the HOST. > +Information about available partitions one can read from target via the > +'mmc part' command. Perhaps this should say: 'mmc part' or 'part list' commands. > +The optional [test_file] parameter is for specifying the exact test file > +to use. > \ No newline at end of file That's probably not right. > diff --git a/test/ums/ums_gadget_test.sh b/test/ums/ums_gadget_test.sh > +../dfu/dfu_gadget_test_init.sh 33M 97M I'm just curious what's special about those two sizes. > +ums_test_file () { > + mount -t $2 /dev/$MEM_DEV $MNT_DIR > + if [ -f $MNT_DIR/dat_* ]; then > + rm $MNT_DIR/dat_* > + fi > + sync > + > + cp ./$1 $MNT_DIR > + sync > + umount $MNT_DIR I'm not sure any of those "sync"s are necessary; "mount" should sync as part of its own operation. > + MD5_TX=$MD5SUM > + sleep 1 Why do we need to sleep? > +if [ $# -lt 3 ]; then > + echo "Wrong number of arguments" > + echo "Example:" > + echo "sudo ./ums_gadget_test.sh 1 vfat /mnt [test_file]" > + die > +fi > + > +MNT_DIR=$3 I think here, we should assign all positional arguments to named variables rather than using $1/... later on; something like: PART_NUM=$1; shift FSTYPE=$1; shift MNT_DIR=$1; shift TEST_FILES=$@ For MNT_DIR, can we simply create a temporary directory (e.g. /mnt/tmp-ums-test-$$) so the user doesn't have to pass in the name? The script requires root after all. > +MEM_DEV=`dmesg | tail -n 10 | grep -E " sd[a-z]:" - | cut -d ':' -f 1` > +MEM_DEV=$(echo $MEM_DEV | cut -d ']' -f2 | tr -d ' ') May as well use `` or $() consistently for those two lines. This seems slightly dangerous; what if my system has been plugged in for a long time and I've plugged in some other USB storage device since. Better to take the device name on the command-line, or perhaps to take the USB VID/PID on the command-line, then scan sysfs for a USB device with matching VID/PID, and find out what device node is hosted by it. > \ No newline at end of file Probably should fix that too. Can you add a trap handler so that if the user CTRL-C's the script, the disk is unmounted, the mount directory is removed (if you make the script create one internally), and any temporary files are deleted. Right now, cleanup only runs if the script successfully runs to the end.
Hi Stephen, > On 07/29/2014 07:45 AM, Lukasz Majewski wrote: > > This commit adds new test for UMS USB gadget to u-boot mainline > > tree. It it similar in operation to the one already available in > > test/dfu directory. > > Patches 1 and 2, > Acked-by: Stephen Warren <swarren@nvidia.com> > > For this patch, I wonder whether: > > a) Should it do some raw dd tests too, for partitions/devices without > a filesystem? Perhaps we should have separate scripts (for each of > DFU and UMS) for filesystem and raw testing. So, this could be > addressed later. I think that we should prepare separate scripts. One script per tested functionality. > > b) Should this script (optionally?) create the filesystem itself, so > the test is completely self-contained. Otherwise, the user has to > manually run e.g. parted and mk*fs themselves, by which time they've > already tested UMS a fair bit without this script. The test should be extended to accept an additional flat - e.g. --create_fs. As you pointed out, this could save some time. > > c) Do we really need the "Y" parameters (filesystem type) to the > script? "mount" will automatically try all known filesystem types on > my Linux host at least, and it would make it simpler to run the > script if you simply dropped the "-t" option to "mount". I agree. The Y parameter will be removed and we will allow mount to do the job. > > > diff --git a/test/ums/README b/test/ums/README > > > +Example usage: > > +1. On the target: > > + create UMS exportable partition with a proper file system > > created on > > + it (e.g. EXT4, FAT). > > + ums 0 mmc 0 > > +2. On the host: > > + sudo test/ums/ums_gadget_test.sh X Y dir [test_file] > > + e.g. sudo test/ums/ums_gadget_test.sh 1 vfat /mnt ./dat_14M.img > > can you s/X/PARTNUM/ s/Y/fstype/ here. That'd make the example > command a bit more explanatory even without the paragraph below that > explains what X and Y are, and also make it easier to correlate the > description with the command. Ok, I will do that. > > > + > > +... where X is the partition number on which UMS operates and the > > Y is +the file system. The 'dir' parameter is the mount directory > > on the HOST. +Information about available partitions one can read > > from target via the +'mmc part' command. > > Perhaps this should say: > 'mmc part' or 'part list' commands. Ok. > > > +The optional [test_file] parameter is for specifying the exact > > test file +to use. > > \ No newline at end of file > > That's probably not right. Ok. > > > diff --git a/test/ums/ums_gadget_test.sh > > b/test/ums/ums_gadget_test.sh > > > +../dfu/dfu_gadget_test_init.sh 33M 97M > > I'm just curious what's special about those two sizes. Nothing special. I just wanted to have sufficiently large files to test UMS capabilities (since I assume, that DFU will not transfer so large files very often). With UMS it is not so important to test the corner cases values (as we did with DFU tests), but to check how well transfers of large files is performed. > > > +ums_test_file () { > > > + mount -t $2 /dev/$MEM_DEV $MNT_DIR > > + if [ -f $MNT_DIR/dat_* ]; then > > + rm $MNT_DIR/dat_* > > + fi > > + sync > > + > > + cp ./$1 $MNT_DIR > > + sync > > + umount $MNT_DIR > > I'm not sure any of those "sync"s are necessary; "mount" should sync > as part of its own operation. Yes, I agree. mount/umount do the sync as well. I will fix this. > > > + MD5_TX=$MD5SUM > > + sleep 1 > > Why do we need to sleep? On my linux box there were some problems without this delay (probably caused by time needed for plugging in USB device). Maybe on your setup it will work smoothly. > > > +if [ $# -lt 3 ]; then > > + echo "Wrong number of arguments" > > + echo "Example:" > > + echo "sudo ./ums_gadget_test.sh 1 vfat /mnt [test_file]" > > + die > > +fi > > + > > +MNT_DIR=$3 > > I think here, we should assign all positional arguments to named > variables rather than using $1/... later on; something like: > > PART_NUM=$1; shift > FSTYPE=$1; shift > MNT_DIR=$1; shift > TEST_FILES=$@ > Ok. > For MNT_DIR, can we simply create a temporary directory (e.g. > /mnt/tmp-ums-test-$$) so the user doesn't have to pass in the name? > The script requires root after all. Ok. > > > +MEM_DEV=`dmesg | tail -n 10 | grep -E " sd[a-z]:" - | cut -d ':' > > -f 1` +MEM_DEV=$(echo $MEM_DEV | cut -d ']' -f2 | tr -d ' ') > > May as well use `` or $() consistently for those two lines. According to the reply written below, we will not need this code. > > This seems slightly dangerous; what if my system has been plugged in > for a long time and I've plugged in some other USB storage device > since. Better to take the device name on the command-line, or perhaps > to take the USB VID/PID on the command-line, then scan sysfs for a > USB device with matching VID/PID, and find out what device node is > hosted by it. Your proposition is more reliable. I think that the idProduct/idVendor shall be passed to the script. > > > \ No newline at end of file > > Probably should fix that too. Ok. > > Can you add a trap handler so that if the user CTRL-C's the script, > the disk is unmounted, the mount directory is removed (if you make > the script create one internally), and any temporary files are > deleted. Right now, cleanup only runs if the script successfully runs > to the end. Good idea. I will add this to v2.
diff --git a/test/ums/README b/test/ums/README new file mode 100644 index 0000000..cb7b27d --- /dev/null +++ b/test/ums/README @@ -0,0 +1,17 @@ +UMS test script. + +Example usage: +1. On the target: + create UMS exportable partition with a proper file system created on + it (e.g. EXT4, FAT). + ums 0 mmc 0 +2. On the host: + sudo test/ums/ums_gadget_test.sh X Y dir [test_file] + e.g. sudo test/ums/ums_gadget_test.sh 1 vfat /mnt ./dat_14M.img + +... where X is the partition number on which UMS operates and the Y is +the file system. The 'dir' parameter is the mount directory on the HOST. +Information about available partitions one can read from target via the +'mmc part' command. +The optional [test_file] parameter is for specifying the exact test file +to use. \ No newline at end of file diff --git a/test/ums/ums_gadget_test.sh b/test/ums/ums_gadget_test.sh new file mode 100755 index 0000000..a304c1c --- /dev/null +++ b/test/ums/ums_gadget_test.sh @@ -0,0 +1,130 @@ +#! /bin/bash + +# Copyright (C) 2014 Samsung Electronics +# Lukasz Majewski <l.majewski@samsung.com> +# +# UMS operation test script +# +# SPDX-License-Identifier: GPL-2.0+ + +set -e # any command return not equal to zero +clear + +COLOUR_RED="\33[31m" +COLOUR_GREEN="\33[32m" +COLOUR_DEFAULT="\33[0m" + +DIR=./ +SUFFIX=img +RCV_DIR=rcv/ +LOG_FILE=./log/log-`date +%d-%m-%Y_%H-%M-%S` + +cd `dirname $0` +../dfu/dfu_gadget_test_init.sh 33M 97M + +cleanup () { + rm -rf $RCV_DIR +} + +die () { + printf " $COLOUR_RED FAILED $COLOUR_DEFAULT \n" + cleanup + exit 1 +} + +calculate_md5sum () { + MD5SUM=`md5sum $1` + MD5SUM=`echo $MD5SUM | cut -d ' ' -f1` + echo "md5sum:"$MD5SUM +} + +ums_test_file () { + printf "$COLOUR_GREEN========================================================================================= $COLOUR_DEFAULT\n" + printf "File:$COLOUR_GREEN %s $COLOUR_DEFAULT\n" $1 + + mount -t $2 /dev/$MEM_DEV $MNT_DIR + if [ -f $MNT_DIR/dat_* ]; then + rm $MNT_DIR/dat_* + fi + sync + + cp ./$1 $MNT_DIR + sync + umount $MNT_DIR + + + echo -n "TX: " + calculate_md5sum $1 + + MD5_TX=$MD5SUM + sleep 1 + N_FILE=$DIR$RCV_DIR${1:2}"_rcv" + + mount -t $2 /dev/$MEM_DEV $MNT_DIR + cp $MNT_DIR/$1 $N_FILE || die $? + sync + rm $MNT_DIR/$1 + sync + umount $MNT_DIR + + echo -n "RX: " + calculate_md5sum $N_FILE + MD5_RX=$MD5SUM + + if [ "$MD5_TX" == "$MD5_RX" ]; then + printf " $COLOUR_GREEN -------> OK $COLOUR_DEFAULT \n" + else + printf " $COLOUR_RED -------> FAILED $COLOUR_DEFAULT \n" + cleanup + exit 1 + fi +} + +printf "$COLOUR_GREEN========================================================================================= $COLOUR_DEFAULT\n" +echo "U-boot UMS test program" + +if [ $EUID -ne 0 ]; then + echo "You must be root to do this." 1>&2 + exit 100 +fi + +if [ $# -lt 3 ]; then + echo "Wrong number of arguments" + echo "Example:" + echo "sudo ./ums_gadget_test.sh 1 vfat /mnt [test_file]" + die +fi + +MNT_DIR=$3 + +mkdir -p $RCV_DIR + +MEM_DEV=`dmesg | tail -n 10 | grep -E " sd[a-z]:" - | cut -d ':' -f 1` +MEM_DEV=$(echo $MEM_DEV | cut -d ']' -f2 | tr -d ' ') + +if [ -z $MEM_DEV ]; then + echo "Connect target" + echo "e.g. ums 0 mmc 0" + die +fi + +MEM_DEV=$MEM_DEV$1 + +printf "Mount: /dev/$MEM_DEV FS: %s\n" $2 + +if [ -n "$4" ]; then + if [ ! -e $4 ]; then + echo "No file: $4" + die + fi + ums_test_file $4 $2 +else + for file in $DIR*.$SUFFIX + do + ums_test_file $file $2 + done +fi + +cleanup + +exit 0
This commit adds new test for UMS USB gadget to u-boot mainline tree. It it similar in operation to the one already available in test/dfu directory. Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> --- test/ums/README | 17 ++++++ test/ums/ums_gadget_test.sh | 130 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 147 insertions(+) create mode 100644 test/ums/README create mode 100755 test/ums/ums_gadget_test.sh \ No newline at end of file