[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. Signed-off-by: Ivo van Doorn <[EMAIL PROTECTED]> Available on server: http://mendiosus.nl/rt2x00/rt2x00-22-rings.diff pgp37E3iQh6EF.pgp Description: PGP signature
Re: [PATCH 22/32] rt2x00: Allocate ring structures in single array
On Fri, 28 Apr 2006 00:03:13 +0200, Ivo van Doorn wrote: > Use seperate function to convert a dscape ring ID > to the address of the actual ring. Just minor things: > [...] > @@ -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 { > - 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? > + 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? > + > 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; > - 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? > + } > > memcpy(&ring->tx_params, params, sizeof(*params)); > Ditto for rt2500pci and rt2500usb. Thanks, -- Jiri Benc SUSE Labs - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 22/32] rt2x00: Allocate ring structures in single array
On Friday 28 April 2006 12:52, Jiri Benc wrote: > On Fri, 28 Apr 2006 00:03:13 +0200, Ivo van Doorn wrote: > > Use seperate function to convert a dscape ring ID > > to the address of the actual ring. > > Just minor things: > > > [...] > > @@ -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 { > > - 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. > > + 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. > > + > > 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; > > - 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. pgpX3vN5g2DzR.pgp Description: PGP signature
Re: [PATCH 22/32] rt2x00: Allocate ring structures in single array
On Fri, 28 Apr 2006 16:25:09 +0200, Ivo van Doorn wrote: > 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. BUG() is not a good idea, this is not critical enough to panic the kernel. -- Jiri Benc SUSE Labs - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 22/32] rt2x00: Allocate ring structures in single array
On Friday 28 April 2006 16:30, Jiri Benc wrote: > On Fri, 28 Apr 2006 16:25:09 +0200, Ivo van Doorn wrote: > > 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. > > 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. pgpHaIquBhJ92.pgp Description: PGP signature