p54_rx_frame_sent will alter the tx_queue. Therefore we should hold
the lock to protect against concurrent p54_assign_address calls.
Signed-off-by: Christian Lamparter <[email protected]>
---
hmm,
(looking at [GIT]: Networking debate)
linux-next. since there is no known regression that this patch could possibly fix...
---
diff -Nurp a/drivers/net/wireless/p54/p54common.c b/drivers/net/wireless/p54/p54common.c
--- a/drivers/net/wireless/p54/p54common.c 2008-08-23 21:13:37.000000000 +0200
+++ b/drivers/net/wireless/p54/p54common.c 2008-08-24 02:11:35.000000000 +0200
@@ -432,7 +432,9 @@ static void p54_rx_frame_sent(struct iee
struct memrecord *range = NULL;
u32 freed = 0;
u32 last_addr = priv->rx_start;
+ unsigned long flags;
+ spin_lock_irqsave(&priv->tx_queue.lock, flags);
while (entry != (struct sk_buff *)&priv->tx_queue) {
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(entry);
range = (void *)info->driver_data;
@@ -453,6 +455,8 @@ static void p54_rx_frame_sent(struct iee
last_addr = range->end_addr;
__skb_unlink(entry, &priv->tx_queue);
+ spin_unlock_irqrestore(&priv->tx_queue.lock, flags);
+
memset(&info->status, 0, sizeof(info->status));
entry_hdr = (struct p54_control_hdr *) entry->data;
entry_data = (struct p54_tx_control_allocdata *) entry_hdr->data;
@@ -470,12 +474,14 @@ static void p54_rx_frame_sent(struct iee
info->status.ack_signal = le16_to_cpu(payload->ack_rssi);
skb_pull(entry, sizeof(*hdr) + pad + sizeof(*entry_data));
ieee80211_tx_status_irqsafe(dev, entry);
- break;
+ goto out;
} else
last_addr = range->end_addr;
entry = entry->next;
}
+ spin_unlock_irqrestore(&priv->tx_queue.lock, flags);
+out:
if (freed >= IEEE80211_MAX_RTS_THRESHOLD + 0x170 +
sizeof(struct p54_control_hdr))
p54_wake_free_queues(dev);
On Sunday 24 August 2008 18:07:43 Larry Finger wrote:
> Chr wrote:
> > p54_rx_frame_sent will alter the tx_queue. Therefore we should hold
> > the lock to protect against concurrent p54_assign_address calls.
> >
> > Signed-off-by: Christian Lamparter <[email protected]>
> > ---
> > hmm,
> >
> > (looking at [GIT]: Networking debate)
> > linux-next. since there is no known regression that this patch could possibly fix...
> > ---
> > diff -Nurp a/drivers/net/wireless/p54/p54common.c b/drivers/net/wireless/p54/p54common.c
> > --- a/drivers/net/wireless/p54/p54common.c 2008-08-23 21:13:37.000000000 +0200
> > +++ b/drivers/net/wireless/p54/p54common.c 2008-08-24 02:11:35.000000000 +0200
> > @@ -432,7 +432,9 @@ static void p54_rx_frame_sent(struct iee
> > struct memrecord *range = NULL;
> > u32 freed = 0;
> > u32 last_addr = priv->rx_start;
> > + unsigned long flags;
> >
> > + spin_lock_irqsave(&priv->tx_queue.lock, flags);
> > while (entry != (struct sk_buff *)&priv->tx_queue) {
> > struct ieee80211_tx_info *info = IEEE80211_SKB_CB(entry);
> > range = (void *)info->driver_data;
> > @@ -453,6 +455,8 @@ static void p54_rx_frame_sent(struct iee
> >
> > last_addr = range->end_addr;
> > __skb_unlink(entry, &priv->tx_queue);
> > + spin_unlock_irqrestore(&priv->tx_queue.lock, flags);
> > +
> > memset(&info->status, 0, sizeof(info->status));
> > entry_hdr = (struct p54_control_hdr *) entry->data;
> > entry_data = (struct p54_tx_control_allocdata *) entry_hdr->data;
> > @@ -470,12 +474,14 @@ static void p54_rx_frame_sent(struct iee
> > info->status.ack_signal = le16_to_cpu(payload->ack_rssi);
> > skb_pull(entry, sizeof(*hdr) + pad + sizeof(*entry_data));
> > ieee80211_tx_status_irqsafe(dev, entry);
> > - break;
> > + goto out;
> > } else
> > last_addr = range->end_addr;
> > entry = entry->next;
> > }
> > + spin_unlock_irqrestore(&priv->tx_queue.lock, flags);
> >
> > +out:
> > if (freed >= IEEE80211_MAX_RTS_THRESHOLD + 0x170 +
> > sizeof(struct p54_control_hdr))
> > p54_wake_free_queues(dev);
>
> I have a question about this. The code is OK, but I wonder if it might
> not be clearer if the spin_unlock_irqrestore() in the second hunk were
> deleted, and the label for "out" in the third hunk were moved up one
> statement. That way the matching lock/unlock pair would be more obvious.
Well, that way a lot more code is inside of the critical section.
I'd say it's worth the "obfuscation", as this is a hotpath.
--
Greetings Michael.
On Sunday 24 August 2008 19:55:05 Michael Buesch wrote:
> On Sunday 24 August 2008 18:07:43 Larry Finger wrote:
> > Chr wrote:
> > > p54_rx_frame_sent will alter the tx_queue. Therefore we should hold
> > > the lock to protect against concurrent p54_assign_address calls.
> > >
> > > Signed-off-by: Christian Lamparter <[email protected]>
> > > ---
> > > hmm,
> > >
> > > (looking at [GIT]: Networking debate)
> > > linux-next. since there is no known regression that this patch could
> > > possibly fix... ---
> > > diff -Nurp a/drivers/net/wireless/p54/p54common.c
> > > b/drivers/net/wireless/p54/p54common.c ---
> > > a/drivers/net/wireless/p54/p54common.c 2008-08-23 21:13:37.000000000
> > > +0200 +++ b/drivers/net/wireless/p54/p54common.c 2008-08-24
> > > 02:11:35.000000000 +0200 @@ -432,7 +432,9 @@ static void
> > > p54_rx_frame_sent(struct iee
> > > struct memrecord *range = NULL;
> > > u32 freed = 0;
> > > u32 last_addr = priv->rx_start;
> > > + unsigned long flags;
> > >
> > > + spin_lock_irqsave(&priv->tx_queue.lock, flags);
> > > while (entry != (struct sk_buff *)&priv->tx_queue) {
> > > struct ieee80211_tx_info *info = IEEE80211_SKB_CB(entry);
> > > range = (void *)info->driver_data;
> > > @@ -453,6 +455,8 @@ static void p54_rx_frame_sent(struct iee
> > >
> > > last_addr = range->end_addr;
> > > __skb_unlink(entry, &priv->tx_queue);
> > > + spin_unlock_irqrestore(&priv->tx_queue.lock, flags);
> > > +
> > > memset(&info->status, 0, sizeof(info->status));
> > > entry_hdr = (struct p54_control_hdr *) entry->data;
> > > entry_data = (struct p54_tx_control_allocdata *) entry_hdr->data;
> > > @@ -470,12 +474,14 @@ static void p54_rx_frame_sent(struct iee
> > > info->status.ack_signal = le16_to_cpu(payload->ack_rssi);
> > > skb_pull(entry, sizeof(*hdr) + pad + sizeof(*entry_data));
> > > ieee80211_tx_status_irqsafe(dev, entry);
> > > - break;
> > > + goto out;
> > > } else
> > > last_addr = range->end_addr;
> > > entry = entry->next;
> > > }
> > > + spin_unlock_irqrestore(&priv->tx_queue.lock, flags);
> > >
> > > +out:
> > > if (freed >= IEEE80211_MAX_RTS_THRESHOLD + 0x170 +
> > > sizeof(struct p54_control_hdr))
> > > p54_wake_free_queues(dev);
> >
> > I have a question about this. The code is OK, but I wonder if it might
> > not be clearer if the spin_unlock_irqrestore() in the second hunk were
> > deleted, and the label for "out" in the third hunk were moved up one
> > statement. That way the matching lock/unlock pair would be more obvious.
>
> Well, that way a lot more code is inside of the critical section.
> I'd say it's worth the "obfuscation", as this is a hotpath.
indeed... that's what I thought would be a better trade-off between
- formal / function correctness
- (locking) guide-lines conformance
- (patch-)size
But yes, originally I planned to spend a extra empty line right about the
spin_unlock_irqrestore (the one inside the _while_ block), for
"eye friendliness"...
but I stuffed that on a second thought. I thought this extra
empty line would be waste of valuable resources, maybe someone
would complain about the amount of "cheese holes" in the code ;-).
Regards,
Chr
Chr wrote:
> p54_rx_frame_sent will alter the tx_queue. Therefore we should hold
> the lock to protect against concurrent p54_assign_address calls.
>
> Signed-off-by: Christian Lamparter <[email protected]>
> ---
> hmm,
>
> (looking at [GIT]: Networking debate)
> linux-next. since there is no known regression that this patch could possibly fix...
> ---
> diff -Nurp a/drivers/net/wireless/p54/p54common.c b/drivers/net/wireless/p54/p54common.c
> --- a/drivers/net/wireless/p54/p54common.c 2008-08-23 21:13:37.000000000 +0200
> +++ b/drivers/net/wireless/p54/p54common.c 2008-08-24 02:11:35.000000000 +0200
> @@ -432,7 +432,9 @@ static void p54_rx_frame_sent(struct iee
> struct memrecord *range = NULL;
> u32 freed = 0;
> u32 last_addr = priv->rx_start;
> + unsigned long flags;
>
> + spin_lock_irqsave(&priv->tx_queue.lock, flags);
> while (entry != (struct sk_buff *)&priv->tx_queue) {
> struct ieee80211_tx_info *info = IEEE80211_SKB_CB(entry);
> range = (void *)info->driver_data;
> @@ -453,6 +455,8 @@ static void p54_rx_frame_sent(struct iee
>
> last_addr = range->end_addr;
> __skb_unlink(entry, &priv->tx_queue);
> + spin_unlock_irqrestore(&priv->tx_queue.lock, flags);
> +
> memset(&info->status, 0, sizeof(info->status));
> entry_hdr = (struct p54_control_hdr *) entry->data;
> entry_data = (struct p54_tx_control_allocdata *) entry_hdr->data;
> @@ -470,12 +474,14 @@ static void p54_rx_frame_sent(struct iee
> info->status.ack_signal = le16_to_cpu(payload->ack_rssi);
> skb_pull(entry, sizeof(*hdr) + pad + sizeof(*entry_data));
> ieee80211_tx_status_irqsafe(dev, entry);
> - break;
> + goto out;
> } else
> last_addr = range->end_addr;
> entry = entry->next;
> }
> + spin_unlock_irqrestore(&priv->tx_queue.lock, flags);
>
> +out:
> if (freed >= IEEE80211_MAX_RTS_THRESHOLD + 0x170 +
> sizeof(struct p54_control_hdr))
> p54_wake_free_queues(dev);
I have a question about this. The code is OK, but I wonder if it might
not be clearer if the spin_unlock_irqrestore() in the second hunk were
deleted, and the label for "out" in the third hunk were moved up one
statement. That way the matching lock/unlock pair would be more obvious.
Larry