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

[Luca Coelho <luca@coelho.fi>]

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.