rt2x00: Allocate ring structures in single array
Subsystems (2)
PATCH 22/32 rt2x00: Allocate ring structures in single array
From: Ivo van Doorn <[email protected]>
Allocate all ring structures seperately and as an array of rings. This will make the rt2x00_pci and rt2x00_usb structures more generic for all rt2x00 modules.
Use seperate function to convert a dscape ring ID to the address of the actual ring.
Available on server:
Quoted 2 lines · Ivo van Doorn
Use seperate function to convert a dscape ring ID
to the address of the actual ring.
Just minor things:
Quoted 11 lines
[...]
(quoted diff) +0
-6@@ -1386,20 +1412,19 @@ rt2400pci_tx(struct net_device *net_dev, rt2x00_register_read(rt2x00pci, TXCSR0, ®); - if (control->queue == IEEE80211_TX_QUEUE_DATA0) { - ring = &rt2x00pci->prio; - rt2x00_set_field32(®, TXCSR0_KICK_PRIO, 1); - } else if (control->queue == IEEE80211_TX_QUEUE_DATA1) { - ring = &rt2x00pci->tx; - rt2x00_set_field32(®, TXCSR0_KICK_TX, 1);
Review rt2400pci_tx(struct net_device *net_dev, · line 1419
Quoted 10 lines
(quoted diff) +5
-5- } else if (control->queue == IEEE80211_TX_QUEUE_AFTER_BEACON) { - ring = &rt2x00pci->atim; - rt2x00_set_field32(®, TXCSR0_KICK_ATIM, 1); - } else { - ERROR("Frame received for invalid queue."); + /* + * Determine which ring to put packet on. + */ + ring = rt2x00pci_get_ring(rt2x00pci, control->queue); + if (unlikely(!ring)) {
Should not happen. Maybe some message that user should report a bug?
Review rt2400pci_tx(struct net_device *net_dev, · line 1426
Quoted 7 lines
(quoted diff) +4
-0+ ERROR("Attempt to send packet over invalid queue %d.\n", + control->queue); return NET_RX_DROP; } + if (rt2x00_ring_full(ring)) + return NET_RX_DROP;
NET_XMIT_DROP?
Quoted 16 lines
(quoted diff) +2
-5+ entry = rt2x00_get_data_entry(ring); txd = entry->desc_addr;@@ -1780,14 +1813,17 @@ rt2400pci_conf_tx(struct net_device *net int queue, const struct ieee80211_tx_queue_params *params) { struct rt2x00_pci *rt2x00pci = ieee80211_dev_hw_data(net_dev); - struct data_ring *ring; + struct data_ring *ring = &rt2x00pci->ring[RING_TX]; - if (queue == IEEE80211_TX_QUEUE_DATA0) - ring = &rt2x00pci->prio; - else if (queue == IEEE80211_TX_QUEUE_DATA1) - ring = &rt2x00pci->tx;
Review rt2400pci_conf_tx(struct net_device *net · line 1825
Quoted 10 lines
(quoted diff) +8
-2- else - return -EINVAL; + /* + * We don't support variating cw_min and cw_max variables + * per queue. So by default we only configure the TX queue, + * and ignore all other configurations. + */ + if (queue != IEEE80211_TX_QUEUE_DATA0) { + NOTICE("Ignoring configuration for queue %d.\n", queue); + return 0;
Is there a reason for not returning a proper error code?
Review rt2400pci_conf_tx(struct net_device *net · line 1829
Quoted 4 lines
(quoted diff) +1
-0+ } memcpy(&ring->tx_params, params, sizeof(*params));
Ditto for rt2500pci and rt2500usb.
Thanks,
Signature · 2 lines
Jiri Benc SUSE Labs
Quoted 21 lines · Jiri Benc
Quoted 2 lines · Ivo van Doorn
Use seperate function to convert a dscape ring ID
to the address of the actual ring.Just minor things:
Quoted 15 lines
[...]
(quoted diff) +0
-10@@ -1386,20 +1412,19 @@ rt2400pci_tx(struct net_device *net_dev, rt2x00_register_read(rt2x00pci, TXCSR0, ®); - if (control->queue == IEEE80211_TX_QUEUE_DATA0) { - ring = &rt2x00pci->prio; - rt2x00_set_field32(®, TXCSR0_KICK_PRIO, 1); - } else if (control->queue == IEEE80211_TX_QUEUE_DATA1) { - ring = &rt2x00pci->tx; - rt2x00_set_field32(®, TXCSR0_KICK_TX, 1); - } else if (control->queue == IEEE80211_TX_QUEUE_AFTER_BEACON) { - ring = &rt2x00pci->atim; - rt2x00_set_field32(®, TXCSR0_KICK_ATIM, 1); - } else {
Quoted 8 lines
Review rt2400pci_tx(struct net_device *net_dev, · line 1419
Quoted 6 lines
(quoted diff) +5
-1- ERROR("Frame received for invalid queue."); + /* + * Determine which ring to put packet on. + */ + ring = rt2x00pci_get_ring(rt2x00pci, control->queue); + if (unlikely(!ring)) {Should not happen. Maybe some message that user should report a bug?
I am not really fan on such messages. But when the control->queue is indeed invalid, it is a serious bug. Perhaps a BUG() should be used as well? If so, I'll put it into a new patch along with the fixes below.
Quoted 9 lines
Review rt2400pci_tx(struct net_device *net_dev, · line 1426
Quoted 7 lines
(quoted diff) +4
-0+ ERROR("Attempt to send packet over invalid queue %d.\n", + control->queue); return NET_RX_DROP; } + if (rt2x00_ring_full(ring)) + return NET_RX_DROP;
NET_XMIT_DROP?
Absolutely true, I'll fix this as well.
Quoted 12 lines
Quoted 12 lines
(quoted diff) +2
-1+ entry = rt2x00_get_data_entry(ring); txd = entry->desc_addr;@@ -1780,14 +1813,17 @@ rt2400pci_conf_tx(struct net_device *net int queue, const struct ieee80211_tx_queue_params *params) { struct rt2x00_pci *rt2x00pci = ieee80211_dev_hw_data(net_dev); - struct data_ring *ring; + struct data_ring *ring = &rt2x00pci->ring[RING_TX];
Quoted 16 lines
Quoted 4 lines
(quoted diff) +0
-4- if (queue == IEEE80211_TX_QUEUE_DATA0) - ring = &rt2x00pci->prio; - else if (queue == IEEE80211_TX_QUEUE_DATA1) - ring = &rt2x00pci->tx;Review rt2400pci_conf_tx(struct net_device *net · line 1825
Quoted 10 lines
(quoted diff) +8
-2- else - return -EINVAL; + /* + * We don't support variating cw_min and cw_max variables + * per queue. So by default we only configure the TX queue, + * and ignore all other configurations. + */ + if (queue != IEEE80211_TX_QUEUE_DATA0) { + NOTICE("Ignoring configuration for queue %d.\n", queue); + return 0;Is there a reason for not returning a proper error code?
Good question, in rt2400pci the behaviour is something that is not somethign the dscape could expect. Since we don't allow the configuration of all available tx queues.
I had made it return 0, since I wasn't sure how the stack would appreciate this behaviour.
But I have checked this in the stack since then, and apparently, tx_conf is called without any notice of available tx queues. Only when WMM is available the return value is checked,
so it is safe to return a error value.
But this behaviour should never have appeared in rt2500pci and rt2500usb, so I'll put this fix in the patch as well.
Thanks.
Quoted 3 lines · Ivo van Doorn
I am not really fan on such messages. But when the
control->queueis indeed invalid, it is a serious bug. Perhaps aBUG()should be used as well? If so, I'll put it into a new patch along with the fixes below.
BUG() is not a good idea, this is not critical enough to panic the kernel.
Signature · 2 lines
Jiri Benc SUSE Labs
Quoted 6 lines · Jiri Benc
Quoted 3 lines · Ivo van Doorn
I am not really fan on such messages. But when the
control->queueis indeed invalid, it is a serious bug. Perhaps aBUG()should be used as well? If so, I'll put it into a new patch along with the fixes below.
BUG()is not a good idea, this is not critical enough to panic the kernel.
Ok, I"ll just change the debug message then. :)
Thanks.