Re: [PATCH v2 16/23] test_firmware: enable custom fallback testing on limited kernel configs

[Luis R. Rodriguez]

On Wed, Nov 29, 2017 at 11:22:35AM +0100, Greg KH wrote:
> On Mon, Nov 20, 2017 at 10:24:02AM -0800, Luis R. Rodriguez wrote:
> > When a kernel is not built with:
> > 
> > CONFIG_HAS_FW_LOADER_USER_HELPER_FALLBACK=y
> > 
> > We don't currently enable testing fw_fallback.sh. For kernels that
> > still enable the fallback mechanism, its possible to use the async
> > request firmware API call request_firmware_nowait() using the custom
> > interface to use the fallback mechanism, so we should be able to test
> > this but we currently cannot.
> > 
> > We can enable testing without CONFIG_HAS_FW_LOADER_USER_HELPER_FALLBACK=y
> > by relying on /proc/config.gz (CONFIG_IKCONFIG_PROC), if present. If you
> > don't have this we'll have no option but to rely on old heuristics for now.
> > 
> > Signed-off-by: Luis R. Rodriguez <mcgrof@xxxxxxxxxx>
> > ---
> >  tools/testing/selftests/firmware/config         |  4 +++
> >  tools/testing/selftests/firmware/fw_fallback.sh | 45 ++++++++++++++++++++++++-
> >  2 files changed, 48 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/testing/selftests/firmware/config b/tools/testing/selftests/firmware/config
> > index c8137f70e291..bf634dda0720 100644
> > --- a/tools/testing/selftests/firmware/config
> > +++ b/tools/testing/selftests/firmware/config
> > @@ -1 +1,5 @@
> >  CONFIG_TEST_FIRMWARE=y
> > +CONFIG_FW_LOADER=y
> > +CONFIG_FW_LOADER_USER_HELPER=y
> > +CONFIG_IKCONFIG=y
> > +CONFIG_IKCONFIG_PROC=y
> > diff --git a/tools/testing/selftests/firmware/fw_fallback.sh b/tools/testing/selftests/firmware/fw_fallback.sh
> > index 722cad91df74..a42e437363d9 100755
> > --- a/tools/testing/selftests/firmware/fw_fallback.sh
> > +++ b/tools/testing/selftests/firmware/fw_fallback.sh
> > @@ -6,7 +6,46 @@
> >  # won't find so that we can do the load ourself manually.
> >  set -e
> >  
> > +PROC_CONFIG="/proc/config.gz"
> > +TEST_DIR=$(dirname $0)
> > +
> >  modprobe test_firmware
> > +if [ ! -f $PROC_CONFIG ]; then
> > +	if modprobe configs 2>/dev/null; then
> > +		echo "Loaded configs module"
> > +		if [ ! -f $PROC_CONFIG ]; then
> > +			echo "You must have the following enabled in your kernel:" >&2
> > +			cat $TEST_DIR/config >&2
> > +			echo "Resorting to old heuristics" >&2
> > +		fi
> > +	else
> > +		echo "Failed to load configs module, using old heuristics" >&2
> > +	fi
> > +fi
> > +
> > +kconfig_has()
> > +{
> > +	if [ -f $PROC_CONFIG ]; then
> > +		if zgrep -q $1 $PROC_CONFIG 2>/dev/null; then
> > +			echo "yes"
> > +		else
> > +			echo "no"
> > +		fi
> > +	else
> > +		# We currently don't have easy heuristics to infer this
> > +		# so best we can do is just try to use the kernel assuming
> > +		# you had enabled it. This matches the old behaviour.
> > +		if [ "$1" = "CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y" ]; then
> > +			echo "yes"
> > +		elif [ "$1" = "CONFIG_FW_LOADER_USER_HELPER=y" ]; then
> > +			if [ -d /sys/class/firmware/ ]; then
> > +				echo yes
> > +			else
> > +				echo no
> > +			fi
> > +		fi
> > +	fi
> > +}
> 
> Shouldn't these functions be part of the kselftest core so that all
> tests can take advantage of them instead of having to hand-roll them for
> every individual test?

At a first glance it would seem to be nice.

Note that in our case we'd still need a way to work without CONFIG_IKCONFIG_PROC,
hence that big else condition, to ensure it works for kernels without
CONFIG_IKCONFIG_PROC set, so we'd still end up with our own fw_kconfig_has(),
and if we had a generic helper it'd look very similar... something like:

fw_kconfig_has()
{
	if [ has_proc_config ]; then
		echo $(kconfig_has("$1"))
	else
		# We currently don't have easy heuristics to infer this
		# so best we can do is just try to use the kernel assuming
		# you had enabled it. This matches the old behaviour.
		if [ "$1" = "CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y" ]; then
			echo "yes"
		elif [ "$1" = "CONFIG_FW_LOADER_USER_HELPER=y" ]; then
			if [ -d /sys/class/firmware/ ]; then
				echo yes
			else
				echo no
			fi
		fi
	fi
}

Not sure if there is a big gain then for now.

Shuah, what do you think? Is it worth it? Do we have a generic bash library we
can use?  If not, if I add one, so that other scripts can source it, where
should I add it?

> And is there no way at runtime to tell what the options are and just
> not run that type of test?

For CONFIG_FW_LOADER_USER_HELPER=y yes, its handled in that else condition.

For CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y though no, there is no way. The
else condition has a big comment indicating there is no current *easy*
run time heuristic possible. This remains true specially since we have to
support these scripts to run on older kernels as well.

  Luis