linux-kernel - Re: [PATCH v7 1/5] firmware: add extensible driver data params

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170511182818.GZ28800@wotan.suse.de>
Date:   Thu, 11 May 2017 20:28:18 +0200
From:   "Luis R. Rodriguez" <mcgrof@...e.com>
To:     "Li, Yi" <yi1.li@...ux.intel.com>
Cc:     "Luis R. Rodriguez" <mcgrof@...nel.org>,
        gregkh@...uxfoundation.org, 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 Thu, May 11, 2017 at 01:17:39PM -0500, Li, Yi wrote:
> 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);
> 

Indeed, its just to ensure if the user makes a mistake and uses a sync
callback for an async request we get a compile error, all without using
typdefs or anything nasty.

  Luis