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

[Ivo van Doorn <ivdoorn@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3247 bytes --]

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.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]