Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <9a76e550-fd1e-851c-e322-4bf51ab11e97@gmail.com>
Date:   Mon, 15 May 2023 09:58:18 +0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH 3/3] selftests: ALSA: Add test for the 'valsa' driver
To:     Mark Brown <broonie@kernel.org>
Cc:     corbet@lwn.net, akpm@linux-foundation.org, perex@perex.cz,
        tiwai@suse.com, skhan@linuxfoundation.org,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        alsa-devel@alsa-project.org, linux-kselftest@vger.kernel.org,
        gregkh@linuxfoundation.org, himadrispandya@gmail.com,
        linux-kernel-mentees@lists.linuxfoundation.org
References: <20230513202037.158777-1-ivan.orlov0322@gmail.com>
 <20230513202037.158777-3-ivan.orlov0322@gmail.com>
 <ZGGKwA10shGGUr87@finisterre.sirena.org.uk>
Content-Language: en-US
From:   Ivan Orlov <ivan.orlov0322@gmail.com>
In-Reply-To: <ZGGKwA10shGGUr87@finisterre.sirena.org.uk>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 5/15/23 05:28, Mark Brown wrote:
> On Sun, May 14, 2023 at 12:20:37AM +0400, Ivan Orlov wrote:
> 
>> +uid=$(id -u)
>> +if [ $uid -ne 0 ]; then
>> +	echo "$0: Must be run as root"
>> +	exit 1
>> +fi
> 
> It is not an error to run the selftest as a non-root user, the test
> should be skipped.

Alright, thanks!
>> +modprobe snd-valsa
> 
> We don't check if the module was already loaded.
> 
>> +if [ ! -e /sys/kernel/debug/valsa/pc_test ]; then
>> +	mount -t debugfs none /sys/kernel/debug
>> +
>> +	if [ ! -e /sys/kernel/debug/valsa/pc_test ]; then
>> +		echo "$0: Error mounting debugfs"
>> +		exit 4
>> +	fi
>> +fi
> 
> This will unconditionally attempt to mount debugfs in a standard
> location and won't clean up after itself, if the system didn't have
> debugfs mounted for some reason then this will leave it sitting there.

Yes, I agree... I'll remove this in the future versions, I think the 
approach with skipping in case of missing debugfs would be better.

By the way, I used the 'fpa' test as an example, and it looks like it 
should be fixed as well...

> Would it not be better to have a C program that actually calls the ioctl
> rather than a custom debugfs thing that may or may not be wired up to do
> the same thing as an ioctl would?  It seems like other than whatever
> this ioctl test actually does this is all just a simplified version of
> the existing pcm-test.

Well, the idea was to test the playback buffer consistency - the driver 
has the functionality of testing the playback buffer for containing the 
looped pattern (and if the buffer doesn't contain the looped pattern the 
test fails). So, firstly we get the capture buffer with this pattern 
(via arecord), and then send it to the driver for the test (via aplay).

The pcm-test (as I understand) performs only time checks, not the checks 
of the data played/captured, and I think it is pointless to test the 
time again. But I agree, that C test implementation would be better - 
using this approach I can perform additional buffer checks and cover 
more functionality of the driver.

Thank you very much for the review!

Kind regards,
Ivan Orlov.