Date:   Thu, 30 Nov 2017 14:40:53 -0700
From:   Shuah Khan <shuah@...nel.org>
To:     "Luis R. Rodriguez" <mcgrof@...nel.org>,
        Greg KH <gregkh@...uxfoundation.org>
Cc:     akpm@...ux-foundation.org, keescook@...omium.org,
        mfuzzey@...keon.com, zohar@...ux.vnet.ibm.com, dhowells@...hat.com,
        pali.rohar@...il.com, tiwai@...e.de, arend.vanspriel@...adcom.com,
        zajec5@...il.com, nbroeking@...com, markivx@...eaurora.org,
        stephen.boyd@...aro.org, broonie@...nel.org,
        dmitry.torokhov@...il.com, dwmw2@...radead.org,
        torvalds@...ux-foundation.org, Abhay_Salunke@...l.com,
        bjorn.andersson@...aro.org, jewalt@...innovations.com,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        Shuah Khan <shuahkh@....samsung.com>,
        Shuah Khan <shuah@...nel.org>
Subject: Re: [PATCH v2 16/23] test_firmware: enable custom fallback testing on
 limited kernel configs

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@...nel.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

Powered by blists - more mailing lists