Use a spin lock to prevent concurrent access
to variables dealing with RX stream mode handling.
Currently, no protection is implemented - which
causes problems in RX.
Signed-off-by: Sujith <[email protected]>
---
drivers/net/wireless/ath/ath9k/hif_usb.c | 8 ++++++++
drivers/net/wireless/ath/ath9k/hif_usb.h | 1 +
2 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c
index 3afc747..8838cdf 100644
--- a/drivers/net/wireless/ath/ath9k/hif_usb.c
+++ b/drivers/net/wireless/ath/ath9k/hif_usb.c
@@ -330,6 +330,8 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev,
u16 pkt_len, pkt_tag, pool_index = 0;
u8 *ptr;
+ spin_lock(&hif_dev->rx_lock);
+
rx_remain_len = hif_dev->rx_remain_len;
rx_pkt_len = hif_dev->rx_transfer_len;
@@ -356,6 +358,8 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev,
}
}
+ spin_unlock(&hif_dev->rx_lock);
+
while (index < len) {
ptr = (u8 *) skb->data;
@@ -373,6 +377,7 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev,
index = index + 4 + pkt_len + pad_len;
if (index > MAX_RX_BUF_SIZE) {
+ spin_lock(&hif_dev->rx_lock);
hif_dev->rx_remain_len = index - MAX_RX_BUF_SIZE;
hif_dev->rx_transfer_len =
MAX_RX_BUF_SIZE - chk_idx - 4;
@@ -384,6 +389,7 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev,
dev_err(&hif_dev->udev->dev,
"ath9k_htc: RX memory allocation"
" error\n");
+ spin_unlock(&hif_dev->rx_lock);
goto err;
}
skb_reserve(nskb, 32);
@@ -394,6 +400,7 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev,
/* Record the buffer pointer */
hif_dev->remain_skb = nskb;
+ spin_unlock(&hif_dev->rx_lock);
} else {
nskb = __dev_alloc_skb(pkt_len + 32, GFP_ATOMIC);
if (!nskb) {
@@ -612,6 +619,7 @@ static int ath9k_hif_usb_alloc_rx_urbs(struct hif_device_usb *hif_dev)
int i, ret;
init_usb_anchor(&hif_dev->rx_submitted);
+ spin_lock_init(&hif_dev->rx_lock);
for (i = 0; i < MAX_RX_URB_NUM; i++) {
diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.h b/drivers/net/wireless/ath/ath9k/hif_usb.h
index ea9257b..ed9708c 100644
--- a/drivers/net/wireless/ath/ath9k/hif_usb.h
+++ b/drivers/net/wireless/ath/ath9k/hif_usb.h
@@ -93,6 +93,7 @@ struct hif_device_usb {
int rx_pkt_len;
int rx_transfer_len;
int rx_pad_len;
+ spinlock_t rx_lock;
u8 flags; /* HIF_USB_* */
};
--
1.7.0.3
2010/4/7 Sujith <[email protected]>:
> Ming Lei wrote:
>> 2010/4/6 Sujith <[email protected]>:
>> > Use a spin lock to prevent concurrent access
>> > to variables dealing with RX stream mode handling.
>>
>> which variables are needed to be protected?
>>
>> It seems hif_dev->rx_transfer_len, hif_dev->remain_skb
>> and hif_dev->rx_pkt_len are only accessed in ath9k_hif_usb_rx_stream,
>> which is only called in hard irq(urb->complete) context and now is
>> serialized strictly, so the protection is useless, isn't it?
>
> On a SMP machine, the RX callback can run on two CPUs, in this case,
> appropriate protection is needed between the two hard-irq handlers.
I am not sure if the same irq handler can run concurrently on different CPUs.
Even though it is, ehci/uhci handler have done such protection, so it
is not necessary to add such protection in the complete handler of usb
interface driver.
>
>> > Currently, no protection is implemented - which
>> > causes problems in RX.
>>
>> which problems? Could you describe them in detail?
>
> The RX stream position-hooks would get mangled up if two
> IRQ handlers access them without any kind of protection.
> This corrupts the stream data - causing data loss.
>
> Sujith
>
--
Lei Ming
2010/4/6 Sujith <[email protected]>:
> Use a spin lock to prevent concurrent access
> to variables dealing with RX stream mode handling.
which variables are needed to be protected?
It seems hif_dev->rx_transfer_len, hif_dev->remain_skb
and hif_dev->rx_pkt_len are only accessed in ath9k_hif_usb_rx_stream,
which is only called in hard irq(urb->complete) context and now is
serialized strictly, so the protection is useless, isn't it?
> Currently, no protection is implemented - which
> causes problems in RX.
which problems? Could you describe them in detail?
Thanks,
>
> Signed-off-by: Sujith <[email protected]>
> ---
> ?drivers/net/wireless/ath/ath9k/hif_usb.c | ? ?8 ++++++++
> ?drivers/net/wireless/ath/ath9k/hif_usb.h | ? ?1 +
> ?2 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c
> index 3afc747..8838cdf 100644
> --- a/drivers/net/wireless/ath/ath9k/hif_usb.c
> +++ b/drivers/net/wireless/ath/ath9k/hif_usb.c
> @@ -330,6 +330,8 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev,
> ? ? ? ?u16 pkt_len, pkt_tag, pool_index = 0;
> ? ? ? ?u8 *ptr;
>
> + ? ? ? spin_lock(&hif_dev->rx_lock);
> +
> ? ? ? ?rx_remain_len = hif_dev->rx_remain_len;
> ? ? ? ?rx_pkt_len = hif_dev->rx_transfer_len;
>
> @@ -356,6 +358,8 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev,
> ? ? ? ? ? ? ? ?}
> ? ? ? ?}
>
> + ? ? ? spin_unlock(&hif_dev->rx_lock);
> +
> ? ? ? ?while (index < len) {
> ? ? ? ? ? ? ? ?ptr = (u8 *) skb->data;
>
> @@ -373,6 +377,7 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev,
> ? ? ? ? ? ? ? ? ? ? ? ?index = index + 4 + pkt_len + pad_len;
>
> ? ? ? ? ? ? ? ? ? ? ? ?if (index > MAX_RX_BUF_SIZE) {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? spin_lock(&hif_dev->rx_lock);
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?hif_dev->rx_remain_len = index - MAX_RX_BUF_SIZE;
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?hif_dev->rx_transfer_len =
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?MAX_RX_BUF_SIZE - chk_idx - 4;
> @@ -384,6 +389,7 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?dev_err(&hif_dev->udev->dev,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?"ath9k_htc: RX memory allocation"
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?" error\n");
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? spin_unlock(&hif_dev->rx_lock);
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?goto err;
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?}
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?skb_reserve(nskb, 32);
> @@ -394,6 +400,7 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev,
>
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?/* Record the buffer pointer */
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?hif_dev->remain_skb = nskb;
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? spin_unlock(&hif_dev->rx_lock);
> ? ? ? ? ? ? ? ? ? ? ? ?} else {
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?nskb = __dev_alloc_skb(pkt_len + 32, GFP_ATOMIC);
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?if (!nskb) {
> @@ -612,6 +619,7 @@ static int ath9k_hif_usb_alloc_rx_urbs(struct hif_device_usb *hif_dev)
> ? ? ? ?int i, ret;
>
> ? ? ? ?init_usb_anchor(&hif_dev->rx_submitted);
> + ? ? ? spin_lock_init(&hif_dev->rx_lock);
>
> ? ? ? ?for (i = 0; i < MAX_RX_URB_NUM; i++) {
>
> diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.h b/drivers/net/wireless/ath/ath9k/hif_usb.h
> index ea9257b..ed9708c 100644
> --- a/drivers/net/wireless/ath/ath9k/hif_usb.h
> +++ b/drivers/net/wireless/ath/ath9k/hif_usb.h
> @@ -93,6 +93,7 @@ struct hif_device_usb {
> ? ? ? ?int rx_pkt_len;
> ? ? ? ?int rx_transfer_len;
> ? ? ? ?int rx_pad_len;
> + ? ? ? spinlock_t rx_lock;
> ? ? ? ?u8 flags; /* HIF_USB_* */
> ?};
>
> --
> 1.7.0.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>
--
Lei Ming
Ming Lei wrote:
> 2010/4/6 Sujith <[email protected]>:
> > Use a spin lock to prevent concurrent access
> > to variables dealing with RX stream mode handling.
>
> which variables are needed to be protected?
>
> It seems hif_dev->rx_transfer_len, hif_dev->remain_skb
> and hif_dev->rx_pkt_len are only accessed in ath9k_hif_usb_rx_stream,
> which is only called in hard irq(urb->complete) context and now is
> serialized strictly, so the protection is useless, isn't it?
On a SMP machine, the RX callback can run on two CPUs, in this case,
appropriate protection is needed between the two hard-irq handlers.
> > Currently, no protection is implemented - which
> > causes problems in RX.
>
> which problems? Could you describe them in detail?
The RX stream position-hooks would get mangled up if two
IRQ handlers access them without any kind of protection.
This corrupts the stream data - causing data loss.
Sujith
Ming Lei wrote:
> I am not sure if the same irq handler can run concurrently on different CPUs.
> Even though it is, ehci/uhci handler have done such protection, so it
> is not necessary to add such protection in the complete handler of usb
> interface driver.
What protection is done by ehci ?
Is more information on this available anywhere ?
Sujith
2010/4/7 Sujith <[email protected]>:
> Ming Lei wrote:
>> I am not sure if the same irq handler can run concurrently on different CPUs.
>> Even though it is, ehci/uhci handler have done such protection, so it
>> is not necessary to add such protection in the complete handler of usb
>> interface driver.
>
> What protection is done by ehci ?
> Is more information on this available anywhere ?
I should have thought that ehci->lock(ehci_irq()) may do such things, but
it is released before calling urb->complete, so the patch is correct, sorry for
my noise.
Thanks,
--
Lei Ming