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

[Greg KH <gregkh@linuxfoundation.org>]

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?

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

> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
>  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?


> 
> diff --git a/drivers/net/wireless/intersil/p54/eeprom.c b/drivers/net/wireless/intersil/p54/eeprom.c
> index d4c73d39336f..b8184cbc6770 100644
> --- a/drivers/net/wireless/intersil/p54/eeprom.c
> +++ b/drivers/net/wireless/intersil/p54/eeprom.c
> @@ -16,7 +16,7 @@
>   * published by the Free Software Foundation.
>   */
>  
> -#include <linux/firmware.h>
> +#include <linux/drvdata.h>
>  #include <linux/etherdevice.h>
>  #include <linux/sort.h>
>  #include <linux/slab.h>
> diff --git a/drivers/net/wireless/intersil/p54/fwio.c b/drivers/net/wireless/intersil/p54/fwio.c
> index 4ac6764f4897..dc27049e4533 100644
> --- a/drivers/net/wireless/intersil/p54/fwio.c
> +++ b/drivers/net/wireless/intersil/p54/fwio.c
> @@ -17,7 +17,7 @@
>   */
>  
>  #include <linux/slab.h>
> -#include <linux/firmware.h>
> +#include <linux/drvdata.h>
>  #include <linux/etherdevice.h>
>  #include <linux/export.h>
>  
> @@ -27,7 +27,8 @@
>  #include "eeprom.h"
>  #include "lmac.h"
>  
> -int p54_parse_firmware(struct ieee80211_hw *dev, const struct firmware *fw)
> +int p54_parse_firmware(struct ieee80211_hw *dev,
> +		       const struct drvdata *fw)
>  {
>  	struct p54_common *priv = dev->priv;
>  	struct exp_if *exp_if;
> diff --git a/drivers/net/wireless/intersil/p54/led.c b/drivers/net/wireless/intersil/p54/led.c
> index 9a8fedd3c0f5..4d13598d3968 100644
> --- a/drivers/net/wireless/intersil/p54/led.c
> +++ b/drivers/net/wireless/intersil/p54/led.c
> @@ -16,7 +16,7 @@
>   * published by the Free Software Foundation.
>   */
>  
> -#include <linux/firmware.h>
> +#include <linux/drvdata.h>
>  #include <linux/etherdevice.h>
>  
>  #include <net/mac80211.h>
> diff --git a/drivers/net/wireless/intersil/p54/main.c b/drivers/net/wireless/intersil/p54/main.c
> index d5a3bf91a03e..a1c546cd232c 100644
> --- a/drivers/net/wireless/intersil/p54/main.c
> +++ b/drivers/net/wireless/intersil/p54/main.c
> @@ -17,7 +17,7 @@
>   */
>  
>  #include <linux/slab.h>
> -#include <linux/firmware.h>
> +#include <linux/drvdata.h>
>  #include <linux/etherdevice.h>
>  #include <linux/module.h>
>  
> diff --git a/drivers/net/wireless/intersil/p54/p54.h b/drivers/net/wireless/intersil/p54/p54.h
> index 529939e611cd..5bbe9d77e5fc 100644
> --- a/drivers/net/wireless/intersil/p54/p54.h
> +++ b/drivers/net/wireless/intersil/p54/p54.h
> @@ -268,7 +268,8 @@ struct p54_common {
>  /* interfaces for the drivers */
>  int p54_rx(struct ieee80211_hw *dev, struct sk_buff *skb);
>  void p54_free_skb(struct ieee80211_hw *dev, struct sk_buff *skb);
> -int p54_parse_firmware(struct ieee80211_hw *dev, const struct firmware *fw);
> +int p54_parse_firmware(struct ieee80211_hw *dev,
> +		       const struct drvdata *fw);
>  int p54_parse_eeprom(struct ieee80211_hw *dev, void *eeprom, int len);
>  int p54_read_eeprom(struct ieee80211_hw *dev);
>  
> diff --git a/drivers/net/wireless/intersil/p54/p54pci.c b/drivers/net/wireless/intersil/p54/p54pci.c
> index 27a49068d32d..0e7fd9ba7186 100644
> --- a/drivers/net/wireless/intersil/p54/p54pci.c
> +++ b/drivers/net/wireless/intersil/p54/p54pci.c
> @@ -15,7 +15,7 @@
>  
>  #include <linux/pci.h>
>  #include <linux/slab.h>
> -#include <linux/firmware.h>
> +#include <linux/drvdata.h>
>  #include <linux/etherdevice.h>
>  #include <linux/delay.h>
>  #include <linux/completion.h>
> @@ -490,7 +490,7 @@ static int p54p_open(struct ieee80211_hw *dev)
>  	return 0;
>  }
>  
> -static void p54p_firmware_step2(const struct firmware *fw,
> +static void p54p_firmware_step2(const struct drvdata *fw,
>  				void *context)
>  {
>  	struct p54p_priv *priv = context;
> @@ -520,8 +520,6 @@ static void p54p_firmware_step2(const struct firmware *fw,
>  
>  out:
>  
> -	complete(&priv->fw_loaded);
> -
>  	if (err) {
>  		struct device *parent = pdev->dev.parent;
>  
> @@ -542,6 +540,17 @@ static void p54p_firmware_step2(const struct firmware *fw,
>  	pci_dev_put(pdev);
>  }
>  
> +static int p54p_load_firmware(struct p54p_priv *priv)
> +{
> +	const struct drvdata_req_params req_params = {
> +		DRVDATA_KEEP_ASYNC(p54p_firmware_step2, priv),
> +	};
> +
> +	return drvdata_request_async("isl3886pci", &req_params,
> +				     &priv->pdev->dev,
> +				     &priv->fw_async_cookie);
> +}
> +
>  static int p54p_probe(struct pci_dev *pdev,
>  				const struct pci_device_id *id)
>  {
> @@ -595,7 +604,6 @@ static int p54p_probe(struct pci_dev *pdev,
>  	priv = dev->priv;
>  	priv->pdev = pdev;
>  
> -	init_completion(&priv->fw_loaded);
>  	SET_IEEE80211_DEV(dev, &pdev->dev);
>  	pci_set_drvdata(pdev, dev);
>  
> @@ -620,9 +628,7 @@ static int p54p_probe(struct pci_dev *pdev,
>  	spin_lock_init(&priv->lock);
>  	tasklet_init(&priv->tasklet, p54p_tasklet, (unsigned long)dev);
>  
> -	err = request_firmware_nowait(THIS_MODULE, 1, "isl3886pci",
> -				      &priv->pdev->dev, GFP_KERNEL,
> -				      priv, p54p_firmware_step2);
> +	err = p54p_load_firmware(priv);
>  	if (!err)
>  		return 0;
>  
> @@ -652,9 +658,9 @@ static void p54p_remove(struct pci_dev *pdev)
>  		return;
>  
>  	priv = dev->priv;
> -	wait_for_completion(&priv->fw_loaded);
> +	drvdata_synchronize_request(priv->fw_async_cookie);
>  	p54_unregister_common(dev);
> -	release_firmware(priv->firmware);
> +	release_drvdata(priv->firmware);
>  	pci_free_consistent(pdev, sizeof(*priv->ring_control),
>  			    priv->ring_control, priv->ring_control_dma);
>  	iounmap(priv->map);
> diff --git a/drivers/net/wireless/intersil/p54/p54pci.h b/drivers/net/wireless/intersil/p54/p54pci.h
> index 68405c142f97..00c30e1fc60b 100644
> --- a/drivers/net/wireless/intersil/p54/p54pci.h
> +++ b/drivers/net/wireless/intersil/p54/p54pci.h
> @@ -94,7 +94,7 @@ struct p54p_priv {
>  	struct pci_dev *pdev;
>  	struct p54p_csr __iomem *map;
>  	struct tasklet_struct tasklet;
> -	const struct firmware *firmware;
> +	const struct drvdata *firmware;
>  	spinlock_t lock;
>  	struct p54p_ring_control *ring_control;
>  	dma_addr_t ring_control_dma;
> @@ -105,7 +105,7 @@ struct p54p_priv {
>  	struct sk_buff *tx_buf_data[32];
>  	struct sk_buff *tx_buf_mgmt[4];
>  	struct completion boot_comp;
> -	struct completion fw_loaded;
> +	async_cookie_t fw_async_cookie;
>  };
>  
>  #endif /* P54USB_H */
> diff --git a/drivers/net/wireless/intersil/p54/p54spi.c b/drivers/net/wireless/intersil/p54/p54spi.c
> index 7ab2f43ab425..c0118048c01f 100644
> --- a/drivers/net/wireless/intersil/p54/p54spi.c
> +++ b/drivers/net/wireless/intersil/p54/p54spi.c
> @@ -23,7 +23,7 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/interrupt.h>
> -#include <linux/firmware.h>
> +#include <linux/drvdata.h>
>  #include <linux/delay.h>
>  #include <linux/irq.h>
>  #include <linux/spi/spi.h>
> @@ -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?

thanks,

greg k-h