[v2,16/23] test_firmware: enable custom fallback testing on limited kernel configs
diff mbox

Message ID 20171120182409.27348-17-mcgrof@kernel.org
State New
Headers show

Commit Message

Luis Chamberlain Nov. 20, 2017, 6:24 p.m. UTC
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@kernel.org>
---
 tools/testing/selftests/firmware/config         |  4 +++
 tools/testing/selftests/firmware/fw_fallback.sh | 45 ++++++++++++++++++++++++-
 2 files changed, 48 insertions(+), 1 deletion(-)

Comments

Greg KH Nov. 29, 2017, 10:20 a.m. UTC | #1
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@kernel.org>
> ---
>  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"

How does this work when you are running the test on a target that does
not have this kernel build option?

thanks,

greg k-h
Greg KH Nov. 29, 2017, 10:22 a.m. UTC | #2
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@kernel.org>
> ---
>  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?

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

thanks,

greg k-h
Greg KH Nov. 29, 2017, 10:28 a.m. UTC | #3
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@kernel.org>

I've applied the patches up to here in this series.

thanks,

greg k-h
Luis Chamberlain Nov. 30, 2017, 8:31 p.m. UTC | #4
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@kernel.org>
> > ---
> >  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
shuah Nov. 30, 2017, 9:40 p.m. UTC | #5
On 11/30/2017 01:31 PM, Luis R. Rodriguez wrote:
> 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@kernel.org>
>>> ---
>>>  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?

There is no generic script right now. It depends on whether or not a
generic script will help. Can it made generic or test would still need
their customizations anyway.

Furthermore, this generic script will become a dependency for tests and
will need to be included in the install targets and also when O=dir
cases. hence, the question on how generic can the script be and whether
are not tests can avoid customization.

I am finding in the case of selftests the generic framework sometimes
add more complexity. In some cases it is worth it like lib.mk, however
it takes work to override it as well.

I have some reservations about introducing a generic script for this.

thanks,
-- Shuah

Patch
diff mbox

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
+}
 
 DIR=/sys/devices/virtual/misc/test_firmware
 
@@ -14,6 +53,7 @@  DIR=/sys/devices/virtual/misc/test_firmware
 # These days no one enables CONFIG_FW_LOADER_USER_HELPER so check for that
 # as an indicator for CONFIG_FW_LOADER_USER_HELPER.
 HAS_FW_LOADER_USER_HELPER=$(if [ -d /sys/class/firmware/ ]; then echo yes; else echo no; fi)
+HAS_FW_LOADER_USER_HELPER_FALLBACK=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y)
 
 if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
        OLD_TIMEOUT=$(cat /sys/class/firmware/timeout)
@@ -286,7 +326,10 @@  run_sysfs_custom_load_tests()
 	fi
 }
 
-run_sysfs_main_tests
+if [ "$HAS_FW_LOADER_USER_HELPER_FALLBACK" = "yes" ]; then
+	run_sysfs_main_tests
+fi
+
 run_sysfs_custom_load_tests
 
 exit 0