Re: [PATCH v4 3/3] p54: convert to sysdata API

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

On Thu, Jan 26, 2017 at 01:54:20PM -0800, Linus Torvalds wrote:
> On Thu, Jan 26, 2017 at 1:50 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> >
> > OK I've added a respective helper call which would map 1-1 with the
> > old sync mechanism to enable a 1-1 change, this will be called
> > driver_data_request_simple(), but let me know if there is a preference
> > for something else.
> 
> So just looking at this patch, what's the *advantage* to the driver writer?

So for the driver writer it provides a clean way to logically split up what is
to be done for certain situations if the firmware is not present or is present.
Without this the code is a bit unruly, and this is actually a mild case. There
are much crazier chained conditionals (iwlwifi is one that has a long chain of
firmwares) but a goal here was to just provide only the most basic bump in
logic so that further enhancements/functionality is added later.

> Apart from the actual new feature, this patch seems to actively make
> the driver uglier.
> 
> I mentioned this before, but replacing "request_firmware()" with
> "driver_data_request_simple()" is SIMPLY NOT AN IMPROVEMENT.

I strongly agree with this.

> The new name is longer and _less_ descriptive.
> 
> So I'm really not seeing why you want to make these conversions that
> just make code worse.

The real goal here was first to actually provide a flexible API to enable
more advanced features to be added without having to affect existing
callers, as has been done before. So hence the const struct driver_data_req_params
approach and only two basic calls -- a sync and async call. This was after long ago we
had revised how we would go about adding firmware signing support to the kernel.

My first approach in addressing firmware signing was to mimic how we handle
have module signing: everyone gets it (even those on the old API), using one
default key. The flexible API was then a secondary step, to enable users to
customize signature requirements.  As we discussed things it was clear that we
wanted the ability to support firmware signing with the ability to provide
alternative key requirements from the very start. Having an extensible firmware API
in place first would enable the flexibility to let us decide what requirements
we want to put in place for firmware signing without concern for making a slew
of collateral evolutions as requirements change.

The flexible API then would be, as is in this series, completely optional.
Only if you want to reap benefit of some of the new features would you use it.

So unless the flexible API is reproachable in and of itself perhaps the thing
to do is leave all drivers as-is (without no conversion) and only convert
once we have a full gain value-add. For instance later adding support to easily
chain a series of firmware requests (not just 2), or once we have firmware
signing support and a driver want to reap benefit from it.

Thoughts ?

  Luis