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

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

[Ivo van Doorn]

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

From: Ivo van Doorn <IvDoorn@gmail.com>

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 <IvDoorn@gmail.com>

Available on server:

http://mendiosus.nl/rt2x00/rt2x00-22-rings.diff

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

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

[Jiri Benc]

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?

> +		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

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

[Ivo van Doorn]

[-- 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 --]

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

[Jiri Benc]

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

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

[Ivo van Doorn]

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

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.

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