<div dir="ltr">Hi Petr:<br><div><span style="color:rgb(51,51,51);font-family:"Microsoft YaHei";font-size:16px">Thank you for taking the trouble to help me. I do appreciate it.</span></div><div><font color="#333333" face="Microsoft YaHei"><span style="font-size:16px">Sorry about the so many mistakes,</span></font></div><div><span style="color:rgb(51,51,51);font-family:"Microsoft YaHei";font-size:16px">I will fix the code in next vesion. </span></div><div><br>Thanks again for you help!<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Petr Vorel <<a href="mailto:pvorel@suse.cz">pvorel@suse.cz</a>> 于2020年3月19日周四 下午6:28写道：<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Cixi,<br>
<br>
> Porting UART test from ltp-ddt back to ltp. only test 115200 UART_RATE.<br>
Thanks for your effort. There are several problems with this test, I'll try to<br>
address them all.<br>
<br>
> [TODO] support more rate, and add test HWFLOW function test.<br>
Could this be in v2?<br>
<br>
> diff --git a/runtest/kernel_ddt b/runtest/kernel_ddt<br>
> new file mode 100644<br>
> index 000000000..30e9a0269<br>
> --- /dev/null<br>
> +++ b/runtest/kernel_ddt<br>
> @@ -0,0 +1 @@<br>
> +uart serialcheck.sh<br>
I wonder if there needs to be in it's own runtest file.<br>
Maybe yes, but I'd propose more meaningful name (serial, uart).<br>
What is ddt anyway?<br>
<br>
> diff --git a/testcases/kernel/device-drivers/Makefile<br>
> b/testcases/kernel/device-drivers/Makefile<br>
> index 55e0d25a0..a214f211b 100644<br>
> --- a/testcases/kernel/device-drivers/Makefile<br>
> +++ b/testcases/kernel/device-drivers/Makefile<br>
> @@ -27,6 +27,7 @@ SUBDIRS := acpi \<br>
>     rtc \<br>
>     tbio \<br>
>     uaccess \<br>
> +   uart \<br>
>     zram<br>
<br>
>  include $(top_srcdir)/include/mk/<a href="http://generic_trunk_target.mk" rel="noreferrer" target="_blank">generic_trunk_target.mk</a><br>
> diff --git a/testcases/kernel/device-drivers/uart/Makefile<br>
> b/testcases/kernel/device-drivers/uart/Makefile<br>
> new file mode 100644<br>
> index 000000000..0d73f6635<br>
> --- /dev/null<br>
> +++ b/testcases/kernel/device-drivers/uart/Makefile<br>
> @@ -0,0 +1,22 @@<br>
> +# Copyright (c) 2015 Oracle and/or its affiliates. All Rights Reserved.<br>
> +#<br>
> +# This program is free software; you can redistribute it and/or<br>
> +# modify it under the terms of the GNU General Public License as<br>
> +# published by the Free Software Foundation; either version 2 of<br>
> +# the License, or (at your option) any later version.<br>
> +#<br>
> +# This program is distributed in the hope that it would be useful,<br>
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of<br>
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the<br>
> +# GNU General Public License for more details.<br>
> +#<br>
> +# You should have received a copy of the GNU General Public License<br>
> +# along with this program; if not, write the Free Software Foundation,<br>
> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA<br>
Please next time instead of verbose GPL text ^ use just:<br>
# SPDX-License-Identifier: GPL-2.0-or-later<br>
<br>
> +<br>
> +top_srcdir ?= ../../../..<br>
> +include $(top_srcdir)/include/mk/<a href="http://testcases.mk" rel="noreferrer" target="_blank">testcases.mk</a><br>
> +<br>
> +INSTALL_TARGETS := *.sh<br>
> +<br>
> +include $(top_srcdir)/include/mk/<a href="http://generic_leaf_target.mk" rel="noreferrer" target="_blank">generic_leaf_target.mk</a><br>
> diff --git a/testcases/kernel/device-drivers/uart/serialcheck.sh<br>
> b/testcases/kernel/device-drivers/uart/serialcheck.sh<br>
> new file mode 100755<br>
> index 000000000..f4cf13e02<br>
> --- /dev/null<br>
> +++ b/testcases/kernel/device-drivers/uart/serialcheck.sh<br>
> @@ -0,0 +1,111 @@<br>
> +#!/bin/sh<br>
> +###############################################################################<br>
> +#<br>
> +# Copyright (C) 2015 Texas Instruments Incorporated - <a href="http://www.ti.com/" rel="noreferrer" target="_blank">http://www.ti.com/</a><br>
> +# Copyright (C) 2019, Unisoc Communications Inc.<br>
> +#<br>
> +# This program is free software; you can redistribute it and/or<br>
> +# modify it under the terms of the GNU General Public License as<br>
> +# published by the Free Software Foundation version 2.<br>
> +#<br>
> +# This program is distributed "as is" WITHOUT ANY WARRANTY of any<br>
> +# kind, whether express or implied; without even the implied warranty<br>
> +# of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the<br>
> +# GNU General Public License for more details.<br>
> +#<br>
> +###############################################################################<br>
And here ^:<br>
# SPDX-License-Identifier: GPL-2.0-or-later<br>
> +<br>
> +# @desc Test UART ports using git://<br>
> <a href="http://git.breakpoint.cc/bigeasy/serialcheck.git" rel="noreferrer" target="_blank">git.breakpoint.cc/bigeasy/serialcheck.git</a><br>
It looks your mailer wraps lines, that's unusable for applying.<br>
Could you use git format-patch and git send-email for generating patches and<br>
sending them?<br>
<br>
> +<br>
> +#### Functions definitions ####<br>
Please avoid these useless comments.<br>
<br>
> +usage()<br>
> +{<br>
> +    echo "usage: ./${0##*/} [-r UART_RATE] [-l LOOPS] [-x to enable HW<br>
> flow control]"<br>
Also here is wrapped.<br>
<br>
But you're supposed to use TST_OPTS TST_PARSE_ARGS and TST_USAGE from The API<br>
<a href="https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#233-optional-command-line-parameters" rel="noreferrer" target="_blank">https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#233-optional-command-line-parameters</a><br>
<br>
> +    exit 1<br>
> +}<br>
> +<br>
> +# Default values<br>
> +: ${UART_RATE:=115200}<br>
> +: ${UART_LOOPS:=5}<br>
> +: ${UART_HWFLOW:=0}<br>
Even this is a valid syntax, please use more convinient:<br>
UART_RATE="${UART_RATE:=115200}"<br>
<br>
> +<br>
> +PORTS_TO_TEST=();<br>
> +UART_PORTS=();<br>
> +ARRAY=(`find /sys/class/tty/*/uartclk`);<br>
Arrays are bashisms (bash specific), we don't allow them in LTP, as we require<br>
POSIX shell syntax, which is portable (some systems doesn't have bash, but dash<br>
or other shell e.g. toybox or busybox on Android. I guess you target embedded<br>
system with this test):<br>
<a href="https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#132-shell-coding-style" rel="noreferrer" target="_blank">https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#132-shell-coding-style</a><br>
<br>
Most of the time arrays can be replaced by string separated by space.<br>
If you need these devices, in /sys/class/tty/*/uartclk, it could be done like:<br>
ports=$(for i in /sys/class/tty/*/uartclk ; do echo $i | cut -d '/' -f 5; done)<br>
<br>
> +<br>
> +check_requirements()<br>
> +{<br>
> + which serialcheck<br>
<br>
<br>
> + ret=$?<br>
> + if [ $ret -eq 0 ];then<br>
> + tst_res TINFO "serialcheck command is in system,continue to test"<br>
> + else<br>
> + tst_brk TCONF "test failed for lack of requirement,returned is $ret"<br>
> + fi<br>
> +<br>
Useless blank line here.<br>
> +}<br>
TINFO is not much useful, I'd avoid that. And check_requirements should be a setup function, not called directly in do_test:<br>
TST_SETUP=check_requirements<br>
<br>
But given that whole function just check serialcheck, whole function should be<br>
replaced just by:<br>
<br>
TST_NEEDS_CMDS="serialcheck"<br>
<a href="https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#232-library-environment-variables-for-shell" rel="noreferrer" target="_blank">https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#232-library-environment-variables-for-shell</a><br>
<br>
BTW serialcheck source isn't probably packaged in distros<br>
<a href="https://git.breakpoint.cc/cgit/bigeasy/serialcheck.git/tree/serialcheck.c" rel="noreferrer" target="_blank">https://git.breakpoint.cc/cgit/bigeasy/serialcheck.git/tree/serialcheck.c</a><br>
Maybe we could just adapt it to LTP and include it as well (if we consider whole<br>
testing useful).<br>
<br>
> +<br>
> +create_test_file()<br>
> +{<br>
> +    temp_test_file=`mktemp`<br>
I guess you need to use TST_NEEDS_TMPDIR=1<br>
and then just any file in it. Or use $$ (e.g. file.$$) if you want to have<br>
concurrency, but we usually don't care).<br>
> +    dd if=/dev/urandom of=$temp_test_file count=1 bs=$((UART_RATE / 2))<br>
You also need to add dd to TST_NEEDS_CMDS.<br>
<br>
> +}<br>
> +<br>
> +get_uart_ports()<br>
> +{<br>
> +for i in ${ARRAY[@]}; do<br>
> +    PORT=/dev/`echo $i | cut -d'/' -f 5`<br>
Well, you use cut yourself, so why that complicated code with arrays?<br>
<br>
> +    # Activate port in case it will be initialized only when startup<br>
> +    echo "DDT TESTING" > $PORT 2>/dev/null<br>
> +    if [ `cat $i` -ne 0 ]; then<br>
> +        UART_PORTS=("${UART_PORTS[@]}" "$PORT")<br>
> +    fi<br>
> +done<br>
> +}<br>
> +<br>
> +filter_out_used_ports()<br>
> +{<br>
> + which lsof<br>
> + ret=$?<br>
> + if [ $ret -eq 0 ];then<br>
> + tst_res TINFO "lsof command exist, filter out used ports";<br>
> + else<br>
> + tst_brk TCONF "test failed for lack of requirement,returned is $ret"<br>
> + fi<br>
Again whole block is useless, just add lsof into TST_NEEDS_CMDS.<br>
<br>
> +<br>
> +    for i in ${UART_PORTS[@]}; do<br>
> +        lsof | grep $i &> /dev/null ||<br>
> PORTS_TO_TEST=("${PORTS_TO_TEST[@]}" $i)<br>
> +    done<br>
> +}<br>
> +<br>
> +run_serial_test()<br>
> +{<br>
> + create_test_file<br>
create_test_file should be just 2 lines of code in setup function.<br>
<br>
> +    for i in ${PORTS_TO_TEST[@]}; do<br>
Array => bashism :(.<br>
<br>
> +        if [ $UART_HWFLOW -eq 0 ]; then<br>
> +            { sleep 1; serialcheck -b $UART_RATE -d $i -f $temp_test_file<br>
> -l $UART_LOOPS -m t -k; }&<br>
Again line wrapped :(<br>
<br>
> +            PID=$!<br>
> +            serialcheck -b $UART_RATE -d $i -f $temp_test_file -l<br>
Hm, why do you run 2 instances?<br>
<br>
> $UART_LOOPS -m r -k || { kill -- -$PID 2>/dev/null; tst_res TFAIL "TEST<br>
> FAILED"; }<br>
Using complicated code in { } isn't much readable. I'd put it into:<br>
if ! $UART_LOOPS -m r -k; then<br>
        kill ...<br>
fi<br>
<br>
> +        else<br>
> +            { sleep 1; serialcheck -b $UART_RATE -d $i -f $temp_test_file<br>
> -l $UART_LOOPS -m t -h; } &<br>
> +            PID=$!<br>
> +            serialcheck -b $UART_RATE -d $i -f $temp_test_file -l<br>
> $UART_LOOPS -m r -h || { kill -- -$PID 2>/dev/null; tst_res TFAIL "TEST<br>
> FAILED"; }<br>
<br>
This can be written better to not repeat much yourself.<br>
Whole if and else block is the same exept -h and -k parameter are different.<br>
Why not put this extra parameter into variable?<br>
Why sleep added into {} block?<br>
<br>
> +        fi<br>
> +    done<br>
> +    rm $temp_test_file<br>
> + tst_res TPASS "uart test passed"<br>
> +}<br>
> +<br>
> +<br>
> +TST_TESTFUNC=do_test<br>
> +. tst_test.sh<br>
We usually put this at the beginning of the test.<br>
Please see some tests as examples (testcases/commands/df/df01.sh,<br>
testcases/commands/mkfs/mkfs01.sh, testcases/commands/lsmod/lsmod01.sh, ...)<br>
<br>
> +<br>
> +do_test()<br>
> +{<br>
> + check_requirements<br>
<br>
> + get_uart_ports<br>
> + filter_out_used_ports<br>
> + run_serial_test<br>
> +}<br>
> +<br>
> +tst_run<br>
<br>
Kind regards,<br>
Petr<br>
</blockquote></div>