Re: [PATCH 22/32] rt2x00: Allocate ring structures in single array

[Ivo van Doorn]

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, &reg);
> >  
> > -   if (control->queue == IEEE80211_TX_QUEUE_DATA0) {
> > -           ring = &rt2x00pci->prio;
> > -           rt2x00_set_field32(&reg, TXCSR0_KICK_PRIO, 1);
> > -   } else if (control->queue == IEEE80211_TX_QUEUE_DATA1) {
> > -           ring = &rt2x00pci->tx;
> > -           rt2x00_set_field32(&reg, TXCSR0_KICK_TX, 1);
> > -   } else if (control->queue == IEEE80211_TX_QUEUE_AFTER_BEACON) {
> > -           ring = &rt2x00pci->atim;
> > -           rt2x00_set_field32(&reg, 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