linux-kernel - Re: [PATCH v6 2/5] firmware: add extensible driver data API

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: <1493273387.14233.13.camel@coelho.fi>
Date:   Thu, 27 Apr 2017 09:09:47 +0300
From:   Luca Coelho <luca@...lho.fi>
To:     "Luis R. Rodriguez" <mcgrof@...nel.org>
Cc:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "pjones@...hat.com" <pjones@...hat.com>,
        "moritz.fischer@...us.com" <moritz.fischer@...us.com>,
        "takahiro.akashi@...aro.org" <takahiro.akashi@...aro.org>,
        "dhowells@...hat.com" <dhowells@...hat.com>,
        "pmladek@...e.com" <pmladek@...e.com>,
        "Berg, Johannes" <johannes.berg@...el.com>,
        "rjw@...ysocki.net" <rjw@...ysocki.net>,
        "yi1.li@...ux.intel.com" <yi1.li@...ux.intel.com>,
        "kvalo@...eaurora.org" <kvalo@...eaurora.org>,
        "luto@...nel.org" <luto@...nel.org>,
        "arend.vanspriel@...adcom.com" <arend.vanspriel@...adcom.com>,
        "rafal@...ecki.pl" <rafal@...ecki.pl>,
        "dwmw2@...radead.org" <dwmw2@...radead.org>,
        "wagi@...om.org" <wagi@...om.org>,
        "atull@...nsource.altera.com" <atull@...nsource.altera.com>,
        "Grumbach, Emmanuel" <emmanuel.grumbach@...el.com>
Subject: Re: [PATCH v6 2/5] firmware: add extensible driver data API

On Thu, 2017-04-27 at 05:16 +0200, Luis R. Rodriguez wrote:
> > > +int driver_data_request_sync(const char *name,
> > > +                        const struct driver_data_req_params *req_params,
> > > +                        struct device *device)
> > > +{
> > > +   const struct firmware *driver_data;
> > > +   const struct driver_data_reqs *sync_reqs;
> > > +   struct driver_data_params params = {
> > > +           .req_params = *req_params,
> > > +   };
> > > +   int ret;
> > > +
> > > +   if (!device || !req_params || !name || name[0] == '\0')
> > > +           return -EINVAL;
> > > +
> > > +   if (req_params->sync_reqs.mode != DRIVER_DATA_SYNC)
> > > +           return -EINVAL;
> > 
> > Why do you need to check this here? If the caller is calling _sync(),
> > it's because that's what it needs.  This mode value here seems
> > redundant.
> 
> Because we allow one data structure to be used for the specified requirements
> and technically someone can make a mistake and use the wrong macros to set up
> the data structure. This ensures users don't async macros to set up the
> parameters and then use the sync call. Eventually the underlying
> firmware_class.c code does its own conditional checks on this as well.

If this could only happen in a programming error, maybe it's worth a
WARN() then, to make it easier to spot?


> > OTOH, if you do have a reason for this value, then you could use
> > driver_data_request_sync() in this if.
> 
> You mean to allow *one* API call for all. Sure, that's possible, but I think
> its nicer to at least expose async/sync mechanisms on the caller side. The
> sync/async differences seem like a reasonable enough place to split the API.

I don't remember the details of this anymore, but doesn't the
driver_data_sync() function does exactly the same check? I meant that
you could do this:

	if(WARN_ON_ONCE(!driver_data_request_sync()))
		return -EINVAL;


And yes, I think either a single API call for all or not having the mode
in the struct would be cleaner.  I think there are better ways to
prevent coding errors (since this should only happen if the user code is
implemented incorrectly).

--
Cheers,
Luca.