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

[Luis R. Rodriguez]

On Thu, Jan 19, 2017 at 12:38:57PM +0100, Greg KH wrote:
> On Thu, Jan 12, 2017 at 07:02:44AM -0800, Luis R. Rodriguez wrote:
> > The Coccinelle sysdata patches were used to help with
> > this transition. The changes have been carefully manually
> > vetted for. With the conversion we modify the cases that do
> > not need the firmware to be kept so that the sysdata API
> > can release it for us. Using the new sysdata API also means
> > we can get rid of our own completions.
> > 
> > v2: was not present
> > v3: initial release
> > v4: small cosmetic fixes
> > v5: bike shed changes
> > v6: forgot to change one piece of code during the bikeshed name change
> > 
> > Generated-by: Coccinelle SmPL
> 
> What is this tag for?

Every no wand then some tool scrapes for commit logs to see if
Coccinelle was used to help with a kernel commit. There are different
heuristics, this tag is to help make a more unique search more easily
identifiable as I used Coccinelle to do the original port.

> Also, meta-comment, put your vN: lines below the --- line like the
> kernel documentation says to do.

Oh, OK sounds good.

> > Signed-off-by: Luis R. Rodriguez <mcgrof@xxxxxxxxxx>
> > ---
> >  drivers/net/wireless/intersil/p54/eeprom.c |  2 +-
> >  drivers/net/wireless/intersil/p54/fwio.c   |  5 +-
> >  drivers/net/wireless/intersil/p54/led.c    |  2 +-
> >  drivers/net/wireless/intersil/p54/main.c   |  2 +-
> >  drivers/net/wireless/intersil/p54/p54.h    |  3 +-
> >  drivers/net/wireless/intersil/p54/p54pci.c | 26 ++++++----
> >  drivers/net/wireless/intersil/p54/p54pci.h |  4 +-
> >  drivers/net/wireless/intersil/p54/p54spi.c | 80 +++++++++++++++++++-----------
> >  drivers/net/wireless/intersil/p54/p54spi.h |  2 +-
> >  drivers/net/wireless/intersil/p54/p54usb.c | 18 +++----
> >  drivers/net/wireless/intersil/p54/p54usb.h |  4 +-
> >  drivers/net/wireless/intersil/p54/txrx.c   |  2 +-
> >  12 files changed, 89 insertions(+), 61 deletions(-)
> 
> why does the "new" api require more lines?

This is a bare bones flexible API with only a few new tiny features to start
with, one of them was to enable the API do the freeing of the driver data for
you. In the kernel we have devres to help with this but devres only helps if
you would use the API call on probe. We want to support the ability to let the
API free the driver data for you even if your call is outside of probe, for this
to work we need a callback. For async calls this is rather trivial given we
already have a callback, for sync calls this means a new routine is needed.
Freeing the data for you is an option, but I decided to keep the callback
requirement even if you didn't want the free'ing to be done for you. The
addition of a callback is what accounts for the slight increase on this driver.

I could try avoiding the callback if no freeing is needed.

> > --- a/drivers/net/wireless/intersil/p54/p54spi.c
> > +++ b/drivers/net/wireless/intersil/p54/p54spi.c
> > @@ -162,53 +162,73 @@ static int p54spi_spi_write_dma(struct p54s_priv *priv, __le32 base,
> >  	return 0;
> >  }
> >  
> > +static int p54spi_request_firmware_found_cb(void *context,
> > +					    const struct drvdata *drvdata)
> > +{
> > +	int ret;
> > +	struct p54s_priv *priv = context;
> > +
> > +	priv->firmware = drvdata;
> > +	ret = p54_parse_firmware(priv->hw, priv->firmware);
> > +	if (ret)
> > +		release_drvdata(priv->firmware);
> > +
> > +	return ret;
> > +}
> > +
> >  static int p54spi_request_firmware(struct ieee80211_hw *dev)
> >  {
> >  	struct p54s_priv *priv = dev->priv;
> > +	const struct drvdata_req_params req_params = {
> > +		DRVDATA_KEEP_SYNC(p54spi_request_firmware_found_cb, priv),
> > +	};
> >  	int ret;
> >  
> >  	/* FIXME: should driver use it's own struct device? */
> > -	ret = request_firmware(&priv->firmware, "3826.arm", &priv->spi->dev);
> > -
> > +	ret = drvdata_request("3826.arm", &req_params, &priv->spi->dev);
> >  	if (ret < 0) {
> > -		dev_err(&priv->spi->dev, "request_firmware() failed: %d", ret);
> > -		return ret;
> > +		dev_err(&priv->spi->dev,
> > +			"firmware request failed: %d", ret);
> 
> shouldn't the call report this error to the kernel log?  Why must each
> user print it out themselves again?

Great point. The API already has:

static int _drvdata_request(const struct drvdata **drvdata_p,
                           const char *name,
                           const struct drvdata_req_params *params,
                           struct device *device)
{
...
       if (ret && !params->optional)
               pr_err("Direct driver data load for %s failed with error %d\n",
                      name, ret);
...
}

So it is not needed for driver to moan about failures here.

  Luis