| Message ID | 1374129193-3533-2-git-send-email-michal.kazior@tieto.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Michal Kazior <michal.kazior@tieto.com> writes: > It was possible to submit new HTC commands > after/while HTC stopped. This led to memory > corruption in some rare cases. > > Signed-off-by: Michal Kazior <michal.kazior@tieto.com> [...] > @@ -957,7 +955,6 @@ void ath10k_htc_stop(struct ath10k_htc *htc) > } > > ath10k_hif_stop(htc->ar); > - ath10k_htc_reset_endpoint_states(htc); > } Is this on purpose? I can't fit it to the description.
On 19 July 2013 13:04, Kalle Valo <kvalo@qca.qualcomm.com> wrote: > Michal Kazior <michal.kazior@tieto.com> writes: > >> It was possible to submit new HTC commands >> after/while HTC stopped. This led to memory >> corruption in some rare cases. >> >> Signed-off-by: Michal Kazior <michal.kazior@tieto.com> > > [...] > >> @@ -957,7 +955,6 @@ void ath10k_htc_stop(struct ath10k_htc *htc) >> } >> >> ath10k_hif_stop(htc->ar); >> - ath10k_htc_reset_endpoint_states(htc); >> } > > Is this on purpose? I can't fit it to the description. You're right. I should've mentioned that in the commit message. This line is simply unnecessary. Do you prefer fixing the commit message accordingly or should I post it as a separate patch? Pozdrawiam / Best regards, Micha? Kazior. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Michal Kazior <michal.kazior@tieto.com> writes: > On 19 July 2013 13:04, Kalle Valo <kvalo@qca.qualcomm.com> wrote: >> Michal Kazior <michal.kazior@tieto.com> writes: >> >>> It was possible to submit new HTC commands >>> after/while HTC stopped. This led to memory >>> corruption in some rare cases. >>> >>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com> >> >> [...] >> >>> @@ -957,7 +955,6 @@ void ath10k_htc_stop(struct ath10k_htc *htc) >>> } >>> >>> ath10k_hif_stop(htc->ar); >>> - ath10k_htc_reset_endpoint_states(htc); >>> } >> >> Is this on purpose? I can't fit it to the description. > > You're right. I should've mentioned that in the commit message. > > This line is simply unnecessary. Do you prefer fixing the commit > message accordingly or should I post it as a separate patch? A separate patch, please. Makes it easier to bisect issues later on.
diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c index 7d5a366..f5610f1 100644 --- a/drivers/net/wireless/ath/ath10k/htc.c +++ b/drivers/net/wireless/ath/ath10k/htc.c @@ -251,10 +251,14 @@ int ath10k_htc_send(struct ath10k_htc *htc, return -ENOENT; } - skb_push(skb, sizeof(struct ath10k_htc_hdr)); - spin_lock_bh(&htc->tx_lock); + if (htc->stopped) { + spin_unlock_bh(&htc->tx_lock); + return -ESHUTDOWN; + } + __skb_queue_tail(&ep->tx_queue, skb); + skb_push(skb, sizeof(struct ath10k_htc_hdr)); spin_unlock_bh(&htc->tx_lock); queue_work(htc->ar->workqueue, &ep->send_work); @@ -267,23 +271,17 @@ static int ath10k_htc_tx_completion_handler(struct ath10k *ar, { struct ath10k_htc *htc = &ar->htc; struct ath10k_htc_ep *ep = &htc->endpoint[eid]; - bool stopping; ath10k_htc_notify_tx_completion(ep, skb); /* the skb now belongs to the completion handler */ + /* note: when using TX credit flow, the re-checking of queues happens + * when credits flow back from the target. in the non-TX credit case, + * we recheck after the packet completes */ spin_lock_bh(&htc->tx_lock); - stopping = htc->stopping; - spin_unlock_bh(&htc->tx_lock); - - if (!ep->tx_credit_flow_enabled && !stopping) - /* - * note: when using TX credit flow, the re-checking of - * queues happens when credits flow back from the target. - * in the non-TX credit case, we recheck after the packet - * completes - */ + if (!ep->tx_credit_flow_enabled && !htc->stopped) queue_work(ar->workqueue, &ep->send_work); + spin_unlock_bh(&htc->tx_lock); return 0; } @@ -948,7 +946,7 @@ void ath10k_htc_stop(struct ath10k_htc *htc) struct ath10k_htc_ep *ep; spin_lock_bh(&htc->tx_lock); - htc->stopping = true; + htc->stopped = true; spin_unlock_bh(&htc->tx_lock); for (i = ATH10K_HTC_EP_0; i < ATH10K_HTC_EP_COUNT; i++) { @@ -957,7 +955,6 @@ void ath10k_htc_stop(struct ath10k_htc *htc) } ath10k_hif_stop(htc->ar); - ath10k_htc_reset_endpoint_states(htc); } /* registered target arrival callback from the HIF layer */ @@ -969,6 +966,7 @@ int ath10k_htc_init(struct ath10k *ar) spin_lock_init(&htc->tx_lock); + htc->stopped = false; ath10k_htc_reset_endpoint_states(htc); /* setup HIF layer callbacks */ diff --git a/drivers/net/wireless/ath/ath10k/htc.h b/drivers/net/wireless/ath/ath10k/htc.h index 1606c9f..e1dd8c7 100644 --- a/drivers/net/wireless/ath/ath10k/htc.h +++ b/drivers/net/wireless/ath/ath10k/htc.h @@ -335,7 +335,7 @@ struct ath10k_htc { struct ath10k *ar; struct ath10k_htc_ep endpoint[ATH10K_HTC_EP_COUNT]; - /* protects endpoint and stopping fields */ + /* protects endpoint and stopped fields */ spinlock_t tx_lock; struct ath10k_htc_ops htc_ops; @@ -349,7 +349,7 @@ struct ath10k_htc { struct ath10k_htc_svc_tx_credits service_tx_alloc[ATH10K_HTC_EP_COUNT]; int target_credit_size; - bool stopping; + bool stopped; }; int ath10k_htc_init(struct ath10k *ar);
It was possible to submit new HTC commands after/while HTC stopped. This led to memory corruption in some rare cases. Signed-off-by: Michal Kazior <michal.kazior@tieto.com> --- drivers/net/wireless/ath/ath10k/htc.c | 28 +++++++++++++--------------- drivers/net/wireless/ath/ath10k/htc.h | 4 ++-- 2 files changed, 15 insertions(+), 17 deletions(-)