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

["Luis R. Rodriguez" <mcgrof@kernel.org>]

On Thu, Apr 27, 2017 at 09:09:47AM +0300, Luca Coelho wrote:
> 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).

You're kind of right, this is stupid. The mode is already implied by the
function used, and its an internal thing, so we can just deal with it
ourselves. The reason why we can't just use the sync cb is its completely
optional, we need a piece of information to tell the internal code its
purpose so it can decide what data structures to check for.

I'll try folding the mode into the priv params and remove the checks.

  Luis