Date:   Thu, 11 May 2017 13:17:39 -0500
From:   "Li, Yi" <yi1.li@...ux.intel.com>
To:     "Luis R. Rodriguez" <mcgrof@...nel.org>, gregkh@...uxfoundation.org
Cc:     wagi@...om.org, dwmw2@...radead.org, rafal@...ecki.pl,
        arend.vanspriel@...adcom.com, rjw@...ysocki.net,
        atull@...nsource.altera.com, moritz.fischer@...us.com,
        pmladek@...e.com, johannes.berg@...el.com,
        emmanuel.grumbach@...el.com, luciano.coelho@...el.com,
        kvalo@...eaurora.org, luto@...nel.org,
        torvalds@...ux-foundation.org, keescook@...omium.org,
        takahiro.akashi@...aro.org, dhowells@...hat.com, pjones@...hat.com,
        hdegoede@...hat.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 1/5] firmware: add extensible driver data params



On 5/2/2017 3:49 AM, Luis R. Rodriguez wrote:
> As the firmware API evolves we keep extending functions with more arguments.
> Stop this nonsense by proving an extensible data structure which can be used
> to represent both user parameters and private internal parameters.
>
> We introduce 3 data structures:
>
>    o struct driver_data_req_params  - used for user specified parameters
>    o struct driver_data_priv_params - used for internal use only
>    o struct driver_data_params - stiches both of the the above together,
> 				only for internal use
>
> This starts off by just making the existing APIs use the new data
> structures, it will make subsequent changes easier to review which will
> be adding new flexible APIs.
>
> A side consequences is get to replace all the old internal "firmware
> behavior  options" flags with enums we properly document, remove the
> blinding #ifdefs, and compartamentlize the userhelper fallback code
> more appropriately unde CONFIG_FW_LOADER_USER_HELPER_FALLBACK.
>
> This commit should introduces no functional changes (TM).
>
> Signed-off-by: Luis R. Rodriguez <mcgrof@...nel.org>
> ---
>   drivers/base/firmware_class.c | 331 ++++++++++++++++++++++++++++++++----------
>   include/linux/driver_data.h   |  82 +++++++++++
>   2 files changed, 339 insertions(+), 74 deletions(-)
>   create mode 100644 include/linux/driver_data.h
> ...
> diff --git a/include/linux/driver_data.h b/include/linux/driver_data.h
> new file mode 100644
> index 000000000000..7ce3216a9a99
> --- /dev/null
> +++ b/include/linux/driver_data.h
> @@ -0,0 +1,82 @@
> +#ifndef _LINUX_DRIVER_DATA_H
> +#define _LINUX_DRIVER_DATA_H
> +
> +#include <linux/types.h>
> +#include <linux/compiler.h>
> +#include <linux/gfp.h>
> +#include <linux/device.h>
> +
> +/*
> + * Driver Data internals
> + *
> + * Copyright (C) 2017 Luis R. Rodriguez <mcgrof@...nel.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of copyleft-next (version 0.3.1 or later) as published
> + * at http://copyleft-next.org/.
> + */
> +
> +/**
> + * struct driver_data_async_cbs - callbacks for handling driver data requests
> + * @found_cb: callback to be used when the driver data has been found. A
> + *	callback is required. If the requested driver data is found it will
> + *	passed on the callback, using the context set on @found_ctx.
> + * @found_ctx: preferred context to be used as the second argument to
> + * 	@found_cb.
> + *
> + * Used for specifying callbacks and contexts used for when asynchronous driver
> + * data requests have completed. If no driver data is found the error will be
> + * passed on the respective callback.
> + */
> +struct driver_data_async_cbs {
> +	void (*found_cb)(const struct firmware *driver_data,
> +			 void *context,
> +			 int error);

Any reason why the call back function format are not consistent between 
async and sync mode.  In sync mode, the context is before struct 
firmware. in Async mode, it's reversed.

struct driver_data_sync_cbs {
     int __must_check
         (*found_cb)(void *context,
                 const struct firmware *driver_data,
                 int error);

Yi
> +	void *found_ctx;
> +};
> +
> +/**
> + * union driver_data_cbs - callbacks for driver data request
> + * @async: callbacks for handling driver data when asynchronous requests
> + * 	are made.
> + *
> + * Used for placement of callbacks used for handling results from driver
> + * data requests.
> + */
> +union driver_data_cbs {
> +	struct driver_data_async_cbs async;
> +};
> +
> +/**
> + * enum driver_data_reqs - requirements of the driver data request
> + * @DRIVER_DATA_REQ_OPTIONAL: if set it is not a hard requirement by the
> + *	caller that the file requested be present. An error will not be recorded
> + *	if the file is not found.
> + */
> +enum driver_data_reqs {
> +	DRIVER_DATA_REQ_OPTIONAL			= 1 << 0,
> +};
> +
> +/**
> + * struct driver_data_req_params - driver data request parameters
> + * @hold_module: module to hold during the driver data request operation. By
> + * 	default if sync requests set this to NULL the firmware_class module
> + * 	will be refcounted during operation.
> + * @gfp: flags to use for allocations when constructing the driver data request,
> + *	prior to scheduling. Unused on driver_data_request_sync().
> + * @reqs: set of &enum driver_data_reqs flags used to configure the driver
> + * 	data request. All of the specified requirements must be met.
> + * @cbs: set of callbacks to use for the driver data request.
> + *
> + * This data structure is intended to carry all requirements and specifications
> + * required to complete the task to get the requested driver date file to the
> + * caller.
> + */
> +struct driver_data_req_params {
> +	struct module *hold_module;
> +	gfp_t gfp;
> +	u64 reqs;
> +	const union driver_data_cbs cbs;
> +};
> +
> +#endif /* _LINUX_DRIVER_DATA_H */

Powered by blists - more mailing lists