Re: [PATCH v2 5/5] firmware: add an extensible system data helpers

[Josh Boyer]

On Thu, Oct 1, 2015 at 1:44 PM, Luis R. Rodriguez
<mcgrof@xxxxxxxxxxxxxxxx> wrote:
> diff --git a/include/linux/sysdata.h b/include/linux/sysdata.h
> new file mode 100644
> index 000000000000..a69cf5ef082c
> --- /dev/null
> +++ b/include/linux/sysdata.h
> @@ -0,0 +1,208 @@
> +#ifndef _LINUX_SYSDATA_H
> +#define _LINUX_SYSDATA_H
> +
> +#include <linux/types.h>
> +#include <linux/compiler.h>
> +#include <linux/gfp.h>
> +
> +/*
> + * System Data internals
> + *
> + * Copyright (C) 2015 Luis R. Rodriguez <mcgrof@xxxxxxxxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public Licence
> + * as published by the Free Software Foundation; either version
> + * 2 of the Licence, or (at your option) any later version.
> + */
> +
> +struct sysdata_file {
> +       size_t size;
> +       const u8 *data;
> +
> +       /* sysdata loader private fields */
> +       void *priv;
> +};
> +
> +/**
> + * enum sync_data_mode - system data mode of operation
> + *
> + * SYNCDATA_SYNC: your call to request system data is synchronous. We will
> + *     look for the system data file you have requested immediatley.
> + * SYNCDATA_ASYNC: your call to request system data is asynchronous. We will
> + *     schedule the search for your system data file to be run at a later
> + *     time.
> + */
> +enum sync_data_mode {
> +       SYNCDATA_SYNC,
> +       SYNCDATA_ASYNC,
> +};
> +
> +/* one per sync_data_mode */
> +union sysdata_file_cbs {
> +       struct {
> +               int __must_check (*found_cb)(void *, const struct sysdata_file *);
> +               void *found_context;
> +
> +               int __must_check (*opt_fail_cb)(void *);
> +               void *opt_fail_context;
> +       } sync;
> +       struct {
> +               void (*found_cb)(const struct sysdata_file *, void *);
> +               void *found_context;
> +
> +               void (*opt_fail_cb)(void *);
> +               void *opt_fail_context;
> +       } async;
> +};
> +
> +struct sysdata_file_sync_reqs {
> +       enum sync_data_mode mode;
> +       struct module *module;
> +       gfp_t gfp;
> +};
> +
> +/**
> + * struct sysdata_file_desc - system data file descriptor
> + * @optional: if true it is not a hard requirement by the caller that this
> + *     file be present. An error will not be recorded if the file is not
> + *     found.
> + * @keep: if set the caller wants to claim ownership over the system data
> + *     through one of its callbacks, it must later free it with
> + *     release_sysdata_file(). By default this is set to false and the kernel
> + *     will release the system data file for you after callback processing
> + *     has completed.
> + * @sync_reqs: synchronization requirements, this will be taken care for you
> + *     by default if you are usingy sdata_file_request(), otherwise you
> + *     should provide your own requirements.
> + *
> + * This structure is set the by the driver and passed to the system data
> + * file helpers sysdata_file_request() or sysdata_file_request_async().
> + * It is intended to carry all requirements and specifications required
> + * to complete the task to get the requested system date file to the caller.
> + * If you wish to extend functionality of system data file requests you
> + * should extend this data structure and make use of the extensions on
> + * the callers to avoid unnecessary collateral evolutions.
> + *
> + * You are allowed to provide a callback to handle if a system data file was
> + * found or not. You do not need to provide a callback. You may also set
> + * an optional flag which would enable you to declare that the system data
> + * file is optional and that if it is not found an alternative callback be
> + * run for you.
> + *
> + * Refer to sysdata_file_request() and sysdata_file_request_async() for more
> + * details.
> + */
> +struct sysdata_file_desc {
> +       bool optional;
> +       bool keep;
> +       struct sysdata_file_sync_reqs sync_reqs;
> +       union sysdata_file_cbs cbs;
> +};
> +
> +/*
> + * We keep these template definitions to a minimum for the most
> + * popular requests.
> + */
> +
> +/* Typical sync data case */
> +#define SYSDATA_SYNC_FOUND(__found_cb, __context)                      \
> +       .cbs.sync.found_cb = __found_cb,                                \
> +       .cbs.sync.found_context = __context
> +
> +/* If you have one fallback routine */
> +#define SYSDATA_SYNC_OPT_CB(__found_cb, __context)                     \
> +       .cbs.sync.opt_fail_cb = __found_cb,                             \
> +       .cbs.sync.opt_fail_context = __context
> +
> +/*
> + * Used to define the default asynchronization requirements for
> + * sysdata_file_request_async(). Drivers can override.
> + */
> +#define SYSDATA_DEFAULT_ASYNC(__found_cb, __context)                   \
> +       .sync_reqs = {                                                  \
> +               .mode = SYNCDATA_ASYNC,                                 \
> +               .module = THIS_MODULE,                                  \
> +               .gfp = GFP_KERNEL,                                      \
> +       },                                                              \
> +       .cbs.async = {                                                  \
> +               .found_cb = __found_cb,                                 \
> +               .found_context = __context,                             \
> +       }
> +
> +#define desc_sync_found_cb(desc)       (desc->cbs.sync.found_cb)
> +#define desc_sync_found_context(desc)  (desc->cbs.sync.found_context)
> +static inline int desc_sync_found_call_cb(const struct sysdata_file_desc *desc,
> +                                         const struct sysdata_file *sysdata)
> +{
> +       BUG_ON(desc->sync_reqs.mode != SYNCDATA_SYNC);

ngh...  Why do these inline functions all have BUG_ONs in them?  If it
is to catch a programming error, why can't you just return EINVAL like
you do in the async function case?  (Even that WARN_ON seems
excessive).

Basically you're bringing a user's box down unnecessarily with BUG_ON here.

I need to look at this all more closely, but this caught my eye right away.

josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/