Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1562642 > unrolled thread

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

Started byGreg KH <gregkh@linuxfoundation.org>
First post2017-01-19 12:50 +0100
Last post2017-01-27 22:40 +0100
Articles 14 — 4 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 3/3] p54: convert to sysdata API Greg KH <gregkh@linuxfoundation.org> - 2017-01-19 12:50 +0100
    Re: [PATCH v4 3/3] p54: convert to sysdata API "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-01-19 17:30 +0100
      Re: [PATCH v4 3/3] p54: convert to sysdata API Linus Torvalds <torvalds@linux-foundation.org> - 2017-01-26 23:00 +0100
        Re: [PATCH v4 3/3] p54: convert to sysdata API "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-01-27 19:30 +0100
          Re: [PATCH v4 3/3] p54: convert to sysdata API Linus Torvalds <torvalds@linux-foundation.org> - 2017-01-27 22:00 +0100
            Re: [PATCH v4 3/3] p54: convert to sysdata API "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-01-27 22:40 +0100
      Re: [PATCH v4 3/3] p54: convert to sysdata API "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-01-26 23:00 +0100
        Re: [PATCH v4 3/3] p54: convert to sysdata API Greg KH <gregkh@linuxfoundation.org> - 2017-01-27 08:50 +0100
          Re: [PATCH v4 3/3] p54: convert to sysdata API Rafał Miłecki <zajec5@gmail.com> - 2017-01-27 12:30 +0100
            Re: [PATCH v4 3/3] p54: convert to sysdata API Greg KH <gregkh@linuxfoundation.org> - 2017-01-27 15:10 +0100
              Re: [PATCH v4 3/3] p54: convert to sysdata API Rafał Miłecki <zajec5@gmail.com> - 2017-01-27 15:20 +0100
                Re: [PATCH v4 3/3] p54: convert to sysdata API Greg KH <gregkh@linuxfoundation.org> - 2017-01-27 15:40 +0100
                  Re: [PATCH v4 3/3] p54: convert to sysdata API Rafał Miłecki <zajec5@gmail.com> - 2017-01-27 15:50 +0100
                    Re: [PATCH v4 3/3] p54: convert to sysdata API "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-01-27 22:40 +0100

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

FromGreg KH <gregkh@linuxfoundation.org>
Date2017-01-19 12:50 +0100
SubjectRe: [PATCH v4 3/3] p54: convert to sysdata API
Message-ID<t1jo6-10B-11@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

#1562877

From"Luis R. Rodriguez" <mcgrof@kernel.org>
Date2017-01-19 17:30 +0100
Message-ID<t1nL3-3PP-19@gated-at.bofh.it>
In reply to#1562642
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@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?

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

[toc] | [prev] | [next] | [standalone]

#1567736

FromLinus Torvalds <torvalds@linux-foundation.org>
Date2017-01-26 23:00 +0100
Message-ID<t40fg-48w-9@gated-at.bofh.it>
In reply to#1562877
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?

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.

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.

                Linus

[toc] | [prev] | [next] | [standalone]

#1568604

From"Luis R. Rodriguez" <mcgrof@kernel.org>
Date2017-01-27 19:30 +0100
Message-ID<t4jrA-7BT-29@gated-at.bofh.it>
In reply to#1567736
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

[toc] | [prev] | [next] | [standalone]

#1568681

FromLinus Torvalds <torvalds@linux-foundation.org>
Date2017-01-27 22:00 +0100
Message-ID<t4lMJ-uW-3@gated-at.bofh.it>
In reply to#1568604
On Fri, Jan 27, 2017 at 10:23 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>> 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 I've said this before, and I'll say this one more time:

It's fine if we make the internal implementation of some generic "load
data from the filesystem or user" be this kind of new flexible API
that is internally called "driver_data_request()" or whatever.

But dammit, that is NOT AN EXCUSE for then making crap patches that
just replace the existing firmware users.

If the new interface cannot be wrapped in the old names (and the old
semantics) the new interface is shit and should never ever go
anywhere.

So leave the existing users alone. Concentrate on _only_ the parts
where  there is actual and real need of new features. Don't try to
rename or extend current drivers. Don't send out these patches that
make drivers actively uglier. Really.

I really question the whole thing when there are things like this going on.

            Linus

[toc] | [prev] | [next] | [standalone]

#1568692

From"Luis R. Rodriguez" <mcgrof@kernel.org>
Date2017-01-27 22:40 +0100
Message-ID<t4mpr-Y4-1@gated-at.bofh.it>
In reply to#1568681
On Fri, Jan 27, 2017 at 12:53:38PM -0800, Linus Torvalds wrote:
> On Fri, Jan 27, 2017 at 10:23 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> >> 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 I've said this before, and I'll say this one more time:
> 
> It's fine if we make the internal implementation of some generic "load
> data from the filesystem or user" be this kind of new flexible API
> that is internally called "driver_data_request()" or whatever.
> 
> But dammit, that is NOT AN EXCUSE for then making crap patches that
> just replace the existing firmware users.

Works with me.

> If the new interface cannot be wrapped in the old names (and the old
> semantics) the new interface is shit and should never ever go
> anywhere.

There's a few questionable things part of the old API which (UMH lock is one
used even if no UMH is used, the fallback mechanism another) so I've taken out
what I can truly vouch for and its all being shared on the driver_data API.
Extending the old API with yet-more flags is a big concern on my part so will
also recommend new functionality to be focused on the newer API.

> So leave the existing users alone. Concentrate on _only_ the parts
> where  there is actual and real need of new features. Don't try to
> rename or extend current drivers. Don't send out these patches that
> make drivers actively uglier. Really.

Right on.

  Luis

[toc] | [prev] | [next] | [standalone]

#1567740

From"Luis R. Rodriguez" <mcgrof@kernel.org>
Date2017-01-26 23:00 +0100
Message-ID<t40fg-48w-11@gated-at.bofh.it>
In reply to#1562877
On Thu, Jan 19, 2017 at 05:27:51PM +0100, Luis R. Rodriguez wrote:
> 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:
> > > ---
> > >  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.

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.

With this the only visible delta now is from taking advantage of new
features. In p54's case this would re-organize the mess in
drivers/net/wireless/intersil/p54/p54spi.c, the diff stat is a bit
larger for that file just because of this but I think in this case
its very much worth the small additions. In this case two routines are
added for handling the work through callbacks on a sync call.

 1 file changed, 38 insertions(+), 30 deletions(-)

diff --git a/drivers/net/wireless/intersil/p54/p54spi.c b/drivers/net/wireless/intersil/p54/p54spi.c
index 7ab2f43ab425..6183a8bfa149 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/driver_data.h>
 #include <linux/delay.h>
 #include <linux/irq.h>
 #include <linux/spi/spi.h>
@@ -168,47 +168,55 @@ static int p54spi_request_firmware(struct ieee80211_hw *dev)
 	int ret;
 
 	/* FIXME: should driver use it's own struct device? */
-	ret = request_firmware(&priv->firmware, "3826.arm", &priv->spi->dev);
-
-	if (ret < 0) {
-		dev_err(&priv->spi->dev, "request_firmware() failed: %d", ret);
+	ret = driver_data_request_simple("3826.arm", &priv->spi->dev,
+					 &priv->firmware);
+	if (ret < 0)
 		return ret;
-	}
 
 	ret = p54_parse_firmware(dev, priv->firmware);
 	if (ret) {
-		release_firmware(priv->firmware);
+		release_driver_data(priv->firmware);
 		return ret;
 	}
-
 	return 0;
 }
 
-static int p54spi_request_eeprom(struct ieee80211_hw *dev)
+#ifdef CONFIG_P54_SPI_DEFAULT_EEPROM
+static int p54spi_load_eeprom_default(void *context)
 {
-	struct p54s_priv *priv = dev->priv;
-	const struct firmware *eeprom;
-	int ret;
+	struct p54s_priv *priv = context;
+	struct ieee80211_hw *dev = priv->hw;
 
-	/* allow users to customize their eeprom.
-	 */
+	dev_info(&priv->spi->dev, "loading default eeprom...\n");
+	return p54_parse_eeprom(dev, (void *) p54spi_eeprom,
+				sizeof(p54spi_eeprom));
+}
+#endif
 
-	ret = request_firmware_direct(&eeprom, "3826.eeprom", &priv->spi->dev);
-	if (ret < 0) {
+static int p54spi_load_eeprom_cb(void *context,
+				 const struct driver_data *driver_data)
+{
+	struct p54s_priv *priv = context;
+	struct ieee80211_hw *dev = priv->hw;
+
+	dev_info(&priv->spi->dev, "loading user eeprom...\n");
+	return p54_parse_eeprom(dev, (void *) driver_data->data,
+				(int)driver_data->size);
+}
+static int p54spi_request_eeprom(struct ieee80211_hw *dev)
+{
+	struct p54s_priv *priv = dev->priv;
+	const struct driver_data_req_params req_params = {
+		DRIVER_DATA_DEFAULT_SYNC(p54spi_load_eeprom_cb, priv),
 #ifdef CONFIG_P54_SPI_DEFAULT_EEPROM
-		dev_info(&priv->spi->dev, "loading default eeprom...\n");
-		ret = p54_parse_eeprom(dev, (void *) p54spi_eeprom,
-				       sizeof(p54spi_eeprom));
-#else
-		dev_err(&priv->spi->dev, "Failed to request user eeprom\n");
-#endif /* CONFIG_P54_SPI_DEFAULT_EEPROM */
-	} else {
-		dev_info(&priv->spi->dev, "loading user eeprom...\n");
-		ret = p54_parse_eeprom(dev, (void *) eeprom->data,
-				       (int)eeprom->size);
-		release_firmware(eeprom);
-	}
-	return ret;
+		DRIVER_DATA_SYNC_OPT_CB(p54spi_load_eeprom_default, priv),
+#endif
+	};
+	/*
+	 * allow users to customize their eeprom.
+	 */
+	return driver_data_request("3826.eeprom", &req_params,
+				   &priv->spi->dev);
 }
 
 static int p54spi_upload_firmware(struct ieee80211_hw *dev)
@@ -692,7 +700,7 @@ static int p54spi_remove(struct spi_device *spi)
 
 	gpio_free(p54spi_gpio_power);
 	gpio_free(p54spi_gpio_irq);
-	release_firmware(priv->firmware);
+	release_driver_data(priv->firmware);
 
 	mutex_destroy(&priv->mutex);

[toc] | [prev] | [next] | [standalone]

#1567920

FromGreg KH <gregkh@linuxfoundation.org>
Date2017-01-27 08:50 +0100
Message-ID<t49sd-1kd-7@gated-at.bofh.it>
In reply to#1567740
On Thu, Jan 26, 2017 at 10:50:05PM +0100, Luis R. Rodriguez wrote:
> On Thu, Jan 19, 2017 at 05:27:51PM +0100, Luis R. Rodriguez wrote:
> > 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:
> > > > ---
> > > >  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.
> 
> 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.
> 
> With this the only visible delta now is from taking advantage of new
> features. In p54's case this would re-organize the mess in
> drivers/net/wireless/intersil/p54/p54spi.c, the diff stat is a bit
> larger for that file just because of this but I think in this case
> its very much worth the small additions. In this case two routines are
> added for handling the work through callbacks on a sync call.
> 
>  1 file changed, 38 insertions(+), 30 deletions(-)

I agree with Linus, as well as, look, it's still bigger, so you are
making driver developers do more work :(

>  	/* FIXME: should driver use it's own struct device? */
> -	ret = request_firmware(&priv->firmware, "3826.arm", &priv->spi->dev);
> -
> -	if (ret < 0) {
> -		dev_err(&priv->spi->dev, "request_firmware() failed: %d", ret);
> +	ret = driver_data_request_simple("3826.arm", &priv->spi->dev,
> +					 &priv->firmware);
> +	if (ret < 0)
>  		return ret;
> -	}

Hm, a FIXME that you aren't fixing :(

I still fail to see why this new api is worth it at all, sorry.

greg k-h

[toc] | [prev] | [next] | [standalone]

#1568179

FromRafał Miłecki <zajec5@gmail.com>
Date2017-01-27 12:30 +0100
Message-ID<t4cTa-3sv-65@gated-at.bofh.it>
In reply to#1567920
On 27 January 2017 at 08:47, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Jan 26, 2017 at 10:50:05PM +0100, Luis R. Rodriguez wrote:
>> On Thu, Jan 19, 2017 at 05:27:51PM +0100, Luis R. Rodriguez wrote:
>> > 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:
>> > > > ---
>> > > >  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.
>>
>> 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.
>>
>> With this the only visible delta now is from taking advantage of new
>> features. In p54's case this would re-organize the mess in
>> drivers/net/wireless/intersil/p54/p54spi.c, the diff stat is a bit
>> larger for that file just because of this but I think in this case
>> its very much worth the small additions. In this case two routines are
>> added for handling the work through callbacks on a sync call.
>>
>>  1 file changed, 38 insertions(+), 30 deletions(-)
>
> I agree with Linus, as well as, look, it's still bigger, so you are
> making driver developers do more work :(
>
>>       /* FIXME: should driver use it's own struct device? */
>> -     ret = request_firmware(&priv->firmware, "3826.arm", &priv->spi->dev);
>> -
>> -     if (ret < 0) {
>> -             dev_err(&priv->spi->dev, "request_firmware() failed: %d", ret);
>> +     ret = driver_data_request_simple("3826.arm", &priv->spi->dev,
>> +                                      &priv->firmware);
>> +     if (ret < 0)
>>               return ret;
>> -     }
>
> Hm, a FIXME that you aren't fixing :(
>
> I still fail to see why this new api is worth it at all, sorry.

Maybe we could try cleaning up existing firmware API and see if we
really hit something that can't be solved in any sane way? What do you
think?

I'd love to help with that, I started with a trivial cleaning patch:
[PATCH V2] firmware: simplify defining and handling FW_OPT_FALLBACK
https://patchwork.kernel.org/patch/9469875/

It didn't receive any real negative comments but I also have no idea
how could pick it up for me and send in some pull request. Any
suggestions?

-- 
Rafał

[toc] | [prev] | [next] | [standalone]

#1568387

FromGreg KH <gregkh@linuxfoundation.org>
Date2017-01-27 15:10 +0100
Message-ID<t4fnY-58H-23@gated-at.bofh.it>
In reply to#1568179
On Fri, Jan 27, 2017 at 12:25:48PM +0100, Rafał Miłecki wrote:
> On 27 January 2017 at 08:47, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Thu, Jan 26, 2017 at 10:50:05PM +0100, Luis R. Rodriguez wrote:
> >> On Thu, Jan 19, 2017 at 05:27:51PM +0100, Luis R. Rodriguez wrote:
> >> > 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:
> >> > > > ---
> >> > > >  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.
> >>
> >> 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.
> >>
> >> With this the only visible delta now is from taking advantage of new
> >> features. In p54's case this would re-organize the mess in
> >> drivers/net/wireless/intersil/p54/p54spi.c, the diff stat is a bit
> >> larger for that file just because of this but I think in this case
> >> its very much worth the small additions. In this case two routines are
> >> added for handling the work through callbacks on a sync call.
> >>
> >>  1 file changed, 38 insertions(+), 30 deletions(-)
> >
> > I agree with Linus, as well as, look, it's still bigger, so you are
> > making driver developers do more work :(
> >
> >>       /* FIXME: should driver use it's own struct device? */
> >> -     ret = request_firmware(&priv->firmware, "3826.arm", &priv->spi->dev);
> >> -
> >> -     if (ret < 0) {
> >> -             dev_err(&priv->spi->dev, "request_firmware() failed: %d", ret);
> >> +     ret = driver_data_request_simple("3826.arm", &priv->spi->dev,
> >> +                                      &priv->firmware);
> >> +     if (ret < 0)
> >>               return ret;
> >> -     }
> >
> > Hm, a FIXME that you aren't fixing :(
> >
> > I still fail to see why this new api is worth it at all, sorry.
> 
> Maybe we could try cleaning up existing firmware API and see if we
> really hit something that can't be solved in any sane way? What do you
> think?
> 
> I'd love to help with that, I started with a trivial cleaning patch:
> [PATCH V2] firmware: simplify defining and handling FW_OPT_FALLBACK
> https://patchwork.kernel.org/patch/9469875/
> 
> It didn't receive any real negative comments but I also have no idea
> how could pick it up for me and send in some pull request. Any
> suggestions?

Does that patch really "simplify" anything?  Anyway, resend it if the
maintainer of the subsystem ignores it (you did cc: the correct people,
right?)

And yes, I always like seeing things that simplify apis, and the
firmware interface could really use that, which I thought is what Luis
was trying to do here, which is why I keep pushing back on this
patchset as it doesn't seem to be happening.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1568401

FromRafał Miłecki <zajec5@gmail.com>
Date2017-01-27 15:20 +0100
Message-ID<t4fxE-5cg-19@gated-at.bofh.it>
In reply to#1568387
On 27 January 2017 at 15:07, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Jan 27, 2017 at 12:25:48PM +0100, Rafał Miłecki wrote:
>> On 27 January 2017 at 08:47, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Thu, Jan 26, 2017 at 10:50:05PM +0100, Luis R. Rodriguez wrote:
>> >> On Thu, Jan 19, 2017 at 05:27:51PM +0100, Luis R. Rodriguez wrote:
>> >> > 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:
>> >> > > > ---
>> >> > > >  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.
>> >>
>> >> 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.
>> >>
>> >> With this the only visible delta now is from taking advantage of new
>> >> features. In p54's case this would re-organize the mess in
>> >> drivers/net/wireless/intersil/p54/p54spi.c, the diff stat is a bit
>> >> larger for that file just because of this but I think in this case
>> >> its very much worth the small additions. In this case two routines are
>> >> added for handling the work through callbacks on a sync call.
>> >>
>> >>  1 file changed, 38 insertions(+), 30 deletions(-)
>> >
>> > I agree with Linus, as well as, look, it's still bigger, so you are
>> > making driver developers do more work :(
>> >
>> >>       /* FIXME: should driver use it's own struct device? */
>> >> -     ret = request_firmware(&priv->firmware, "3826.arm", &priv->spi->dev);
>> >> -
>> >> -     if (ret < 0) {
>> >> -             dev_err(&priv->spi->dev, "request_firmware() failed: %d", ret);
>> >> +     ret = driver_data_request_simple("3826.arm", &priv->spi->dev,
>> >> +                                      &priv->firmware);
>> >> +     if (ret < 0)
>> >>               return ret;
>> >> -     }
>> >
>> > Hm, a FIXME that you aren't fixing :(
>> >
>> > I still fail to see why this new api is worth it at all, sorry.
>>
>> Maybe we could try cleaning up existing firmware API and see if we
>> really hit something that can't be solved in any sane way? What do you
>> think?
>>
>> I'd love to help with that, I started with a trivial cleaning patch:
>> [PATCH V2] firmware: simplify defining and handling FW_OPT_FALLBACK
>> https://patchwork.kernel.org/patch/9469875/
>>
>> It didn't receive any real negative comments but I also have no idea
>> how could pick it up for me and send in some pull request. Any
>> suggestions?
>
> Does that patch really "simplify" anything?  Anyway, resend it if the
> maintainer of the subsystem ignores it (you did cc: the correct people,
> right?)

According to the MAINTAINERS there isn't firmware API tree /
maintainer. Also this is just a cleanup so I don't know if I should
expect some random maintainer (e.g. wireless tree) to pick it.

-- 
Rafał

[toc] | [prev] | [next] | [standalone]

#1568403

FromGreg KH <gregkh@linuxfoundation.org>
Date2017-01-27 15:40 +0100
Message-ID<t4fQZ-5j4-1@gated-at.bofh.it>
In reply to#1568401
On Fri, Jan 27, 2017 at 03:14:14PM +0100, Rafał Miłecki wrote:
> > Does that patch really "simplify" anything?  Anyway, resend it if the
> > maintainer of the subsystem ignores it (you did cc: the correct people,
> > right?)
> 
> According to the MAINTAINERS there isn't firmware API tree /
> maintainer. Also this is just a cleanup so I don't know if I should
> expect some random maintainer (e.g. wireless tree) to pick it.

I don't think you looked very hard:

 $ ./scripts/get_maintainer.pl --file drivers/base/firmware_class.c
 Ming Lei <ming.lei@canonical.com> (maintainer:FIRMWARE LOADER (request_firmware))
 "Luis R. Rodriguez" <mcgrof@kernel.org> (maintainer:FIRMWARE LOADER (request_firmware))
 Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:DRIVER CORE, KOBJECTS, DEBUGFS, KERNFS AND SYSFS)
 linux-kernel@vger.kernel.org (open list:FIRMWARE LOADER (request_firmware))

Please try again...

greg k-h

[toc] | [prev] | [next] | [standalone]

#1568412

FromRafał Miłecki <zajec5@gmail.com>
Date2017-01-27 15:50 +0100
Message-ID<t4g0H-5mK-27@gated-at.bofh.it>
In reply to#1568403
On 27 January 2017 at 15:30, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Jan 27, 2017 at 03:14:14PM +0100, Rafał Miłecki wrote:
>> > Does that patch really "simplify" anything?  Anyway, resend it if the
>> > maintainer of the subsystem ignores it (you did cc: the correct people,
>> > right?)
>>
>> According to the MAINTAINERS there isn't firmware API tree /
>> maintainer. Also this is just a cleanup so I don't know if I should
>> expect some random maintainer (e.g. wireless tree) to pick it.
>
> I don't think you looked very hard:
>
>  $ ./scripts/get_maintainer.pl --file drivers/base/firmware_class.c
>  Ming Lei <ming.lei@canonical.com> (maintainer:FIRMWARE LOADER (request_firmware))
>  "Luis R. Rodriguez" <mcgrof@kernel.org> (maintainer:FIRMWARE LOADER (request_firmware))
>  Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:DRIVER CORE, KOBJECTS, DEBUGFS, KERNFS AND SYSFS)
>  linux-kernel@vger.kernel.org (open list:FIRMWARE LOADER (request_firmware))
>
> Please try again...

My memory totally failed me :( Sorry

-- 
Rafał

[toc] | [prev] | [next] | [standalone]

#1568700

From"Luis R. Rodriguez" <mcgrof@kernel.org>
Date2017-01-27 22:40 +0100
Message-ID<t4mps-Y4-21@gated-at.bofh.it>
In reply to#1568412
On Fri, Jan 27, 2017 at 03:39:36PM +0100, Rafał Miłecki wrote:
> On 27 January 2017 at 15:30, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Fri, Jan 27, 2017 at 03:14:14PM +0100, Rafał Miłecki wrote:
> >> > Does that patch really "simplify" anything?  Anyway, resend it if the
> >> > maintainer of the subsystem ignores it (you did cc: the correct people,
> >> > right?)
> >>
> >> According to the MAINTAINERS there isn't firmware API tree /
> >> maintainer. Also this is just a cleanup so I don't know if I should
> >> expect some random maintainer (e.g. wireless tree) to pick it.
> >
> > I don't think you looked very hard:
> >
> >  $ ./scripts/get_maintainer.pl --file drivers/base/firmware_class.c
> >  Ming Lei <ming.lei@canonical.com> (maintainer:FIRMWARE LOADER (request_firmware))
> >  "Luis R. Rodriguez" <mcgrof@kernel.org> (maintainer:FIRMWARE LOADER (request_firmware))
> >  Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:DRIVER CORE, KOBJECTS, DEBUGFS, KERNFS AND SYSFS)
> >  linux-kernel@vger.kernel.org (open list:FIRMWARE LOADER (request_firmware))
> >
> > Please try again...
> 
> My memory totally failed me :( Sorry

I was eventually copied on the patch but there was also a side discussion on
you wanting FW_OPT_NO_WARN. Can you review if the proposed driver_data API does
what I think you wanted. Although request_firmware_direct() only avoids warning
on a sync all the diver_data API also supports this for async calls. If its
something else I'd prefer we evaluate extending the driver_data API before
loosely adding yet another API call using the old API.

  Luis

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web