Re: Re: [PATCH] mwifiex: add tdls uapsd support module parameter

[Dmitry Torokhov]

On Thu, Jul 20, 2017 at 10:16 AM, Brian Norris <briannor...@chromium.org> wrote:
> Hi,
>
> On Thu, Jul 20, 2017 at 10:54:16AM +, Xinming Hu wrote:
>> Hi Brian,
>>
>> > -Original Message-
>> > From: Brian Norris [mailto:briannor...@chromium.org]
>> > Sent: 2017年7月20日 4:10
>> > To: Xinming Hu
>> > Cc: Linux Wireless; Kalle Valo; Dmitry Torokhov; raja...@google.com; 
>> > Zhiyuan
>> > Yang; Tim Song; Cathy Luo; Xinming Hu
>> > Subject: [EXT] Re: [PATCH] mwifiex: add tdls uapsd support module parameter
>> >
>> > External Email
>> >
>> > --
>> > On Wed, Jul 19, 2017 at 06:36:27AM +, Xinming Hu wrote:
>> > > From: Xinming Hu <h...@marvell.com>
>> > >
>> > > Add module parameter to control tdls uapsd support, which is default
>> > > disabled.
>> >
>> > Why default disabled, when it looks like it used to be on by default?
>>
>> Oho, I should comment more in description, it is now confusing.
>> We just fixed an tdls uapsd issue in latest firmware, so try to disable
>> this feature as a workaround to the old firmware. At the same time,
>> it is optional to enable this feature in specific case.
>
> That helps a bit, thanks.
>
>> I will add more comments in V2.
>> Please let us know if you have more comments.
>
> I won't insist on changes, but for something like this, it seems
> reasonable (if you have really fixed the issue in the latest firmware)
> to just provide the knob to disable as a backup, not as a default. If
> someone is going to update their kernel (to include this patch), but not
> update their firmware, then they probably should know enough to be able
> to provide the module parameter to disable.
>
> Or alternatively: how is anyone supposed to know whether their current
> firmware is broken or not? And how is this feature ever going to be
> default-enabled again? New chipsets with new firmware should hopefully
> not have the same bugs, no?

Better yet, the driver could check the firmware version, and if it is
known bad disable the feature automatically. Then there is no need to
provide this knob.

Thanks.

-- 
Dmitry

Re: [PATCH] mwifiex: correct channel stat buffer overflows

[Dmitry Torokhov]

On Thu, Jun 29, 2017 at 06:23:54PM -0700, Brian Norris wrote:
> mwifiex records information about various channels as it receives scan
> information. It does this by appending to a buffer that was sized
> to the max number of supported channels on any band, but there are
> numerous problems:
> 
> (a) scans can return info from more than one band (e.g., both 2.4 and 5
> GHz), so the determined "max" is not large enough
> (b) some firmware appears to return multiple results for a given
> channel, so the max *really* isn't large enough
> (c) there is no bounds checking when stashing these stats, so problems
> (a) and (b) can easily lead to buffer overflows
> 
> Let's patch this by setting a slightly-more-correct max (that accounts
> for a combination of both 2.4G and 5G bands) and adding a bounds check
> when writing to our statistics buffer.
> 
> Due to problem (b), we still might not properly report all known survey
> information (e.g., with "iw  survey dump"), since duplicate results
> (or otherwise "larger than expected" results) will cause some
> truncation. But that's a problem for a future bugfix.
> 
> (And because of this known deficiency, only log the excess at the WARN
> level, since that isn't visible by default in this driver and would
> otherwise be a bit too noisy.)
> 
> Fixes: bf35443314ac ("mwifiex: channel statistics support for mwifiex")
> Cc: <sta...@vger.kernel.org>
> Cc: Avinash Patil <pat...@marvell.com>
> Cc: Xinming Hu <h...@marvell.com>
> Signed-off-by: Brian Norris <briannor...@chromium.org>
> ---
> I've got a ton of other patches still queued up locally, and I hope to send
> them soon. But I realized this one is a nasty bug (with a trivial fix), so 
> it's
> probably best to get this out the door quickly.

This does make sense to me.

Reviewed-by: Dmitry Torokhov <dmitry.torok...@gmail.com>

> 
>  drivers/net/wireless/marvell/mwifiex/cfg80211.c | 2 +-
>  drivers/net/wireless/marvell/mwifiex/scan.c | 6 ++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c 
> b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> index a850ec0054e2..82f4e796ed39 100644
> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> @@ -4219,7 +4219,7 @@ int mwifiex_init_channel_scan_gap(struct 
> mwifiex_adapter *adapter)
>   if (adapter->config_bands & BAND_A)
>   n_channels_a = mwifiex_band_5ghz.n_channels;
>  
> - adapter->num_in_chan_stats = max_t(u32, n_channels_bg, n_channels_a);
> + adapter->num_in_chan_stats = n_channels_bg + n_channels_a;
>   adapter->chan_stats = vmalloc(sizeof(*adapter->chan_stats) *
> adapter->num_in_chan_stats);
>  
> diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c 
> b/drivers/net/wireless/marvell/mwifiex/scan.c
> index ae9630b49342..9900855746ac 100644
> --- a/drivers/net/wireless/marvell/mwifiex/scan.c
> +++ b/drivers/net/wireless/marvell/mwifiex/scan.c
> @@ -2492,6 +2492,12 @@ mwifiex_update_chan_statistics(struct mwifiex_private 
> *priv,
> sizeof(struct mwifiex_chan_stats);
>  
>   for (i = 0 ; i < num_chan; i++) {
> + if (adapter->survey_idx >= adapter->num_in_chan_stats) {
> + mwifiex_dbg(adapter, WARN,
> + "FW reported too many channel results (max 
> %d)\n",
> + adapter->num_in_chan_stats);
> + return;
> + }
>   chan_stats.chan_num = fw_chan_stats->chan_num;
>   chan_stats.bandcfg = fw_chan_stats->bandcfg;
>   chan_stats.flags = fw_chan_stats->flags;
> -- 
> 2.13.2.725.g09c95d1e9-goog
> 

-- 
Dmitry

Re: [PATCH v3 2/2] mwifiex: pcie: add card_reset() support

[Dmitry Torokhov]

On Mon, May 01, 2017 at 12:37:00PM -0700, Brian Norris wrote:
> Similar to the SDIO driver, we should implement this so that we will
> automatically reset the device whenever there's a command timeout or
> similar.
> 
> Signed-off-by: Brian Norris <briannor...@chromium.org>

Reviewed-by: Dmitry Torokhov <dmitry.torok...@gmail.com>

> ---
> v3: keep all the new reset code in patch 2, not patch 1
> 
> v2: use atomic test/set, based on Dmitry's suggestion
> ---
>  drivers/net/wireless/marvell/mwifiex/pcie.c | 19 +++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
> b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index 5f56e8e6d612..78688ff6ecd0 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -2822,6 +2822,13 @@ static void mwifiex_pcie_device_dump_work(struct 
> mwifiex_adapter *adapter)
>   mwifiex_upload_device_dump(adapter, drv_info, drv_info_size);
>  }
>  
> +static void mwifiex_pcie_card_reset_work(struct mwifiex_adapter *adapter)
> +{
> + struct pcie_service_card *card = adapter->card;
> +
> + pci_reset_function(card->dev);
> +}
> +
>  static void mwifiex_pcie_work(struct work_struct *work)
>  {
>   struct pcie_service_card *card =
> @@ -2830,6 +2837,9 @@ static void mwifiex_pcie_work(struct work_struct *work)
>   if (test_and_clear_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP,
>  >work_flags))
>   mwifiex_pcie_device_dump_work(card->adapter);
> + if (test_and_clear_bit(MWIFIEX_IFACE_WORK_CARD_RESET,
> +>work_flags))
> + mwifiex_pcie_card_reset_work(card->adapter);
>  }
>  
>  /* This function dumps FW information */
> @@ -2842,6 +2852,14 @@ static void mwifiex_pcie_device_dump(struct 
> mwifiex_adapter *adapter)
>   schedule_work(>work);
>  }
>  
> +static void mwifiex_pcie_card_reset(struct mwifiex_adapter *adapter)
> +{
> + struct pcie_service_card *card = adapter->card;
> +
> + if (!test_and_set_bit(MWIFIEX_IFACE_WORK_CARD_RESET, >work_flags))
> + schedule_work(>work);
> +}
> +
>  static void mwifiex_pcie_free_buffers(struct mwifiex_adapter *adapter)
>  {
>   struct pcie_service_card *card = adapter->card;
> @@ -3271,6 +3289,7 @@ static struct mwifiex_if_ops pcie_ops = {
>   .cleanup_mpa_buf =  NULL,
>   .init_fw_port = mwifiex_pcie_init_fw_port,
>   .clean_pcie_ring =  mwifiex_clean_pcie_ring_buf,
> + .card_reset =   mwifiex_pcie_card_reset,
>   .reg_dump = mwifiex_pcie_reg_dump,
>   .device_dump =  mwifiex_pcie_device_dump,
>   .down_dev = mwifiex_pcie_down_dev,
> -- 
> 2.13.0.rc0.306.g87b477812d-goog
> 

-- 
Dmitry

Re: [PATCH v3 1/2] mwifiex: initiate card-specific work atomically

[Dmitry Torokhov]

On Mon, May 01, 2017 at 12:36:59PM -0700, Brian Norris wrote:
> The non-atomic test + set is a little awkward here, and it technically
> means we might double-schedule work unnecessarily. AFAICT, this is not
> really a problem, since the extra "work" will be a no-op (the flag(s)
> will be cleared by then), but it's still an anti-pattern.
> 
> Rewrite this to use the atomic test_and_set_bit() helper instead.
> 
> Signed-off-by: Brian Norris <briannor...@chromium.org>

Reviewed-by: Dmitry Torokhov <dmitry.torok...@gmail.com>

> ---
> v3: fix bisectability (unused code warning) -- some code from patch 2
> crept in here
> 
> v2: new
> ---
>  drivers/net/wireless/marvell/mwifiex/pcie.c |  9 +++--
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 16 +---
>  2 files changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
> b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index ac62bce50e96..5f56e8e6d612 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -2837,12 +2837,9 @@ static void mwifiex_pcie_device_dump(struct 
> mwifiex_adapter *adapter)
>  {
>   struct pcie_service_card *card = adapter->card;
>  
> - if (test_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, >work_flags))
> - return;
> -
> - set_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, >work_flags);
> -
> - schedule_work(>work);
> + if (!test_and_set_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP,
> +   >work_flags))
> + schedule_work(>work);
>  }
>  
>  static void mwifiex_pcie_free_buffers(struct mwifiex_adapter *adapter)
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c 
> b/drivers/net/wireless/marvell/mwifiex/sdio.c
> index 0af1c6733c92..d38d31bb9b79 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> @@ -2533,12 +2533,8 @@ static void mwifiex_sdio_card_reset(struct 
> mwifiex_adapter *adapter)
>  {
>   struct sdio_mmc_card *card = adapter->card;
>  
> - if (test_bit(MWIFIEX_IFACE_WORK_CARD_RESET, >work_flags))
> - return;
> -
> - set_bit(MWIFIEX_IFACE_WORK_CARD_RESET, >work_flags);
> -
> - schedule_work(>work);
> + if (!test_and_set_bit(MWIFIEX_IFACE_WORK_CARD_RESET, >work_flags))
> + schedule_work(>work);
>  }
>  
>  /* This function dumps FW information */
> @@ -2546,11 +2542,9 @@ static void mwifiex_sdio_device_dump(struct 
> mwifiex_adapter *adapter)
>  {
>   struct sdio_mmc_card *card = adapter->card;
>  
> - if (test_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, >work_flags))
> - return;
> -
> - set_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, >work_flags);
> - schedule_work(>work);
> + if (!test_and_set_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP,
> +   >work_flags))
> + schedule_work(>work);
>  }
>  
>  /* Function to dump SDIO function registers and SDIO scratch registers in 
> case
> -- 
> 2.13.0.rc0.306.g87b477812d-goog
> 

-- 
Dmitry

Re: [PATCH v2 2/4] mwifiex: fall back mwifiex_dbg to pr_info when adapter->dev not set

[Dmitry Torokhov]

Hi Xinming,

On Fri, Apr 7, 2017 at 3:51 AM, Xinming Hu  wrote:
> From: Xinming Hu 
>
> mwifiex_dbg will do nothing before adapter->dev get assigned. several logs
> lost in this case. it can be avoided by fall back to pr_info.
>
> Signed-off-by: Xinming Hu 
> ---
> v2: enhance adapter->dev null case.(Brain)
> ---
>  drivers/net/wireless/marvell/mwifiex/main.c | 17 -
>  drivers/net/wireless/marvell/mwifiex/main.h |  2 ++
>  2 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
> b/drivers/net/wireless/marvell/mwifiex/main.c
> index 98fd491..f3e772f 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -1749,18 +1749,25 @@ void _mwifiex_dbg(const struct mwifiex_adapter 
> *adapter, int mask,
>  {
> struct va_format vaf;
> va_list args;
> +   char msg[MWIFIEX_LOG_LEN];
>
> -   if (!adapter->dev || !(adapter->debug_mask & mask))
> +   if (!(adapter->debug_mask & mask))
> return;
>
> va_start(args, fmt);
>
> -   vaf.fmt = fmt;
> -   vaf.va = 
> -
> -   dev_info(adapter->dev, "%pV", );
> +   if (!adapter->dev) {
> +   vsnprintf(msg, MWIFIEX_LOG_LEN, fmt, args);
> +   } else {
> +   vaf.fmt = fmt;
> +   vaf.va = 
> +   dev_info(adapter->dev, "%pV", );
> +   }
>
> va_end(args);
> +
> +   if (!adapter->dev)
> +   pr_info("%s", msg);

Why not:

vaf.fmt = fmt;
vaf.va = 

if (adapter->dev)
dev_info(adapter->dev, "%pV", );
else
pr_info("mwifiex: %pV", );

va_end(args);

Also, instead of static "mwifiex" prefix maybe make sure you have
pr_fmt() set properly (I am not sure if it is set or not).

Thanks.

-- 
Dmitry

Re: [RFC PATCH] Revert "mwifiex: fix system hang problem after resume"

[Dmitry Torokhov]

On Fri, Mar 31, 2017 at 01:21:36PM -0700, Brian Norris wrote:
> This reverts commit 437322ea2a36d112e20aa7282c869bf924b3a836.
> 
> This above-mentioned "fix" does not actually do anything to prevent a
> race condition. It simply papers over it so that the issue doesn't
> appear.
> 
> If this is a real problem, it should be explained better than the above
> commit does, and an alternative, non-racy solution should be found.
> 
> For further reason to revert this: there's ot reason we can't try
> resetting the card when it's *actually* stuck in host-sleep mode. So
> instead, this is unnecessarily creating scenarios where we can't recover
> Wifi.
> 
> Cc: Amitkumar Karwar <akar...@marvell.com>
> Signed-off-by: Brian Norris <briannor...@chromium.org>

FWIW:

Reviewed-by: Dmitry Torokhov <dmitry.torok...@gmail.com>

> ---
> Amit, please take a look. AIUI, your "fix" is wrong, and quite racy. If you
> still think it's needed, can you please propose an alternative? Or at least
> explain more why this is needed? Thanks.
> 
>  drivers/net/wireless/marvell/mwifiex/init.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/init.c 
> b/drivers/net/wireless/marvell/mwifiex/init.c
> index 756948385b60..0dab77b526de 100644
> --- a/drivers/net/wireless/marvell/mwifiex/init.c
> +++ b/drivers/net/wireless/marvell/mwifiex/init.c
> @@ -60,7 +60,7 @@ static void wakeup_timer_fn(unsigned long data)
>   adapter->hw_status = MWIFIEX_HW_STATUS_RESET;
>   mwifiex_cancel_all_pending_cmd(adapter);
>  
> - if (adapter->if_ops.card_reset && !adapter->hs_activated)
> + if (adapter->if_ops.card_reset)
>   adapter->if_ops.card_reset(adapter);
>  }
>  
> -- 
> 2.12.2.564.g063fe858b8-goog
> 

-- 
Dmitry

Re: [PATCH 3/3] mwifiex: pcie: avoid hardcode wifi-only firmware name

[Dmitry Torokhov]

On Thu, Mar 30, 2017 at 2:19 AM, Xinming Hu  wrote:
> From: Xinming Hu 
>
> Wifi-only firmware name should be chipset specific.
>
> Signed-off-by: Xinming Hu 
> Signed-off-by: Amitkumar Karwar 
> ---
>  drivers/net/wireless/marvell/mwifiex/pcie.c | 8 +++-
>  drivers/net/wireless/marvell/mwifiex/pcie.h | 6 ++
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
> b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index 59cb01a..282aa9a 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -351,6 +351,12 @@ static void mwifiex_pcie_reset_notify(struct pci_dev 
> *pdev, bool prepare)
> struct pcie_service_card *card = pci_get_drvdata(pdev);
> struct mwifiex_adapter *adapter = card->adapter;
>
> +   if (!card->pcie.flr_support) {
> +   pr_err("%s: FLR disabled in current chipset\n", __func__);
> +   return;
> +   }
> +
> +   adapter = card->adapter;
> if (!adapter) {
> dev_err(>dev, "%s: adapter structure is not valid\n",
> __func__);
> @@ -3047,7 +3053,7 @@ static void mwifiex_pcie_up_dev(struct mwifiex_adapter 
> *adapter)
>  * during pcie FLR, so that bluetooth part of firmware which is
>  * already running doesn't get affected.
>  */
> -   strcpy(adapter->fw_name, PCIE8997_DEFAULT_WIFIFW_NAME);
> +   strcpy(adapter->fw_name, card->pcie.wifi_fw_name);
>
> /* tx_buf_size might be changed to 3584 by firmware during
>  * data transfer, we should reset it to default size.
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.h 
> b/drivers/net/wireless/marvell/mwifiex/pcie.h
> index 00e8ee5..d6c8526 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.h
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.h
> @@ -285,6 +285,8 @@ struct mwifiex_pcie_device {
> struct memory_type_mapping *mem_type_mapping_tbl;
> u8 num_mem_types;
> bool can_ext_scan;
> +   u8 flr_support;

This sounds like a boolean, but given that you can key off
wifi_fw_name being NULL I am not sure it is needed.

> +   char *wifi_fw_name;

const char *.

That said, I consider the whole wifi-only firmware business is quite
fragile. Can we have unified firmware and have driver figure out what
part shoudl be [re]loaded?

>  };
>
>  static const struct mwifiex_pcie_device mwifiex_pcie8766 = {
> @@ -293,6 +295,7 @@ struct mwifiex_pcie_device {
> .tx_buf_size = MWIFIEX_TX_DATA_BUF_SIZE_2K,
> .can_dump_fw = false,
> .can_ext_scan = true,
> +   .flr_support = 0,
>  };
>
>  static const struct mwifiex_pcie_device mwifiex_pcie8897 = {
> @@ -303,6 +306,7 @@ struct mwifiex_pcie_device {
> .mem_type_mapping_tbl = mem_type_mapping_tbl_w8897,
> .num_mem_types = ARRAY_SIZE(mem_type_mapping_tbl_w8897),
> .can_ext_scan = true,
> +   .flr_support = 0,
>  };
>
>  static const struct mwifiex_pcie_device mwifiex_pcie8997 = {
> @@ -313,6 +317,8 @@ struct mwifiex_pcie_device {
> .mem_type_mapping_tbl = mem_type_mapping_tbl_w8997,
> .num_mem_types = ARRAY_SIZE(mem_type_mapping_tbl_w8997),
> .can_ext_scan = true,
> +   .flr_support = 1,
> +   .wifi_fw_name = "mrvl/pcie8997_wlan_v4.bin",
>  };
>
>  struct mwifiex_evt_buf_desc {
> --
> 1.8.1.4
>

Thanks.

-- 
Dmitry

Re: [PATCH 2/3] mwifiex: using general print function during device intialization

[Dmitry Torokhov]

On Thu, Mar 30, 2017 at 2:19 AM, Xinming Hu  wrote:
> From: Xinming Hu 
>
> adapter->dev is initialized after mwifiex_register done, before that
> print message by general pr_* function

No, we should move away from naked pr_*() as much as possible. Please
change  mwifiex_register() to accept "dev" parameter and assign
adapter->dev early enough so that the standard mwifiex_err() calls are
usable.

Also consider changing _mwifiex_dbg() to handle cases when
adapter->dev is NULL and fall back to pr_.

>
> Signed-off-by: Xinming Hu 
> Signed-off-by: Amitkumar Karwar 
> ---
>  drivers/net/wireless/marvell/mwifiex/pcie.c | 78 
> -
>  drivers/net/wireless/marvell/mwifiex/sdio.c |  3 +-
>  2 files changed, 34 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
> b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index e381def..59cb01a 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -60,7 +60,7 @@ static int mwifiex_pcie_probe_of(struct device *dev)
>
> mapping.addr = pci_map_single(card->dev, skb->data, size, flags);
> if (pci_dma_mapping_error(card->dev, mapping.addr)) {
> -   mwifiex_dbg(adapter, ERROR, "failed to map pci memory!\n");
> +   pr_err("failed to map pci memory!\n");
> return -1;
> }
> mapping.len = size;
> @@ -594,7 +594,7 @@ static int mwifiex_init_rxq_ring(struct mwifiex_adapter 
> *adapter)
> skb = mwifiex_alloc_dma_align_buf(MWIFIEX_RX_DATA_BUF_SIZE,
>   GFP_KERNEL);
> if (!skb) {
> -   mwifiex_dbg(adapter, ERROR,
> +   pr_err(
> "Unable to allocate skb for RX ring.\n");
> kfree(card->rxbd_ring_vbase);
> return -ENOMEM;
> @@ -651,8 +651,7 @@ static int mwifiex_pcie_init_evt_ring(struct 
> mwifiex_adapter *adapter)
> /* Allocate skb here so that firmware can DMA data from it */
> skb = dev_alloc_skb(MAX_EVENT_SIZE);
> if (!skb) {
> -   mwifiex_dbg(adapter, ERROR,
> -   "Unable to allocate skb for EVENT 
> buf.\n");
> +   pr_err("Unable to allocate skb for EVENT buf.\n");
> kfree(card->evtbd_ring_vbase);
> return -ENOMEM;
> }
> @@ -818,16 +817,14 @@ static int mwifiex_pcie_create_txbd_ring(struct 
> mwifiex_adapter *adapter)
>  card->txbd_ring_size,
>  >txbd_ring_pbase);
> if (!card->txbd_ring_vbase) {
> -   mwifiex_dbg(adapter, ERROR,
> -   "allocate consistent memory (%d bytes) failed!\n",
> -   card->txbd_ring_size);
> +   pr_err("allocate consistent memory (%d bytes) failed!\n",
> +  card->txbd_ring_size);
> return -ENOMEM;
> }
> -   mwifiex_dbg(adapter, DATA,
> -   "info: txbd_ring - base: %p, pbase: %#x:%x, len: %x\n",
> -   card->txbd_ring_vbase, (unsigned 
> int)card->txbd_ring_pbase,
> -   (u32)((u64)card->txbd_ring_pbase >> 32),
> -   card->txbd_ring_size);
> +   pr_notice("info: txbd_ring - base: %p, pbase: %#x:%x, len: %x\n",
> + card->txbd_ring_vbase, (unsigned int)card->txbd_ring_pbase,
> + (u32)((u64)card->txbd_ring_pbase >> 32),
> + card->txbd_ring_size);
>
> return mwifiex_init_txq_ring(adapter);
>  }
> @@ -875,24 +872,21 @@ static int mwifiex_pcie_create_rxbd_ring(struct 
> mwifiex_adapter *adapter)
> card->rxbd_ring_size = sizeof(struct mwifiex_pcie_buf_desc) *
>MWIFIEX_MAX_TXRX_BD;
>
> -   mwifiex_dbg(adapter, INFO,
> -   "info: rxbd_ring: Allocating %d bytes\n",
> -   card->rxbd_ring_size);
> +   pr_info("info: rxbd_ring: Allocating %d bytes\n",
> +   card->rxbd_ring_size);
> card->rxbd_ring_vbase = pci_alloc_consistent(card->dev,
>  card->rxbd_ring_size,
>  >rxbd_ring_pbase);
> if (!card->rxbd_ring_vbase) {
> -   mwifiex_dbg(adapter, ERROR,
> -   "allocate consistent memory (%d bytes) failed!\n",
> -   card->rxbd_ring_size);
> +   pr_err("allocate consistent memory (%d bytes) failed!\n",
> +  card->rxbd_ring_size);
> return -ENOMEM;
> 

Re: [PATCH 1/3] mwifiex: remove unnecessary wakeup interrupt number sanity check

[Dmitry Torokhov]

On Thu, Mar 30, 2017 at 2:19 AM, Xinming Hu <huxinming...@gmail.com> wrote:
> From: Xinming Hu <h...@marvell.com>
>
> Sanity check of interrupt number in interrupt handler is unnecessary and
> confusion, remove it.

I'd reworded the explanation a bit: "If wakeup interrupt handler is
called, we know that the wakeup interrupt number is valid, there is no
need to check it."

Otherwise:

Reviewed-by: Dmitry Torokhov <d...@chromium.org>

>
> Signed-off-by: Xinming Hu <h...@marvell.com>
> Signed-off-by: Amitkumar Karwar <akar...@marvell.com>
> ---
>  drivers/net/wireless/marvell/mwifiex/main.c | 8 +++-
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
> b/drivers/net/wireless/marvell/mwifiex/main.c
> index 30f4994..5e82602 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -1503,11 +1503,9 @@ static irqreturn_t mwifiex_irq_wakeup_handler(int irq, 
> void *priv)
>  {
> struct mwifiex_adapter *adapter = priv;
>
> -   if (adapter->irq_wakeup >= 0) {
> -   dev_dbg(adapter->dev, "%s: wake by wifi", __func__);
> -   adapter->wake_by_wifi = true;
> -   disable_irq_nosync(irq);
> -   }
> +   dev_dbg(adapter->dev, "%s: wake by wifi", __func__);
> +   adapter->wake_by_wifi = true;
> +   disable_irq_nosync(irq);
>
> /* Notify PM core we are wakeup source */
> pm_wakeup_event(adapter->dev, 0);
> --
> 1.8.1.4
>

Re: [PATCH v2] mwifiex: fix kernel crash after shutdown command timeout

[Dmitry Torokhov]

On Thu, Mar 16, 2017 at 03:58:52PM +0530, Amitkumar Karwar wrote:
> We observed a SHUTDOWN command timeout during reboot stress test
> due to a corner case firmware bug. It leads to use-after-free on
> adapter structure pointer and crash.
> 
> Let's add MWIFIEX_IFACE_WORK_DONT_RUN work flag to avoid executing
> any work scheduled after cancel_work_sync() call in teardown path
> to resolve the issue.
> 
> Signed-off-by: Amitkumar Karwar 
> ---
> v2: New work_flag has been added to resolve the issue cleanly as per
> Brian's suggestion.
> ---
>  drivers/net/wireless/marvell/mwifiex/main.h | 1 +
>  drivers/net/wireless/marvell/mwifiex/pcie.c | 4 
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 4 
>  3 files changed, 9 insertions(+)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.h 
> b/drivers/net/wireless/marvell/mwifiex/main.h
> index 5c82972..d5b1fd6 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.h
> +++ b/drivers/net/wireless/marvell/mwifiex/main.h
> @@ -510,6 +510,7 @@ struct mwifiex_roc_cfg {
>  enum mwifiex_iface_work_flags {
>   MWIFIEX_IFACE_WORK_DEVICE_DUMP,
>   MWIFIEX_IFACE_WORK_CARD_RESET,
> + MWIFIEX_IFACE_WORK_DONT_RUN,
>  };
>  
>  struct mwifiex_private {
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
> b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index a0d9180..bb3d798 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -294,6 +294,7 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
>   if (!adapter || !adapter->priv_num)
>   return;
>  
> + set_bit(MWIFIEX_IFACE_WORK_DONT_RUN, >work_flags);
>   cancel_work_sync(>work);
>  
>   reg = card->pcie.reg;
> @@ -2721,6 +2722,9 @@ static void mwifiex_pcie_work(struct work_struct *work)
>   struct pcie_service_card *card =
>   container_of(work, struct pcie_service_card, work);
>  
> + if (test_bit(MWIFIEX_IFACE_WORK_DONT_RUN, >work_flags))
> + return;

I do not see how this could possible prevent use-after-free, assuming
that the "card" memory is gone by the time mwifiex_pcie_work() gets to
run. You need to check this flag before queueing firmware dump work, and
make sure it is not racy with setting this flag in mwifiex_pcie_remove()
(and sdio).

Thanks.

-- 
Dmitry

Re: [PATCH v2 2/3] mwifiex: pcie: don't loop/retry interrupt status checks

[Dmitry Torokhov]

On Tue, Jan 17, 2017 at 11:48:22AM -0800, Brian Norris wrote:
> On Sun, Jan 15, 2017 at 04:54:52PM -0800, Dmitry Torokhov wrote:
> > On Fri, Jan 13, 2017 at 03:35:37PM -0800, Brian Norris wrote:
> > > The following sequence occurs when using IEEE power-save on 8997:
> > > (a) driver sees SLEEP event
> > > (b) driver issues SLEEP CONFIRM
> > > (c) driver recevies CMD interrupt; within the interrupt processing loop,
> > > we do (d) and (e):
> > > (d) wait for FW sleep cookie (and often time out; it takes a while), FW
> > > is putting card into low power mode
> > > (e) re-check PCIE_HOST_INT_STATUS register; quit loop with 0 value
> > > 
> > > But at (e), no one actually signaled an interrupt (i.e., we didn't check
> > > adapter->int_status). And what's more, because the card is going to
> > > sleep, this register read appears to take a very long time in some cases
> > > -- 3 milliseconds in my case!
> > > 
> > > Now, I propose that (e) is completely unnecessary. If there were any
> > > additional interrupts signaled after the start of this loop, then the
> > > interrupt handler would have set adapter->int_status to non-zero and
> > > queued more work for the main loop -- and we'd catch it on the next
> > > iteration of the main loop.
> > > 
> > > So this patch drops all the looping/re-reading of PCIE_HOST_INT_STATUS,
> > > which avoids the problematic (and slow) register read in step (e).
> > > 
> > > Incidentally, this is a very similar issue to the one fixed in commit
> > > ec815dd2a5f1 ("mwifiex: prevent register accesses after host is
> > > sleeping"), except that the register read is just very slow instead of
> > > fatal in this case.
> > > 
> > > Tested on 8997 in both MSI and (though not technically supported at the
> > > moment) MSI-X mode.
> > 
> > Well, that kills interrupt mitigation and with PCIE that might be
> > somewhat important (SDIO is too slow to be important I think) and might
> > cost you throughput.
> 
> Hmm, well I don't see us disabling interrupts in here, at least for MSI
> mode, so it doesn't actually look like a mitigation mechanism. More like
> a redundancy. But I'm not an expert on MSI, and definitely not on
> network performance.

Well, right, maybe not mitigation, but at least you have a chance to
avoid scheduling latency at times.

> 
> Also, FWIW, I did some fairly non-scientific tests of this on my
> systems, and I didn't see much difference. I can run better tests, and
> even collect data on how often we loop here vs. see new interrupts.

That would be great. Maybe packet aggregation takes care of interrupts
arriving "too closely" together most of the time, I dunno.

> 
> > OTOH maybe Marvell should convert PICE to NAPI to make this more
> > obvious and probably more correct.
> 
> From my brief reading, that sounds like a better way to make this
> configurable.
> 
> So I'm not sure which way you'd suggest then; take a patch like this,
> which makes the driver more clear and less buggy? Or write some
> different patch that isolates just the power-save related condition, so
> we break out of this look [1]?

I think it really depends on the test results. If we do not see
degradation in both throughput and latency then I think we can take this
patch and then see if switching to NAPI would be the ultimate solution.

> 
> I'm also interested in any opinions from the Marvell side -- potentially
> testing results, rationale behind this code structure, or even a better
> alternative patch.
> 
> Brian
> 
> [1] i.e., along the lines of commit ec815dd2a5f1 ("mwifiex: prevent
> register accesses after host is sleeping").

Thanks.

-- 
Dmitry

Re: [PATCH v2 2/3] mwifiex: pcie: don't loop/retry interrupt status checks

[Dmitry Torokhov]

On Fri, Jan 13, 2017 at 03:35:37PM -0800, Brian Norris wrote:
> The following sequence occurs when using IEEE power-save on 8997:
> (a) driver sees SLEEP event
> (b) driver issues SLEEP CONFIRM
> (c) driver recevies CMD interrupt; within the interrupt processing loop,
> we do (d) and (e):
> (d) wait for FW sleep cookie (and often time out; it takes a while), FW
> is putting card into low power mode
> (e) re-check PCIE_HOST_INT_STATUS register; quit loop with 0 value
> 
> But at (e), no one actually signaled an interrupt (i.e., we didn't check
> adapter->int_status). And what's more, because the card is going to
> sleep, this register read appears to take a very long time in some cases
> -- 3 milliseconds in my case!
> 
> Now, I propose that (e) is completely unnecessary. If there were any
> additional interrupts signaled after the start of this loop, then the
> interrupt handler would have set adapter->int_status to non-zero and
> queued more work for the main loop -- and we'd catch it on the next
> iteration of the main loop.
> 
> So this patch drops all the looping/re-reading of PCIE_HOST_INT_STATUS,
> which avoids the problematic (and slow) register read in step (e).
> 
> Incidentally, this is a very similar issue to the one fixed in commit
> ec815dd2a5f1 ("mwifiex: prevent register accesses after host is
> sleeping"), except that the register read is just very slow instead of
> fatal in this case.
> 
> Tested on 8997 in both MSI and (though not technically supported at the
> moment) MSI-X mode.

Well, that kills interrupt mitigation and with PCIE that might be
somewhat important (SDIO is too slow to be important I think) and might
cost you throughput.

OTOH maybe Marvell should convert PICE to NAPI to make this more
obvious and probably more correct.

> 
> Signed-off-by: Brian Norris 
> ---
> v2:
>  * new in v2, replacing an attempt to mess with step (d) above
> ---
>  drivers/net/wireless/marvell/mwifiex/pcie.c | 102 
> +---
>  1 file changed, 32 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
> b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index 3f4cda2d3b61..194e0e04c3b1 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -2332,79 +2332,41 @@ static int mwifiex_process_pcie_int(struct 
> mwifiex_adapter *adapter)
>   }
>   }
>   }
> - while (pcie_ireg & HOST_INTR_MASK) {
> - if (pcie_ireg & HOST_INTR_DNLD_DONE) {
> - pcie_ireg &= ~HOST_INTR_DNLD_DONE;
> - mwifiex_dbg(adapter, INTR,
> - "info: TX DNLD Done\n");
> - ret = mwifiex_pcie_send_data_complete(adapter);
> - if (ret)
> - return ret;
> - }
> - if (pcie_ireg & HOST_INTR_UPLD_RDY) {
> - pcie_ireg &= ~HOST_INTR_UPLD_RDY;
> - mwifiex_dbg(adapter, INTR,
> - "info: Rx DATA\n");
> - ret = mwifiex_pcie_process_recv_data(adapter);
> - if (ret)
> - return ret;
> - }
> - if (pcie_ireg & HOST_INTR_EVENT_RDY) {
> - pcie_ireg &= ~HOST_INTR_EVENT_RDY;
> - mwifiex_dbg(adapter, INTR,
> - "info: Rx EVENT\n");
> - ret = mwifiex_pcie_process_event_ready(adapter);
> - if (ret)
> - return ret;
> - }
> -
> - if (pcie_ireg & HOST_INTR_CMD_DONE) {
> - pcie_ireg &= ~HOST_INTR_CMD_DONE;
> - if (adapter->cmd_sent) {
> - mwifiex_dbg(adapter, INTR,
> - "info: CMD sent Interrupt\n");
> - adapter->cmd_sent = false;
> - }
> - /* Handle command response */
> - ret = mwifiex_pcie_process_cmd_complete(adapter);
> - if (ret)
> - return ret;
> - if (adapter->hs_activated)
> - return ret;
> - }
> -
> - if (card->msi_enable) {
> - spin_lock_irqsave(>int_lock, flags);
> - adapter->int_status = 0;
> - spin_unlock_irqrestore(>int_lock, flags);
> - }
> -
> - if (mwifiex_pcie_ok_to_access_hw(adapter)) {
> - if (mwifiex_read_reg(adapter, PCIE_HOST_INT_STATUS,
> -  _ireg)) {
> - mwifiex_dbg(adapter, ERROR,
> - "Read register failed\n");
> - 

Re: [PATCH v4 3/3] mwifiex: Enable WoWLAN for both sdio and pcie

[Dmitry Torokhov]

On Tue, Nov 15, 2016 at 07:06:04PM +0530, Amitkumar Karwar wrote:
> From: Rajat Jain 
> 
> Commit ce4f6f0c353b ("mwifiex: add platform specific wakeup interrupt
> support") added WoWLAN feature only for sdio. This patch moves that
> code to the common module so that all the interface drivers can use
> it for free. It enables pcie and sdio for its use currently.
> 
> Signed-off-by: Rajat Jain 
> ---
> v2: v1 doesn't apply smoothly on latest code due to recently merged
> patch "mwifiex: report error to PCIe for suspend failure". Minor
> conflict is resolved in v2
> v4: Same as v2, v3
> ---
>  drivers/net/wireless/marvell/mwifiex/main.c | 41 
>  drivers/net/wireless/marvell/mwifiex/main.h | 25 ++
>  drivers/net/wireless/marvell/mwifiex/pcie.c |  2 +
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 72 
> ++---
>  drivers/net/wireless/marvell/mwifiex/sdio.h |  8 
>  5 files changed, 73 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
> b/drivers/net/wireless/marvell/mwifiex/main.c
> index 835d330..948f5c2 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -1552,14 +1552,55 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, 
> bool prepare)
>  }
>  EXPORT_SYMBOL_GPL(mwifiex_do_flr);
>  
> +static irqreturn_t mwifiex_irq_wakeup_handler(int irq, void *priv)
> +{
> + struct mwifiex_adapter *adapter = priv;
> +
> + if (adapter->irq_wakeup >= 0) {
> + dev_dbg(adapter->dev, "%s: wake by wifi", __func__);
> + adapter->wake_by_wifi = true;
> + disable_irq_nosync(irq);
> + }
> +
> + /* Notify PM core we are wakeup source */
> + pm_wakeup_event(adapter->dev, 0);
> +
> + return IRQ_HANDLED;
> +}
> +
>  static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
>  {
> + int ret;
>   struct device *dev = adapter->dev;
>  
>   if (!dev->of_node)
>   return;
>  
>   adapter->dt_node = dev->of_node;
> + adapter->irq_wakeup = irq_of_parse_and_map(adapter->dt_node, 0);
> + if (!adapter->irq_wakeup) {
> + dev_info(dev, "fail to parse irq_wakeup from device tree\n");
> + return;
> + }

I'd rather we used of_irq_get() here, because ti allows handling
deferrals. of_irq_get_byname() would be even better, but I guess we
already have bindings in the wild...

> +
> + ret = devm_request_irq(dev, adapter->irq_wakeup,
> +mwifiex_irq_wakeup_handler, IRQF_TRIGGER_LOW,

irq_of_parse_and_map() (and of_irq_get()) will set trigger flags,
why do we override them?

> +"wifi_wake", adapter);
> + if (ret) {
> + dev_err(dev, "Failed to request irq_wakeup %d (%d)\n",
> + adapter->irq_wakeup, ret);
> + goto err_exit;
> + }
> +
> + disable_irq(adapter->irq_wakeup);
> + if (device_init_wakeup(dev, true)) {
> + dev_err(dev, "fail to init wakeup for mwifiex\n");
> + goto err_exit;

Leaking interrupt (not forever, but if we are not using wakeup irq there
is no need to have it claimed).

> + }
> + return;
> +
> +err_exit:
> + adapter->irq_wakeup = 0;
>  }

I also do not see anyone actually calling mwifiex_probe_of() in this
patch?

>  
>  /*
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.h 
> b/drivers/net/wireless/marvell/mwifiex/main.h
> index 549e1ba..ae5afe5 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.h
> +++ b/drivers/net/wireless/marvell/mwifiex/main.h
> @@ -1011,6 +1011,10 @@ struct mwifiex_adapter {
>   bool usb_mc_setup;
>   struct cfg80211_wowlan_nd_info *nd_info;
>   struct ieee80211_regdomain *regd;
> +
> + /* Wake-on-WLAN (WoWLAN) */
> + int irq_wakeup;
> + bool wake_by_wifi;
>  };
>  
>  void mwifiex_process_tx_queue(struct mwifiex_adapter *adapter);
> @@ -1410,6 +1414,27 @@ static inline u8 mwifiex_is_tdls_link_setup(u8 status)
>   return false;
>  }
>  
> +/* Disable platform specific wakeup interrupt */
> +static inline void mwifiex_disable_wake(struct mwifiex_adapter *adapter)
> +{
> + if (adapter->irq_wakeup >= 0) {
> + disable_irq_wake(adapter->irq_wakeup);
> + if (!adapter->wake_by_wifi)
> + disable_irq(adapter->irq_wakeup);
> + }
> +}
> +
> +/* Enable platform specific wakeup interrupt */
> +static inline void mwifiex_enable_wake(struct mwifiex_adapter *adapter)
> +{
> + /* Enable platform specific wakeup interrupt */
> + if (adapter->irq_wakeup >= 0) {
> + adapter->wake_by_wifi = false;
> + enable_irq(adapter->irq_wakeup);
> + enable_irq_wake(adapter->irq_wakeup);
> + }
> +}
> +
>  int mwifiex_init_shutdown_fw(struct mwifiex_private *priv,
>u32 func_init_shutdown);
>  int 

Re: [PATCH v2 2/5] mwifiex: use spinlock for 'mwifiex_processing' in shutdown_drv

[Dmitry Torokhov]

On Thu, Nov 3, 2016 at 12:27 PM, Brian Norris <briannor...@chromium.org> wrote:
> On Thu, Nov 03, 2016 at 09:15:04AM -0700, Dmitry Torokhov wrote:
>> On Thu, Nov 03, 2016 at 08:34:06AM +, Xinming Hu wrote:
>> > > -Original Message-
>> > > From: linux-wireless-ow...@vger.kernel.org
>> > > [mailto:linux-wireless-ow...@vger.kernel.org] On Behalf Of Dmitry 
>> > > Torokhov
>> > >
>> > > Instead please remove call to mwifiex_shutdown_drv() in the main routine
>> > > and "if (adapter->mwifiex_processing)" check here.
>> > >
>> >
>> > mwifiex_main_process will be used from interrupt or workqueue.
>> > Now we have disabled interrupt and flush workqueue, so
>> > mwifiex_main_process won't be scheduled in the future.
>> > But mwifiex_main_process might just running in context of last
>> > interrupt, so we need wait current main_process complete in
>> > mwifiex_shutdown_drv.
>>
>> synchronize_irq() is your friend then.
>
> Hmm, that sounds right, but IIUC, the "interrupt context" is actually
> only used for SDIO, and for SDIO, the driver doesn't actually have
> access to the IRQ number. The MMC/SDIO layer has some extra abstraction
> around the IRQ. So this may be more difficult than it appears.
>
> Do we need a sdio_synchronize_irq() API?

Actually the ->disable_irq() method should be responsible for making
sure it does not complete while interrupt handler is running. As far
as I can see, for SDIO case, we end up calling sdio_card_irq_put()
which stops kernel thread and won't return while the thread is
running. For other interfaces we need to check. IIRC USB lacks
->disable_irq() altogether and this is something that shoudl be fixed
(by doing usb_kill_urb() at the minimum).

Thanks.

-- 
Dmitry

Re: [PATCH v2 2/5] mwifiex: use spinlock for 'mwifiex_processing' in shutdown_drv

[Dmitry Torokhov]

On Thu, Nov 03, 2016 at 08:34:06AM +, Xinming Hu wrote:
> Hi Dmitry,
> 
> > -Original Message-
> > From: linux-wireless-ow...@vger.kernel.org
> > [mailto:linux-wireless-ow...@vger.kernel.org] On Behalf Of Dmitry Torokhov
> > Sent: 2016年10月28日 1:44
> > To: Amitkumar Karwar
> > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> > raja...@google.com; briannor...@google.com
> > Subject: Re: [PATCH v2 2/5] mwifiex: use spinlock for 'mwifiex_processing' 
> > in
> > shutdown_drv
> > 
> > Hi Amit,
> > 
> > On Thu, Oct 27, 2016 at 02:42:40PM +0530, Amitkumar Karwar wrote:
> > > This variable is guarded by spinlock at all other places. This patch
> > > takes care of missing spinlock usage in mwifiex_shutdown_drv().
> > 
> > Since in the previous discussion you stated that we inhibit interrupts and 
> > flush
> > the workqueue so that mwifiex_shutdown_drv() can't run simultaneously with
> > the main processing routine, why do we need this?
> > 
> > Instead please remove call to mwifiex_shutdown_drv() in the main routine
> > and "if (adapter->mwifiex_processing)" check here.
> > 
> 
> mwifiex_main_process will be used from interrupt or workqueue.
> Now we have disabled interrupt and flush workqueue, so mwifiex_main_process 
> won't be scheduled in the future.
> But mwifiex_main_process might just running in context of last interrupt, so 
> we need wait current main_process complete in mwifiex_shutdown_drv.

synchronize_irq() is your friend then.

Thanks.

-- 
Dmitry

Re: [PATCH v2 2/5] mwifiex: use spinlock for 'mwifiex_processing' in shutdown_drv

[Dmitry Torokhov]

Hi Amit,

On Thu, Oct 27, 2016 at 02:42:40PM +0530, Amitkumar Karwar wrote:
> This variable is guarded by spinlock at all other places. This patch
> takes care of missing spinlock usage in mwifiex_shutdown_drv().

Since in the previous discussion you stated that we inhibit interrupts
and flush the workqueue so that mwifiex_shutdown_drv() can't run
simultaneously with the main processing routine, why do we need this?

Instead please remove call to mwifiex_shutdown_drv() in the main routine
and "if (adapter->mwifiex_processing)" check here.

Thanks.

> 
> Signed-off-by: Amitkumar Karwar 
> ---
> v2: Same as v1
> ---
>  drivers/net/wireless/marvell/mwifiex/init.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/init.c 
> b/drivers/net/wireless/marvell/mwifiex/init.c
> index 82839d9..8e5e424 100644
> --- a/drivers/net/wireless/marvell/mwifiex/init.c
> +++ b/drivers/net/wireless/marvell/mwifiex/init.c
> @@ -670,11 +670,14 @@ mwifiex_shutdown_drv(struct mwifiex_adapter *adapter)
>  
>   adapter->hw_status = MWIFIEX_HW_STATUS_CLOSING;
>   /* wait for mwifiex_process to complete */
> + spin_lock_irqsave(>main_proc_lock, flags);
>   if (adapter->mwifiex_processing) {
> + spin_unlock_irqrestore(>main_proc_lock, flags);
>   mwifiex_dbg(adapter, WARN,
>   "main process is still running\n");
>   return ret;
>   }
> + spin_unlock_irqrestore(>main_proc_lock, flags);
>  
>   /* cancel current command */
>   if (adapter->curr_cmd) {
> -- 
> 1.9.1
> 

-- 
Dmitry

Re: [PATCH] mwifiex: don't do unbalanced free()'ing in cleanup_if()

[Dmitry Torokhov]

On Wed, Oct 26, 2016 at 04:43:54PM -0700, Brian Norris wrote:
> On Wed, Oct 26, 2016 at 04:35:54PM -0700, Dmitry Torokhov wrote:
> > On Wed, Oct 26, 2016 at 04:29:20PM -0700, Brian Norris wrote:
> 
> > > diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c 
> > > b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > index 8718950004f3..f04cf5a551b3 100644
> > > --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> 
> > > @@ -2291,6 +2287,14 @@ static void mwifiex_recreate_adapter(struct 
> > > sdio_mmc_card *card)
> > >  
> > >   mwifiex_sdio_remove(func);
> > >  
> > > + /*
> > > +  * Normally, we would let the driver core take care of releasing these.
> > > +  * But we're not letting the driver core handle this one. See above
> > > +  * TODO.
> > > +  */
> > > + sdio_set_drvdata(func, NULL);
> > > + devm_kfree(>dev, card);
> > 
> > Ugh, this really messes the unwind order... I guess it is OK since it is
> > the only resource allocated with devm, but I'd be happier if we could
> > reuse existing "card" structure.
> 
> I'm really not interested in cleaning up the hacky reset function here
> (see the other TODOs here). I'm sure it's broken in other ways too. In
> its current "design" (if you can call it that) where we remove and
> re-probe the device, I'm not sure there's a way to get it to reuse the
> 'card'.

Ah, I see now... Nevermind then.

Thanks.

-- 
Dmitry

Re: [PATCH] mwifiex: don't do unbalanced free()'ing in cleanup_if()

[Dmitry Torokhov]

On Wed, Oct 26, 2016 at 04:29:20PM -0700, Brian Norris wrote:
> The cleanup_if() callback is the inverse of init_if(). We allocate our
> 'card' interface structure in the probe() function, but we free it in
> cleanup_if(). That gives a few problems:
> (a) we leak this memory if probe() fails before we reach init_if()
> (b) we can't safely utilize 'card' after cleanup_if() -- namely, in
> remove() or suspend(), both of which might race with the cleanup
> paths in our asynchronous FW initialization path
> 
> Solution: just use devm_kzalloc(), which will free this structure
> properly when the device is removed -- and drop the set_drvdata(...,
> NULL), since the driver core does this for us. This also removes the
> temptation to use drvdata == NULL as a hack for checking if the device
> has been "cleaned up."
> 
> I *do* leave the set_drvdata(..., NULL) for the hacky SDIO
> mwifiex_recreate_adapter(), since the device core won't be able to clear
> that one for us.
> 
> Signed-off-by: Brian Norris 
> ---
>  drivers/net/wireless/marvell/mwifiex/pcie.c |  5 +
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 16 ++--
>  drivers/net/wireless/marvell/mwifiex/usb.c  |  7 +--
>  3 files changed, 12 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
> b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index 063c707844d3..3047c1ab944a 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -189,7 +189,7 @@ static int mwifiex_pcie_probe(struct pci_dev *pdev,
>   pr_debug("info: vendor=0x%4.04X device=0x%4.04X rev=%d\n",
>pdev->vendor, pdev->device, pdev->revision);
>  
> - card = kzalloc(sizeof(struct pcie_service_card), GFP_KERNEL);
> + card = devm_kzalloc(>dev, sizeof(*card), GFP_KERNEL);
>   if (!card)
>   return -ENOMEM;
>  
> @@ -2815,7 +2815,6 @@ static int mwifiex_pcie_init(struct mwifiex_adapter 
> *adapter)
>  err_set_dma_mask:
>   pci_disable_device(pdev);
>  err_enable_dev:
> - pci_set_drvdata(pdev, NULL);
>   return ret;
>  }
>  
> @@ -2849,9 +2848,7 @@ static void mwifiex_pcie_cleanup(struct mwifiex_adapter 
> *adapter)
>   pci_disable_device(pdev);
>   pci_release_region(pdev, 2);
>   pci_release_region(pdev, 0);
> - pci_set_drvdata(pdev, NULL);
>   }
> - kfree(card);
>  }
>  
>  static int mwifiex_pcie_request_irq(struct mwifiex_adapter *adapter)
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c 
> b/drivers/net/wireless/marvell/mwifiex/sdio.c
> index 8718950004f3..f04cf5a551b3 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> @@ -152,7 +152,7 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct 
> sdio_device_id *id)
>   pr_debug("info: vendor=0x%4.04X device=0x%4.04X class=%d function=%d\n",
>func->vendor, func->device, func->class, func->num);
>  
> - card = kzalloc(sizeof(struct sdio_mmc_card), GFP_KERNEL);
> + card = devm_kzalloc(>dev, sizeof(*card), GFP_KERNEL);
>   if (!card)
>   return -ENOMEM;
>  
> @@ -185,7 +185,7 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct 
> sdio_device_id *id)
>  
>   if (ret) {
>   dev_err(>dev, "failed to enable function\n");
> - goto err_free;
> + return ret;
>   }
>  
>   /* device tree node parsing and platform specific configuration*/
> @@ -210,8 +210,6 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct 
> sdio_device_id *id)
>   sdio_claim_host(func);
>   sdio_disable_func(func);
>   sdio_release_host(func);
> -err_free:
> - kfree(card);
>  
>   return ret;
>  }
> @@ -2240,8 +2238,6 @@ static void mwifiex_cleanup_sdio(struct mwifiex_adapter 
> *adapter)
>   kfree(card->mpa_rx.len_arr);
>   kfree(card->mpa_tx.buf);
>   kfree(card->mpa_rx.buf);
> - sdio_set_drvdata(card->func, NULL);
> - kfree(card);
>  }
>  
>  /*
> @@ -2291,6 +2287,14 @@ static void mwifiex_recreate_adapter(struct 
> sdio_mmc_card *card)
>  
>   mwifiex_sdio_remove(func);
>  
> + /*
> +  * Normally, we would let the driver core take care of releasing these.
> +  * But we're not letting the driver core handle this one. See above
> +  * TODO.
> +  */
> + sdio_set_drvdata(func, NULL);
> + devm_kfree(>dev, card);

Ugh, this really messes the unwind order... I guess it is OK since it is
the only resource allocated with devm, but I'd be happier if we could
reuse existing "card" structure.

Thanks.

-- 
Dmitry

Re: [PATCH v6] mwifiex: parse device tree node for PCIe

[Dmitry Torokhov]

On Wed, Oct 26, 2016 at 01:56:34PM -0700, Brian Norris wrote:
> On Wed, Oct 26, 2016 at 01:51:48PM -0700, Rajat Jain wrote:
> >On Wed, Oct 26, 2016 at 1:46 PM, Dmitry Torokhov
> ><dmitry.torok...@gmail.com> wrote:
> >  On Wed, Oct 26, 2016 at 01:17:36PM -0700, Brian Norris wrote:
> >  Sorry, I just saw this... Why do we need devicetree data for
> >  discoverable bus (PCI)? How does the driver work on systems that do not
> >  use DT? Why do we need them to behave differently?
> > 
> >There are a couple of out-of-band GPIO pins from Marvell chip that can
> >serve as wake-up pins (wake up the CPU when asserted). The Marvell chip
> >has to be told which GPIO pin is to be used as the wake-up pin. The pin 
> > to
> >be used is system / platform dependent. (On some systems it could be
> >GPIO13, on others it could be GPIO14 etc depending on how the marvell 
> > chip
> >is wired up to the CPU).

So wakeup pin is not wired to PCIe WAKE?

> There's also calibration data. See "marvell,caldata*" and
> "marvell,wakeup-pin" properties. Currently only for SDIO, in
> Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt, but
> we're adding support for PCIe.

How would it all work if I moved the PCIe module from one device to
another?

Thanks.

-- 
Dmitry

Re: [PATCH v6] mwifiex: parse device tree node for PCIe

[Dmitry Torokhov]

On Wed, Oct 26, 2016 at 01:17:36PM -0700, Brian Norris wrote:
> Hi Rajat,
> 
> On Fri, Oct 21, 2016 at 02:21:09PM -0700, Rajat Jain wrote:
> > From: Xinming Hu 
> > 
> > This patch derives device tree node from pcie bus layer framework, and
> > fixes a minor memory leak in mwifiex_pcie_probe() (in failure path).
> > Device tree bindings file has been renamed(marvell-sd8xxx.txt ->
> > marvell-8xxx.txt) to accommodate PCIe changes.
> > 
> > Signed-off-by: Xinming Hu 
> > Signed-off-by: Amitkumar Karwar 
> > Signed-off-by: Rajat Jain 
> > Reviewed-by: Brian Norris 
> > ---
> > v2: Included vendor and product IDs in compatible strings for PCIe
> > chipsets(Rob Herring)
> > v3: Patch is created using -M option so that it will only include diff of
> > original and renamed files(Rob Herring)
> > Resend v3: Resending the patch because I missed to include device tree 
> > mailing
> > while sending v3.
> > v4: Fix error handling, also move-on even if no device tree node is present.
> > v5: Update commit log to include memory leak, return -EINVAL instead of -1.
> 
> I've been working on reworking some bugfixes for this driver, and I
> noticed we have some problems w.r.t. memory leaks, and the "memory leak"
> fix is not actually a fix. See below.

Sorry, I just saw this... Why do we need devicetree data for
discoverable bus (PCI)? How does the driver work on systems that do not
use DT? Why do we need them to behave differently?

Thanks.

-- 
Dmitry

Re: [PATCH 2/5] mwifiex: use spinlock for 'mwifiex_processing' in shutdown_drv

[Dmitry Torokhov]

Hi Amit,

On Wed, Oct 26, 2016 at 03:23:08PM +, Amitkumar Karwar wrote:
> 
> This race won't occur. At this point of time(i.e while calling 
> mwifiex_shutdown_drv() in deinit), following things are completed. We don't 
> expect mwifiex_main_process() to be scheduled.
> 1) Connection to peer device is terminated at the beginning of teardown 
> thread. So we don't receive any Tx data from kernel.
> 2) Last command SHUTDOWN is exchanged with firmware. So there won't be any 
> activity/interrupt from firmware.
> 3) Interrupts are disabled.
> 4) "adapter->surprise_removed" flag is set. It will skip 
> mwifiex_main_process() calls.
> 
> ---
> static void mwifiex_main_work_queue(struct work_struct *work)
> {
> struct mwifiex_adapter *adapter =
> container_of(work, struct mwifiex_adapter, main_work);
> 
> if (adapter->surprise_removed)
> return;
> mwifiex_main_process(adapter);
> }
> --
> 5) We have "mwifiex_terminate_workqueue(adapter)" call to flush and destroy 
> workqueue.

OK, but if interrupts are disabled and you ensure that work is flushed
or completed before you call mwifiex_shutdown_drv() then I do not
understand why you need all of this at all? Why do you need to check
status in mwifiex_shutdown_drv() and why do you want
mwifiex_main_process() to call mwifiex_shutdown_drv() in certain cases?
Can you simply remove all this stuff?

Thanks.

-- 
Dmitry

Re: [PATCH v4 1/3] mwifiex: reset card->adapter during device unregister

[Dmitry Torokhov]

On Tue, Oct 25, 2016 at 03:12:44PM +, Amitkumar Karwar wrote:
> Hi Brian,
> 
> Thanks for review.
> 
> > From: Brian Norris [mailto:briannor...@chromium.org]
> > Sent: Tuesday, October 25, 2016 6:22 AM
> > To: Amitkumar Karwar
> > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> > raja...@google.com; Xinming Hu; abhishe...@google.com; Dmitry Torokhov
> > Subject: Re: [PATCH v4 1/3] mwifiex: reset card->adapter during device
> > unregister
> > 
> > Hi Amit,
> > 
> > On Thu, Oct 20, 2016 at 01:11:31PM +, Amitkumar Karwar wrote:
> > > > From: Brian Norris [mailto:briannor...@chromium.org]
> > > > Sent: Tuesday, October 11, 2016 5:53 AM
> > > > To: Amitkumar Karwar
> > > > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> > > > raja...@google.com; Xinming Hu; abhishe...@google.com; Dmitry
> > > > Torokhov
> > > > Subject: Re: [PATCH v4 1/3] mwifiex: reset card->adapter during
> > > > device unregister
> > > >
> > > > On Mon, Oct 10, 2016 at 01:53:32PM -0700, Brian Norris wrote:
> > > > > On Thu, Oct 06, 2016 at 11:36:24PM +0530, Amitkumar Karwar wrote:
> > > > > > From: Xinming Hu <h...@marvell.com>
> > > > > >
> > > > > > card->adapter gets initialized during device registration.
> > > > > > As it's not cleared, we may end up accessing invalid memory in
> > > > > > some corner cases. This patch fixes the problem.
> > > > > >
> > > > > > Signed-off-by: Xinming Hu <h...@marvell.com>
> > > > > > Signed-off-by: Amitkumar Karwar <akar...@marvell.com>
> > > > > > ---
> > > > > > v4: Same as v1, v2, v3
> > > > > > ---
> > > > > >  drivers/net/wireless/marvell/mwifiex/pcie.c | 1 +
> > > > > > drivers/net/wireless/marvell/mwifiex/sdio.c | 1 +
> > > > > >  2 files changed, 2 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > > b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > > index f1eeb73..ba9e068 100644
> > > > > > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > > @@ -3042,6 +3042,7 @@ static void mwifiex_unregister_dev(struct
> > > > mwifiex_adapter *adapter)
> > > > > > pci_disable_msi(pdev);
> > > > > >}
> > > > > > }
> > > > > > +   card->adapter = NULL;
> > > > > >  }
> > > > > >
> > > > > >  /* This function initializes the PCI-E host memory space, WCB
> > > > rings, etc.
> > > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > > > b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > > > index 8718950..4cad1c2 100644
> > > > > > --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > > > +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > > > @@ -2066,6 +2066,7 @@ mwifiex_unregister_dev(struct
> > > > > > mwifiex_adapter
> > > > *adapter)
> > > > > > struct sdio_mmc_card *card = adapter->card;
> > > > > >
> > > > > > if (adapter->card) {
> > > > > > +   card->adapter = NULL;
> > > > > > sdio_claim_host(card->func);
> > > > > > sdio_disable_func(card->func);
> > > > > > sdio_release_host(card->func);
> > > > >
> > > > > As discussed on v1, I had qualms about the raciness between
> > > > > reads/writes of card->adapter, but I believe we:
> > > > > (a) can't have any command activity while writing the ->adapter
> > > > > field (either we're just init'ing the device, or we've disabled
> > > > > interrupts and are tearing it down) and
> > > > > (b) can't have a race between suspend()/resume() and
> > > > > unregister_dev(), since unregister_dev() is called from device
> > > > > remove() (which should not be concurrent with suspend()).
> > > > >
> > > > > Also, I thought you had the same problem in usb.c, but 

Re: [PATCH 2/5] mwifiex: use spinlock for 'mwifiex_processing' in shutdown_drv

[Dmitry Torokhov]

On Tue, Oct 25, 2016 at 04:11:14PM +, Amitkumar Karwar wrote:
> Hi Dmitry,
> 
> > From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
> > Sent: Tuesday, October 25, 2016 5:28 AM
> > To: Brian Norris
> > Cc: Amitkumar Karwar; linux-wireless@vger.kernel.org; Cathy Luo; Nishant
> > Sarmukadam
> > Subject: Re: [PATCH 2/5] mwifiex: use spinlock for 'mwifiex_processing'
> > in shutdown_drv
> > 
> > On Mon, Oct 24, 2016 at 12:19:15PM -0700, Brian Norris wrote:
> > > On Mon, Oct 24, 2016 at 07:51:29PM +0530, Amitkumar Karwar wrote:
> > > > This variable is guarded by spinlock at all other places. This patch
> > > > takes care of missing spinlock usage in mwifiex_shutdown_drv().
> > > >
> > > > Signed-off-by: Amitkumar Karwar <akar...@marvell.com>
> > > > ---
> > > >  drivers/net/wireless/marvell/mwifiex/init.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/net/wireless/marvell/mwifiex/init.c
> > > > b/drivers/net/wireless/marvell/mwifiex/init.c
> > > > index 82839d9..8e5e424 100644
> > > > --- a/drivers/net/wireless/marvell/mwifiex/init.c
> > > > +++ b/drivers/net/wireless/marvell/mwifiex/init.c
> > > > @@ -670,11 +670,14 @@ mwifiex_shutdown_drv(struct mwifiex_adapter
> > > > *adapter)
> > > >
> > > > adapter->hw_status = MWIFIEX_HW_STATUS_CLOSING;
> > > > /* wait for mwifiex_process to complete */
> > > > +   spin_lock_irqsave(>main_proc_lock, flags);
> > > > if (adapter->mwifiex_processing) {
> > >
> > > I'm not quite sure why we have this check in the first place; if the
> > > main process is still running, then don't we have bigger problems here
> > > anyway? But this is definitely the "right" way to check this flag, so:
> > 
> > No, this is definitely not right way to check it. What exactly does this
> > spinlock protect? It seems that the intent is to make sure we do not
> > call mwifiex_shutdown_drv() while mwifiex_main_process() is executing.
> > It even says above that we are "waiting" for it (but we do not, we may
> > bail out or we may not, depends on luck).
> > 
> > To implement this properly you not only need to take a lock and check
> > the flag, but keep the lock until mwifiex_shutdown_drv() is done, or use
> > some other way to let mwifiex_main_process() know it should not access
> > the adapter while mwifiex_shutdown_drv() is running.
> > 
> > NACK.
> > 
> 
> As I explained in other email, here is the sequence.
> 1) We find mwifiex_processing is true in mwifiex_shitdown_drv(). 
> "-EINPROGRESS" is returned by the function and hw_status state is changed to 
> MWIFIEX_HW_STATUS_CLOSING.
> 2) We wait until main_process is completed.
> 
> if (mwifiex_shutdown_drv(adapter) == -EINPROGRESS)
> wait_event_interruptible(adapter->init_wait_q,
>  adapter->init_wait_q_woken);
> 
> 3) We have following check at the end of main_process()
> 
> exit_main_proc:
> if (adapter->hw_status == MWIFIEX_HW_STATUS_CLOSING)
> mwifiex_shutdown_drv(adapter);
> 
> 4) Here at the end of mwifiex_shutdown_drv(), we are setting 
> "adapter->init_wait_q_woken" and waking up the thread in point (2)

1. We do not find mwifiex_processing to be true
2. We proceed to try and shut down the driver
3. Interrupt is raised in the meantime, kicking work item
4. mwifiex_main_process() sees that adapter->hw_status is
MWIFIEX_HW_STATUS_CLOSING and jumps to exit_main_proc
5. mwifiex_main_process() calls into mwifiex_shutdown_drv() that is now
racing with another copy of the same.

It seems to me that mwifiex_main_process() is [almost] always used from
a workqueue. Can you instead of sprinkling spinlocks ensure that
mwifiex_shutdown_drv():

1. Inhibits scheduling of mwifiex_main_process()
2. Does cancel_work_sync(...) to ensure that mwifiex_main_process() does
not run
3. Continues shutting down the driver.

Alternatively, do these have to be spinlocks? Can you make them mutexes
and take them for the duration of mwifiex_main_process() and
mwifiex_shutdown_drv() and others, as needed?

Thanks.

-- 
Dmitry

Re: [PATCH 5/5] mwifiex: wait for firmware dump completion in remove_card

[Dmitry Torokhov]

On Mon, Oct 24, 2016 at 07:51:32PM +0530, Amitkumar Karwar wrote:
> From: Xinming Hu 
> 
> This patch ensures to wait for firmware dump completion in
> mwifiex_remove_card().
> 
> For sdio interface, reset_trigger variable is used to identify
> if mwifiex_sdio_remove() is called by sdio_work during reset or
> the call is from sdio subsystem.
> 
> This patch fixes a kernel crash observed during reboot when firmware
> dump operation is in process.
> 
> Signed-off-by: Xinming Hu 
> Signed-off-by: Amitkumar Karwar 
> ---
>  drivers/net/wireless/marvell/mwifiex/pcie.c |  2 ++
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 15 ++-
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
> b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index 986bf07..4512e86 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -528,6 +528,8 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
>   if (!adapter || !adapter->priv_num)
>   return;
>  
> + cancel_work_sync(_work);
> +
>   if (user_rmmod && !adapter->mfg_mode) {
>  #ifdef CONFIG_PM_SLEEP
>   if (adapter->is_suspended)
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c 
> b/drivers/net/wireless/marvell/mwifiex/sdio.c
> index 4cad1c2..f974260 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> @@ -46,6 +46,15 @@
>   */
>  static u8 user_rmmod;
>  
> +/* reset_trigger variable is used to identify if mwifiex_sdio_remove()
> + * is called by sdio_work during reset or the call is from sdio subsystem.
> + * We will cancel sdio_work only if the call is from sdio subsystem.
> + */
> +static u8 reset_triggered;

It would be really great if the driver supported multiple devices. IOW
please avoid module-globals.

> +
> +static void mwifiex_sdio_work(struct work_struct *work);
> +static DECLARE_WORK(sdio_work, mwifiex_sdio_work);
> +
>  static struct mwifiex_if_ops sdio_ops;
>  static unsigned long iface_work_flags;
>  
> @@ -289,6 +298,9 @@ mwifiex_sdio_remove(struct sdio_func *func)
>   if (!adapter || !adapter->priv_num)
>   return;
>  
> + if (!reset_triggered)
> + cancel_work_sync(_work);
> +
>   mwifiex_dbg(adapter, INFO, "info: SDIO func num=%d\n", func->num);
>  
>   if (user_rmmod && !adapter->mfg_mode) {
> @@ -2290,7 +2302,9 @@ static void mwifiex_recreate_adapter(struct 
> sdio_mmc_card *card)
>* discovered and initializes them from scratch.
>*/
>  
> + reset_triggered = 1;
>   mwifiex_sdio_remove(func);
> + reset_triggered = 0;

What exactly happens if I trigger mwifiex_sdio_remove() from sysfs at
the same time this code is running?

>  
>   /* power cycle the adapter */
>   sdio_claim_host(func);
> @@ -2621,7 +2635,6 @@ static void mwifiex_sdio_work(struct work_struct *work)
>   mwifiex_sdio_card_reset_work(save_adapter);
>  }
>  
> -static DECLARE_WORK(sdio_work, mwifiex_sdio_work);
>  /* This function resets the card */
>  static void mwifiex_sdio_card_reset(struct mwifiex_adapter *adapter)
>  {
> -- 
> 1.9.1
> 

-- 
Dmitry

Re: [PATCH 3/5] mwifiex: do not free firmware dump memory in shutdown_drv

[Dmitry Torokhov]

On Mon, Oct 24, 2016 at 07:51:30PM +0530, Amitkumar Karwar wrote:
> From: Xinming Hu 
> 
> mwifiex_upload_device_dump() already takes care of freeing firmware dump
> memory. Doing the same thing in mwifiex_shutdown_drv() is redundant.
> 
> Signed-off-by: Xinming Hu 
> Signed-off-by: Amitkumar Karwar 
> ---
>  drivers/net/wireless/marvell/mwifiex/init.c | 19 ---
>  1 file changed, 19 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/init.c 
> b/drivers/net/wireless/marvell/mwifiex/init.c
> index 8e5e424..365efb8 100644
> --- a/drivers/net/wireless/marvell/mwifiex/init.c
> +++ b/drivers/net/wireless/marvell/mwifiex/init.c
> @@ -407,8 +407,6 @@ static void mwifiex_free_lock_list(struct mwifiex_adapter 
> *adapter)
>  static void
>  mwifiex_adapter_cleanup(struct mwifiex_adapter *adapter)
>  {
> - int idx;
> -
>   if (!adapter) {
>   pr_err("%s: adapter is NULL\n", __func__);
>   return;
> @@ -426,23 +424,6 @@ mwifiex_adapter_cleanup(struct mwifiex_adapter *adapter)
>   mwifiex_dbg(adapter, INFO, "info: free cmd buffer\n");
>   mwifiex_free_cmd_buffer(adapter);
>  
> - for (idx = 0; idx < adapter->num_mem_types; idx++) {
> - struct memory_type_mapping *entry =
> - >mem_type_mapping_tbl[idx];
> -
> - if (entry->mem_ptr) {
> - vfree(entry->mem_ptr);
> - entry->mem_ptr = NULL;
> - }
> - entry->mem_size = 0;
> - }
> -
> - if (adapter->drv_info_dump) {
> - vfree(adapter->drv_info_dump);
> - adapter->drv_info_dump = NULL;
> - adapter->drv_info_size = 0;
> - }

Why do you even keep the pointer to dump memory in the adapter
structure? You allocate it in mwifiex_drv_info_dump() and immediately
use it in mwifiex_upload_device_dump(). Why not simply pass the pointer
between the functions?

Thanks.

-- 
Dmitry

Re: [PATCH 2/5] mwifiex: use spinlock for 'mwifiex_processing' in shutdown_drv

[Dmitry Torokhov]

On Mon, Oct 24, 2016 at 12:19:15PM -0700, Brian Norris wrote:
> On Mon, Oct 24, 2016 at 07:51:29PM +0530, Amitkumar Karwar wrote:
> > This variable is guarded by spinlock at all other places. This patch
> > takes care of missing spinlock usage in mwifiex_shutdown_drv().
> > 
> > Signed-off-by: Amitkumar Karwar 
> > ---
> >  drivers/net/wireless/marvell/mwifiex/init.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/net/wireless/marvell/mwifiex/init.c 
> > b/drivers/net/wireless/marvell/mwifiex/init.c
> > index 82839d9..8e5e424 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/init.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/init.c
> > @@ -670,11 +670,14 @@ mwifiex_shutdown_drv(struct mwifiex_adapter *adapter)
> >  
> > adapter->hw_status = MWIFIEX_HW_STATUS_CLOSING;
> > /* wait for mwifiex_process to complete */
> > +   spin_lock_irqsave(>main_proc_lock, flags);
> > if (adapter->mwifiex_processing) {
> 
> I'm not quite sure why we have this check in the first place; if the
> main process is still running, then don't we have bigger problems here
> anyway? But this is definitely the "right" way to check this flag, so:

No, this is definitely not right way to check it. What exactly does this
spinlock protect? It seems that the intent is to make sure we do not
call mwifiex_shutdown_drv() while mwifiex_main_process() is executing.
It even says above that we are "waiting" for it (but we do not, we may
bail out or we may not, depends on luck).

To implement this properly you not only need to take a lock and check
the flag, but keep the lock until mwifiex_shutdown_drv() is done, or
use some other way to let mwifiex_main_process() know it should not
access the adapter while mwifiex_shutdown_drv() is running.

NACK.

Thanks.

> 
> Reviewed-by: Brian Norris 
> 
> > +   spin_unlock_irqrestore(>main_proc_lock, flags);
> > mwifiex_dbg(adapter, WARN,
> > "main process is still running\n");
> > return ret;
> > }
> > +   spin_unlock_irqrestore(>main_proc_lock, flags);
> >  
> > /* cancel current command */
> > if (adapter->curr_cmd) {
> > -- 
> > 1.9.1
> > 

-- 
Dmitry

Re: [PATCH 2/5] mwifiex: use spinlock for 'mwifiex_processing' in shutdown_drv

[Dmitry Torokhov]

On Mon, Oct 24, 2016 at 07:51:29PM +0530, Amitkumar Karwar wrote:
> This variable is guarded by spinlock at all other places. This patch
> takes care of missing spinlock usage in mwifiex_shutdown_drv().
> 
> Signed-off-by: Amitkumar Karwar 
> ---
>  drivers/net/wireless/marvell/mwifiex/init.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/init.c 
> b/drivers/net/wireless/marvell/mwifiex/init.c
> index 82839d9..8e5e424 100644
> --- a/drivers/net/wireless/marvell/mwifiex/init.c
> +++ b/drivers/net/wireless/marvell/mwifiex/init.c
> @@ -670,11 +670,14 @@ mwifiex_shutdown_drv(struct mwifiex_adapter *adapter)
>  
>   adapter->hw_status = MWIFIEX_HW_STATUS_CLOSING;
>   /* wait for mwifiex_process to complete */
> + spin_lock_irqsave(>main_proc_lock, flags);
>   if (adapter->mwifiex_processing) {
> + spin_unlock_irqrestore(>main_proc_lock, flags);
>   mwifiex_dbg(adapter, WARN,
>   "main process is still running\n");
>   return ret;
>   }
> + spin_unlock_irqrestore(>main_proc_lock, flags);

What happens if adapter->mwifiex_processing will become true here?

>  
>   /* cancel current command */
>   if (adapter->curr_cmd) {
> -- 
> 1.9.1
> 

-- 
Dmitry

Re: [PATCH v2 1/2] mwifiex: reset card->adapter during device unregister

[Dmitry Torokhov]

Hi Brian,

On Mon, Oct 10, 2016 at 4:47 PM, Brian Norris <briannor...@chromium.org> wrote:
> Hi Dmitry,
>
> On Mon, Oct 10, 2016 at 04:43:06PM -0700, Dmitry Torokhov wrote:
>> On Thu, Oct 6, 2016 at 6:03 AM, Amitkumar Karwar <akar...@marvell.com> wrote:
>> >> From: linux-wireless-ow...@vger.kernel.org [mailto:linux-wireless-
>> >> ow...@vger.kernel.org] On Behalf Of Brian Norris
>> >> Sent: Wednesday, October 05, 2016 10:00 PM
>> >> To: Amitkumar Karwar
>> >> Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
>> >> raja...@google.com; Xinming Hu
>> >> Subject: Re: [PATCH v2 1/2] mwifiex: reset card->adapter during device
>> >> unregister
>> >>
>> >> On Wed, Oct 05, 2016 at 02:04:53PM +, Amitkumar Karwar wrote:
>> >> > > From: Brian Norris [mailto:briannor...@chromium.org]
>> >> > > Sent: Wednesday, October 05, 2016 3:28 AM
>> >> > > To: Amitkumar Karwar
>> >> > > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
>> >> > > raja...@google.com; briannor...@google.com; Xinming Hu
>> >> > > Subject: Re: [PATCH v2 1/2] mwifiex: reset card->adapter during
>> >> > > device unregister
>> >> > >
>> >> > > On Tue, Oct 04, 2016 at 10:38:24PM +0530, Amitkumar Karwar wrote:
>> >>
>> >> > > > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
>> >> > > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
>> >> > > > @@ -3042,6 +3042,7 @@ static void mwifiex_unregister_dev(struct
>> >> > > mwifiex_adapter *adapter)
>> >> > > > pci_disable_msi(pdev);
>> >> > > >}
>> >> > > > }
>> >> > > > +   card->adapter = NULL;
>> >> > >
>> >> > > I think you have a similar problem here as in patch 2; there is no
>> >> > > locking to protect fields in struct pcie_service_card or struct
>> >> > > sdio_mmc_card below. That problem kind of already exists, except
>> >> > > that you only write the value of card->adapter once at registration
>> >> > > time, so it's not actually unsafe. But now that you're introducing a
>> >> > > second write, you have a problem.
>> >> > >
>> >> > > Brian
>> >> > >
>> >> >
>> >> > We have a global "add_remove_card_sem" semaphore in our code for
>> >> > synchronizing initialization and teardown threads. Ideally "init +
>> >> > teardown/reboot" should not have a race issue with this logic
>> >> >
>> >> > Later there was a loophole introduced in this after using async
>> >> > firmware download API. During initialization, firmware downloads
>> >> > asynchronously in a separate thread where might have released the
>> >> > semaphore. I am working on a patch to fix this.
>> >> >
>> >> > So "card->adapter" doesn't need to have locking. Even if we have two
>> >> > write operations, those two threads can't run simultaneously due to
>> >> > above mentioned logic.
>> >>
>> >> What about writes racing with reads? You have lots of unsynchronized
>> >> cases that read this, although most of them should be halted by now
>> >> (e.g., cmd processing). I was looking at suspend() in particular, which
>> >> I thought you were looking at in this patch series.
>> >
>> > Please note that "card->adapter" is used only in pcie.c/sdio.c/usb.c files
>> >
>> > Writes won't have race with reads.
>> >
>> > 1) write 1  --- "card->adapter = adapter;" in mwifiex_register_dev()
>> > This place is at the beginning of initialization.
>> > mwifiex_pcie_probe() -> mwifiex_add_card() -> 
>> > adapter->if_ops.register_dev()
>> > There is no chance that "card->adapter" is read anywhere at this 
>> > point. FW is not yet downloaded
>> >
>> > 2) write 2  "card->adapter = NULL;" in mwifiex_unregister_dev()
>> > This place the end of teardown phase.
>> > Interrupts are disabled and all cleanup is done. We have 
>> > "card->adapter" NULL checks at entry point of suspend/remove/resume, if 
>> > they get called after this.
>>
>> How exactly are you preventing ->suspend() from being called *while*
>> you are running mwifiex_unregister_dev()? I.e. user slamming the lid
>> of a laptop and throwing it in their backpack?
>
> As I already commented, isn't this primarily [*] called from the driver
> remove() callback? remove() doesn't race with suspend(), does it?

Nope, there is request_firmware_nowait() path that is asynchronous
with device registration/unregistration and power management. Maybe
the right way is to move the driver to async probing and switch that
request_firmware_nowait() into plain request_firmware(). I haven't
looked that closely.

The wording "we may end up accessing invalid memory in some corner
cases" which is a synonym for "the code is racy" caught my eye, thus
the response on the patch.

Thanks.

-- 
Dmitry

Re: [PATCH v2 1/2] mwifiex: reset card->adapter during device unregister

[Dmitry Torokhov]

On Thu, Oct 6, 2016 at 6:03 AM, Amitkumar Karwar  wrote:
> Hi Brian,
>
>> From: linux-wireless-ow...@vger.kernel.org [mailto:linux-wireless-
>> ow...@vger.kernel.org] On Behalf Of Brian Norris
>> Sent: Wednesday, October 05, 2016 10:00 PM
>> To: Amitkumar Karwar
>> Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
>> raja...@google.com; Xinming Hu
>> Subject: Re: [PATCH v2 1/2] mwifiex: reset card->adapter during device
>> unregister
>>
>> Hi,
>>
>> On Wed, Oct 05, 2016 at 02:04:53PM +, Amitkumar Karwar wrote:
>> > > From: Brian Norris [mailto:briannor...@chromium.org]
>> > > Sent: Wednesday, October 05, 2016 3:28 AM
>> > > To: Amitkumar Karwar
>> > > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
>> > > raja...@google.com; briannor...@google.com; Xinming Hu
>> > > Subject: Re: [PATCH v2 1/2] mwifiex: reset card->adapter during
>> > > device unregister
>> > >
>> > > Hi,
>> > >
>> > > On Tue, Oct 04, 2016 at 10:38:24PM +0530, Amitkumar Karwar wrote:
>>
>> > > > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
>> > > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
>> > > > @@ -3042,6 +3042,7 @@ static void mwifiex_unregister_dev(struct
>> > > mwifiex_adapter *adapter)
>> > > > pci_disable_msi(pdev);
>> > > >}
>> > > > }
>> > > > +   card->adapter = NULL;
>> > >
>> > > I think you have a similar problem here as in patch 2; there is no
>> > > locking to protect fields in struct pcie_service_card or struct
>> > > sdio_mmc_card below. That problem kind of already exists, except
>> > > that you only write the value of card->adapter once at registration
>> > > time, so it's not actually unsafe. But now that you're introducing a
>> > > second write, you have a problem.
>> > >
>> > > Brian
>> > >
>> >
>> > We have a global "add_remove_card_sem" semaphore in our code for
>> > synchronizing initialization and teardown threads. Ideally "init +
>> > teardown/reboot" should not have a race issue with this logic
>> >
>> > Later there was a loophole introduced in this after using async
>> > firmware download API. During initialization, firmware downloads
>> > asynchronously in a separate thread where might have released the
>> > semaphore. I am working on a patch to fix this.
>> >
>> > So "card->adapter" doesn't need to have locking. Even if we have two
>> > write operations, those two threads can't run simultaneously due to
>> > above mentioned logic.
>>
>> What about writes racing with reads? You have lots of unsynchronized
>> cases that read this, although most of them should be halted by now
>> (e.g., cmd processing). I was looking at suspend() in particular, which
>> I thought you were looking at in this patch series.
>
> Please note that "card->adapter" is used only in pcie.c/sdio.c/usb.c files
>
> Writes won't have race with reads.
>
> 1) write 1  --- "card->adapter = adapter;" in mwifiex_register_dev()
> This place is at the beginning of initialization.
> mwifiex_pcie_probe() -> mwifiex_add_card() -> 
> adapter->if_ops.register_dev()
> There is no chance that "card->adapter" is read anywhere at this 
> point. FW is not yet downloaded
>
> 2) write 2  "card->adapter = NULL;" in mwifiex_unregister_dev()
> This place the end of teardown phase.
> Interrupts are disabled and all cleanup is done. We have 
> "card->adapter" NULL checks at entry point of suspend/remove/resume, if they 
> get called after this.

How exactly are you preventing ->suspend() from being called *while*
you are running mwifiex_unregister_dev()? I.e. user slamming the lid
of a laptop and throwing it in their backpack?

Thanks.

-- 
Dmitry

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Dmitry Torokhov]

On Wed, Aug 03, 2016 at 05:55:40PM +0200, Luis R. Rodriguez wrote:
> 
> I accept all help and would be glad to make enhancements instead of
> the old API through new API. The biggest thing here first I think is
> adding devm support, that I think should address what seemed to be
> the need to add more code for a transformation into the API. I'd

I am confused. Why do we need devm support, given that devm is only
valid in probe() paths[*] and we do know that we do not want to load
firmware in probe() paths because it may cause blocking?

[*] Yes, I know there are calls to devm* outside of probe() but I am
pretty sure they are buggy unless they explicitly freed with devm* as
well and then there is no point. IN all other cases it is likely wrong
as it messes up with order of freeing resources.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Dmitry Torokhov]

On Wed, Aug 03, 2016 at 01:43:31PM +0200, Arend van Spriel wrote:
> On 03-08-16 09:42, Dmitry Torokhov wrote:
> > On Tue, Aug 2, 2016 at 12:41 AM, Luis R. Rodriguez <mcg...@kernel.org> 
> > wrote:
> >> On Tue, Aug 02, 2016 at 08:53:55AM +0200, Daniel Wagner wrote:
> >>> On 08/02/2016 08:34 AM, Luis R. Rodriguez wrote:
> >>>> On Tue, Aug 02, 2016 at 07:49:19AM +0200, Daniel Wagner wrote:
> >>>>>> The sysdata API's main goal rather is to provide a flexible API first,
> >>>>>> compartamentalizing the usermode helper was secondary. But now it seems
> >>>>>> I may just also add devm support too to help simplify code further.
> >>>>>
> >>>>> I missed the point that you plan to add usermode helper support to
> >>>>> the sysdata API.
> >>>>
> >>>> I had no such plans, when I have asked folks so far about "hey are you
> >>>> really in need for it, OK what for? " and "what extended uses do you
> >>>> envision?" so I far I have not gotten any replies at all. So -- instead
> >>>> sysdata currently ignores it.
> >>>
> >>> So you argue for the remoteproc use case with 100+ MB firmware that
> >>> if there is a way to load after pivot_root() (or other additional
> >>> firmware partition shows up) then there is no need at all for
> >>> usermode helper?
> >>
> >> No, I'm saying I'd like to hear valid uses cases for the usermode helper 
> >> and so
> >> far I have only found using coccinelle grammar 2 explicit users, that's 
> >> it. My
> >> patch series (not yet merge) then annotates these as valid as I've verified
> >> through their documentation they have some quirky requirement.
> > 
> > In certain configurations (embedded) people do not want to use
> > initramfs nor modules nor embed firmware into the kernel. In this case
> > usermode helper + firmware calss timeout handling provides necessary
> > wait for the root filesystem to be mounted.
> 
> And there are people who don't have a usermode helper running at all in
> their configuration, but I guess they should disable the helper.

Right, if they don't that means their system is misconfigured.

> 
> In my opinion the kernel should provide functionality to user-space and
> user-space providing functionality to the kernel should be avoided.

Why? We have bunch of stuff running in userspace for the kernel. Fuse
for example. I am sure there are more.

> 
> > If we solve waiting for rootfs (or something else that may contain
> > firmware) then these cases will not need to use usermode helper.
> 
> If firmware (or whatever) API could get notification of mount syscall it
> could be used to retry firmware loading instead of periodic polling.
> That leaves the question raised by you about when to stop trying. The
> initlevel stuff is probably a user-space only concept, right? So no
> ideas how the kernel itself could decide except for a "long" timeout.

The kernel really does not know, it can only guess. The firmware may get
delivered by motorized carrier pidgeons. But distribution does know how
they set up, so they are in position to tell the kernel "go" or "give
up".

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Dmitry Torokhov]

On Tue, Aug 2, 2016 at 12:41 AM, Luis R. Rodriguez  wrote:
> On Tue, Aug 02, 2016 at 08:53:55AM +0200, Daniel Wagner wrote:
>> On 08/02/2016 08:34 AM, Luis R. Rodriguez wrote:
>> >On Tue, Aug 02, 2016 at 07:49:19AM +0200, Daniel Wagner wrote:
>> >>>The sysdata API's main goal rather is to provide a flexible API first,
>> >>>compartamentalizing the usermode helper was secondary. But now it seems
>> >>>I may just also add devm support too to help simplify code further.
>> >>
>> >>I missed the point that you plan to add usermode helper support to
>> >>the sysdata API.
>> >
>> >I had no such plans, when I have asked folks so far about "hey are you
>> >really in need for it, OK what for? " and "what extended uses do you
>> >envision?" so I far I have not gotten any replies at all. So -- instead
>> >sysdata currently ignores it.
>>
>> So you argue for the remoteproc use case with 100+ MB firmware that
>> if there is a way to load after pivot_root() (or other additional
>> firmware partition shows up) then there is no need at all for
>> usermode helper?
>
> No, I'm saying I'd like to hear valid uses cases for the usermode helper and 
> so
> far I have only found using coccinelle grammar 2 explicit users, that's it. My
> patch series (not yet merge) then annotates these as valid as I've verified
> through their documentation they have some quirky requirement.

In certain configurations (embedded) people do not want to use
initramfs nor modules nor embed firmware into the kernel. In this case
usermode helper + firmware calss timeout handling provides necessary
wait for the root filesystem to be mounted.

If we solve waiting for rootfs (or something else that may contain
firmware) then these cases will not need to use usermode helper.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Dmitry Torokhov]

On Mon, Aug 01, 2016 at 10:19:51AM -0700, Bjorn Andersson wrote:
> On Sat 30 Jul 09:58 PDT 2016, Luis R. Rodriguez wrote:
> 
> > On Sat, Jul 30, 2016 at 02:42:41PM +0200, Arend van Spriel wrote:
> > > + Luis (again) ;-)
> > > 
> > > On 29-07-16 08:13, Daniel Wagner wrote:
> > > > On 07/28/2016 09:01 PM, Bjorn Andersson wrote:
> > > >> On Thu 28 Jul 11:33 PDT 2016, Dmitry Torokhov wrote:
> > > >>
> > > >>> On Thu, Jul 28, 2016 at 09:55:11AM +0200, Daniel Wagner wrote:
> > > >>>> From: Daniel Wagner <daniel.wag...@bmw-carit.de>
> > > >>>>
> > > >> [..]
> > > >>>
> > > >>> Do not quite like it... I'd rather asynchronous request give out a
> > > >>> firmware status pointer that could be used later on.
> > > 
> > > Excellent. Why not get rid of the callback function as well and have
> > > fw_loading_wait() return result (0 = firmware available, < 0 = fail).
> > > Just to confirm, you are proposing a new API function next to
> > > request_firmware_nowait(), right?
> > 
> > If proposing new firmware_class patches please bounce / Cc me, I've
> > recently asked for me to be added to MAINTAINERS so I get these
> > e-mails as I'm working on a new flexible API which would allow us
> > to extend the firmware API without having to care about the old
> > stupid usermode helper at all.
> > 
> 
> In the remoteproc world there are several systems where we see 100+MB of
> firmware being loaded. It's unfeasible to have these files in an
> initramfs, so we need a way to indicate to the kernel that the
> second/primary (or a dedicated firmware partition) is mounted.
> 
> We're currently loading these files with request_firmware_nowait(), so
> either one can either use kernel modules or the user-helper fallback to
> delay the loading until the files are available. (I don't like to
> enforce the usage of kernel modules)
> 
> I'm open to alternative ways of having firmware loading wait on
> secondary filesystems to be mounted, but have not yet tried to tackle
> this problem. I do believe something that waits and retries the firmware
> load as additional file systems gets mounted would be prettier than
> forcing user space to tell us it's time to move on.

Hmm, but when do you stop? How do you know that you are not going to get
yet another fs mounted and that one will finally have the firmware you
are looking for? The kernel does now, but distribution/product
integrator does. That is why I think the most reliable way is to see if
firmware is built-in, otherwise wait and let userspace do its thing.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Dmitry Torokhov]

On July 30, 2016 9:58:17 AM PDT, "Luis R. Rodriguez" <mcg...@kernel.org> wrote:
>On Sat, Jul 30, 2016 at 02:42:41PM +0200, Arend van Spriel wrote:
>> + Luis (again) ;-)
>> 
>> On 29-07-16 08:13, Daniel Wagner wrote:
>> > On 07/28/2016 09:01 PM, Bjorn Andersson wrote:
>> >> On Thu 28 Jul 11:33 PDT 2016, Dmitry Torokhov wrote:
>> >>
>> >>> On Thu, Jul 28, 2016 at 09:55:11AM +0200, Daniel Wagner wrote:
>> >>>> From: Daniel Wagner <daniel.wag...@bmw-carit.de>
>> >>>>
>> >> [..]
>> >>>
>> >>> Do not quite like it... I'd rather asynchronous request give out
>a
>> >>> firmware status pointer that could be used later on.
>> 
>> Excellent. Why not get rid of the callback function as well and have
>> fw_loading_wait() return result (0 = firmware available, < 0 = fail).
>> Just to confirm, you are proposing a new API function next to
>> request_firmware_nowait(), right?
>
>If proposing new firmware_class patches please bounce / Cc me, I've
>recently asked for me to be added to MAINTAINERS so I get these
>e-mails as I'm working on a new flexible API which would allow us
>to extend the firmware API without having to care about the old
>stupid usermode helper at all.

I am not sure why we started calling usermode helper "stupid". We only had to 
implement direct kernel firmware loading because udev/stsremd folks had 
"interesting" ideas how events should be handled; but having userspace to feed 
us data is not stupid.

If we want to overhaul firmware loading support we need to figure out how to 
support case when a driver want to [asynchronously] request 
firmware/config/blob and the rest of the system is not ready. Even if we want 
kernel to do read/load the data we need userspace to tell kernel when firmware 
partition is available, until then the kernel should not fail the request.


Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Dmitry Torokhov]

On July 30, 2016 5:42:41 AM PDT, Arend van Spriel 
<arend.vanspr...@broadcom.com> wrote:
>+ Luis (again) ;-)
>
>On 29-07-16 08:13, Daniel Wagner wrote:
>> On 07/28/2016 09:01 PM, Bjorn Andersson wrote:
>>> On Thu 28 Jul 11:33 PDT 2016, Dmitry Torokhov wrote:
>>>
>>>> On Thu, Jul 28, 2016 at 09:55:11AM +0200, Daniel Wagner wrote:
>>>>> From: Daniel Wagner <daniel.wag...@bmw-carit.de>
>>>>>
>>> [..]
>>>>
>>>> Do not quite like it... I'd rather asynchronous request give out a
>>>> firmware status pointer that could be used later on.
>
>Excellent. Why not get rid of the callback function as well and have
>fw_loading_wait() return result (0 = firmware available, < 0 = fail).
>Just to confirm, you are proposing a new API function next to
>request_firmware_nowait(), right?

Yes, that would be a new API call. Maybe we could replace old API with the new 
at some point.


>>>> pcu->fw_st = request_firmware_async(IMS_PCU_FIRMWARE_NAME,
>>>> -   pcu,
>>>> -   ims_pcu_process_async_firmware);
>+  pcu);
>>>> if (IS_ERR(pcu->fw_st))
>>>> return PTR_ERR(pcu->fw_st);
>>>>
>>>> 
>>>>
>>>> err = fw_loading_wait(pcu->fw_st);
>   if (err)
>   return err;
>
>   fw = fwstat_get_firmware(pcu->fw_st);
>
>Or whatever consistent prefix it is going to be.
>
>>>>
>>>
>>> In the remoteproc case (patch 6) this would clean up the code,
>rather
>>> than replacing the completion API 1 to 1. I like it!
>> 
>> IIRC most drivers do it the same way. So request_firmware_async()
>indeed
>> would be good thing to have. Let me try that.
>
>While the idea behind this series is a good one I am wondering about
>the
>need for these drivers to use the asynchronous API. The historic reason
>might be to avoid timeout caused by user-mode helper, but that may no
>longer apply and these drivers could be better off using
>request_firmware_direct().

Actually systems using this driver rely on usermode helper to provide necessary 
delay and load the firmware from storage once root partition is mounted. 
Converting to request_firmware_direct() would break them.


Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Dmitry Torokhov]

On Thu, Jul 28, 2016 at 09:55:11AM +0200, Daniel Wagner wrote:
> From: Daniel Wagner 
> 
> Loading firmware is an operation many drivers implement in various ways
> around the completion API. And most of them do it almost in the same
> way. Let's reuse the firmware_stat API which is used also by the
> firmware_class loader. Apart of streamlining the firmware loading states
> we also document it slightly better.
> 
> Signed-off-by: Daniel Wagner 
> ---
>  drivers/input/misc/ims-pcu.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index 9c0ea36..cda1fbf 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -109,7 +109,7 @@ struct ims_pcu {
>  
>   u32 fw_start_addr;
>   u32 fw_end_addr;
> - struct completion async_firmware_done;
> + struct firmware_stat fw_st;
>  
>   struct ims_pcu_buttons buttons;
>   struct ims_pcu_gamepad *gamepad;
> @@ -940,7 +940,7 @@ static void ims_pcu_process_async_firmware(const struct 
> firmware *fw,
>   release_firmware(fw);
>  
>  out:
> - complete(>async_firmware_done);
> + fw_loading_done(pcu->fw_st);

Why does the driver have to do it? If firmware loader manages this, then
it should let waiters know when callback finishes. 

>  }
>  
>  /*
> @@ -1967,7 +1967,7 @@ static int ims_pcu_init_bootloader_mode(struct ims_pcu 
> *pcu)
>   ims_pcu_process_async_firmware);
>   if (error) {
>   /* This error is not fatal, let userspace have another chance */
> - complete(>async_firmware_done);
> + fw_loading_abort(pcu->fw_st);

Why should the driver signal abort if it does not manage completion in
this case?

>   }
>  
>   return 0;
> @@ -1976,7 +1976,7 @@ static int ims_pcu_init_bootloader_mode(struct ims_pcu 
> *pcu)
>  static void ims_pcu_destroy_bootloader_mode(struct ims_pcu *pcu)
>  {
>   /* Make sure our initial firmware request has completed */
> - wait_for_completion(>async_firmware_done);
> + fw_loading_wait(pcu->fw_st);
>  }
>  
>  #define IMS_PCU_APPLICATION_MODE 0
> @@ -2000,7 +2000,7 @@ static int ims_pcu_probe(struct usb_interface *intf,
>   pcu->bootloader_mode = id->driver_info == IMS_PCU_BOOTLOADER_MODE;
>   mutex_init(>cmd_mutex);
>   init_completion(>cmd_done);
> - init_completion(>async_firmware_done);
> + firmware_stat_init(>fw_st);

Do not quite like it... I'd rather asynchronous request give out a
firmware status pointer that could be used later on.

pcu->fw_st = request_firmware_async(IMS_PCU_FIRMWARE_NAME,
pcu,
ims_pcu_process_async_firmware);
if (IS_ERR(pcu->fw_st))
return PTR_ERR(pcu->fw_st);



fw_loading_wait(pcu->fw_st);

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v0 3/8] firmware: Factor out firmware load helpers

[Dmitry Torokhov]

On Thu, Jul 28, 2016 at 09:55:07AM +0200, Daniel Wagner wrote:
> +int __firmware_stat_wait(struct firmware_stat *fwst,
> + long timeout)
> +{
> + int err;
> + err = swait_event_interruptible_timeout(fwst->wq,
> + is_fw_sync_done(READ_ONCE(fwst->status)),
> + timeout);
> + if (err == 0 && fwst->status == FW_STATUS_ABORT)
> + return -ENOENT;
> +
> + return err;
> +}
> +EXPORT_SYMBOL(__firmware_stat_wait);
> +
> +void __firmware_stat_set(struct firmware_stat *fwst, unsigned long status)
> +{
> + WRITE_ONCE(fwst->status, status);
> + swake_up(>wq);

Do we need to notify everyone for FW_STATUS_LOADING status?

The driver users do not care for sure.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 000/182] Rid struct gpio_chip from container_of() usage

[Dmitry Torokhov]

On Mon, Dec 14, 2015 at 1:18 AM, Linus Walleij <linus.wall...@linaro.org> wrote:
> On Wed, Dec 9, 2015 at 8:30 PM, Dmitry Torokhov
> <dmitry.torok...@gmail.com> wrote:
>> On Wed, Dec 09, 2015 at 02:08:35PM +0100, Linus Walleij wrote:
>>> This removes the use of container_of() constructions from *all*
>>> GPIO drivers in the kernel. It is done by instead adding an
>>> optional void *data pointer to the struct gpio_chip and an
>>> accessor function, gpiochip_get_data() to get it from a driver.
>>>
>>> WHY?
>>>
>>> Because we want to have a proper userspace ABI for GPIO chips,
>>> which involves using a character device that the user opens
>>> and closes. While the character device is open, the underlying
>>> kernel objects must not go away.
>>>
>>> Currently the GPIO drivers keep their state in the struct
>>> gpio_chip, and that is often allocated by the drivers, very
>>> often as a part of a containing per-instance state container
>>> struct for the driver:
>>>
>>> struct foo_state {
>>>struct gpio_chip chip;  <- OMG my state is there
>>> };
>>>
>>> Drivers cannot allocate and manage this state: if a user has the
>>> character device open, the objects allocated must stay around
>>> even if the driver goes away. Instead drivers need to pass a
>>> descriptor to the GPIO core, and then the core should allocate
>>> and manage the lifecycle of things related to the device, such
>>> as the chardev itself or the struct device related to the GPIO
>>> device.
>>
>> Yes, but it does not mean that the object that is being maintained by
>> the subsystem and that us attached to character device needs to be
>> gpio_chip itself. You can have something like
>>
>> struct gpio_chip_chardev {
>> struct cdev chardev;
>> struct gpio_chip *chip;
>> bool dead;
>> };
>
> There needs to be a struct device too, amongst other things.

Could be; I was not trying to provide finished data structure but just
illustrate possible solution.

>
>>
>> struct gpio_chip {
>> ...
>> struct gpio_chip_chardev *chardev;
>> ...
>> };
>>
>> You alloctae the new structure when you register/export gpio chip in
>> gpio subsystem core and leave all the individual drivers alone.
>
> The current idea I have is something in the middle. Drivers have to
> change a bit. The important part is that gpiolib handles allocation of
> anything containing states. I'm thinking along the lines of Russell's
> proposal to use netdev_alloc()'s design pattern.
>
> The problem is that currently gpio_chip contains a lot of
> stateful variables (like the dynamically allocated array of GPIO descriptors
> etc) and those are used by the gpiolib core, so they have to be moved away
> from gpio_chip.
>
> So what happens if I don't change the design pattern:
>
> int ret = gpiochip_add(_chip);
> ...
> gpiochip_remove(_chip);
>
> At this point we have to cross-reference the pointer to my chip to
> find the chip to remove. This goes for anything that takes the struct
> gpio_chip *
> as parameter, like gpiochip_add_pin_range(), gpiochip_request_own_desc()
> etc etc. So something inside gpiolib must take a gpio_chip * pointer and
> turn that into the actual state container, e.g, a struct gpio_device.
> Since struct gpio_chip needs to be static and stateless, it cannot contain
> a pointer back to its struct gpio_device.

Why does gpio_chip have to be stateless? I am not saying that it
should or should not, I just want to better understand what you are
trying to achieve.

>
> That means basically comparing pointers across a list of gpio_device's
> to find it. And that's ... very kludgy. But if people think it's better to 
> avoid
> changing all drivers I will consider it.
>
> I think it is better if the GPIO drivers get a handle on the
> real gpio_device * to be used when calling these gpiochip_* related
> functions and also in the callbacks, which is a bigger refactoring
> indeed.
>
> Part of this is trying to be elegant and mimic other subsystems and not
> have GPIO be too hopelessly wayward about things.
>
> If I compare to how struct input_dev is done, you appear to also use the
> pattern Russell suggested with input_dev_allocate() akin to
> netdev_alloc(), and the allocated struct holds all the vtable and states etc,
> and I think it is a good pattern, and that GPIO should conform.

The main difference between gpio_chip (at least as it stands
currently) and input devices and corresponding private driver data is
that input device and driver data has different lifetimes; input
devices may stick around even though driver is unbound from
corresponding device and driver's private data freed.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 000/182] Rid struct gpio_chip from container_of() usage

[Dmitry Torokhov]

On Wed, Dec 09, 2015 at 02:08:35PM +0100, Linus Walleij wrote:
> This removes the use of container_of() constructions from *all*
> GPIO drivers in the kernel. It is done by instead adding an
> optional void *data pointer to the struct gpio_chip and an
> accessor function, gpiochip_get_data() to get it from a driver.
> 
> WHY?
> 
> Because we want to have a proper userspace ABI for GPIO chips,
> which involves using a character device that the user opens
> and closes. While the character device is open, the underlying
> kernel objects must not go away.
> 
> Currently the GPIO drivers keep their state in the struct
> gpio_chip, and that is often allocated by the drivers, very
> often as a part of a containing per-instance state container
> struct for the driver:
> 
> struct foo_state {
>struct gpio_chip chip;  <- OMG my state is there
> };
> 
> Drivers cannot allocate and manage this state: if a user has the
> character device open, the objects allocated must stay around
> even if the driver goes away. Instead drivers need to pass a
> descriptor to the GPIO core, and then the core should allocate
> and manage the lifecycle of things related to the device, such
> as the chardev itself or the struct device related to the GPIO
> device.

Yes, but it does not mean that the object that is being maintained by
the subsystem and that us attached to character device needs to be
gpio_chip itself. You can have something like

struct gpio_chip_chardev {
struct cdev chardev;
struct gpio_chip *chip;
bool dead;
};

struct gpio_chip {
...
struct gpio_chip_chardev *chardev;
...
};

You alloctae the new structure when you register/export gpio chip in
gpio subsystem core and leave all the individual drivers alone.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] brcmfmac: fix error handling of irq_of_parse_and_map

[Dmitry Torokhov]

Return value of irq_of_parse_and_map() is unsigned int, with 0
indicating failure, so testing for negative result never works.

Signed-off-by: Dmitry Torokhov d...@chromium.org
---

Not tested, found by casual code inspection.

 drivers/net/wireless/brcm80211/brcmfmac/of.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/of.c 
b/drivers/net/wireless/brcm80211/brcmfmac/of.c
index eb3fce82..c824570 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/of.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/of.c
@@ -40,8 +40,8 @@ void brcmf_of_probe(struct brcmf_sdio_dev *sdiodev)
return;
 
irq = irq_of_parse_and_map(np, 0);
-   if (irq  0) {
-   brcmf_err(interrupt could not be mapped: err=%d\n, irq);
+   if (!irq) {
+   brcmf_err(interrupt could not be mapped\n);
devm_kfree(dev, sdiodev-pdata);
return;
}
-- 
2.1.0.rc2.206.gedb03e5


-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] NFC: pn544: i2c: fix error handling of irq_of_parse_and_map

[Dmitry Torokhov]

Return value of irq_of_parse_and_map() is unsigned int, with 0
indicating failure, so testing for negative result never works.

Signed-off-by: Dmitry Torokhov d...@chromium.org
---

Not tested, found by casual code inspection.

 drivers/nfc/pn544/i2c.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/nfc/pn544/i2c.c b/drivers/nfc/pn544/i2c.c
index 440291a..cfb3f0b 100644
--- a/drivers/nfc/pn544/i2c.c
+++ b/drivers/nfc/pn544/i2c.c
@@ -918,13 +918,12 @@ static int pn544_hci_i2c_of_request_resources(struct 
i2c_client *client)
}
 
/* IRQ */
-   ret = irq_of_parse_and_map(pp, 0);
-   if (ret  0) {
-   nfc_err(client-dev,
-   Unable to get irq, error: %d\n, ret);
+   client-irq = irq_of_parse_and_map(pp, 0);
+   if (!client-irq) {
+   nfc_err(client-dev, Unable to get irq\n);
+   ret = -EINVAL;
goto err_gpio_fw;
}
-   client-irq = ret;
 
return 0;
 
-- 
2.1.0.rc2.206.gedb03e5


-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] NFC: st21nfca: fix error handling of irq_of_parse_and_map

[Dmitry Torokhov]

Return value of irq_of_parse_and_map() is unsigned int, with 0
indicating failure, so testing for negative result never works.

Also report error returned by devm_gpio_request_one instead of
clobbering it with -ENODEV.

Signed-off-by: Dmitry Torokhov d...@chromium.org
---
 drivers/nfc/st21nfca/i2c.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/nfc/st21nfca/i2c.c b/drivers/nfc/st21nfca/i2c.c
index 0ea756b..6d6d282 100644
--- a/drivers/nfc/st21nfca/i2c.c
+++ b/drivers/nfc/st21nfca/i2c.c
@@ -531,20 +531,19 @@ static int st21nfca_hci_i2c_of_request_resources(struct 
i2c_client *client)
  clf_enable);
if (r) {
nfc_err(client-dev, Failed to request enable pin\n);
-   return -ENODEV;
+   return r;
}
 
phy-gpio_ena = gpio;
 
/* IRQ */
-   r = irq_of_parse_and_map(pp, 0);
-   if (r  0) {
-   nfc_err(client-dev, Unable to get irq, error: %d\n, r);
-   return r;
+   client-irq = irq_of_parse_and_map(pp, 0);
+   if (!client-irq) {
+   nfc_err(client-dev, Unable to get irq\n);
+   return -EINVAL;
}
 
phy-irq_polarity = irq_get_trigger_type(r);
-   client-irq = r;
 
return 0;
 }
-- 
2.1.0.rc2.206.gedb03e5


-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] NFC: st21nfcb: fix error handling of irq_of_parse_and_map

[Dmitry Torokhov]

Return value of irq_of_parse_and_map() is unsigned int, with 0
indicating failure, so testing for negative result never works.

Also report error returned by devm_gpio_request_one instead of
clobbering it with -ENODEV.

Signed-off-by: Dmitry Torokhov d...@chromium.org
---

Not tested, found by casual code inspection.

 drivers/nfc/st21nfcb/i2c.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/nfc/st21nfcb/i2c.c b/drivers/nfc/st21nfcb/i2c.c
index c5d2427..abe73ec 100644
--- a/drivers/nfc/st21nfcb/i2c.c
+++ b/drivers/nfc/st21nfcb/i2c.c
@@ -258,19 +258,18 @@ static int st21nfcb_nci_i2c_of_request_resources(struct 
i2c_client *client)
GPIOF_OUT_INIT_HIGH, clf_reset);
if (r) {
nfc_err(client-dev, Failed to request reset pin\n);
-   return -ENODEV;
+   return r;
}
phy-gpio_reset = gpio;
 
/* IRQ */
-   r = irq_of_parse_and_map(pp, 0);
-   if (r  0) {
-   nfc_err(client-dev, Unable to get irq, error: %d\n, r);
-   return r;
+   client-irq = irq_of_parse_and_map(pp, 0);
+   if (!client-irq) {
+   nfc_err(client-dev, Unable to get irq\n);
+   return -EINVAL;
}
 
phy-irq_polarity = irq_get_trigger_type(r);
-   client-irq = r;
 
return 0;
 }
-- 
2.1.0.rc2.206.gedb03e5


-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html