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;
Date:   Mon, 15 May 2023 10:28:32 +0900
From:   Mark Brown <broonie@kernel.org>
To:     Ivan Orlov <ivan.orlov0322@gmail.com>
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
Subject: Re: [PATCH 3/3] selftests: ALSA: Add test for the 'valsa' driver
Message-ID: <ZGGKwA10shGGUr87@finisterre.sirena.org.uk>
References: <20230513202037.158777-1-ivan.orlov0322@gmail.com>
 <20230513202037.158777-3-ivan.orlov0322@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="7tubSUVrD+xV4Inh"
Content-Disposition: inline
In-Reply-To: <20230513202037.158777-3-ivan.orlov0322@gmail.com>
Precedence: bulk


--7tubSUVrD+xV4Inh
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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.

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

> +success="1"
> +arecord -D hw:CARD=valsa,DEV=0 -c 1 --buffer-size=8192 -f S16_LE -r 8000 --duration=4 out.wav
> +
> +if [[ $(< /sys/kernel/debug/valsa/ioctl_test) == "$success" ]]; then
> +	echo "$0: ioctl test: success"
> +else
> +	echo "$0: ioctl test: fail"
> +fi

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.

--7tubSUVrD+xV4Inh
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmRhir8ACgkQJNaLcl1U
h9DDEQf+IjmNUuTXgXNfQLkMLf76FazxTesXhLu7ittBq9lGs7S5/GQaJ8QTMgzC
TddmGnsS6C774id02yqhg4QU04zGQQ66amxkeAcdylaQZlK8e7U4UwKtXHq4UTDU
X79NmH2oYOE6keq6GYZ7r1KsmfOMD0flHA6+vuq2Lak8PEHms/OlHvQtyNmA7DTE
YUYAzmBzh+zGmq5dbWloNGsJgmdgdcXh6x8eIMDiCDiCo4JKekDXNlY0f/G7wLuh
3U5d9Ud/q2VmgXeCMLkCzKR7o9F1p4ym3MLLM2Up9GevfInfJwTih6XyvtW8WKyR
GdU4RbkyizlI7hiqcmOyBJR1b0Ypcw==
=E/Qm
-----END PGP SIGNATURE-----

--7tubSUVrD+xV4Inh--