[U-Boot] [PATCH 3/3] test: ums: Add script for testing UMS gadget operation

[Stephen Warren]

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 at 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.