Re: [PATCH v2 1/2] firmware_class: encapsulate firmware loading status

[Luis R. Rodriguez]

On Tue, Aug 23, 2016 at 11:00:19AM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner@xxxxxxxxxxxx>
> 
> The firmware user helper code tracks the current state of the loading
> process via an member of struct firmware_buf and a completion.

You can specify the members, the unsigned long status and the completion.

> Let's
> encapsulate this simple state machine into struct fw_status.

Please rephrase instead to:

"We only need this for the usermode helper as such we can encapsulate
all this data into its own data structure."

> We don't need the fw_lock to protect the state machine anymore.

You can the instead follow up explaining, "Doing this helps us compartamentalize
the usermode helper code further."

Can you cut that out into a separate patch?

Then another patch can be: "The usermode helper code only needs access to a
status variable, no RWM operations are done on it, so we can replace the
use of the global lock with READ_ONCE() and WRITE_ONCE()."

> First of
> all we don't do any RWM operations instead we do either a write or read
> to the status variable. Reading the status variable via READ_ONCE
> ensures we see the new state whenever we get woken. We are not allowed
> to use the complete_all() more than once so we need to filter out all
> state transition but the last. That is okay since only the final state
> DONE|ABORTED is of interest.
> 
> Besides that we also safe a few bytes:
> 
> cris etrax-100lx_defconfig
> 
> $ bloat-o-meter firmware_class.o.old firmware_class.o.new
> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-234 (-234)
> function                                     old     new   delta
> _request_firmware                           1228     994    -234
> Total: Before=2179, After=1945, chg -10.74%
> 
> x86_64 FW_LOADER !FW_LOADER_USER_HELPER_FALLBACK
> 
> $ bloat-o-meter firmware_class.o.old firmware_class.o.new
> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-194 (-194)
> function                                     old     new   delta
> _request_firmware                           2118    1924    -194
> Total: Before=6811, After=6617, chg -2.85%
> 
> x86_64 FW_LOADER FW_LOADER_USER_HELPER_FALLBACK
> 
> $ bloat-o-meter firmware_class.o.old firmware_class.o.new
> add/remove: 0/0 grow/shrink: 2/5 up/down: 4/-193 (-189)
> function                                     old     new   delta
> fw_shutdown_notify                            81      83      +2
> firmware_data_read                           162     164      +2
> fw_pm_notify                                 396     394      -2
> firmware_loading_store                       458     441     -17
> firmware_data_write                          504     475     -29
> firmware_loading_show                         92      61     -31
> _request_firmware                           2931    2817    -114
> Total: Before=10328, After=10139, chg -1.83%
> 
> Note the variable loading_timeout and the inline function
> firmware_loadint_timeout() are used also when no user helper is enabled.
> So they need to be moved out side of the ifdef section.
> 
> Signed-off-by: Daniel Wagner <daniel.wagner@xxxxxxxxxxxx>
> Cc: Ming Lei <ming.lei@xxxxxxxxxxxxx>
> Cc: Luis R. Rodriguez <mcgrof@xxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> ---
>  drivers/base/firmware_class.c | 158 +++++++++++++++++++++++++-----------------
>  1 file changed, 93 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 22d1760..d3dcf87 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -91,19 +91,88 @@ static inline bool fw_is_builtin_firmware(const struct firmware *fw)
>  }
>  #endif
>  
> +static int loading_timeout = 60;	/* In seconds */
> +
> +static inline long firmware_loading_timeout(void)
> +{
> +	return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
> +}

All this code is being kept when CONFIG_FW_LOADER_USER_HELPER is not enabled, that
seems odd given firmware_loading_timeout() is used when CONFIG_FW_LOADER_USER_HELPER
is disabled in two places, in one case its not needed at all, in the second its in this
piece of code:

        retval = fw_status_wait_timeout(&buf->fw_st, timeout);                  
        if (retval == -ERESTARTSYS || !retval) {      
		...
	 }
I'd rather instead have a wrapper for a user mode helper timeout check and
have that return 0 for when CONFIG_FW_LOADER_USER_HELPER is not selected.
This way the global variable loading_timeout and anything that accesses it
is just left inside CONFIG_FW_LOADER_USER_HELPER.

> +
> +#ifdef CONFIG_FW_LOADER_USER_HELPER
> +
>  enum {
> +	FW_STATUS_UNKNOWN,
>  	FW_STATUS_LOADING,
>  	FW_STATUS_DONE,
> -	FW_STATUS_ABORT,
> +	FW_STATUS_ABORTED,
>  };
>  
> -static int loading_timeout = 60;	/* In seconds */
> +struct fw_status {
> +	unsigned long status;
> +	struct completion completion;
> +};
>  
> -static inline long firmware_loading_timeout(void)
> +static void fw_status_init(struct fw_status *fw_st)
>  {
> -	return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
> +	fw_st->status = FW_STATUS_UNKNOWN;
> +	init_completion(&fw_st->completion);
> +}
> +
> +static unsigned long __fw_status_get(struct fw_status *fw_st)
> +{
> +	return READ_ONCE(fw_st->status);
> +}
> +
> +static int fw_status_wait_timeout(struct fw_status *fw_st, long timeout)
> +{
> +	unsigned long status;
> +	int err;
> +
> +	err = wait_for_completion_interruptible_timeout(&fw_st->completion,
> +							timeout);
> +	status = READ_ONCE(fw_st->status);
> +	if (err == 0 && status == FW_STATUS_ABORTED)
> +		return -ENOENT;
> +
> +	return err;
> +}
> +
> +static void __fw_status_set(struct fw_status *fw_st,
> +			  unsigned long status)
> +{
> +	WRITE_ONCE(fw_st->status, status);
> +
> +	if (status == FW_STATUS_DONE ||
> +			status == FW_STATUS_ABORTED)
> +		complete_all(&fw_st->completion);
>  }
>  
> +#define fw_status_start(fw_st)					\
> +	__fw_status_set(fw_st, FW_STATUS_LOADING)
> +#define fw_status_done(fw_st)					\
> +	__fw_status_set(fw_st, FW_STATUS_DONE)
> +#define fw_status_aborted(fw_st)				\
> +	__fw_status_set(fw_st, FW_STATUS_ABORTED)
> +#define fw_status_is_loading(fw_st)				\
> +	(__fw_status_get(fw_st) == FW_STATUS_LOADING)
> +#define fw_status_is_done(fw_st)				\
> +	(__fw_status_get(fw_st) == FW_STATUS_DONE)
> +#define fw_status_is_aborted(fw_st)				\
> +	(__fw_status_get(fw_st) == FW_STATUS_ABORTED)
> +
> +#else /* CONFIG_FW_LOADER_USER_HELPER */
> +
> +#define fw_status_wait_timeout(fw_st, long)	0
> +
> +#define fw_status_start(fw_st)

You don't need this, in fact if you had it as-is and the code for when
CONFIG_FW_LOADER_USER_HELPER is disabled would use it you may end up in
a compile error.

> +#define fw_status_done(fw_st)

You need this for both.

> +#define fw_status_aborted(fw_st)
> +#define fw_status_is_loading(fw_st)
> +#define fw_status_is_done(fw_st)

All these are no longer needed for when CONFIG_FW_LOADER_USER_HELPER is
disabled.

> +#define fw_status_is_aborted(fw_st)		false

This is needed for both.

Given all this it seems to be best to rebrand these helpers as usermode helper
related. In fact the next step is to see if this looks best to just stuffing all
this user mode helper code into its own C file. But we can do that later.

Other than that looks good!

  Luis