Re: [RFC v2 1/4] mac80211: Add TXQ scheduling API
On 2018-07-21 13:54, Toke Høiland-Jørgensen wrote: Rajkumar Manoharan writes: On 2018-07-19 07:18, Toke Høiland-Jørgensen wrote: Rajkumar Manoharan writes: On 2018-07-13 06:39, Toke Høiland-Jørgensen wrote: Rajkumar Manoharan writes: It is not generally predictable how many times this will loop before exiting... Agree.. It would be better If the driver does not worry about txq sequence numbering. Perhaps one more API (ieee80211_first_txq) could solve this. Will leave it to you. That is basically what the second parameter to next_txq() does in the current patchset. It just needs to be renamed... Agree. As next_txq() increments seqno while starting loop for each AC, It seems bit confusing. i.e A single ath_txq_schedule_all call will bump schedule_seqno by 4. right? Hmm, good point. Guess there should be one seqno per ac... I would prefer to keep separate list for each AC ;) Prepared one on top of your earlier merged changes. Now it needs revisit. Let assume below case where CPU1 is dequeuing skb from isr context and CPU2 is enqueuing skbs into same txq. CPU1 CPU2 ath_txq_schedule -> ieee80211_next_txq(first) -> inc_seq -> delete from list -> txq->seqno = local->seqno ieee80211_tx/fast_xmit -> ieee80211_queue_skb -> ieee80211_schedule_txq(reset_seqno) -> list_add -> txqi->seqno = local->seqno - 1 In above sequence, it is quite possible that the same txq will be served repeatedly and other txqs will be starved. am I right? IMHO seqno mechanism will not guarantee that txqs will be processed only once in an iteration. Yeah, ieee80211_queue_skb() shouldn't set reset_seqno; was experimenting with that, and guess I ended up picking the wrong value. Thanks for pointing that out :) A simple change in argument may break algo. What would be seqno of first packet of txq if queue_skb() isn't reset seqno? I am fine in passing start of loop as arg to next_ntx(). But prefer to keep loop detecting within mac80211 itself by tracking txq same as ath10k. So that no change is needed in schedule_txq() arg list. thoughts? Well, it could conceivably be done in a way that doesn't require taking the activeq_lock. Adding another STOP flag to the TXQ, for instance. From an API point of view I think that is more consistent with what we have already... Make sense. ieee80211_txq_may_pull would be better place to decide whether given txq is allowed for transmission. It also makes drivers do not have to worry about deficit. Still I may need ieee80211_reorder_txq API after processing txq. isn't it? The way I was assuming this would work (and what ath9k does), is that a driver only operates on one TXQ at a time; so it can get that txq, process it, and re-schedule it. In which case no other API is needed; the rotating can be done in next_txq(), and schedule_txq() can insert the txq back into the rotation as needed. However, it sounds like this is not how ath10k does things? See below. correct. The current rotation works only in push-only mode. i.e when firmware is not deciding txqs and the driver picks priority txq from active_txqs list. Unfortunately rotation won't work when the driver selects txq other than first in the list. In any case separate API needed for rotation when the driver is processing few packets from txq instead of all pending frames. Rotation is not dependent on the TXQ draining completely. Dequeueing a few packets, then rescheduling the TXQ is fine. The problem is that when the driver accesses txq directly, it wont call next_txq(). So the txq will not dequeued from list and schedule_txq() wont be effective. right? ieee80211_txq_may_pull() - check whether txq is allowed for tx_dequeue() that helps to keep deficit check in mac80211 itself ieee80211_reorder_txq() - after dequeuing skb (in direct txq access), txq will be deleted from list and if there are pending frames, it will be requeued. And if so, how does that interact with ath10k_mac_tx_push_pending()? That function is basically doing the same thing that I explained above for ath9k; in the previous version of this patch series I modified that to use next_txq(). But is that a different TX path, or am I misunderstanding you? If you could point me to which parts of the ath10k code I should be looking at, that would be helpful as well :) Depends on firmware htt_tx_mode (push/push_pull), ath10k_mac_tx_push_pending() downloads all/partial frames to firmware. Please take a look at ath10k_mac_tx_can_push() in push_pending(). Let me know If you n
[PATCH 3/9] ath9k: force rx_clear when disabling rx
From: Felix Fietkau
This makes stopping Rx more reliable and should reduce the frequency of
Rx related DMA stop warnings. Don't use rx_clear in TX99 mode.
Signed-off-by: Felix Fietkau
Signed-off-by: Helmut Schaa
---
drivers/net/wireless/ath/ath9k/mac.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/mac.c
b/drivers/net/wireless/ath/ath9k/mac.c
index 58d02c19b6d0..c9d2bf3fa135 100644
--- a/drivers/net/wireless/ath/ath9k/mac.c
+++ b/drivers/net/wireless/ath/ath9k/mac.c
@@ -678,13 +678,18 @@ void ath9k_hw_startpcureceive(struct ath_hw *ah, bool
is_scanning)
ath9k_ani_reset(ah, is_scanning);
- REG_CLR_BIT(ah, AR_DIAG_SW, (AR_DIAG_RX_DIS | AR_DIAG_RX_ABORT));
+ REG_CLR_BIT(ah, AR_DIAG_SW,
+ AR_DIAG_RX_DIS | AR_DIAG_RX_ABORT | AR_DIAG_FORCE_RX_CLEAR);
}
EXPORT_SYMBOL(ath9k_hw_startpcureceive);
void ath9k_hw_abortpcurecv(struct ath_hw *ah)
{
- REG_SET_BIT(ah, AR_DIAG_SW, AR_DIAG_RX_ABORT | AR_DIAG_RX_DIS);
+ u32 reg = AR_DIAG_RX_DIS | AR_DIAG_RX_ABORT;
+
+ if (!IS_ENABLED(CPTCFG_ATH9K_TX99))
+ reg |= AR_DIAG_FORCE_RX_CLEAR;
+ REG_SET_BIT(ah, AR_DIAG_SW, reg);
ath9k_hw_disable_mib_counters(ah);
}
--
2.17.0
[PATCH 9/9] ath9k: fix more-data flag for buffered multicast packets
The flag needs to be cleared for the last packet in the list, not the
first one. Fixes some issues with multicast packet loss for powersave
clients connected to an ath9k AP.
Signed-off-by: Felix Fietkau
---
drivers/net/wireless/ath/ath9k/xmit.c | 12 +++-
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c
b/drivers/net/wireless/ath/ath9k/xmit.c
index bae0f6c045e1..43b6c8508e49 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -2436,7 +2436,6 @@ void ath_tx_cabq(struct ieee80211_hw *hw, struct
ieee80211_vif *vif,
.txq = sc->beacon.cabq
};
struct ath_tx_info info = {};
- struct ieee80211_hdr *hdr;
struct ath_buf *bf_tail = NULL;
struct ath_buf *bf;
LIST_HEAD(bf_q);
@@ -2480,15 +2479,10 @@ void ath_tx_cabq(struct ieee80211_hw *hw, struct
ieee80211_vif *vif,
if (list_empty(&bf_q))
return;
- bf = list_first_entry(&bf_q, struct ath_buf, list);
- hdr = (struct ieee80211_hdr *) bf->bf_mpdu->data;
-
- if (hdr->frame_control & cpu_to_le16(IEEE80211_FCTL_MOREDATA)) {
- hdr->frame_control &= ~cpu_to_le16(IEEE80211_FCTL_MOREDATA);
- dma_sync_single_for_device(sc->dev, bf->bf_buf_addr,
- sizeof(*hdr), DMA_TO_DEVICE);
- }
+ bf = list_last_entry(&bf_q, struct ath_buf, list);
+ ath9k_set_moredata(sc, bf, false);
+ bf = list_first_entry(&bf_q, struct ath_buf, list);
ath_txq_lock(sc, txctl.txq);
ath_tx_fill_desc(sc, bf, txctl.txq, 0);
ath_tx_txqaddbuf(sc, txctl.txq, &bf_q, false);
--
2.17.0
[PATCH 6/9] ath9k: report tx status on EOSP
Fixes missed indications of end of U-APSD service period to mac80211
Signed-off-by: Felix Fietkau
---
drivers/net/wireless/ath/ath9k/xmit.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c
b/drivers/net/wireless/ath/ath9k/xmit.c
index 56a0d1b7527a..d366170f01cf 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -86,7 +86,8 @@ static void ath_tx_status(struct ieee80211_hw *hw, struct
sk_buff *skb)
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
struct ieee80211_sta *sta = info->status.status_driver_data[0];
- if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS) {
+ if (info->flags & (IEEE80211_TX_CTL_REQ_TX_STATUS |
+ IEEE80211_TX_STATUS_EOSP)) {
ieee80211_tx_status(hw, skb);
return;
}
--
2.17.0
[PATCH 2/9] ath9k: don't run periodic and nf calibation at the same time
The checks already prevents periodic cal from being started while noise
floor calibration runs. It is missing checks for the other way around.
Signed-off-by: Felix Fietkau
---
drivers/net/wireless/ath/ath9k/ar9002_calib.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/ar9002_calib.c
b/drivers/net/wireless/ath/ath9k/ar9002_calib.c
index 50fcd343c41a..fd9db8ca99d7 100644
--- a/drivers/net/wireless/ath/ath9k/ar9002_calib.c
+++ b/drivers/net/wireless/ath/ath9k/ar9002_calib.c
@@ -676,10 +676,10 @@ static int ar9002_hw_calibrate(struct ath_hw *ah, struct
ath9k_channel *chan,
return 0;
ah->cal_list_curr = currCal = currCal->calNext;
- if (currCal->calState == CAL_WAITING) {
+ if (currCal->calState == CAL_WAITING)
ath9k_hw_reset_calibration(ah, currCal);
- return 0;
- }
+
+ return 0;
}
/* Do NF cal only at longer intervals */
--
2.17.0
[PATCH 5/9] ath9k: clear potentially stale EOSP status bit in intermediate queues
Prevents spurious ieee80211_sta_eosp calls. Signed-off-by: Felix Fietkau --- drivers/net/wireless/ath/ath9k/xmit.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c index cab24b43ac88..56a0d1b7527a 100644 --- a/drivers/net/wireless/ath/ath9k/xmit.c +++ b/drivers/net/wireless/ath/ath9k/xmit.c @@ -969,7 +969,8 @@ ath_tx_get_tid_subframe(struct ath_softc *sc, struct ath_txq *txq, bf->bf_lastbf = bf; tx_info = IEEE80211_SKB_CB(skb); - tx_info->flags &= ~IEEE80211_TX_CTL_CLEAR_PS_FILT; + tx_info->flags &= ~(IEEE80211_TX_CTL_CLEAR_PS_FILT | + IEEE80211_TX_STATUS_EOSP); /* * No aggregation session is running, but there may be frames -- 2.17.0
[PATCH 4/9] ath9k: fix moredata bit in PS buffered frame release
Signed-off-by: Felix Fietkau
---
drivers/net/wireless/ath/ath9k/xmit.c | 20
1 file changed, 20 insertions(+)
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c
b/drivers/net/wireless/ath/ath9k/xmit.c
index 7fdb152be0bb..cab24b43ac88 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -1659,6 +1659,22 @@ void ath_tx_aggr_wakeup(struct ath_softc *sc, struct
ath_node *an)
}
}
+
+static void
+ath9k_set_moredata(struct ath_softc *sc, struct ath_buf *bf, bool val)
+{
+ struct ieee80211_hdr *hdr;
+ u16 mask = cpu_to_le16(IEEE80211_FCTL_MOREDATA);
+ u16 mask_val = mask * val;
+
+ hdr = (struct ieee80211_hdr *) bf->bf_mpdu->data;
+ if ((hdr->frame_control & mask) != mask_val) {
+ hdr->frame_control = (hdr->frame_control & ~mask) | mask_val;
+ dma_sync_single_for_device(sc->dev, bf->bf_buf_addr,
+ sizeof(*hdr), DMA_TO_DEVICE);
+ }
+}
+
void ath9k_release_buffered_frames(struct ieee80211_hw *hw,
struct ieee80211_sta *sta,
u16 tids, int nframes,
@@ -1689,6 +1705,7 @@ void ath9k_release_buffered_frames(struct ieee80211_hw
*hw,
if (!bf)
break;
+ ath9k_set_moredata(sc, bf, true);
list_add_tail(&bf->list, &bf_q);
ath_set_rates(tid->an->vif, tid->an->sta, bf);
if (bf_isampdu(bf)) {
@@ -1712,6 +1729,9 @@ void ath9k_release_buffered_frames(struct ieee80211_hw
*hw,
if (list_empty(&bf_q))
return;
+ if (!more_data)
+ ath9k_set_moredata(sc, bf_tail, false);
+
info = IEEE80211_SKB_CB(bf_tail->bf_mpdu);
info->flags |= IEEE80211_TX_STATUS_EOSP;
--
2.17.0
[PATCH 7/9] ath9k: fix block-ack window tracking issues
Ensure that a buffer gets tracked as part of the block-ack window as
soon as it's dequeued from the tid for the first time. Ensure that
double calls to ath_tx_addto_baw (e.g. on retransmission) don't cause
any issues.
Signed-off-by: Felix Fietkau
---
drivers/net/wireless/ath/ath9k/xmit.c | 29 +--
1 file changed, 18 insertions(+), 11 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c
b/drivers/net/wireless/ath/ath9k/xmit.c
index d366170f01cf..bae0f6c045e1 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -62,7 +62,7 @@ static void ath_tx_rc_status(struct ath_softc *sc, struct
ath_buf *bf,
struct ath_tx_status *ts, int nframes, int nbad,
int txok);
static void ath_tx_update_baw(struct ath_softc *sc, struct ath_atx_tid *tid,
- int seqno);
+ struct ath_buf *bf);
static struct ath_buf *ath_tx_setup_buffer(struct ath_softc *sc,
struct ath_txq *txq,
struct ath_atx_tid *tid,
@@ -296,7 +296,7 @@ static void ath_tx_flush_tid(struct ath_softc *sc, struct
ath_atx_tid *tid)
}
if (fi->baw_tracked) {
- ath_tx_update_baw(sc, tid, bf->bf_state.seqno);
+ ath_tx_update_baw(sc, tid, bf);
sendbar = true;
}
@@ -312,10 +312,15 @@ static void ath_tx_flush_tid(struct ath_softc *sc, struct
ath_atx_tid *tid)
}
static void ath_tx_update_baw(struct ath_softc *sc, struct ath_atx_tid *tid,
- int seqno)
+ struct ath_buf *bf)
{
+ struct ath_frame_info *fi = get_frame_info(bf->bf_mpdu);
+ u16 seqno = bf->bf_state.seqno;
int index, cindex;
+ if (!fi->baw_tracked)
+ return;
+
index = ATH_BA_INDEX(tid->seq_start, seqno);
cindex = (tid->baw_head + index) & (ATH_TID_MAX_BUFS - 1);
@@ -336,6 +341,9 @@ static void ath_tx_addto_baw(struct ath_softc *sc, struct
ath_atx_tid *tid,
u16 seqno = bf->bf_state.seqno;
int index, cindex;
+ if (fi->baw_tracked)
+ return;
+
index = ATH_BA_INDEX(tid->seq_start, seqno);
cindex = (tid->baw_head + index) & (ATH_TID_MAX_BUFS - 1);
__set_bit(cindex, tid->tx_buf);
@@ -612,7 +620,7 @@ static void ath_tx_complete_aggr(struct ath_softc *sc,
struct ath_txq *txq,
* complete the acked-ones/xretried ones; update
* block-ack window
*/
- ath_tx_update_baw(sc, tid, seqno);
+ ath_tx_update_baw(sc, tid, bf);
if (rc_update && (acked_cnt == 1 || txfail_cnt == 1)) {
memcpy(tx_info->control.rates, rates,
sizeof(rates));
@@ -642,7 +650,7 @@ static void ath_tx_complete_aggr(struct ath_softc *sc,
struct ath_txq *txq,
* run out of tx buf.
*/
if (!tbf) {
- ath_tx_update_baw(sc, tid, seqno);
+ ath_tx_update_baw(sc, tid, bf);
ath_tx_complete_buf(sc, bf, txq,
&bf_head, NULL, ts,
@@ -1011,11 +1019,14 @@ ath_tx_get_tid_subframe(struct ath_softc *sc, struct
ath_txq *txq,
INIT_LIST_HEAD(&bf_head);
list_add(&bf->list, &bf_head);
- ath_tx_update_baw(sc, tid, seqno);
+ ath_tx_update_baw(sc, tid, bf);
ath_tx_complete_buf(sc, bf, txq, &bf_head, NULL, &ts,
0);
continue;
}
+ if (bf_isampdu(bf))
+ ath_tx_addto_baw(sc, tid, bf);
+
return bf;
}
@@ -1073,8 +1084,6 @@ ath_tx_form_aggr(struct ath_softc *sc, struct ath_txq
*txq,
bf->bf_next = NULL;
/* link buffers of this frame to the aggregate */
- if (!fi->baw_tracked)
- ath_tx_addto_baw(sc, tid, bf);
bf->bf_state.ndelim = ndelim;
list_add_tail(&bf->list, bf_q);
@@ -1710,10 +1719,8 @@ void ath9k_release_buffered_frames(struct ieee80211_hw
*hw,
ath9k_set_moredata(sc, bf, true);
list_add_tail(&bf->list, &bf_q);
ath_set_rates(tid->an->vif, tid->an->sta, bf);
- if (bf_isampdu(bf)) {
- ath_tx_addto_baw(sc, tid, bf);
+ if (bf_isampdu(bf))
bf->bf_state.bf_type &= ~BUF_AGGR;
-
[PATCH 8/9] ath9k_hw: fix channel maximum power level test
The tx power applied by set_txpower is limited by the CTL (conformance test limit) entries in the EEPROM. These can change based on the user configured regulatory domain. Depending on the EEPROM data this can cause the tx power to become too limited, if the original regdomain CTLs impose lower limits than the CTLs of the user configured regdomain. To fix this issue, set the initial channel limits without any CTL restrictions and only apply the CTL at run time when setting the channel and the real tx power. Signed-off-by: Felix Fietkau --- drivers/net/wireless/ath/ath9k/hw.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c index 1665066f4e24..9dc866404eca 100644 --- a/drivers/net/wireless/ath/ath9k/hw.c +++ b/drivers/net/wireless/ath/ath9k/hw.c @@ -2942,16 +2942,19 @@ void ath9k_hw_apply_txpower(struct ath_hw *ah, struct ath9k_channel *chan, struct ath_regulatory *reg = ath9k_hw_regulatory(ah); struct ieee80211_channel *channel; int chan_pwr, new_pwr; + u16 ctl = NO_CTL; if (!chan) return; + if (!test) + ctl = ath9k_regd_get_ctl(reg, chan); + channel = chan->chan; chan_pwr = min_t(int, channel->max_power * 2, MAX_RATE_POWER); new_pwr = min_t(int, chan_pwr, reg->power_limit); - ah->eep_ops->set_txpower(ah, chan, -ath9k_regd_get_ctl(reg, chan), + ah->eep_ops->set_txpower(ah, chan, ctl, get_antenna_gain(ah, chan), new_pwr, test); } -- 2.17.0
[PATCH 1/9] ath9k_hw: set spectral scan enable bit on trigger for AR9003+
AR9002 code and the QCA AR9003+ reference code do the same.
Signed-off-by: Felix Fietkau
---
drivers/net/wireless/ath/ath9k/ar9003_phy.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/wireless/ath/ath9k/ar9003_phy.c
b/drivers/net/wireless/ath/ath9k/ar9003_phy.c
index fe5102ca5010..98c5f524a360 100644
--- a/drivers/net/wireless/ath/ath9k/ar9003_phy.c
+++ b/drivers/net/wireless/ath/ath9k/ar9003_phy.c
@@ -1800,6 +1800,8 @@ static void ar9003_hw_spectral_scan_config(struct ath_hw
*ah,
static void ar9003_hw_spectral_scan_trigger(struct ath_hw *ah)
{
+ REG_SET_BIT(ah, AR_PHY_SPECTRAL_SCAN,
+ AR_PHY_SPECTRAL_SCAN_ENABLE);
/* Activate spectral scan */
REG_SET_BIT(ah, AR_PHY_SPECTRAL_SCAN,
AR_PHY_SPECTRAL_SCAN_ACTIVE);
--
2.17.0
Re: [PATCH 11/20] wil6210: send L2UF on behalf of newly associated station
On 2018-07-22 23:01, Johannes Berg wrote: On Sun, 2018-07-22 at 10:47 +0300, Maya Erez wrote: From: Ahmad Masri Send L2UF (Level 2 Update Frame) to update forwarding tables in layer 2 in AP mode. Wil6210 driver creates and sends this frame once a new station connects to the AP. Might be worth unifying with ieee80211_send_layer2_update() into a new cfg80211 function? johannes We will do that. I'll revert this patch. -- Maya Erez Qualcomm Israel, Inc. on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH 04/10] NFC: st95hf: remove logging from spi functions
The callers of these functions already log errors, so there's no need to do
it from two places.
Signed-off-by: Daniel Mack
---
drivers/nfc/st95hf/spi.c | 19 ---
1 file changed, 4 insertions(+), 15 deletions(-)
diff --git a/drivers/nfc/st95hf/spi.c b/drivers/nfc/st95hf/spi.c
index e2d3bbcc8c34..d5894d4546b1 100644
--- a/drivers/nfc/st95hf/spi.c
+++ b/drivers/nfc/st95hf/spi.c
@@ -47,8 +47,6 @@ int st95hf_spi_send(struct st95hf_spi_context *spicontext,
result = spi_sync(spidev, &m);
if (result) {
- dev_err(&spidev->dev, "error: sending cmd to st95hf using SPI =
%d\n",
- result);
mutex_unlock(&spicontext->spi_lock);
return result;
}
@@ -62,12 +60,10 @@ int st95hf_spi_send(struct st95hf_spi_context *spicontext,
result = wait_for_completion_timeout(&spicontext->done,
msecs_to_jiffies(1000));
/* check for timeout or success */
- if (!result) {
- dev_err(&spidev->dev, "error: response not ready timeout\n");
+ if (!result)
result = -ETIMEDOUT;
- } else {
+ else
result = 0;
- }
mutex_unlock(&spicontext->spi_lock);
@@ -79,7 +75,7 @@ EXPORT_SYMBOL_GPL(st95hf_spi_send);
int st95hf_spi_recv_response(struct st95hf_spi_context *spicontext,
unsigned char *receivebuff)
{
- int len = 0;
+ int ret, len;
struct spi_transfer tx_takedata;
struct spi_message m;
struct spi_device *spidev = spicontext->spidev;
@@ -89,8 +85,6 @@ int st95hf_spi_recv_response(struct st95hf_spi_context
*spicontext,
{.rx_buf = receivebuff, .len = 2, .cs_change = 1,},
};
- int ret = 0;
-
memset(&tx_takedata, 0x0, sizeof(struct spi_transfer));
mutex_lock(&spicontext->spi_lock);
@@ -102,8 +96,6 @@ int st95hf_spi_recv_response(struct st95hf_spi_context
*spicontext,
ret = spi_sync(spidev, &m);
if (ret) {
- dev_err(&spidev->dev, "spi_recv_resp, data length error = %d\n",
- ret);
mutex_unlock(&spicontext->spi_lock);
return ret;
}
@@ -127,11 +119,8 @@ int st95hf_spi_recv_response(struct st95hf_spi_context
*spicontext,
ret = spi_sync(spidev, &m);
mutex_unlock(&spicontext->spi_lock);
- if (ret) {
- dev_err(&spidev->dev, "spi_recv_resp, data read error = %d\n",
- ret);
+ if (ret)
return ret;
- }
return len;
}
--
2.17.1
[PATCH 03/10] NFC: st95hf: drop illegal kfree_skb() in IRQ handler
In the error path of the IRQ handler, don't free the skb in flight. The callback in the digital core will do that for us. Doing it from both places causes memory corruptions. Signed-off-by: Daniel Mack --- drivers/nfc/st95hf/core.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/nfc/st95hf/core.c b/drivers/nfc/st95hf/core.c index d58424ab5c48..d857197ec7b2 100644 --- a/drivers/nfc/st95hf/core.c +++ b/drivers/nfc/st95hf/core.c @@ -863,7 +863,6 @@ static irqreturn_t st95hf_irq_thread_handler(int irq, void *st95hfcontext) return IRQ_HANDLED; end: - kfree_skb(skb_resp); wtx = false; cb_arg->rats = false; skb_resp = ERR_PTR(result); -- 2.17.1
[PATCH 02/10] NFC: st95hf: drop nfcdev_free
This flag is unneccesary. We can just nullify `ddev' instead after we freed
it.
Signed-off-by: Daniel Mack
---
drivers/nfc/st95hf/core.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/nfc/st95hf/core.c b/drivers/nfc/st95hf/core.c
index bc1a2070f9bb..d58424ab5c48 100644
--- a/drivers/nfc/st95hf/core.c
+++ b/drivers/nfc/st95hf/core.c
@@ -220,8 +220,6 @@ struct st95_digital_cmd_complete_arg {
* from threaded ISR. Usage of this mutex avoids any race between
* deletion of the object from st95hf_remove() and its access from
* the threaded ISR.
- * @nfcdev_free: flag to have the state of nfc device object.
- * [alive | died]
* @current_protocol: current nfc protocol.
* @current_rf_tech: current rf technology.
* @fwi: frame waiting index, received in reply of RATS according to
@@ -237,7 +235,6 @@ struct st95hf_context {
unsigned char sendrcv_trflag;
struct semaphore exchange_lock;
struct mutex rm_lock;
- bool nfcdev_free;
u8 current_protocol;
u8 current_rf_tech;
int fwi;
@@ -820,8 +817,8 @@ static irqreturn_t st95hf_irq_thread_handler(int irq, void
*st95hfcontext)
goto end;
}
- /* if stcontext->nfcdev_free is true, it means remove already ran */
- if (stcontext->nfcdev_free) {
+ /* if stcontext->ddev is %NULL, it means remove already ran */
+ if (!stcontext->ddev) {
result = -ENODEV;
goto end;
}
@@ -1220,7 +1217,7 @@ static int st95hf_remove(struct spi_device *nfc_spi_dev)
nfc_digital_unregister_device(stcontext->ddev);
nfc_digital_free_device(stcontext->ddev);
- stcontext->nfcdev_free = true;
+ stcontext->ddev = NULL;
mutex_unlock(&stcontext->rm_lock);
--
2.17.1
[PATCH 09/10] NFC: st95hf: two small style nits
Address two minor style issues that I came across while working on the driver. Signed-off-by: Daniel Mack --- drivers/nfc/st95hf/core.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/nfc/st95hf/core.c b/drivers/nfc/st95hf/core.c index 835a1e61c817..4db3c020921c 100644 --- a/drivers/nfc/st95hf/core.c +++ b/drivers/nfc/st95hf/core.c @@ -662,7 +662,8 @@ static int st95hf_error_handling(struct st95hf_context *stcontext, result = -ETIMEDOUT; else result = -EIO; - return result; + + return result; } /* Check for CRC err only if CRC is present in the tag response */ @@ -844,6 +845,7 @@ static irqreturn_t st95hf_irq_thread_handler(int irq, void *st95hfcontext) /* call of callback with error */ cb_arg->complete_cb(stcontext->ddev, cb_arg->cb_usrarg, skb_resp); mutex_unlock(&stcontext->rm_lock); + return IRQ_HANDLED; } -- 2.17.1
[PATCH 08/10] NFC: st95hf: unify sync/async flags
Keep the information whether a command is asynchronous in a boolean flag
everywhere in the code. This way, the enum can go away.
Signed-off-by: Daniel Mack
---
drivers/nfc/st95hf/core.c | 38 --
drivers/nfc/st95hf/spi.c | 12 +---
drivers/nfc/st95hf/spi.h | 8 +---
3 files changed, 22 insertions(+), 36 deletions(-)
diff --git a/drivers/nfc/st95hf/core.c b/drivers/nfc/st95hf/core.c
index e7ecc57dde8f..835a1e61c817 100644
--- a/drivers/nfc/st95hf/core.c
+++ b/drivers/nfc/st95hf/core.c
@@ -101,7 +101,7 @@ struct cmd {
unsigned char cmd_id;
unsigned char no_cmd_params;
unsigned char cmd_params[MAX_CMD_PARAMS];
- enum req_type req;
+ bool is_sync;
};
struct param_list {
@@ -134,63 +134,62 @@ static const struct cmd cmd_array[] = {
.cmd_len = 0x2,
.cmd_id = ECHO_CMD,
.no_cmd_params = 0,
- .req = SYNC,
+ .is_sync = true,
},
[CMD_ISO14443A_CONFIG] = {
.cmd_len = 0x7,
.cmd_id = WRITE_REGISTER_CMD,
.no_cmd_params = 0x4,
.cmd_params = {0x3A, 0x00, 0x5A, 0x04},
- .req = SYNC,
+ .is_sync = true,
},
[CMD_ISO14443A_DEMOGAIN] = {
.cmd_len = 0x7,
.cmd_id = WRITE_REGISTER_CMD,
.no_cmd_params = 0x4,
.cmd_params = {0x68, 0x01, 0x01, 0xDF},
- .req = SYNC,
+ .is_sync = true,
},
[CMD_ISO14443B_DEMOGAIN] = {
.cmd_len = 0x7,
.cmd_id = WRITE_REGISTER_CMD,
.no_cmd_params = 0x4,
.cmd_params = {0x68, 0x01, 0x01, 0x51},
- .req = SYNC,
+ .is_sync = true,
},
[CMD_ISO14443A_PROTOCOL_SELECT] = {
.cmd_len = 0x7,
.cmd_id = PROTOCOL_SELECT_CMD,
.no_cmd_params = 0x4,
.cmd_params = {ISO14443A_PROTOCOL_CODE, 0x00, 0x01, 0xA0},
- .req = SYNC,
+ .is_sync = true,
},
[CMD_ISO14443B_PROTOCOL_SELECT] = {
.cmd_len = 0x7,
.cmd_id = PROTOCOL_SELECT_CMD,
.no_cmd_params = 0x4,
.cmd_params = {ISO14443B_PROTOCOL_CODE, 0x01, 0x03, 0xFF},
- .req = SYNC,
+ .is_sync = true,
},
[CMD_WTX_RESPONSE] = {
.cmd_len = 0x6,
.cmd_id = SEND_RECEIVE_CMD,
.no_cmd_params = 0x3,
.cmd_params = {0xF2, 0x00, TRFLAG_NFCA_STD_FRAME_CRC},
- .req = ASYNC,
},
[CMD_FIELD_OFF] = {
.cmd_len = 0x5,
.cmd_id = PROTOCOL_SELECT_CMD,
.no_cmd_params = 0x2,
.cmd_params = {0x0, 0x0},
- .req = SYNC,
+ .is_sync = true,
},
[CMD_ISO15693_PROTOCOL_SELECT] = {
.cmd_len = 0x5,
.cmd_id = PROTOCOL_SELECT_CMD,
.no_cmd_params = 0x2,
.cmd_params = {ISO15693_PROTOCOL_CODE, 0x0D},
- .req = SYNC,
+ .is_sync = true,
},
};
@@ -279,13 +278,13 @@ static int st95hf_send_recv_cmd(struct st95hf_context
*st95context,
ret = st95hf_spi_send(&st95context->spicontext,
spi_cmd_buffer,
cmd_array[cmd].cmd_len,
- cmd_array[cmd].req);
+ cmd_array[cmd].is_sync);
if (ret) {
dev_err(dev, "st95hf_spi_send failed with error %d\n", ret);
return ret;
}
- if (cmd_array[cmd].req == SYNC && recv_res) {
+ if (cmd_array[cmd].is_sync && recv_res) {
unsigned char st95hf_response_arr[2];
ret = st95hf_spi_recv_response(&st95context->spicontext,
@@ -480,10 +479,8 @@ static int st95hf_send_spi_reset_sequence(struct
st95hf_context *st95context)
int result = 0;
unsigned char reset_cmd = ST95HF_COMMAND_RESET;
- result = st95hf_spi_send(&st95context->spicontext,
-&reset_cmd,
-ST95HF_RESET_CMD_LEN,
-ASYNC);
+ result = st95hf_spi_send(&st95context->spicontext, &reset_cmd,
+ST95HF_RESET_CMD_LEN, false);
if (result) {
dev_err(&st95context->spicontext.spidev->dev,
"spi reset sequence cmd error = %d", result);
@@ -932,8 +929,7 @@ static int st95hf_in_send_cmd(struct nfc_digital_dev *ddev,
stcontext->complete_cb_arg.rats = true;
rc = st95hf_spi_send(&stcontext->spicontext, skb->data,
-skb->len,
-ASYNC);
+skb->len, fa
[PATCH 10/10] NFC: st95hf: add of match table
Add a match table for device tree compatible strings. Interestingly, a
document describing the bindings already exists since a while, but users
currently reply on the implicit matching in the drivers core.
Signed-off-by: Daniel Mack
Cc: devicet...@vger.kernel.org
---
drivers/nfc/st95hf/core.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/nfc/st95hf/core.c b/drivers/nfc/st95hf/core.c
index 4db3c020921c..5a01b454fbc4 100644
--- a/drivers/nfc/st95hf/core.c
+++ b/drivers/nfc/st95hf/core.c
@@ -1012,6 +1012,12 @@ static const struct spi_device_id st95hf_id[] = {
};
MODULE_DEVICE_TABLE(spi, st95hf_id);
+static const struct of_device_id st95hf_of_match[] = {
+ { .compatible = "st,st95hf", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, st95hf_of_match);
+
static int st95hf_probe(struct spi_device *nfc_spi_dev)
{
int ret;
@@ -1189,6 +1195,7 @@ static struct spi_driver st95hf_driver = {
.driver = {
.name = "st95hf",
.owner = THIS_MODULE,
+ .of_match_table = st95hf_of_match,
},
.id_table = st95hf_id,
.probe = st95hf_probe,
--
2.17.1
[PATCH 07/10] NFC: st95hf: re-order command defines
Just a small cleanup to bring the command defines in a numerical order. Signed-off-by: Daniel Mack --- drivers/nfc/st95hf/core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/nfc/st95hf/core.c b/drivers/nfc/st95hf/core.c index 99f84ddfdfef..e7ecc57dde8f 100644 --- a/drivers/nfc/st95hf/core.c +++ b/drivers/nfc/st95hf/core.c @@ -45,10 +45,10 @@ /* Command Send Interface */ /* ST95HF_COMMAND_SEND CMD Ids */ -#define ECHO_CMD 0x55 -#define WRITE_REGISTER_CMD 0x9 #define PROTOCOL_SELECT_CMD0x2 #define SEND_RECEIVE_CMD 0x4 +#define WRITE_REGISTER_CMD 0x9 +#define ECHO_CMD 0x55 /* Select protocol codes */ #define ISO15693_PROTOCOL_CODE 0x1 -- 2.17.1
[PATCH 06/10] NFC: st95hf: move skb allocation to ISR
The driver currently assumes that interrupts can only occur between the
time when when a command has been sent to the device and the response
to it.
This assumption, however, is wrong. The antenna of the chip is likely to
catch a lot of environmental noise, and once in a while, the device will
latch the interrupt to signal a protocol error, and keep it latched until
the response bytes are read from the chip.
As the code currently stands, skbs for responses are only prepared when
a command is sent, and the ISR bails out early if that wasn't the case.
Hence, the interrupt remains latched, and no further communication with
device is possible.
To fix this, move the call to nfc_alloc_recv_skb() from
st95hf_in_send_cmd() to st95hf_irq_thread_handler() so we can always read
back the interrupt reason.
Signed-off-by: Daniel Mack
---
drivers/nfc/st95hf/core.c | 36 ++--
1 file changed, 6 insertions(+), 30 deletions(-)
diff --git a/drivers/nfc/st95hf/core.c b/drivers/nfc/st95hf/core.c
index 6761ab90f68d..99f84ddfdfef 100644
--- a/drivers/nfc/st95hf/core.c
+++ b/drivers/nfc/st95hf/core.c
@@ -196,7 +196,6 @@ static const struct cmd cmd_array[] = {
/* st95_digital_cmd_complete_arg stores client context */
struct st95_digital_cmd_complete_arg {
- struct sk_buff *skb_resp;
nfc_digital_cmd_complete_t complete_cb;
void *cb_usrarg;
bool rats;
@@ -783,12 +782,10 @@ static irqreturn_t st95hf_irq_thread_handler(int irq,
void *st95hfcontext)
spidevice = &stcontext->spicontext.spidev->dev;
cb_arg = &stcontext->complete_cb_arg;
- skb_resp = cb_arg->skb_resp;
- if (unlikely(!skb_resp)) {
- WARN(1, "unknown context in ST95HF ISR");
+ skb_resp = nfc_alloc_recv_skb(MAX_RESPONSE_BUFFER_SIZE, GFP_KERNEL);
+ if (WARN_ON(!skb_resp))
return IRQ_NONE;
- }
mutex_lock(&stcontext->rm_lock);
res_len = st95hf_spi_recv_response(&stcontext->spicontext,
@@ -838,12 +835,6 @@ static irqreturn_t st95hf_irq_thread_handler(int irq, void
*st95hfcontext)
/* call digital layer callback */
cb_arg->complete_cb(stcontext->ddev, cb_arg->cb_usrarg, skb_resp);
- /*
-* This skb is now owned by the core layer.
-* Make sure not to use it again.
-*/
- cb_arg->skb_resp = NULL;
-
/* up the semaphore before returning */
mutex_unlock(&stcontext->rm_lock);
@@ -913,15 +904,7 @@ static int st95hf_in_send_cmd(struct nfc_digital_dev *ddev,
void *arg)
{
struct st95hf_context *stcontext = nfc_digital_get_drvdata(ddev);
- int rc;
- struct sk_buff *skb_resp;
- int len_data_to_tag = 0;
-
- skb_resp = nfc_alloc_recv_skb(MAX_RESPONSE_BUFFER_SIZE, GFP_KERNEL);
- if (!skb_resp) {
- rc = -ENOMEM;
- goto error;
- }
+ int rc, len_data_to_tag = 0;
switch (stcontext->current_rf_tech) {
case NFC_DIGITAL_RF_TECH_106A:
@@ -933,8 +916,7 @@ static int st95hf_in_send_cmd(struct nfc_digital_dev *ddev,
len_data_to_tag = skb->len;
break;
default:
- rc = -EINVAL;
- goto free_skb_resp;
+ return -EINVAL;
}
skb_push(skb, 3);
@@ -942,7 +924,6 @@ static int st95hf_in_send_cmd(struct nfc_digital_dev *ddev,
skb->data[1] = SEND_RECEIVE_CMD;
skb->data[2] = len_data_to_tag;
- stcontext->complete_cb_arg.skb_resp = skb_resp;
stcontext->complete_cb_arg.cb_usrarg = arg;
stcontext->complete_cb_arg.complete_cb = cb;
@@ -956,17 +937,12 @@ static int st95hf_in_send_cmd(struct nfc_digital_dev
*ddev,
if (rc) {
dev_err(&stcontext->nfcdev->dev,
"Error %d trying to perform data_exchange", rc);
- goto free_skb_resp;
+ return rc;
}
kfree_skb(skb);
- return rc;
-
-free_skb_resp:
- kfree_skb(skb_resp);
-error:
- return rc;
+ return 0;
}
/* p2p will be supported in a later release ! */
--
2.17.1
[PATCH 05/10] NFC: st95hf: remove exchange_lock
This patch removes the exchange_lock sempahore. Its intended function
was two-fold:
a) Lock the remove() callback of the driver against the ISR, so that
the resources only go away after the ISR has finished. This is
unnecessary though, because `rm_lock' does that already, in
combination with the nullification of `scontext->ddev'.
b) Indicate whether a command was sent previously. If the semaphore
is found unused in the threaded ISR, an error is reported.
This case can be handled much nicer by checking whether `skb_resp'
is present in the context. For this, nullify the `skb_resp' pointer
in the callback context.
Signed-off-by: Daniel Mack
---
drivers/nfc/st95hf/core.c | 52 +++
1 file changed, 9 insertions(+), 43 deletions(-)
diff --git a/drivers/nfc/st95hf/core.c b/drivers/nfc/st95hf/core.c
index d857197ec7b2..6761ab90f68d 100644
--- a/drivers/nfc/st95hf/core.c
+++ b/drivers/nfc/st95hf/core.c
@@ -214,8 +214,6 @@ struct st95_digital_cmd_complete_arg {
* @st95hf_supply: regulator "consumer" for NFC device.
* @sendrcv_trflag: last byte of frame send by sendrecv command
* of st95hf. This byte contains transmission flag info.
- * @exchange_lock: semaphore used for signaling the st95hf_remove
- * function that the last outstanding async nfc request is finished.
* @rm_lock: mutex for ensuring safe access of nfc digital object
* from threaded ISR. Usage of this mutex avoids any race between
* deletion of the object from st95hf_remove() and its access from
@@ -233,7 +231,6 @@ struct st95hf_context {
struct st95_digital_cmd_complete_arg complete_cb_arg;
struct regulator *st95hf_supply;
unsigned char sendrcv_trflag;
- struct semaphore exchange_lock;
struct mutex rm_lock;
u8 current_protocol;
u8 current_rf_tech;
@@ -785,29 +782,14 @@ static irqreturn_t st95hf_irq_thread_handler(int irq,
void *st95hfcontext)
struct st95_digital_cmd_complete_arg *cb_arg;
spidevice = &stcontext->spicontext.spidev->dev;
+ cb_arg = &stcontext->complete_cb_arg;
+ skb_resp = cb_arg->skb_resp;
- /*
-* check semaphore, if not down() already, then we don't
-* know in which context the ISR is called and surely it
-* will be a bug. Note that down() of the semaphore is done
-* in the corresponding st95hf_in_send_cmd() and then
-* only this ISR should be called. ISR will up() the
-* semaphore before leaving. Hence when the ISR is called
-* the correct behaviour is down_trylock() should always
-* return 1 (indicating semaphore cant be taken and hence no
-* change in semaphore count).
-* If not, then we up() the semaphore and crash on
-* a BUG() !
-*/
- if (!down_trylock(&stcontext->exchange_lock)) {
- up(&stcontext->exchange_lock);
+ if (unlikely(!skb_resp)) {
WARN(1, "unknown context in ST95HF ISR");
return IRQ_NONE;
}
- cb_arg = &stcontext->complete_cb_arg;
- skb_resp = cb_arg->skb_resp;
-
mutex_lock(&stcontext->rm_lock);
res_len = st95hf_spi_recv_response(&stcontext->spicontext,
skb_resp->data);
@@ -856,8 +838,13 @@ static irqreturn_t st95hf_irq_thread_handler(int irq, void
*st95hfcontext)
/* call digital layer callback */
cb_arg->complete_cb(stcontext->ddev, cb_arg->cb_usrarg, skb_resp);
+ /*
+* This skb is now owned by the core layer.
+* Make sure not to use it again.
+*/
+ cb_arg->skb_resp = NULL;
+
/* up the semaphore before returning */
- up(&stcontext->exchange_lock);
mutex_unlock(&stcontext->rm_lock);
return IRQ_HANDLED;
@@ -868,8 +855,6 @@ static irqreturn_t st95hf_irq_thread_handler(int irq, void
*st95hfcontext)
skb_resp = ERR_PTR(result);
/* call of callback with error */
cb_arg->complete_cb(stcontext->ddev, cb_arg->cb_usrarg, skb_resp);
- /* up the semaphore before returning */
- up(&stcontext->exchange_lock);
mutex_unlock(&stcontext->rm_lock);
return IRQ_HANDLED;
}
@@ -965,25 +950,12 @@ static int st95hf_in_send_cmd(struct nfc_digital_dev
*ddev,
ddev->curr_protocol == NFC_PROTO_ISO14443)
stcontext->complete_cb_arg.rats = true;
- /*
-* down the semaphore to indicate to remove func that an
-* ISR is pending, note that it will not block here in any case.
-* If found blocked, it is a BUG!
-*/
- rc = down_killable(&stcontext->exchange_lock);
- if (rc) {
- WARN(1, "Semaphore is not found up in st95hf_in_send_cmd\n");
- return rc;
- }
-
rc = st95hf_spi_send(&stcontext->spicontext, skb->data,
skb->len,
ASYNC);
[PATCH 01/10] Revert "NFC: st95hf: drop illegal kfree_skb()"
This reverts commit c99f996b2ba49 ("NFC: st95hf: drop illegal
kfree_skb()").
It turns out that the st95hf_in_send_cmd() is in fact the sole owner of
this skb, and by not freeing it here, we not only causing a memory leak
but also mess up the refcount of the socket that holds it. This will in
turn lead to activated targets not being cleaned up, even after
stopping userspace processes.
The memory corruption that I was hunting was caused by another
kfree_skb(). This will be fixed in a later commit.
Signed-off-by: Daniel Mack
Fixes: c99f996b2ba49 ("NFC: st95hf: drop illegal kfree_skb()")
---
drivers/nfc/st95hf/core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/nfc/st95hf/core.c b/drivers/nfc/st95hf/core.c
index 36ef0e905ba3..bc1a2070f9bb 100644
--- a/drivers/nfc/st95hf/core.c
+++ b/drivers/nfc/st95hf/core.c
@@ -991,6 +991,8 @@ static int st95hf_in_send_cmd(struct nfc_digital_dev *ddev,
goto free_skb_resp;
}
+ kfree_skb(skb);
+
return rc;
free_skb_resp:
--
2.17.1
[PATCH 00/10] NFC: A bunch of cleanups for st95hf
This is a series of patches for the ST95HF driver. Patch #1 reverts a change that I have submitted earlier and which is sitting in nfc-next already. Given that the tree hasn't been sent out for being merged yet, it could also still be removed with rebasing, in which case #1 is not necessary of course. The changes are all rather simple and are explained in their individual commit logs. Note that this series builds upon the patch titled "nfc: st95hf: remove redundant pointers 'dev' and 'nfcddev'" that Colin posted the other day. Thanks, Daniel Daniel Mack (10): Revert "NFC: st95hf: drop illegal kfree_skb()" NFC: st95hf: drop nfcdev_free NFC: st95hf: drop illegal kfree_skb() in IRQ handler NFC: st95hf: remove logging from spi functions NFC: st95hf: remove exchange_lock NFC: st95hf: move skb allocation to ISR NFC: st95hf: re-order command defines NFC: st95hf: unify sync/async flags NFC: st95hf: two small style nits NFC: st95hf: add of match table drivers/nfc/st95hf/core.c | 135 +++--- drivers/nfc/st95hf/spi.c | 31 +++-- drivers/nfc/st95hf/spi.h | 8 +-- 3 files changed, 49 insertions(+), 125 deletions(-) -- 2.17.1
Cześć piękna
Am Wesley ze Stanów Zjednoczonych, który obecnie służy w misji pokojowej w Syrii. Chcę cię lepiej poznać, jeśli mogę być odważny. Uważam się za łatwego człowieka, a obecnie szukam związku, w którym czuję się kochany.
Re: [PATCH] brcmfmac: fix regression in parsing NVRAM for multiple devices
On 7/22/2018 11:46 PM, Rafał Miłecki wrote:
From: Rafał Miłecki
NVRAM is designed to work with Broadcom's SDK Linux kernel which fakes
PCI domain 0 for all internal MMIO devices. Since official Linux kernel
uses platform devices for that purpose there is a mismatch in numbering
PCI domains.
There used to be a fix for that problem but it was accidentally dropped
during the last firmware loading rework. That resulted in brcmfmac not
being able to extract device specific NVRAM content and all kind of
calibration problems.
Reported-by: Aditya Xavier
Fixes: 2baa3aaee27f ("brcmfmac: introduce brcmf_fw_alloc_request() function")
Cc: sta...@vger.kernel.org # v4.17+
oops. my bad.
Acked-by: Arend van Spriel
Signed-off-by: Rafał Miłecki
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
index 45928b5b8d97..4fffa6988087 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
@@ -1785,7 +1785,8 @@ brcmf_pcie_prepare_fw_request(struct brcmf_pciedev_info
*devinfo)
fwreq->items[BRCMF_PCIE_FW_CODE].type = BRCMF_FW_TYPE_BINARY;
fwreq->items[BRCMF_PCIE_FW_NVRAM].type = BRCMF_FW_TYPE_NVRAM;
fwreq->items[BRCMF_PCIE_FW_NVRAM].flags = BRCMF_FW_REQF_OPTIONAL;
- fwreq->domain_nr = pci_domain_nr(devinfo->pdev->bus);
+ /* NVRAM reserves PCI domain 0 for Broadcom's SDK faked bus */
+ fwreq->domain_nr = pci_domain_nr(devinfo->pdev->bus) + 1;
fwreq->bus_nr = devinfo->pdev->bus->number;
return fwreq;
Re: [PATCH v2 00/12] Add mt76x0 driver
Hi On Mon, Jul 23, 2018 at 11:16:25AM +0200, Lorenzo Bianconi wrote: > > I'm not sure if it can be got from the market easily. > > > > I would like to say, mt7668 is based on new MTK wifi architecture. as > > for mt7662/mt7612/mt7610, they all belong to legacy Ralink based design. > > of course, we will never see any new chip based on Ralink based design > > wifi in the future. > > > > There must be really lots of differences exist between two types of > > devices, such as MCU firmware download, MCU control interface, tx/rx > > path, even hardware register layout all changed and so on. > > > > I guess the hardware abstraction layer needs to be largely adjusted to > > add the new device mt7668u into mt76 driver. And even I think it would > > be easier for development and maintenance if we make a separate driver > > and then pick up some pure software components from mt76 to reuse. > > > > the mt76 driver has a hw independent layer (mt76/mt76-usb modules) that > implements data path (tx/rx side), mac80211 integration and calls hw dependent > code for hw initialization (that approach allows to support mt76x2 and mt7603 > chipsets in the same driver). > I guess it will be feasible (at least in theory) to integrate mt7668 > support in the > mt76 driver (but I can't be more precise since I have no any reference code or > hw to test) > > > Maybe it's the time I want to test your tree first with 7612u (?) or > > mt7610u (?) , what is suggested? and see how big the gap between this > > two type of devices and last write some code based on your tree to test > > mt7668u > > mt76x2u and mt76x0 are not merged yet, but I would suggest to start looking > at mt76x2u since it is already integrated in the mt76 driver while > mt76x0 is a standalone > driver for the moment. > > @Stanislaw what do you think? I think checking common mt76-usb layer for possibility to add support for new MT7688U chip on top of it is reasonable :-) Thanks Stanislaw
Re: [PATCH v2 00/12] Add mt76x0 driver
Hello, On Fri, Jul 20, 2018 at 11:36:34PM +0800, Sean Wang wrote: > > > Sure, there is no any problem for submitting mt7662u.bin and > > > mt7662u_rom_patch.bin if they are added with LICENCE.mediatek. > > > > > > Do you or Lorenzo have a plan to add MT7668u support to mt76? > > > MT7668 is a newer wifi device than MT7610u or MT7662u. > > > > > > > Cool, I am interested in adding MT7668u support to mt76 driver. > > Is the adapter available on the market? > > I'm not sure if it can be got from the market easily. > > I would like to say, mt7668 is based on new MTK wifi architecture. as > for mt7662/mt7612/mt7610, they all belong to legacy Ralink based design. > of course, we will never see any new chip based on Ralink based design > wifi in the future. > > There must be really lots of differences exist between two types of > devices, such as MCU firmware download, MCU control interface, tx/rx > path, even hardware register layout all changed and so on. > > I guess the hardware abstraction layer needs to be largely adjusted to > add the new device mt7668u into mt76 driver. And even I think it would > be easier for development and maintenance if we make a separate driver > and then pick up some pure software components from mt76 to reuse. It could be the case that it's better to develop separate mac80211 driver for this new chip than to integrate it into mt76. FWIW few years back there was failed efforts to add MT7630 devices into venerable rt2x00 driver and Felix Fietkau decision to develop new mt76 driver proved that this is better approach (though MT7620 is still supported via rt2x00 driver, but IMHO would be much better to have it as part of mt76). As usual the devil is in the detail. We have not yet unstreamed Felix' mt7603 driver which is part of mt76 in github tree; https://github.com/openwrt/mt76 I was not aware of that before, but according to description it also supports MT7688 ? ( PCIe version ?). However, for what I can tell, overlap between mt76x2 and mt7603 is very small and make mt7603 a separate driver it drivers/net/wireless/mediatek/ could be a better decision. > Maybe it's the time I want to test your tree first with 7612u (?) or > mt7610u (?) , what is suggested? and see how big the gap between this > two type of devices and last write some code based on your tree to test > mt7668u Both 7610u and 7612u should be supported in my development mt76x0-draft-v2 branch, which add mt76x0 on top of Lorenzo mt76x2u: https://github.com/sgruszka/wireless-drivers-next/commits/mt76x0-draft-v2 so you can test that if you want to :-) Thanks Stanislaw
Re: [PATCH 04/11] mt76x0: usb files
> Lorenzo Bianconi writes:
>
> >>
> >> Lorenzo Bianconi writes:
> >>
> >> > On Jul 05, Stanislaw Gruszka wrote:
> >> >> Add usb files of mt76x0 driver.
> >> >>
> >> >> Signed-off-by: Stanislaw Gruszka
> >>
> >> [...]
> >>
> >> >> +static int mt76x0_suspend(struct usb_interface *usb_intf, pm_message_t
> >> >> state)
> >> >> +{
> >> >> +struct mt76x0_dev *dev = usb_get_intfdata(usb_intf);
> >> >> +
> >> >> +mt76x0_cleanup(dev);
> >> >
> >> > Is it necessary to deallocate rx queue here in case of a system suspend?
> >>
> >> BTW it would really helpful to others if you could edit your quotes. It
> >> takes some time to find your oneliner from a 450 line message.
> >
> > Ack sorry, I will be more verbose next time :).
>
> I think you misunderstood, I actually was asking to be less verbose :)
>
> The oneliner was just fine. I was just suggesting that instead of
> sending a message with 400 lines trim the unnecessary quotation and just
> send a shorter 50 line message. That way you save a lot of time (and
> avoid frustration) for the people reading your mail.
Ops, I completely misunderstood, will do next time sorry :)
Regards,
Lorenzo
>
> --
> Kalle Valo
Re: [PATCH v2 00/12] Add mt76x0 driver
> > On Wed, 2018-07-11 at 10:07 +0200, Lorenzo Bianconi wrote: > > > On Tue, 2018-07-10 at 11:52 +0200, Stanislaw Gruszka wrote: > > > > On Tue, Jul 10, 2018 at 02:50:30PM +0800, Sean Wang wrote: > > > > > > For full support mt7610.bin firmware blob is need, hopefully with > > > > > > the permission of Mediatek, the blob will be pushed into > > > > > > linux-firmware > > > > > > git tree. > > > > > > > > > > > > > > > > Hi, Stanislaw > > > > > > > > > > You can feel free to add mt7610.bin firmware blob to linux-firmware > > > > > with > > > > > the license LICENCE.mediatek I added in [1]. > > > > > > > > > > > > > > > [1] > > > > > http://lists.infradead.org/pipermail/linux-mediatek/2018-June/013759.html > > > > > > > > Thank you Sean! > > > > > > > > I guess there is no problem with similar submission for > > > > mt7662u.bin and mt7662u_rom_patch.bin needed for mt76x2u: > > > > https://marc.info/?l=linux-wireless&m=153045438821562&w=2 > > > > > > > > > > > Thanks Sean, > > > > I will submit mt7662u.bin and mt7662u_rom_patch.bin with LICENCE.mediatek > > soon > > > > > Sure, there is no any problem for submitting mt7662u.bin and > > > mt7662u_rom_patch.bin if they are added with LICENCE.mediatek. > > > > > > Do you or Lorenzo have a plan to add MT7668u support to mt76? > > > MT7668 is a newer wifi device than MT7610u or MT7662u. > > > > > > > Cool, I am interested in adding MT7668u support to mt76 driver. > > Is the adapter available on the market? > > I'm not sure if it can be got from the market easily. > > I would like to say, mt7668 is based on new MTK wifi architecture. as > for mt7662/mt7612/mt7610, they all belong to legacy Ralink based design. > of course, we will never see any new chip based on Ralink based design > wifi in the future. > > There must be really lots of differences exist between two types of > devices, such as MCU firmware download, MCU control interface, tx/rx > path, even hardware register layout all changed and so on. > > I guess the hardware abstraction layer needs to be largely adjusted to > add the new device mt7668u into mt76 driver. And even I think it would > be easier for development and maintenance if we make a separate driver > and then pick up some pure software components from mt76 to reuse. > the mt76 driver has a hw independent layer (mt76/mt76-usb modules) that implements data path (tx/rx side), mac80211 integration and calls hw dependent code for hw initialization (that approach allows to support mt76x2 and mt7603 chipsets in the same driver). I guess it will be feasible (at least in theory) to integrate mt7668 support in the mt76 driver (but I can't be more precise since I have no any reference code or hw to test) > Maybe it's the time I want to test your tree first with 7612u (?) or > mt7610u (?) , what is suggested? and see how big the gap between this > two type of devices and last write some code based on your tree to test > mt7668u mt76x2u and mt76x0 are not merged yet, but I would suggest to start looking at mt76x2u since it is already integrated in the mt76 driver while mt76x0 is a standalone driver for the moment. @Stanislaw what do you think? Regards, Lorenzo > > Sean > > > Regards, > > Lorenzo > > > > > > Regards > > > > Stanislaw > > > > > > > > ___ > > > > Linux-mediatek mailing list > > > > linux-media...@lists.infradead.org > > > > http://lists.infradead.org/mailman/listinfo/linux-mediatek > > > > > > > >
Re: [PATCH 04/11] mt76x0: usb files
Lorenzo Bianconi writes:
>>
>> Lorenzo Bianconi writes:
>>
>> > On Jul 05, Stanislaw Gruszka wrote:
>> >> Add usb files of mt76x0 driver.
>> >>
>> >> Signed-off-by: Stanislaw Gruszka
>>
>> [...]
>>
>> >> +static int mt76x0_suspend(struct usb_interface *usb_intf, pm_message_t
>> >> state)
>> >> +{
>> >> +struct mt76x0_dev *dev = usb_get_intfdata(usb_intf);
>> >> +
>> >> +mt76x0_cleanup(dev);
>> >
>> > Is it necessary to deallocate rx queue here in case of a system suspend?
>>
>> BTW it would really helpful to others if you could edit your quotes. It
>> takes some time to find your oneliner from a 450 line message.
>
> Ack sorry, I will be more verbose next time :).
I think you misunderstood, I actually was asking to be less verbose :)
The oneliner was just fine. I was just suggesting that instead of
sending a message with 400 lines trim the unnecessary quotation and just
send a shorter 50 line message. That way you save a lot of time (and
avoid frustration) for the people reading your mail.
--
Kalle Valo
Re: [PATCH 04/11] mt76x0: usb files
>
> Lorenzo Bianconi writes:
>
> > On Jul 05, Stanislaw Gruszka wrote:
> >> Add usb files of mt76x0 driver.
> >>
> >> Signed-off-by: Stanislaw Gruszka
>
> [...]
>
> >> +static int mt76x0_suspend(struct usb_interface *usb_intf, pm_message_t
> >> state)
> >> +{
> >> +struct mt76x0_dev *dev = usb_get_intfdata(usb_intf);
> >> +
> >> +mt76x0_cleanup(dev);
> >
> > Is it necessary to deallocate rx queue here in case of a system suspend?
>
> BTW it would really helpful to others if you could edit your quotes. It
> takes some time to find your oneliner from a 450 line message.
Ack sorry, I will be more verbose next time :).
I mean that AFAIU it is not necessary to deallocate all buffers in the rx queue
during suspend/resume (using mt76x0_cleanup routine) but we can just mark them
as 'free' and reuse them as soon as the hw is resumed. I guess the same approach
can be used for mcu rx buffer.
Regards,
Lorenzo
>
> --
> Kalle Valo
