Re: [PATCH v2 8/8] p54: convert to sysdata API

From: Luis R. Rodriguez
Date: Fri Jun 17 2016 - 14:35:14 EST


On Thu, Jun 16, 2016 at 05:09:30PM -1000, Linus Torvalds wrote:
> On Thu, Jun 16, 2016 at 3:36 PM, Luis R. Rodriguez <mcgrof@xxxxxxxxxx> wrote:
> >
> > Reason this could not wait is folks seem to want to keep extending the API,
> > which is another reason for this, do we want to put an end to an unflexible
> > API now or should we wait ?
>
> So I absolutely abhor "changes for changes sake".
>
> If the existing code works for existing drivers, let them keep it.
>
> And if a new interface is truly more flexible, then it should be able
> to implement the old interface with no changes, so that drivers
> shouldn't need to be changed/upgraded.

Are we OK to leave only the usermode helper support in place for the
2 drivers I've noted that explicitly require the usermode helper in their
firmware request [0] ?

If so then I can do what you recommend below.

> Then, drivers that actually _want_ new features, or that can take
> advantage of new interfaces to actually make things *simpler*, can
> choose to make those changes. But those changes should have real
> advantages.

Sure agreed.

> Having to have a callback,

This is because devm is not used, as such the consumer is needed prior to
freeing. I can give adding devm support a shot if folks seem to be generally
agree its the right approach. I do expect this will mean tons of code
deletions and helping with bugs.

> or a magical "sysdata_desc" descriptor,

This is one way to make a flexible and extensible API without affecting drivers
with further collateral evolutions as it gets extended. Its no different than
the "flags" lesson learned from writing system calls, for instance.

Descriptor seemed, fitting, let me know if you have any other preference.

> and having a new name ("sysdata") that is less descriptive than the old one
> ("firmware")

Well no, the APIs are used in *a lot* of cases for things that are not firmware
already, and let's recall I originally started working on this to replace CRDA
from userspace to be able to just fetch the signed regulatory database file
from the kernel. Calling it firmware simply makes no sense anymore.

> are all in my opinion making the example patch be a
> step _backwards_ rather than an improvement. It does not look like a
> simpler or more natural interface for a driver.

Hope the above explains the current state. Feedback on desirable changes
welcomed.

[0] https://lkml.kernel.org/r/1466117661-22075-5-git-send-email-mcgrof@xxxxxxxxxx

Luis