* [PATCH 3/5] iwlwifi: dvm: no need to check return value of debugfs_create functions
From: Greg Kroah-Hartman @ 2019-06-12 14:26 UTC (permalink / raw)
To: Johannes Berg
Cc: Greg Kroah-Hartman, Emmanuel Grumbach, Luca Coelho,
Intel Linux Wireless, Kalle Valo, David S. Miller, linux-wireless,
netdev
In-Reply-To: <20190612142658.12792-1-gregkh@linuxfoundation.org>
When calling debugfs functions, there is no need to ever check the
return value. This driver was saving the debugfs file away to be
removed at a later time. However, the 80211 core would delete the whole
directory that the debugfs files are created in, after it asks the
driver to do the deletion, so just rely on the 80211 core to do all of
the cleanup for us, making us not need to keep a pointer to the dentries
around at all.
This cleans up the structure of the driver data a bit and makes the code
a tiny bit smaller.
Cc: Johannes Berg <johannes.berg@intel.com>
Cc: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
Cc: Luca Coelho <luciano.coelho@intel.com>
Cc: Intel Linux Wireless <linuxwifi@intel.com>
Cc: Kalle Valo <kvalo@codeaurora.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/net/wireless/intel/iwlwifi/dvm/rs.c | 29 ++++++---------------
drivers/net/wireless/intel/iwlwifi/dvm/rs.h | 4 ---
2 files changed, 8 insertions(+), 25 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/rs.c b/drivers/net/wireless/intel/iwlwifi/dvm/rs.c
index ef4b9de256f7..91e571c99f81 100644
--- a/drivers/net/wireless/intel/iwlwifi/dvm/rs.c
+++ b/drivers/net/wireless/intel/iwlwifi/dvm/rs.c
@@ -3271,28 +3271,16 @@ static void rs_add_debugfs(void *priv, void *priv_sta,
struct dentry *dir)
{
struct iwl_lq_sta *lq_sta = priv_sta;
- lq_sta->rs_sta_dbgfs_scale_table_file =
- debugfs_create_file("rate_scale_table", 0600, dir,
- lq_sta, &rs_sta_dbgfs_scale_table_ops);
- lq_sta->rs_sta_dbgfs_stats_table_file =
- debugfs_create_file("rate_stats_table", 0400, dir,
- lq_sta, &rs_sta_dbgfs_stats_table_ops);
- lq_sta->rs_sta_dbgfs_rate_scale_data_file =
- debugfs_create_file("rate_scale_data", 0400, dir,
- lq_sta, &rs_sta_dbgfs_rate_scale_data_ops);
- lq_sta->rs_sta_dbgfs_tx_agg_tid_en_file =
- debugfs_create_u8("tx_agg_tid_enable", 0600, dir,
- &lq_sta->tx_agg_tid_en);
-}
+ debugfs_create_file("rate_scale_table", 0600, dir, lq_sta,
+ &rs_sta_dbgfs_scale_table_ops);
+ debugfs_create_file("rate_stats_table", 0400, dir, lq_sta,
+ &rs_sta_dbgfs_stats_table_ops);
+ debugfs_create_file("rate_scale_data", 0400, dir, lq_sta,
+ &rs_sta_dbgfs_rate_scale_data_ops);
+ debugfs_create_u8("tx_agg_tid_enable", 0600, dir,
+ &lq_sta->tx_agg_tid_en);
-static void rs_remove_debugfs(void *priv, void *priv_sta)
-{
- struct iwl_lq_sta *lq_sta = priv_sta;
- debugfs_remove(lq_sta->rs_sta_dbgfs_scale_table_file);
- debugfs_remove(lq_sta->rs_sta_dbgfs_stats_table_file);
- debugfs_remove(lq_sta->rs_sta_dbgfs_rate_scale_data_file);
- debugfs_remove(lq_sta->rs_sta_dbgfs_tx_agg_tid_en_file);
}
#endif
@@ -3318,7 +3306,6 @@ static const struct rate_control_ops rs_ops = {
.free_sta = rs_free_sta,
#ifdef CONFIG_MAC80211_DEBUGFS
.add_sta_debugfs = rs_add_debugfs,
- .remove_sta_debugfs = rs_remove_debugfs,
#endif
};
diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/rs.h b/drivers/net/wireless/intel/iwlwifi/dvm/rs.h
index b2df3a8cc464..92836a815234 100644
--- a/drivers/net/wireless/intel/iwlwifi/dvm/rs.h
+++ b/drivers/net/wireless/intel/iwlwifi/dvm/rs.h
@@ -367,10 +367,6 @@ struct iwl_lq_sta {
struct iwl_traffic_load load[IWL_MAX_TID_COUNT];
u8 tx_agg_tid_en;
#ifdef CONFIG_MAC80211_DEBUGFS
- struct dentry *rs_sta_dbgfs_scale_table_file;
- struct dentry *rs_sta_dbgfs_stats_table_file;
- struct dentry *rs_sta_dbgfs_rate_scale_data_file;
- struct dentry *rs_sta_dbgfs_tx_agg_tid_en_file;
u32 dbg_fixed_rate;
#endif
struct iwl_priv *drv;
--
2.22.0
^ permalink raw reply related
* [PATCH 5/5] mac80211: remove unused and unneeded remove_sta_debugfs callback
From: Greg Kroah-Hartman @ 2019-06-12 14:26 UTC (permalink / raw)
To: Johannes Berg
Cc: Greg Kroah-Hartman, Johannes Berg, David S. Miller,
linux-wireless, netdev
In-Reply-To: <20190612142658.12792-1-gregkh@linuxfoundation.org>
The remove_sta_debugfs callback in struct rate_control_ops is no longer
used by any driver, as there is no need for it (the debugfs directory is
already removed recursivly by the mac80211 core.) Because no one needs
it, just remove it to keep anyone else from accidentally using it in the
future.
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
include/net/mac80211.h | 1 -
net/mac80211/rate.h | 9 ---------
net/mac80211/sta_info.c | 1 -
3 files changed, 11 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 72080d9d617e..f42c61422fdf 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -5939,7 +5939,6 @@ struct rate_control_ops {
void (*add_sta_debugfs)(void *priv, void *priv_sta,
struct dentry *dir);
- void (*remove_sta_debugfs)(void *priv, void *priv_sta);
u32 (*get_expected_throughput)(void *priv_sta);
};
diff --git a/net/mac80211/rate.h b/net/mac80211/rate.h
index d59198191a79..a94ce3804962 100644
--- a/net/mac80211/rate.h
+++ b/net/mac80211/rate.h
@@ -63,15 +63,6 @@ static inline void rate_control_add_sta_debugfs(struct sta_info *sta)
#endif
}
-static inline void rate_control_remove_sta_debugfs(struct sta_info *sta)
-{
-#ifdef CONFIG_MAC80211_DEBUGFS
- struct rate_control_ref *ref = sta->rate_ctrl;
- if (ref && ref->ops->remove_sta_debugfs)
- ref->ops->remove_sta_debugfs(ref->priv, sta->rate_ctrl_priv);
-#endif
-}
-
void ieee80211_check_rate_mask(struct ieee80211_sub_if_data *sdata);
/* Get a reference to the rate control algorithm. If `name' is NULL, get the
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index a4932ee3595c..d2933b9f8197 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -1027,7 +1027,6 @@ static void __sta_info_destroy_part2(struct sta_info *sta)
cfg80211_del_sta_sinfo(sdata->dev, sta->sta.addr, sinfo, GFP_KERNEL);
kfree(sinfo);
- rate_control_remove_sta_debugfs(sta);
ieee80211_sta_debugfs_remove(sta);
cleanup_single_sta(sta);
--
2.22.0
^ permalink raw reply related
* [PATCH 4/5] iwlwifi: mvm: remove unused .remove_sta_debugfs callback
From: Greg Kroah-Hartman @ 2019-06-12 14:26 UTC (permalink / raw)
To: Johannes Berg
Cc: Greg Kroah-Hartman, Emmanuel Grumbach, Luca Coelho,
Intel Linux Wireless, Kalle Valo, David S. Miller, Sara Sharon,
Erel Geron, Avraham Stern, linux-wireless, netdev
In-Reply-To: <20190612142658.12792-1-gregkh@linuxfoundation.org>
The .remove_sta_debugfs callback was not doing anything in this driver,
so remove it as it is not needed.
Cc: Johannes Berg <johannes.berg@intel.com>
Cc: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
Cc: Luca Coelho <luciano.coelho@intel.com>
Cc: Intel Linux Wireless <linuxwifi@intel.com>
Cc: Kalle Valo <kvalo@codeaurora.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Sara Sharon <sara.sharon@intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Erel Geron <erelx.geron@intel.com>
Cc: Avraham Stern <avraham.stern@intel.com>
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/net/wireless/intel/iwlwifi/mvm/rs.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/rs.c b/drivers/net/wireless/intel/iwlwifi/mvm/rs.c
index c182821ab22b..15c1f825b749 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/rs.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/rs.c
@@ -4108,10 +4108,6 @@ static void rs_drv_add_sta_debugfs(void *mvm, void *priv_sta,
MVM_DEBUGFS_ADD_FILE_RS(ss_force, dir, 0600);
}
-
-void rs_remove_sta_debugfs(void *mvm, void *mvm_sta)
-{
-}
#endif
/*
@@ -4139,7 +4135,6 @@ static const struct rate_control_ops rs_mvm_ops_drv = {
.rate_update = rs_drv_rate_update,
#ifdef CONFIG_MAC80211_DEBUGFS
.add_sta_debugfs = rs_drv_add_sta_debugfs,
- .remove_sta_debugfs = rs_remove_sta_debugfs,
#endif
.capa = RATE_CTRL_CAPA_VHT_EXT_NSS_BW,
};
--
2.22.0
^ permalink raw reply related
* [PATCH 2/5] iwlegacy: 4965: no need to check return value of debugfs_create functions
From: Greg Kroah-Hartman @ 2019-06-12 14:26 UTC (permalink / raw)
To: Johannes Berg
Cc: Greg Kroah-Hartman, Stanislaw Gruszka, Kalle Valo,
David S. Miller, linux-wireless, netdev
In-Reply-To: <20190612142658.12792-1-gregkh@linuxfoundation.org>
When calling debugfs functions, there is no need to ever check the
return value. This driver was saving the debugfs file away to be
removed at a later time. However, the 80211 core would delete the whole
directory that the debugfs files are created in, after it asks the
driver to do the deletion, so just rely on the 80211 core to do all of
the cleanup for us, making us not need to keep a pointer to the dentries
around at all.
This cleans up the structure of the driver data a bit and makes the code
a tiny bit smaller.
Cc: Stanislaw Gruszka <sgruszka@redhat.com>
Cc: Kalle Valo <kvalo@codeaurora.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/net/wireless/intel/iwlegacy/4965-rs.c | 31 +++++--------------
drivers/net/wireless/intel/iwlegacy/common.h | 4 ---
2 files changed, 8 insertions(+), 27 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlegacy/4965-rs.c b/drivers/net/wireless/intel/iwlegacy/4965-rs.c
index 54ff83829afb..f640709d9862 100644
--- a/drivers/net/wireless/intel/iwlegacy/4965-rs.c
+++ b/drivers/net/wireless/intel/iwlegacy/4965-rs.c
@@ -2767,29 +2767,15 @@ static void
il4965_rs_add_debugfs(void *il, void *il_sta, struct dentry *dir)
{
struct il_lq_sta *lq_sta = il_sta;
- lq_sta->rs_sta_dbgfs_scale_table_file =
- debugfs_create_file("rate_scale_table", 0600, dir,
- lq_sta, &rs_sta_dbgfs_scale_table_ops);
- lq_sta->rs_sta_dbgfs_stats_table_file =
- debugfs_create_file("rate_stats_table", 0400, dir, lq_sta,
- &rs_sta_dbgfs_stats_table_ops);
- lq_sta->rs_sta_dbgfs_rate_scale_data_file =
- debugfs_create_file("rate_scale_data", 0400, dir, lq_sta,
- &rs_sta_dbgfs_rate_scale_data_ops);
- lq_sta->rs_sta_dbgfs_tx_agg_tid_en_file =
- debugfs_create_u8("tx_agg_tid_enable", 0600, dir,
- &lq_sta->tx_agg_tid_en);
-}
-
-static void
-il4965_rs_remove_debugfs(void *il, void *il_sta)
-{
- struct il_lq_sta *lq_sta = il_sta;
- debugfs_remove(lq_sta->rs_sta_dbgfs_scale_table_file);
- debugfs_remove(lq_sta->rs_sta_dbgfs_stats_table_file);
- debugfs_remove(lq_sta->rs_sta_dbgfs_rate_scale_data_file);
- debugfs_remove(lq_sta->rs_sta_dbgfs_tx_agg_tid_en_file);
+ debugfs_create_file("rate_scale_table", 0600, dir, lq_sta,
+ &rs_sta_dbgfs_scale_table_ops);
+ debugfs_create_file("rate_stats_table", 0400, dir, lq_sta,
+ &rs_sta_dbgfs_stats_table_ops);
+ debugfs_create_file("rate_scale_data", 0400, dir, lq_sta,
+ &rs_sta_dbgfs_rate_scale_data_ops);
+ debugfs_create_u8("tx_agg_tid_enable", 0600, dir,
+ &lq_sta->tx_agg_tid_en);
}
#endif
@@ -2816,7 +2802,6 @@ static const struct rate_control_ops rs_4965_ops = {
.free_sta = il4965_rs_free_sta,
#ifdef CONFIG_MAC80211_DEBUGFS
.add_sta_debugfs = il4965_rs_add_debugfs,
- .remove_sta_debugfs = il4965_rs_remove_debugfs,
#endif
};
diff --git a/drivers/net/wireless/intel/iwlegacy/common.h b/drivers/net/wireless/intel/iwlegacy/common.h
index 986646af8dfd..3c6f41763dde 100644
--- a/drivers/net/wireless/intel/iwlegacy/common.h
+++ b/drivers/net/wireless/intel/iwlegacy/common.h
@@ -2822,10 +2822,6 @@ struct il_lq_sta {
struct il_traffic_load load[TID_MAX_LOAD_COUNT];
u8 tx_agg_tid_en;
#ifdef CONFIG_MAC80211_DEBUGFS
- struct dentry *rs_sta_dbgfs_scale_table_file;
- struct dentry *rs_sta_dbgfs_stats_table_file;
- struct dentry *rs_sta_dbgfs_rate_scale_data_file;
- struct dentry *rs_sta_dbgfs_tx_agg_tid_en_file;
u32 dbg_fixed_rate;
#endif
struct il_priv *drv;
--
2.22.0
^ permalink raw reply related
* [PATCH 1/5] iwlegacy: 3945: no need to check return value of debugfs_create functions
From: Greg Kroah-Hartman @ 2019-06-12 14:26 UTC (permalink / raw)
To: Johannes Berg
Cc: Greg Kroah-Hartman, Stanislaw Gruszka, Kalle Valo,
David S. Miller, linux-wireless, netdev
When calling debugfs functions, there is no need to ever check the
return value. This driver was saving the debugfs file away to be
removed at a later time. However, the 80211 core would delete the whole
directory that the debugfs files are created in, after it asks the
driver to do the deletion, so just rely on the 80211 core to do all of
the cleanup for us, making us not need to keep a pointer to the dentries
around at all.
This cleans up the structure of the driver data a bit and makes the code
a tiny bit smaller.
Cc: Stanislaw Gruszka <sgruszka@redhat.com>
Cc: Kalle Valo <kvalo@codeaurora.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/net/wireless/intel/iwlegacy/3945-rs.c | 14 ++------------
drivers/net/wireless/intel/iwlegacy/3945.h | 3 ---
2 files changed, 2 insertions(+), 15 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlegacy/3945-rs.c b/drivers/net/wireless/intel/iwlegacy/3945-rs.c
index a697edd46e7f..1d810cae6091 100644
--- a/drivers/net/wireless/intel/iwlegacy/3945-rs.c
+++ b/drivers/net/wireless/intel/iwlegacy/3945-rs.c
@@ -861,17 +861,8 @@ il3945_add_debugfs(void *il, void *il_sta, struct dentry *dir)
{
struct il3945_rs_sta *lq_sta = il_sta;
- lq_sta->rs_sta_dbgfs_stats_table_file =
- debugfs_create_file("rate_stats_table", 0600, dir, lq_sta,
- &rs_sta_dbgfs_stats_table_ops);
-
-}
-
-static void
-il3945_remove_debugfs(void *il, void *il_sta)
-{
- struct il3945_rs_sta *lq_sta = il_sta;
- debugfs_remove(lq_sta->rs_sta_dbgfs_stats_table_file);
+ debugfs_create_file("rate_stats_table", 0600, dir, lq_sta,
+ &rs_sta_dbgfs_stats_table_ops);
}
#endif
@@ -898,7 +889,6 @@ static const struct rate_control_ops rs_ops = {
.free_sta = il3945_rs_free_sta,
#ifdef CONFIG_MAC80211_DEBUGFS
.add_sta_debugfs = il3945_add_debugfs,
- .remove_sta_debugfs = il3945_remove_debugfs,
#endif
};
diff --git a/drivers/net/wireless/intel/iwlegacy/3945.h b/drivers/net/wireless/intel/iwlegacy/3945.h
index 00030d43a194..1aeb4b238fcf 100644
--- a/drivers/net/wireless/intel/iwlegacy/3945.h
+++ b/drivers/net/wireless/intel/iwlegacy/3945.h
@@ -87,9 +87,6 @@ struct il3945_rs_sta {
u8 start_rate;
struct timer_list rate_scale_flush;
struct il3945_rate_scale_data win[RATE_COUNT_3945];
-#ifdef CONFIG_MAC80211_DEBUGFS
- struct dentry *rs_sta_dbgfs_stats_table_file;
-#endif
/* used to be in sta_info */
int last_txrate_idx;
--
2.22.0
^ permalink raw reply related
* Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
From: Stanislaw Gruszka @ 2019-06-12 14:21 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: Lorenzo Bianconi, nbd, kvalo, linux-wireless
In-Reply-To: <20190612125905.GB2600@redhat.com>
On Wed, Jun 12, 2019 at 02:59:05PM +0200, Stanislaw Gruszka wrote:
> > > If max RX AMSDU size is 3839B I do not see reason why we allocate
> > > MT_SG_MAX_SIZE=8 of MT_RX_BUF_SIZE=2k buffers for sg_en case.
> > > I thought the reason is that max AMSDU size is 16kB so it fit into
> > > 8 sg buffers of 2k.
> > >
> > > In other words, for me, looks like either
> > > - we can not handle AMSDU for non sg case because we do not
> > > allocate big enough buffer
> >
> > I think AMSDU is mandatory and we currently support it even for non-sg case
> > (since max rx AMSDU is 3839B)
> >
> > > or
> > > - we can just use one PAGE_SIZE buffer for rx and remove sg
> > > buffers for rx completely
> >
> > using sg buffers we can support bigger rx AMSDU size in the future without using
> > huge buffers (e.g. we can try to use IEEE80211_MAX_MPDU_LEN_HT_7935 with
> > mt76x2u)
>
> I think it would be simpler just to allocate 2 pages for 7935B .
And if we could determine that there is no true need to use sg for rx,
I think best approach here would be revert f8f527b16db5 in v5.2 to fix
regression and remove rx sg in -next. That would make code simpler,
allocate 4k instead 16k per packet, allow to use build_skb (4096 - 3839
give enough space for shared info) and not use usb hcd bounce buffer.
Stanislaw
^ permalink raw reply
* Re: [PATCH v3 3/5] brcmfmac: sdio: Disable auto-tuning around commands expected to fail
From: Arend Van Spriel @ 2019-06-12 13:58 UTC (permalink / raw)
To: Ulf Hansson
Cc: Doug Anderson, Hunter, Adrian, Kalle Valo, brcm80211-dev-list.pdl,
linux-rockchip, Double Lo, briannorris, linux-wireless,
Naveen Gupta, Madhan Mohan R, mka, Wright Feng, Chi-Hsien Lin,
netdev, brcm80211-dev-list, Franky Lin, linux-kernel,
Hante Meuleman, YueHaibing, David S. Miller
In-Reply-To: <CAPDyKFpM0+FfvoMo8Z_hxM9rzSjeQZHCsA2SPa8WP+SRDhhsPA@mail.gmail.com>
On 6/12/2019 1:48 PM, Ulf Hansson wrote:
> On Wed, 12 Jun 2019 at 13:11, Arend Van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>>
>> On 6/12/2019 12:10 PM, Ulf Hansson wrote:
>>>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c:
>>>> mmc_set_data_timeout(md, func->card);
>>>> mmc_wait_for_req(func->card->host, mr);
>>> These are not okay, none of these things calls should really be done
>>> from an SDIO func driver.
>>>
>>> It tells me that the func driver is a doing workaround for something
>>> that should be managed in a common way.
>>
>> We are using some low-level functions passing chain of skbuff to the
>> device using CMD53 with scatterlist. If I recall correctly Marvell made
>> an attempt to have a similar function for it in the mmc stack. Not sure
>> if that ever made it in. If so I can rework our driver using that API.
>> If not, I can make a new attempt.
>
> I recall there were some patches, but not sure why we didn't merge them.
>
> Anyway, if you want to move this forward, that would be awesome!
Let's scope it before moving forward. Our use-case is to transfer a
chain of skbuff's. I am pretty sure that is not something we want to
deal with in mmc stack api. So I suppose passing a scatterlist is more
sensible, right? Maybe on sdio layer of the stack we could consider
dealing with skbuff's for network func drivers?
Let me see if I can find those Marvell patches. Might be a good start.
Regards,
Arend
^ permalink raw reply
* Re: [PATCH v3 2/5] mmc: core: API for temporarily disabling auto-retuning due to errors
From: Ulf Hansson @ 2019-06-12 13:25 UTC (permalink / raw)
To: Douglas Anderson
Cc: Kalle Valo, Adrian Hunter, Arend van Spriel,
brcm80211-dev-list.pdl, open list:ARM/Rockchip SoC..., Double Lo,
Brian Norris, linux-wireless, Naveen Gupta, Madhan Mohan R,
Matthias Kaehlcke, Wright Feng, Chi-Hsien Lin, netdev,
brcm80211-dev-list, Jiong Wu, Ritesh Harjani,
linux-mmc@vger.kernel.org, Linux Kernel Mailing List, Shawn Lin,
Wolfram Sang, Avri Altman
In-Reply-To: <20190607223716.119277-3-dianders@chromium.org>
On Sat, 8 Jun 2019 at 00:37, Douglas Anderson <dianders@chromium.org> wrote:
>
> Normally when the MMC core sees an "-EILSEQ" error returned by a host
> controller then it will trigger a retuning of the card. This is
> generally a good idea.
>
> However, if a command is expected to sometimes cause transfer errors
> then these transfer errors shouldn't cause a re-tuning. This
> re-tuning will be a needless waste of time. One example case where a
> transfer is expected to cause errors is when transitioning between
> idle (sometimes referred to as "sleep" in Broadcom code) and active
> state on certain Broadcom WiFi cards. Specifically if the card was
> already transitioning between states when the command was sent it
> could cause an error on the SDIO bus.
>
> Let's add an API that the SDIO card drivers can call that will
> temporarily disable the auto-tuning functionality. Then we can add a
> call to this in the Broadcom WiFi driver and any other driver that
> might have similar needs.
>
> NOTE: this makes the assumption that the card is already tuned well
> enough that it's OK to disable the auto-retuning during one of these
> error-prone situations. Presumably the driver code performing the
> error-prone transfer knows how to recover / retry from errors. ...and
> after we can get back to a state where transfers are no longer
> error-prone then we can enable the auto-retuning again. If we truly
> find ourselves in a case where the card needs to be retuned sometimes
> to handle one of these error-prone transfers then we can always try a
> few transfers first without auto-retuning and then re-try with
> auto-retuning if the first few fail.
>
> Without this change on rk3288-veyron-minnie I periodically see this in
> the logs of a machine just sitting there idle:
> dwmmc_rockchip ff0d0000.dwmmc: Successfully tuned phase to XYZ
>
> Fixes: bd11e8bd03ca ("mmc: core: Flag re-tuning is needed on CRC errors")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> Note that are are a whole boatload of different ways that we could
> provide an API for the Broadcom WiFi SDIO driver. This patch
> illustrates one way but if maintainers feel strongly that this is too
> ugly and have a better idea then I can give it a shot too. From a
> purist point of view I kinda felt that the "expect errors" really
> belonged as part of the mmc_request structure, but getting it into
> there meant changing a whole pile of core SD/MMC APIs. Simply adding
> it to the host seemed to match the current style better and was a less
> intrusive change.
>
> Changes in v3:
> - Took out the spinlock since I believe this is all in one context.
This needs to be clarified, preferable also in a function header.
If I understand correctly, the SDIO func driver needs the host to be
claimed when it calls mmc_expect_errors_begin(). More importantly, it
also needs to be keep it claimed until after it had called
mmc_expect_errors_end(). Correct?
>
> Changes in v2:
> - Updated commit message to clarify based on discussion of v1.
>
> drivers/mmc/core/core.c | 19 +++++++++++++++++--
> include/linux/mmc/core.h | 2 ++
> include/linux/mmc/host.h | 1 +
> 3 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 6db36dc870b5..bc109ec49406 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -144,8 +144,9 @@ void mmc_request_done(struct mmc_host *host, struct mmc_request *mrq)
> int err = cmd->error;
>
> /* Flag re-tuning needed on CRC errors */
> - if ((cmd->opcode != MMC_SEND_TUNING_BLOCK &&
> - cmd->opcode != MMC_SEND_TUNING_BLOCK_HS200) &&
> + if (cmd->opcode != MMC_SEND_TUNING_BLOCK &&
> + cmd->opcode != MMC_SEND_TUNING_BLOCK_HS200 &&
> + !host->expect_errors &&
> (err == -EILSEQ || (mrq->sbc && mrq->sbc->error == -EILSEQ) ||
> (mrq->data && mrq->data->error == -EILSEQ) ||
> (mrq->stop && mrq->stop->error == -EILSEQ)))
> @@ -2163,6 +2164,20 @@ int mmc_sw_reset(struct mmc_host *host)
> }
> EXPORT_SYMBOL(mmc_sw_reset);
>
> +void mmc_expect_errors_begin(struct mmc_host *host)
> +{
> + WARN_ON(host->expect_errors);
Please remove the WARN_ON. If you believe there is a need for
reference counting, then please add that instead (but likely not in
the phase?).
> + host->expect_errors = true;
> +}
> +EXPORT_SYMBOL_GPL(mmc_expect_errors_begin);
> +
> +void mmc_expect_errors_end(struct mmc_host *host)
> +{
> + WARN_ON(!host->expect_errors);
Ditto.
> + host->expect_errors = false;
> +}
> +EXPORT_SYMBOL_GPL(mmc_expect_errors_end);
These new APIs seems to be useful solely for SDIO. Even if it turns
out later that they can be made generic, I suggest to start with a
SDIO func API instead.
However, using a new host variable (->expect_errors) is fine by me.
> +
> static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)
> {
> host->f_init = freq;
> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
> index 134a6483347a..02a13abf0cda 100644
> --- a/include/linux/mmc/core.h
> +++ b/include/linux/mmc/core.h
> @@ -178,6 +178,8 @@ int mmc_wait_for_cmd(struct mmc_host *host, struct mmc_command *cmd,
>
> int mmc_hw_reset(struct mmc_host *host);
> int mmc_sw_reset(struct mmc_host *host);
> +void mmc_expect_errors_begin(struct mmc_host *host);
> +void mmc_expect_errors_end(struct mmc_host *host);
The API prevents a new re-tune to be "scheduled" in case requests are
failing with -EILSEQ.
To better reflect that, may I suggest to rename this to
sdio_retune_crc_disable() and sdio_retune_crc_enable(). Or something
along those lines.
> void mmc_set_data_timeout(struct mmc_data *data, const struct mmc_card *card);
>
> #endif /* LINUX_MMC_CORE_H */
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 43d0f0c496f6..8d553fb8c834 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -398,6 +398,7 @@ struct mmc_host {
> unsigned int retune_now:1; /* do re-tuning at next req */
> unsigned int retune_paused:1; /* re-tuning is temporarily disabled */
> unsigned int use_blk_mq:1; /* use blk-mq */
> + unsigned int expect_errors:1; /* don't trigger retune upon errors */
>
> int rescan_disable; /* disable card detection */
> int rescan_entered; /* used with nonremovable devices */
> --
> 2.22.0.rc2.383.gf4fbbf30c2-goog
>
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
From: Stanislaw Gruszka @ 2019-06-12 12:59 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: Lorenzo Bianconi, nbd, kvalo, linux-wireless
In-Reply-To: <20190612122845.GH8107@localhost.localdomain>
On Wed, Jun 12, 2019 at 02:28:48PM +0200, Lorenzo Bianconi wrote:
> [...]
> >
> > My sense of humor declined quite drastically lastly ;-/
> >
> > > > > but we can be more cautious since probably copying
> > > > > the first 128B will not make any difference
> > > >
> > > > Not sure if I understand what you mean.
> > >
> > > Please correct me if I am wrong but I think max amsdu rx size is 3839B for
> > > mt76. For the sg_en case this frame will span over multiple sg buffers since
> > > sg buffer size is 2048B (2 sg buffers). Moreover if we do not take into account
> > > skb_shared_info when configuring the sg buffer size we will need to always copy
> > > the first 128B of the first buffer since received len will be set to 2048 and
> > > the following if condition will always fail:
> > >
> > > if (SKB_WITH_OVERHEAD(buf_size) >= MT_DMA_HDR_LEN + len) {
> > > }
> >
> > Ok, that I understand.
> >
> > > > > > And skb_shered_info is needed only in first buffer IIUC.
> > > > > >
> > > > > > Also this patch seems to make first patch unnecessary except for
> > > > > > non sg_en case (in which I think rx AMSDU is broken anyway),
> > > > > > so I would prefer just to apply first patch.
> > > > >
> > > > > I do not think rx AMSDU is broken for non sg_en case since the max rx value
> > > > > allowed should be 3839 IIRC and we alloc one page in this case
> > > >
> > > > If that's the case we should be fine, but then I do not understand
> > > > why we allocate 8*2k buffers for sg_en case, isn't that AP can
> > > > sent AMSDU frame 16k big?
> > >
> > > Sorry I did not get what you mean here, could you please explain?
> >
> > If max RX AMSDU size is 3839B I do not see reason why we allocate
> > MT_SG_MAX_SIZE=8 of MT_RX_BUF_SIZE=2k buffers for sg_en case.
> > I thought the reason is that max AMSDU size is 16kB so it fit into
> > 8 sg buffers of 2k.
> >
> > In other words, for me, looks like either
> > - we can not handle AMSDU for non sg case because we do not
> > allocate big enough buffer
>
> I think AMSDU is mandatory and we currently support it even for non-sg case
> (since max rx AMSDU is 3839B)
>
> > or
> > - we can just use one PAGE_SIZE buffer for rx and remove sg
> > buffers for rx completely
>
> using sg buffers we can support bigger rx AMSDU size in the future without using
> huge buffers (e.g. we can try to use IEEE80211_MAX_MPDU_LEN_HT_7935 with
> mt76x2u)
I think it would be simpler just to allocate 2 pages for 7935B .
Stanislaw
^ permalink raw reply
* Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
From: Lorenzo Bianconi @ 2019-06-12 12:28 UTC (permalink / raw)
To: Stanislaw Gruszka; +Cc: Lorenzo Bianconi, nbd, kvalo, linux-wireless
In-Reply-To: <20190612115120.GA3496@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 2358 bytes --]
[...]
>
> My sense of humor declined quite drastically lastly ;-/
>
> > > > but we can be more cautious since probably copying
> > > > the first 128B will not make any difference
> > >
> > > Not sure if I understand what you mean.
> >
> > Please correct me if I am wrong but I think max amsdu rx size is 3839B for
> > mt76. For the sg_en case this frame will span over multiple sg buffers since
> > sg buffer size is 2048B (2 sg buffers). Moreover if we do not take into account
> > skb_shared_info when configuring the sg buffer size we will need to always copy
> > the first 128B of the first buffer since received len will be set to 2048 and
> > the following if condition will always fail:
> >
> > if (SKB_WITH_OVERHEAD(buf_size) >= MT_DMA_HDR_LEN + len) {
> > }
>
> Ok, that I understand.
>
> > > > > And skb_shered_info is needed only in first buffer IIUC.
> > > > >
> > > > > Also this patch seems to make first patch unnecessary except for
> > > > > non sg_en case (in which I think rx AMSDU is broken anyway),
> > > > > so I would prefer just to apply first patch.
> > > >
> > > > I do not think rx AMSDU is broken for non sg_en case since the max rx value
> > > > allowed should be 3839 IIRC and we alloc one page in this case
> > >
> > > If that's the case we should be fine, but then I do not understand
> > > why we allocate 8*2k buffers for sg_en case, isn't that AP can
> > > sent AMSDU frame 16k big?
> >
> > Sorry I did not get what you mean here, could you please explain?
>
> If max RX AMSDU size is 3839B I do not see reason why we allocate
> MT_SG_MAX_SIZE=8 of MT_RX_BUF_SIZE=2k buffers for sg_en case.
> I thought the reason is that max AMSDU size is 16kB so it fit into
> 8 sg buffers of 2k.
>
> In other words, for me, looks like either
> - we can not handle AMSDU for non sg case because we do not
> allocate big enough buffer
I think AMSDU is mandatory and we currently support it even for non-sg case
(since max rx AMSDU is 3839B)
> or
> - we can just use one PAGE_SIZE buffer for rx and remove sg
> buffers for rx completely
using sg buffers we can support bigger rx AMSDU size in the future without using
huge buffers (e.g. we can try to use IEEE80211_MAX_MPDU_LEN_HT_7935 with
mt76x2u)
Regards,
Lorenzo
>
> Do I miss something ?
>
> Stanislaw
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
From: Stanislaw Gruszka @ 2019-06-12 11:51 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: Lorenzo Bianconi, nbd, kvalo, linux-wireless
In-Reply-To: <20190612104921.GF8107@localhost.localdomain>
On Wed, Jun 12, 2019 at 12:49:22PM +0200, Lorenzo Bianconi wrote:
> > On Wed, Jun 12, 2019 at 11:53:03AM +0200, Lorenzo Bianconi wrote:
> > > > On Fri, May 31, 2019 at 11:38:23AM +0200, Lorenzo Bianconi wrote:
> > >
> > > [...]
> > >
> > > > > }
> > > > >
> > > > > urb->num_sgs = max_t(int, i, urb->num_sgs);
> > > > > - urb->transfer_buffer_length = urb->num_sgs * q->buf_size,
> > > > > + urb->transfer_buffer_length = urb->num_sgs * data_size;
> > > > > sg_init_marker(urb->sg, urb->num_sgs);
> > > > >
> > > > > return i ? : -ENOMEM;
> > > > > @@ -611,8 +611,12 @@ static int mt76u_alloc_rx(struct mt76_dev *dev)
> > > > > if (!q->entry)
> > > > > return -ENOMEM;
> > > > >
> > > > > - q->buf_size = dev->usb.sg_en ? MT_RX_BUF_SIZE : PAGE_SIZE;
> > > > > + if (dev->usb.sg_en)
> > > > > + q->buf_size = MT_BUF_WITH_OVERHEAD(MT_RX_BUF_SIZE);
> > > >
> > > > I strongly recommend to not doing this. While this should work
> > > > in theory creating buffer with size of 2k + some bytes might
> > > > trigger various bugs in dma mapping or other low level code.
> > >
> > > even in practice actually :)
> >
> > I wouldn't be sure about this. It's not common to have buffers of
> > such size and crossing pages boundaries. It really can trigger
> > nasty bugs on various IOMMU drivers.
>
> I was just joking, I mean that it worked in the tests I carried out, but I
> agree it can trigger some issues in buggy IOMMU drivers
My sense of humor declined quite drastically lastly ;-/
> > > but we can be more cautious since probably copying
> > > the first 128B will not make any difference
> >
> > Not sure if I understand what you mean.
>
> Please correct me if I am wrong but I think max amsdu rx size is 3839B for
> mt76. For the sg_en case this frame will span over multiple sg buffers since
> sg buffer size is 2048B (2 sg buffers). Moreover if we do not take into account
> skb_shared_info when configuring the sg buffer size we will need to always copy
> the first 128B of the first buffer since received len will be set to 2048 and
> the following if condition will always fail:
>
> if (SKB_WITH_OVERHEAD(buf_size) >= MT_DMA_HDR_LEN + len) {
> }
Ok, that I understand.
> > > > And skb_shered_info is needed only in first buffer IIUC.
> > > >
> > > > Also this patch seems to make first patch unnecessary except for
> > > > non sg_en case (in which I think rx AMSDU is broken anyway),
> > > > so I would prefer just to apply first patch.
> > >
> > > I do not think rx AMSDU is broken for non sg_en case since the max rx value
> > > allowed should be 3839 IIRC and we alloc one page in this case
> >
> > If that's the case we should be fine, but then I do not understand
> > why we allocate 8*2k buffers for sg_en case, isn't that AP can
> > sent AMSDU frame 16k big?
>
> Sorry I did not get what you mean here, could you please explain?
If max RX AMSDU size is 3839B I do not see reason why we allocate
MT_SG_MAX_SIZE=8 of MT_RX_BUF_SIZE=2k buffers for sg_en case.
I thought the reason is that max AMSDU size is 16kB so it fit into
8 sg buffers of 2k.
In other words, for me, looks like either
- we can not handle AMSDU for non sg case because we do not
allocate big enough buffer
or
- we can just use one PAGE_SIZE buffer for rx and remove sg
buffers for rx completely
Do I miss something ?
Stanislaw
^ permalink raw reply
* Re: [PATCH v3 3/5] brcmfmac: sdio: Disable auto-tuning around commands expected to fail
From: Ulf Hansson @ 2019-06-12 11:48 UTC (permalink / raw)
To: Arend Van Spriel
Cc: Doug Anderson, Hunter, Adrian, Kalle Valo,
brcm80211-dev-list.pdl@broadcom.com,
linux-rockchip@lists.infradead.org, Double Lo,
briannorris@chromium.org, linux-wireless@vger.kernel.org,
Naveen Gupta, Madhan Mohan R, mka@chromium.org, Wright Feng,
Chi-Hsien Lin, netdev@vger.kernel.org,
brcm80211-dev-list@cypress.com, Franky Lin,
linux-kernel@vger.kernel.org, Hante Meuleman, YueHaibing,
David S. Miller
In-Reply-To: <c7c6d3f4-ebb1-8964-0616-973fae1ab47d@broadcom.com>
On Wed, 12 Jun 2019 at 13:11, Arend Van Spriel
<arend.vanspriel@broadcom.com> wrote:
>
> On 6/12/2019 12:10 PM, Ulf Hansson wrote:
> >> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c:
> >> mmc_set_data_timeout(md, func->card);
> >> mmc_wait_for_req(func->card->host, mr);
> > These are not okay, none of these things calls should really be done
> > from an SDIO func driver.
> >
> > It tells me that the func driver is a doing workaround for something
> > that should be managed in a common way.
>
> We are using some low-level functions passing chain of skbuff to the
> device using CMD53 with scatterlist. If I recall correctly Marvell made
> an attempt to have a similar function for it in the mmc stack. Not sure
> if that ever made it in. If so I can rework our driver using that API.
> If not, I can make a new attempt.
I recall there were some patches, but not sure why we didn't merge them.
Anyway, if you want to move this forward, that would be awesome!
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH v2 1/2] mt76: usb: fix rx A-MSDU support
From: Lorenzo Bianconi @ 2019-06-12 11:17 UTC (permalink / raw)
To: Stanislaw Gruszka; +Cc: Lorenzo Bianconi, nbd, kvalo, linux-wireless
In-Reply-To: <20190612105801.GA2600@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 2469 bytes --]
> On Wed, Jun 12, 2019 at 12:21:34PM +0200, Lorenzo Bianconi wrote:
> > On Jun 12, Stanislaw Gruszka wrote:
> > > On Wed, Jun 12, 2019 at 11:45:21AM +0200, Lorenzo Bianconi wrote:
> > > > > > +mt76u_build_rx_skb(u8 *data, int len, int buf_size,
> > > > > > + int *nsgs)
> > > > > > +{
> > > > > > + int data_len = min(len, MT_SKB_HEAD_LEN);
> > >
> > > Oh, and this looks unneeded as well as for len < MT_SKB_HEAD_LEN=128
> > > we will go through fast path.
> >
> > I guess if we remove data_len = min(len, MT_SKB_HEAD_LEN) and even *nsgs = 0 at
> > the end we are making some assumptions on the value of MT_SKB_HEAD_LEN and
> > buf_size. In the patch I just avoided them but maybe we can just assume that
> > MT_SKB_HEAD_LEN and buf_size will not changed in the future. What do you
> > think?
>
> Yes, sure. Other drivers just use 128 value directly and don't even
> create a macro for that. And if somebody will decide to change
> buf_size it will not be small value.
Ok, I will simplify it in v2
>
> > > > > mt7601u and iwlmvm just copy hdrlen + 8 and put the rest
> > > > > of the buffer in fragment, which supose to be more efficient,
> > > > > see comment in iwl_mvm_pass_packet_to_mac80211().
> > > >
> > > > Right here we copy 128B instead of 32 but I think it is good to have L3 and L4
> > > > header in the linear area of the skb since otherwise the stack will need to
> > > > align them
> > >
> > > Not sure if understand, I think aliment of L3 & L4 headers will be
> > > the same, assuming ieee80211 header is aligned the same in fragment
> > > buffer and in linear area. But if you think this is better to copy those
> > > to linear area I'm ok with that.
> >
> > Sorry I have not been so clear. I mean in the stack before accessing a given
> > header we will run pskb_may_pull() that can end up copying the skb if there is
> > not enough space in the skb->head
>
> Ok, so L3 and L4 headers should be in linear area of skb and if not
> network stack will copy them from fragment. But I wonder why other
> drivers just copy ieee80211_hdr and SNAP ? Isn't that if we copy
> 128B then is possible that part of the payload will be in linear
> area and part in fragment, whereas is expected that payload
> will not be broken into two parts?
I think the payload can be split in multiple skb fragments, the constraint is
just related to header parsing
Regards,
Lorenzo
>
> Stanislaw
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v3 3/5] brcmfmac: sdio: Disable auto-tuning around commands expected to fail
From: Arend Van Spriel @ 2019-06-12 11:11 UTC (permalink / raw)
To: Ulf Hansson, Doug Anderson
Cc: Hunter, Adrian, Kalle Valo, brcm80211-dev-list.pdl@broadcom.com,
linux-rockchip@lists.infradead.org, Double Lo,
briannorris@chromium.org, linux-wireless@vger.kernel.org,
Naveen Gupta, Madhan Mohan R, mka@chromium.org, Wright Feng,
Chi-Hsien Lin, netdev@vger.kernel.org,
brcm80211-dev-list@cypress.com, Franky Lin,
linux-kernel@vger.kernel.org, Hante Meuleman, YueHaibing,
David S. Miller
In-Reply-To: <CAPDyKFo=QMRTkNYUVSE2AqiZgytkTVRXF0Mvznn6trVT4-cR=Q@mail.gmail.com>
On 6/12/2019 12:10 PM, Ulf Hansson wrote:
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c:
>> mmc_set_data_timeout(md, func->card);
>> mmc_wait_for_req(func->card->host, mr);
> These are not okay, none of these things calls should really be done
> from an SDIO func driver.
>
> It tells me that the func driver is a doing workaround for something
> that should be managed in a common way.
We are using some low-level functions passing chain of skbuff to the
device using CMD53 with scatterlist. If I recall correctly Marvell made
an attempt to have a similar function for it in the mmc stack. Not sure
if that ever made it in. If so I can rework our driver using that API.
If not, I can make a new attempt.
Regards,
Arend
^ permalink raw reply
* Re: [PATCH v2 1/2] mt76: usb: fix rx A-MSDU support
From: Stanislaw Gruszka @ 2019-06-12 10:58 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: Lorenzo Bianconi, nbd, kvalo, linux-wireless
In-Reply-To: <20190612102133.GE8107@localhost.localdomain>
On Wed, Jun 12, 2019 at 12:21:34PM +0200, Lorenzo Bianconi wrote:
> On Jun 12, Stanislaw Gruszka wrote:
> > On Wed, Jun 12, 2019 at 11:45:21AM +0200, Lorenzo Bianconi wrote:
> > > > > +mt76u_build_rx_skb(u8 *data, int len, int buf_size,
> > > > > + int *nsgs)
> > > > > +{
> > > > > + int data_len = min(len, MT_SKB_HEAD_LEN);
> >
> > Oh, and this looks unneeded as well as for len < MT_SKB_HEAD_LEN=128
> > we will go through fast path.
>
> I guess if we remove data_len = min(len, MT_SKB_HEAD_LEN) and even *nsgs = 0 at
> the end we are making some assumptions on the value of MT_SKB_HEAD_LEN and
> buf_size. In the patch I just avoided them but maybe we can just assume that
> MT_SKB_HEAD_LEN and buf_size will not changed in the future. What do you
> think?
Yes, sure. Other drivers just use 128 value directly and don't even
create a macro for that. And if somebody will decide to change
buf_size it will not be small value.
> > > > mt7601u and iwlmvm just copy hdrlen + 8 and put the rest
> > > > of the buffer in fragment, which supose to be more efficient,
> > > > see comment in iwl_mvm_pass_packet_to_mac80211().
> > >
> > > Right here we copy 128B instead of 32 but I think it is good to have L3 and L4
> > > header in the linear area of the skb since otherwise the stack will need to
> > > align them
> >
> > Not sure if understand, I think aliment of L3 & L4 headers will be
> > the same, assuming ieee80211 header is aligned the same in fragment
> > buffer and in linear area. But if you think this is better to copy those
> > to linear area I'm ok with that.
>
> Sorry I have not been so clear. I mean in the stack before accessing a given
> header we will run pskb_may_pull() that can end up copying the skb if there is
> not enough space in the skb->head
Ok, so L3 and L4 headers should be in linear area of skb and if not
network stack will copy them from fragment. But I wonder why other
drivers just copy ieee80211_hdr and SNAP ? Isn't that if we copy
128B then is possible that part of the payload will be in linear
area and part in fragment, whereas is expected that payload
will not be broken into two parts?
Stanislaw
^ permalink raw reply
* Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
From: Lorenzo Bianconi @ 2019-06-12 10:49 UTC (permalink / raw)
To: Stanislaw Gruszka; +Cc: Lorenzo Bianconi, nbd, kvalo, linux-wireless
In-Reply-To: <20190612102502.GB4431@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 2723 bytes --]
> On Wed, Jun 12, 2019 at 11:53:03AM +0200, Lorenzo Bianconi wrote:
> > > On Fri, May 31, 2019 at 11:38:23AM +0200, Lorenzo Bianconi wrote:
> >
> > [...]
> >
> > > > }
> > > >
> > > > urb->num_sgs = max_t(int, i, urb->num_sgs);
> > > > - urb->transfer_buffer_length = urb->num_sgs * q->buf_size,
> > > > + urb->transfer_buffer_length = urb->num_sgs * data_size;
> > > > sg_init_marker(urb->sg, urb->num_sgs);
> > > >
> > > > return i ? : -ENOMEM;
> > > > @@ -611,8 +611,12 @@ static int mt76u_alloc_rx(struct mt76_dev *dev)
> > > > if (!q->entry)
> > > > return -ENOMEM;
> > > >
> > > > - q->buf_size = dev->usb.sg_en ? MT_RX_BUF_SIZE : PAGE_SIZE;
> > > > + if (dev->usb.sg_en)
> > > > + q->buf_size = MT_BUF_WITH_OVERHEAD(MT_RX_BUF_SIZE);
> > >
> > > I strongly recommend to not doing this. While this should work
> > > in theory creating buffer with size of 2k + some bytes might
> > > trigger various bugs in dma mapping or other low level code.
> >
> > even in practice actually :)
>
> I wouldn't be sure about this. It's not common to have buffers of
> such size and crossing pages boundaries. It really can trigger
> nasty bugs on various IOMMU drivers.
I was just joking, I mean that it worked in the tests I carried out, but I
agree it can trigger some issues in buggy IOMMU drivers
>
> > but we can be more cautious since probably copying
> > the first 128B will not make any difference
>
> Not sure if I understand what you mean.
Please correct me if I am wrong but I think max amsdu rx size is 3839B for
mt76. For the sg_en case this frame will span over multiple sg buffers since
sg buffer size is 2048B (2 sg buffers). Moreover if we do not take into account
skb_shared_info when configuring the sg buffer size we will need to always copy
the first 128B of the first buffer since received len will be set to 2048 and
the following if condition will always fail:
if (SKB_WITH_OVERHEAD(buf_size) >= MT_DMA_HDR_LEN + len) {
}
>
> > > And skb_shered_info is needed only in first buffer IIUC.
> > >
> > > Also this patch seems to make first patch unnecessary except for
> > > non sg_en case (in which I think rx AMSDU is broken anyway),
> > > so I would prefer just to apply first patch.
> >
> > I do not think rx AMSDU is broken for non sg_en case since the max rx value
> > allowed should be 3839 IIRC and we alloc one page in this case
>
> If that's the case we should be fine, but then I do not understand
> why we allocate 8*2k buffers for sg_en case, isn't that AP can
> sent AMSDU frame 16k big?
Sorry I did not get what you mean here, could you please explain?
Regards,
Lorenzo
>
> Stanislaw
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
From: Stanislaw Gruszka @ 2019-06-12 10:25 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: Lorenzo Bianconi, nbd, kvalo, linux-wireless
In-Reply-To: <20190612095302.GD8107@localhost.localdomain>
On Wed, Jun 12, 2019 at 11:53:03AM +0200, Lorenzo Bianconi wrote:
> > On Fri, May 31, 2019 at 11:38:23AM +0200, Lorenzo Bianconi wrote:
>
> [...]
>
> > > }
> > >
> > > urb->num_sgs = max_t(int, i, urb->num_sgs);
> > > - urb->transfer_buffer_length = urb->num_sgs * q->buf_size,
> > > + urb->transfer_buffer_length = urb->num_sgs * data_size;
> > > sg_init_marker(urb->sg, urb->num_sgs);
> > >
> > > return i ? : -ENOMEM;
> > > @@ -611,8 +611,12 @@ static int mt76u_alloc_rx(struct mt76_dev *dev)
> > > if (!q->entry)
> > > return -ENOMEM;
> > >
> > > - q->buf_size = dev->usb.sg_en ? MT_RX_BUF_SIZE : PAGE_SIZE;
> > > + if (dev->usb.sg_en)
> > > + q->buf_size = MT_BUF_WITH_OVERHEAD(MT_RX_BUF_SIZE);
> >
> > I strongly recommend to not doing this. While this should work
> > in theory creating buffer with size of 2k + some bytes might
> > trigger various bugs in dma mapping or other low level code.
>
> even in practice actually :)
I wouldn't be sure about this. It's not common to have buffers of
such size and crossing pages boundaries. It really can trigger
nasty bugs on various IOMMU drivers.
> but we can be more cautious since probably copying
> the first 128B will not make any difference
Not sure if I understand what you mean.
> > And skb_shered_info is needed only in first buffer IIUC.
> >
> > Also this patch seems to make first patch unnecessary except for
> > non sg_en case (in which I think rx AMSDU is broken anyway),
> > so I would prefer just to apply first patch.
>
> I do not think rx AMSDU is broken for non sg_en case since the max rx value
> allowed should be 3839 IIRC and we alloc one page in this case
If that's the case we should be fine, but then I do not understand
why we allocate 8*2k buffers for sg_en case, isn't that AP can
sent AMSDU frame 16k big?
Stanislaw
^ permalink raw reply
* Re: [PATCH v2 1/2] mt76: usb: fix rx A-MSDU support
From: Lorenzo Bianconi @ 2019-06-12 10:21 UTC (permalink / raw)
To: Stanislaw Gruszka; +Cc: Lorenzo Bianconi, nbd, kvalo, linux-wireless
In-Reply-To: <20190612100014.GA4431@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 1562 bytes --]
On Jun 12, Stanislaw Gruszka wrote:
> On Wed, Jun 12, 2019 at 11:45:21AM +0200, Lorenzo Bianconi wrote:
> > > > +mt76u_build_rx_skb(u8 *data, int len, int buf_size,
> > > > + int *nsgs)
> > > > +{
> > > > + int data_len = min(len, MT_SKB_HEAD_LEN);
>
> Oh, and this looks unneeded as well as for len < MT_SKB_HEAD_LEN=128
> we will go through fast path.
I guess if we remove data_len = min(len, MT_SKB_HEAD_LEN) and even *nsgs = 0 at
the end we are making some assumptions on the value of MT_SKB_HEAD_LEN and
buf_size. In the patch I just avoided them but maybe we can just assume that
MT_SKB_HEAD_LEN and buf_size will not changed in the future. What do you
think?
>
> > > mt7601u and iwlmvm just copy hdrlen + 8 and put the rest
> > > of the buffer in fragment, which supose to be more efficient,
> > > see comment in iwl_mvm_pass_packet_to_mac80211().
> >
> > Right here we copy 128B instead of 32 but I think it is good to have L3 and L4
> > header in the linear area of the skb since otherwise the stack will need to
> > align them
>
> Not sure if understand, I think aliment of L3 & L4 headers will be
> the same, assuming ieee80211 header is aligned the same in fragment
> buffer and in linear area. But if you think this is better to copy those
> to linear area I'm ok with that.
Sorry I have not been so clear. I mean in the stack before accessing a given
header we will run pskb_may_pull() that can end up copying the skb if there is
not enough space in the skb->head
Regards,
Lorenzo
>
> Stanislaw
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v3 3/5] brcmfmac: sdio: Disable auto-tuning around commands expected to fail
From: Ulf Hansson @ 2019-06-12 10:10 UTC (permalink / raw)
To: Doug Anderson
Cc: Hunter, Adrian, Kalle Valo, Arend van Spriel,
brcm80211-dev-list.pdl@broadcom.com,
linux-rockchip@lists.infradead.org, Double Lo,
briannorris@chromium.org, linux-wireless@vger.kernel.org,
Naveen Gupta, Madhan Mohan R, mka@chromium.org, Wright Feng,
Chi-Hsien Lin, netdev@vger.kernel.org,
brcm80211-dev-list@cypress.com, Franky Lin,
linux-kernel@vger.kernel.org, Hante Meuleman, YueHaibing,
David S. Miller
In-Reply-To: <CAD=FV=U8eo78Ee9xjhGXJMv=8YF9o89KLX024GH3iBRnRjCRvQ@mail.gmail.com>
On Mon, 10 Jun 2019 at 18:50, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Jun 10, 2019 at 1:56 AM Hunter, Adrian <adrian.hunter@intel.com> wrote:
> >
> > > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> > > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> > > @@ -16,6 +16,7 @@
> > > #include <linux/mmc/sdio_ids.h>
> > > #include <linux/mmc/sdio_func.h>
> > > #include <linux/mmc/card.h>
> > > +#include <linux/mmc/core.h>
> >
> > SDIO function drivers should not really include linux/mmc/core.h
> > (Also don't know why linux/mmc/card.h is included)
>
> OK, so I guess you're requesting an extra level of "sdio_" wrappers
> for all the functions I need to call. I don't think the wrappers buy
> us a ton other than to abstract things a little bit and make it look
> prettier. :-) ...but certainly I can code that up if that's what
> everyone wants.
Are the new code you refer to going to be used for anything else but
SDIO? If not, please put them in the sdio specific headers instead.
BTW, apologize for not looking at this series any earlier, but I will
come to it soon.
>
> Just to make sure, I looked in "drivers/net/wireless/" and I do see
> quite a few instances of "mmc_" functions being used. That doesn't
> mean all these instances are correct but it does appear to be
> commonplace. Selected examples:
>
> drivers/net/wireless/ath/ath10k/sdio.c:
> ret = mmc_hw_reset(ar_sdio->func->card->host);
mmc_hw_reset() is already an exported function, used by the mmc block
layer. So I think this is okay.
>
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c:
> mmc_set_data_timeout(md, func->card);
> mmc_wait_for_req(func->card->host, mr);
These are not okay, none of these things calls should really be done
from an SDIO func driver.
It tells me that the func driver is a doing workaround for something
that should be managed in a common way.
>
> drivers/net/wireless/marvell/mwifiex/sdio.c:
> mmc_hw_reset(func->card->host);
Okay.
>
> drivers/net/wireless/rsi/rsi_91x_sdio.c:
> err = mmc_wait_for_cmd(host, &cmd, 3);
Not okay.
>
>
> ...anyway, I'll give it a few days and if nobody else chimes in then
> I'll assume you indeed want "sdio_" wrappers for things and I'll post
> a v4. If patch #1 happens to land in the meantime then I won't
> object. ;-)
Adrian has a very good point. We need to strive to avoid exporting
APIs to here and there and just trust that they will be used wisely.
If the above calls to mmc_wait_for_req|cmd() and
mmc_set_data_timeout() could have been avoided, we would probably have
a more proper solution by now.
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH] wireless: wil6210: no need to check return value of debugfs_create functions
From: Greg Kroah-Hartman @ 2019-06-12 10:09 UTC (permalink / raw)
To: Maya Erez, Kalle Valo; +Cc: David S. Miller, linux-wireless, wil6210
In-Reply-To: <20190612100717.GA19167@kroah.com>
On Wed, Jun 12, 2019 at 12:07:17PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Jun 11, 2019 at 09:10:24PM +0200, Greg Kroah-Hartman wrote:
> > When calling debugfs functions, there is no need to ever check the
> > return value. The function can work or not, but the code logic should
> > never do something different based on this.
> >
> > Cc: Maya Erez <merez@codeaurora.org>
> > Cc: Kalle Valo <kvalo@codeaurora.org>
> > Cc: "David S. Miller" <davem@davemloft.net>
> > Cc: linux-wireless@vger.kernel.org
> > Cc: wil6210@qti.qualcomm.com
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> > drivers/net/wireless/ath/wil6210/debugfs.c | 80 ++++++----------------
> > 1 file changed, 22 insertions(+), 58 deletions(-)
>
> Oops, 0-day finally woke up and shows I messed this patch up. Please
> drop it, I will submit a v2 soon.
{sigh}, no, that was a different patch that broke things. This pathc is
fine, please consider it as-is.
I'll go get more coffee...
greg k-h
^ permalink raw reply
* Re: [PATCH] wireless: wil6210: no need to check return value of debugfs_create functions
From: Greg Kroah-Hartman @ 2019-06-12 10:07 UTC (permalink / raw)
To: Maya Erez, Kalle Valo; +Cc: David S. Miller, linux-wireless, wil6210
In-Reply-To: <20190611191024.GA17227@kroah.com>
On Tue, Jun 11, 2019 at 09:10:24PM +0200, Greg Kroah-Hartman wrote:
> When calling debugfs functions, there is no need to ever check the
> return value. The function can work or not, but the code logic should
> never do something different based on this.
>
> Cc: Maya Erez <merez@codeaurora.org>
> Cc: Kalle Valo <kvalo@codeaurora.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: linux-wireless@vger.kernel.org
> Cc: wil6210@qti.qualcomm.com
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> drivers/net/wireless/ath/wil6210/debugfs.c | 80 ++++++----------------
> 1 file changed, 22 insertions(+), 58 deletions(-)
Oops, 0-day finally woke up and shows I messed this patch up. Please
drop it, I will submit a v2 soon.
thanks,
greg k-h
^ permalink raw reply
* [PATCH] mac80211: drop robust management frames from unknown TA
From: Johannes Berg @ 2019-06-12 10:04 UTC (permalink / raw)
To: linux-wireless; +Cc: Johannes Berg
From: Johannes Berg <johannes.berg@intel.com>
When receiving a robust management frame, drop it if we don't have
rx->sta since then we don't have a security association and thus
couldn't possibly validate the frame.
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
net/mac80211/rx.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 25577ede2986..fd3740000e87 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -3831,6 +3831,8 @@ static bool ieee80211_accept_frame(struct ieee80211_rx_data *rx)
case NL80211_IFTYPE_STATION:
if (!bssid && !sdata->u.mgd.use_4addr)
return false;
+ if (ieee80211_is_robust_mgmt_frame(skb) && !rx->sta)
+ return false;
if (multicast)
return true;
return ether_addr_equal(sdata->vif.addr, hdr->addr1);
--
2.17.2
^ permalink raw reply related
* Re: [PATCH v2 1/2] mt76: usb: fix rx A-MSDU support
From: Stanislaw Gruszka @ 2019-06-12 10:00 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: Lorenzo Bianconi, nbd, kvalo, linux-wireless
In-Reply-To: <20190612094519.GC8107@localhost.localdomain>
On Wed, Jun 12, 2019 at 11:45:21AM +0200, Lorenzo Bianconi wrote:
> > > +mt76u_build_rx_skb(u8 *data, int len, int buf_size,
> > > + int *nsgs)
> > > +{
> > > + int data_len = min(len, MT_SKB_HEAD_LEN);
Oh, and this looks unneeded as well as for len < MT_SKB_HEAD_LEN=128
we will go through fast path.
> > mt7601u and iwlmvm just copy hdrlen + 8 and put the rest
> > of the buffer in fragment, which supose to be more efficient,
> > see comment in iwl_mvm_pass_packet_to_mac80211().
>
> Right here we copy 128B instead of 32 but I think it is good to have L3 and L4
> header in the linear area of the skb since otherwise the stack will need to
> align them
Not sure if understand, I think aliment of L3 & L4 headers will be
the same, assuming ieee80211 header is aligned the same in fragment
buffer and in linear area. But if you think this is better to copy those
to linear area I'm ok with that.
Stanislaw
^ permalink raw reply
* Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
From: Lorenzo Bianconi @ 2019-06-12 9:53 UTC (permalink / raw)
To: Stanislaw Gruszka; +Cc: Lorenzo Bianconi, nbd, kvalo, linux-wireless
In-Reply-To: <20190612091036.GB2965@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 1400 bytes --]
> On Fri, May 31, 2019 at 11:38:23AM +0200, Lorenzo Bianconi wrote:
[...]
> > }
> >
> > urb->num_sgs = max_t(int, i, urb->num_sgs);
> > - urb->transfer_buffer_length = urb->num_sgs * q->buf_size,
> > + urb->transfer_buffer_length = urb->num_sgs * data_size;
> > sg_init_marker(urb->sg, urb->num_sgs);
> >
> > return i ? : -ENOMEM;
> > @@ -611,8 +611,12 @@ static int mt76u_alloc_rx(struct mt76_dev *dev)
> > if (!q->entry)
> > return -ENOMEM;
> >
> > - q->buf_size = dev->usb.sg_en ? MT_RX_BUF_SIZE : PAGE_SIZE;
> > + if (dev->usb.sg_en)
> > + q->buf_size = MT_BUF_WITH_OVERHEAD(MT_RX_BUF_SIZE);
>
> I strongly recommend to not doing this. While this should work
> in theory creating buffer with size of 2k + some bytes might
> trigger various bugs in dma mapping or other low level code.
even in practice actually :) but we can be more cautious since probably copying
the first 128B will not make any difference
>
> And skb_shered_info is needed only in first buffer IIUC.
>
> Also this patch seems to make first patch unnecessary except for
> non sg_en case (in which I think rx AMSDU is broken anyway),
> so I would prefer just to apply first patch.
I do not think rx AMSDU is broken for non sg_en case since the max rx value
allowed should be 3839 IIRC and we alloc one page in this case
Regards,
Lorenzo
>
> Stanislaw
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH] wlcore/wl18xx: Add invert-irq OF property for physically inverted IRQ
From: Eugeniu Rosca @ 2019-06-12 9:45 UTC (permalink / raw)
To: Marc Zyngier, Geert Uytterhoeven, Tony Lindgren, Kalle Valo,
Eyal Reizer
Cc: Simon Horman, David S. Miller, Greg Kroah-Hartman, Randy Dunlap,
Ulf Hansson, John Stultz, linux-wireless, netdev,
Linux Kernel Mailing List, Spyridon Papageorgiou, Joshua Frkuska,
George G . Davis, Andrey Gusakov, Linux-Renesas, Eugeniu Rosca,
Eugeniu Rosca, Thomas Gleixner, Jason Cooper, Linus Walleij
In-Reply-To: <08bc4755-5f47-d792-8b5a-927b5fbe7619@arm.com>
Hi,
cc: Linus Walleij
On Tue, Jun 11, 2019 at 10:00:41AM +0100, Marc Zyngier wrote:
> On 11/06/2019 09:45, Geert Uytterhoeven wrote:
> > CC irqchip
> >
> > Original thread at
> > https://lore.kernel.org/lkml/20190607172958.20745-1-erosca@de.adit-jv.com/
> >
> > On Mon, Jun 10, 2019 at 10:30 AM Tony Lindgren <tony@atomide.com> wrote:
> >> * Kalle Valo <kvalo@codeaurora.org> [190610 07:01]:
> >>> Eugeniu Rosca <erosca@de.adit-jv.com> writes:
> >>>
> >>>> The wl1837mod datasheet [1] says about the WL_IRQ pin:
> >>>>
> >>>> ---8<---
> >>>> SDIO available, interrupt out. Active high. [..]
> >>>> Set to rising edge (active high) on powerup.
> >>>> ---8<---
> >>>>
> >>>> That's the reason of seeing the interrupt configured as:
> >>>> - IRQ_TYPE_EDGE_RISING on HiKey 960/970
> >>>> - IRQ_TYPE_LEVEL_HIGH on a number of i.MX6 platforms
> >>>>
> >>>> We assert that all those platforms have the WL_IRQ pin connected
> >>>> to the SoC _directly_ (confirmed on HiKey 970 [2]).
> >>>>
> >>>> That's not the case for R-Car Kingfisher extension target, which carries
> >>>> a WL1837MODGIMOCT IC. There is an SN74LV1T04DBVR inverter present
> >>>> between the WLAN_IRQ pin of the WL18* chip and the SoC, effectively
> >>>> reversing the requirement quoted from [1]. IOW, in Kingfisher DTS
> >>>> configuration we would need to use IRQ_TYPE_EDGE_FALLING or
> >>>> IRQ_TYPE_LEVEL_LOW.
> >>>>
> >>>> Unfortunately, v4.2-rc1 commit bd763482c82ea2 ("wl18xx: wlan_irq:
> >>>> support platform dependent interrupt types") made a special case out
> >>>> of these interrupt types. After this commit, it is impossible to provide
> >>>> an IRQ configuration via DTS which would describe an inverter present
> >>>> between the WL18* chip and the SoC, generating the need for workarounds
> >>>> like [3].
> >>>>
> >>>> Create a boolean OF property, called "invert-irq" to specify that
> >>>> the WLAN_IRQ pin of WL18* is connected to the SoC via an inverter.
> >>>>
> >>>> This solution has been successfully tested on R-Car H3ULCB-KF-M06 using
> >>>> the DTS configuration [4] combined with the "invert-irq" property.
> >>>>
> >>>> [1] http://www.ti.com/lit/ds/symlink/wl1837mod.pdf
> >>>> [2] https://www.96boards.org/documentation/consumer/hikey/hikey970/hardware-docs/
> >>>> [3] https://github.com/CogentEmbedded/meta-rcar/blob/289fbd4f8354/meta-rcar-gen3-adas/recipes-kernel/linux/linux-renesas/0024-wl18xx-do-not-invert-IRQ-on-WLxxxx-side.patch
> >>>> [4] https://patchwork.kernel.org/patch/10895879/
> >>>> ("arm64: dts: ulcb-kf: Add support for TI WL1837")
> >>>>
> >>>> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> >>>
> >>> Tony&Eyal, do you agree with this?
> >>
> >> Yeah if there's some hardware between the WLAN device and the SoC
> >> inverting the interrupt, I don't think we have clear a way to deal
> >> with it short of setting up a separate irqchip that does the
> >> translation.
> >
> > Yeah, inverting the interrupt type in DT works only for simple devices,
> > that don't need configuration.
> > A simple irqchip driver that just inverts the type sounds like a good
> > solution to me. Does something like that already exists?
>
> We already have plenty of that in the tree, the canonical example
> probably being drivers/irqchip/irq-mtk-sysirq.c. It should be pretty
> easy to turn this driver into something more generic.
I don't think drivers/irqchip/irq-mtk-sysirq.c can serve the
use-case/purpose of this patch. The MTK driver seems to be dealing with
the polarity inversion of on-SoC interrupts which are routed to GiC,
whereas in this patch we are talking about an off-chip interrupt
wired to R-Car GPIO controller.
It looks to me that the nice DTS sketch shared by Linus Walleij in [5]
might come closer to the concept proposed by Geert? FWIW, the
infrastructure/implementation to make this possible is still not ready.
One question to the wlcore/wl18xx maintainers: Why exactly do you give
freedom to users to set the interrupt as LEVEL_LOW/EDGE_FALLING [6]?
Apparently, this:
- complicates the wl18xx driver, thus increasing the chance for bugs
- is not supposed to reflect any HW differences between boards using
LEVEL_LOW/EDGE_FALLING and the boards using LEVEL_HIGH/EDGE_RISING
- doesn't bring any obvious advantage to the users, who are expected to
sense the same behavior regardless of the IRQ type set in DTS
- prevent the users to set IRQ type to LEVEL_LOW/EDGE_FALLING when
there is an inverter present between WL_IRQ and SoC
- seems to be not used almost at all, as 99% of mainline DTS set the
IRQ type to the canonical/NLCP LEVEL_HIGH/EDGE_RISING
[5] https://patchwork.ozlabs.org/patch/1095690/#2167076
("[V1,1/2] gpio: make it possible to set active-state on GPIO lines")
--------------------8<-------------------
gpio0: gpio {
compatible = "foo,chip";
gpio-controller;
(...)
};
inv0: inverter {
compatible = "inverter";
gpio-controller;
gpios = <&gpio0 0 GPIO_ACTIVE_HIGH>;
};
consumer {
compatible = "bar";
gpios = <&inv0 0 GPIO_ACTIVE_HIGH>;
};
--------------------8<-------------------
[6] bd763482c82ea2 ("wl18xx: wlan_irq: support platform dependent interrupt types")
--
Best Regards,
Eugeniu.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox