[PATCH 3/32] rt2x00: use pci_*_consistent for DMA mapping

[PATCH 3/32] rt2x00: use pci_*_consistent for DMA mapping

[Ivo van Doorn]

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

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

Add linux/dma-mapping.h header to allow compilation
on some architectures. Instead of dma_*_coherent
use pci_*consistent functions.

Signed-off-by: Ivo van Doorn <IvDoorn@gmail.com>

diff -uprN wireless-dev-rt2x00/drivers/net/wireless/d80211/rt2x00/rt2400pci.c wireless-dev-rt2x00-patch/drivers/net/wireless/d80211/rt2x00/rt2400pci.c
--- wireless-dev-rt2x00/drivers/net/wireless/d80211/rt2x00/rt2400pci.c	2006-04-27 21:36:17.000000000 +0200
+++ wireless-dev-rt2x00-patch/drivers/net/wireless/d80211/rt2x00/rt2400pci.c	2006-04-27 21:37:02.000000000 +0200
@@ -29,6 +29,7 @@
 #include <linux/version.h>
 #include <linux/init.h>
 #include <linux/pci.h>
+#include <linux/dma-mapping.h>
 #include <linux/delay.h>
 #include <linux/skbuff.h>
 #include <linux/netdevice.h>
@@ -928,8 +929,8 @@ rt2400pci_alloc_ring(
 	/*
 	 * Allocate DMA memory for descriptor and buffer.
 	 */
-	ring->data_addr = dma_alloc_coherent(&rt2x00pci->pci_dev->dev,
-		rt2x00_get_ring_size(ring), &ring->data_dma, GFP_KERNEL);
+	ring->data_addr = pci_alloc_consistent(rt2x00pci->pci_dev,
+		rt2x00_get_ring_size(ring), &ring->data_dma);
 	if (!ring->data_addr) {
 		kfree(ring->entry);
 		return -ENOMEM;
@@ -959,7 +960,7 @@ static void
 rt2400pci_free_ring(struct rt2x00_pci *rt2x00pci, struct data_ring *ring)
 {
 	if (ring->data_addr)
-		dma_free_coherent(&rt2x00pci->pci_dev->dev,
+		pci_free_consistent(rt2x00pci->pci_dev,
 			rt2x00_get_ring_size(ring),
 			ring->data_addr, ring->data_dma);
 	ring->data_addr = NULL;
diff -uprN wireless-dev-rt2x00/drivers/net/wireless/d80211/rt2x00/rt2500pci.c wireless-dev-rt2x00-patch/drivers/net/wireless/d80211/rt2x00/rt2500pci.c
--- wireless-dev-rt2x00/drivers/net/wireless/d80211/rt2x00/rt2500pci.c	2006-04-27 21:36:17.000000000 +0200
+++ wireless-dev-rt2x00-patch/drivers/net/wireless/d80211/rt2x00/rt2500pci.c	2006-04-27 21:37:02.000000000 +0200
@@ -29,6 +29,7 @@
 #include <linux/version.h>
 #include <linux/init.h>
 #include <linux/pci.h>
+#include <linux/dma-mapping.h>
 #include <linux/delay.h>
 #include <linux/skbuff.h>
 #include <linux/netdevice.h>
@@ -964,8 +965,8 @@ rt2500pci_alloc_ring(
 	/*
 	 * Allocate DMA memory for descriptor and buffer.
 	 */
-	ring->data_addr = dma_alloc_coherent(&rt2x00pci->pci_dev->dev,
-		rt2x00_get_ring_size(ring), &ring->data_dma, GFP_KERNEL);
+	ring->data_addr = pci_alloc_consistent(rt2x00pci->pci_dev,
+		rt2x00_get_ring_size(ring), &ring->data_dma);
 	if (!ring->data_addr) {
 		kfree(ring->entry);
 		return -ENOMEM;
@@ -995,7 +996,7 @@ static void
 rt2500pci_free_ring(struct rt2x00_pci *rt2x00pci, struct data_ring *ring)
 {
 	if (ring->data_addr)
-		dma_free_coherent(&rt2x00pci->pci_dev->dev,
+		pci_free_consistent(rt2x00pci->pci_dev,
 			rt2x00_get_ring_size(ring),
 			ring->data_addr, ring->data_dma);
 	ring->data_addr = NULL;

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

Re: [PATCH 3/32] rt2x00: use pci_*_consistent for DMA mapping

[Christoph Hellwig]

On Fri, Apr 28, 2006 at 12:02:52AM +0200, Ivo van Doorn wrote:
> From: Ivo van Doorn <IvDoorn@gmail.com>
> 
> Instead of dma_*_coherent
> use pci_*consistent functions.

No point in doing that, quite reverse as you use the gfp_mask argument
which is a (small) pessimation here.

Re: [PATCH 3/32] rt2x00: use pci_*_consistent for DMA mapping

[Ivo van Doorn]

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

On Friday 28 April 2006 00:11, Christoph Hellwig wrote:
> On Fri, Apr 28, 2006 at 12:02:52AM +0200, Ivo van Doorn wrote:
> > From: Ivo van Doorn <IvDoorn@gmail.com>
> > 
> > Instead of dma_*_coherent
> > use pci_*consistent functions.
> 
> No point in doing that, quite reverse as you use the gfp_mask argument
> which is a (small) pessimation here.

Would it be recommended that I replace the usb_alloc_buffers in the
rt2570 driver with dma_*coherent?
This would get the dma rings allocation and freeing a bit more generic
between PCI and USB and could reduce some duplicate code between them.
But I am not sure if there is any major difference between the two.

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

Re: [PATCH 3/32] rt2x00: use pci_*_consistent for DMA mapping

[Christoph Hellwig]

On Fri, Apr 28, 2006 at 02:57:31PM +0200, Ivo van Doorn wrote:
> On Friday 28 April 2006 00:11, Christoph Hellwig wrote:
> > On Fri, Apr 28, 2006 at 12:02:52AM +0200, Ivo van Doorn wrote:
> > > From: Ivo van Doorn <IvDoorn@gmail.com>
> > > 
> > > Instead of dma_*_coherent
> > > use pci_*consistent functions.
> > 
> > No point in doing that, quite reverse as you use the gfp_mask argument
> > which is a (small) pessimation here.
> 
> Would it be recommended that I replace the usb_alloc_buffers in the
> rt2570 driver with dma_*coherent?
> This would get the dma rings allocation and freeing a bit more generic
> between PCI and USB and could reduce some duplicate code between them.
> But I am not sure if there is any major difference between the two.

I don't have the full code for your driver anywhere near me so I can't
comment on this.  Maybe you could send the code of 'usb_alloc_buffers'
for comments?



---end quoted text---

Re: [PATCH 3/32] rt2x00: use pci_*_consistent for DMA mapping

[Ivo van Doorn]

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

On Friday 28 April 2006 15:15, Christoph Hellwig wrote:
> On Fri, Apr 28, 2006 at 02:57:31PM +0200, Ivo van Doorn wrote:
> > On Friday 28 April 2006 00:11, Christoph Hellwig wrote:
> > > On Fri, Apr 28, 2006 at 12:02:52AM +0200, Ivo van Doorn wrote:
> > > > From: Ivo van Doorn <IvDoorn@gmail.com>
> > > > 
> > > > Instead of dma_*_coherent
> > > > use pci_*consistent functions.
> > > 
> > > No point in doing that, quite reverse as you use the gfp_mask argument
> > > which is a (small) pessimation here.
> > 
> > Would it be recommended that I replace the usb_alloc_buffers in the
> > rt2570 driver with dma_*coherent?
> > This would get the dma rings allocation and freeing a bit more generic
> > between PCI and USB and could reduce some duplicate code between them.
> > But I am not sure if there is any major difference between the two.
> 
> I don't have the full code for your driver anywhere near me so I can't
> comment on this.  Maybe you could send the code of 'usb_alloc_buffers'
> for comments?

Below is the function of the rt2500usb_alloc_ring, it works very similar
to the pci equivalent, except that urbs are allocated and usb_buffer_alloc is used.
The data_ring structure where the information is stored in is also the same as
used in the pci driver.

Basicly what I want to do is move the urbs to somewhere else,
the data_ring structure has been very generic, and the only real difference
between the pci and usb version of the data_entry has been the urb structure pointer.
If the usb_buffer_* calls could be replaced by dma_*coherent a lot
of duplicate code in the pci and usb drivers could be moved into
a seperate header or source file.

struct data_ring{
	/*
	 * net_device where this ring belongs to.
	 */
	struct net_device			*net_dev;

	/*
	 * Work structure for bottom half interrupt handling.
	 */
	struct work_struct			irq_work;

	/*
	 * Base address for the device specific data entries.
	 */
	void					*entry;

	/*
	 * TX queue statistic info.
	 */
	struct ieee80211_tx_queue_stats_data	stats;

	/*
	 * TX Queue parameters.
	 */
	struct ieee80211_tx_queue_params	tx_params;

	/*
	 * Base address for data ring.
	 */
	dma_addr_t				data_dma;
	void					*data_addr;

	/*
	 * Index variables.
	 */
	u8					index;
	u8					index_done;

	/*
	 * Size of device specific data entry structure.
	 */
	u16					entry_size;

	/*
	 * Size of packet and descriptor in bytes.
	 */
	u16					data_size;
	u16					desc_size;
} __attribute__ ((packed));

static int
rt2500usb_alloc_ring(
	struct rt2x00_usb *rt2x00usb,
	struct data_ring *ring,
	void (*handler)(void *),
	const u16 max_entries,
	const u16 data_size,
	const u16 desc_size)
{
	struct data_entry	*entry;
	u8			counter;
	int			status;

	/*
	 *Set device structure.
	 */
	ring->net_dev = usb_get_intfdata(rt2x00usb->usb_intf);

	/*
	 * Initialize work structure for deferred work.
	 */
	INIT_WORK(&ring->irq_work, handler, ring);

	/*
	 * Initialize ring parameters.
	 */
	ring->tx_params.aifs = 2;
	ring->tx_params.cw_min = 5;	/* cw_min: 2^5 = 32. */
	ring->tx_params.cw_max = 10;	/* cw_max: 2^10 = 1024. */

	rt2x00_ring_index_clear(ring);

	ring->stats.limit = max_entries;
	ring->entry_size = sizeof(struct data_entry);
	ring->data_size = data_size;
	ring->desc_size = desc_size;

	/*
	 * Allocate all ring entries.
	 */
	ring->entry = kmalloc(ring->stats.limit * ring->entry_size,
		GFP_KERNEL);
	if (!ring->entry)
		return -ENOMEM;

	/*
	 * Allocate DMA memory for descriptor and buffer.
	 */
	ring->data_addr = usb_buffer_alloc(
		interface_to_usbdev(rt2x00usb->usb_intf),
		rt2x00_get_ring_size(ring), GFP_KERNEL, &ring->data_dma);
	if (!ring->data_addr) {
		kfree(ring->entry);
		return -ENOMEM;
	}

	/*
	 * Initialize all ring entries to contain valid
	 * addresses.
	 */
	status = 0;
	entry = ring->entry;
	for (counter = 0; counter < ring->stats.limit; counter++) {
		entry[counter].ring = ring;
		if (!status)
			entry[counter].urb = usb_alloc_urb(0, GFP_KERNEL);
		else
			entry[counter].urb = NULL;
		if (!entry[counter].urb)
			status = -ENOMEM;
		entry[counter].skb = NULL;
		entry[counter].data_addr = ring->data_addr
			+ (counter * ring->desc_size)
			+ (counter * ring->data_size);
		entry[counter].data_dma = ring->data_dma
			+ (counter * ring->desc_size)
			+ (counter * ring->data_size);
	}

	return status;
}

static void
rt2500usb_free_ring(struct rt2x00_usb *rt2x00usb, struct data_ring *ring)
{
	struct	data_entry	*entry;
	u8			counter;

	if (!ring->entry)
		goto exit;

	entry = ring->entry;
	for (counter = 0; counter < ring->stats.limit; counter++) {
		usb_kill_urb(entry[counter].urb);
		usb_free_urb(entry[counter].urb);
	}

	kfree(ring->entry);
	ring->entry = NULL;

exit:
	if (ring->data_addr)
		usb_buffer_free(
			interface_to_usbdev(rt2x00usb->usb_intf),
			rt2x00_get_ring_size(ring), ring->data_addr,
			ring->data_dma);
	ring->data_addr = NULL;
}

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

Re: [PATCH 3/32] rt2x00: use pci_*_consistent for DMA mapping

[Michael Buesch]

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

On Friday 28 April 2006 15:24, you wrote:
> struct data_ring{
> 	/*
> 	 * net_device where this ring belongs to.
> 	 */
> 	struct net_device			*net_dev;
> 
> 	/*
> 	 * Work structure for bottom half interrupt handling.
> 	 */
> 	struct work_struct			irq_work;
> 
> 	/*
> 	 * Base address for the device specific data entries.
> 	 */
> 	void					*entry;
> 
> 	/*
> 	 * TX queue statistic info.
> 	 */
> 	struct ieee80211_tx_queue_stats_data	stats;
> 
> 	/*
> 	 * TX Queue parameters.
> 	 */
> 	struct ieee80211_tx_queue_params	tx_params;
> 
> 	/*
> 	 * Base address for data ring.
> 	 */
> 	dma_addr_t				data_dma;
> 	void					*data_addr;
> 
> 	/*
> 	 * Index variables.
> 	 */
> 	u8					index;
> 	u8					index_done;
> 
> 	/*
> 	 * Size of device specific data entry structure.
> 	 */
> 	u16					entry_size;
> 
> 	/*
> 	 * Size of packet and descriptor in bytes.
> 	 */
> 	u16					data_size;
> 	u16					desc_size;
> } __attribute__ ((packed));

Why is this packed? I don't believe you write such a data
structure (which contains dscape specific structs) to some
device registers.

-- 
Greetings Michael.

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

Re: [PATCH 3/32] rt2x00: use pci_*_consistent for DMA mapping

[Ivo van Doorn]

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

On Friday 28 April 2006 15:33, Michael Buesch wrote:
> On Friday 28 April 2006 15:24, you wrote:
> > struct data_ring{
> > 	/*
> > 	 * net_device where this ring belongs to.
> > 	 */
> > 	struct net_device			*net_dev;
> > 
> > 	/*
> > 	 * Work structure for bottom half interrupt handling.
> > 	 */
> > 	struct work_struct			irq_work;
> > 
> > 	/*
> > 	 * Base address for the device specific data entries.
> > 	 */
> > 	void					*entry;
> > 
> > 	/*
> > 	 * TX queue statistic info.
> > 	 */
> > 	struct ieee80211_tx_queue_stats_data	stats;
> > 
> > 	/*
> > 	 * TX Queue parameters.
> > 	 */
> > 	struct ieee80211_tx_queue_params	tx_params;
> > 
> > 	/*
> > 	 * Base address for data ring.
> > 	 */
> > 	dma_addr_t				data_dma;
> > 	void					*data_addr;
> > 
> > 	/*
> > 	 * Index variables.
> > 	 */
> > 	u8					index;
> > 	u8					index_done;
> > 
> > 	/*
> > 	 * Size of device specific data entry structure.
> > 	 */
> > 	u16					entry_size;
> > 
> > 	/*
> > 	 * Size of packet and descriptor in bytes.
> > 	 */
> > 	u16					data_size;
> > 	u16					desc_size;
> > } __attribute__ ((packed));
> 
> Why is this packed? I don't believe you write such a data
> structure (which contains dscape specific structs) to some
> device registers.

I can't remember actually, I believe this came from a patch we had received
last year with the legacy drivers from a user. 
Since then most structures have received the __attribute__ ((packed)) tag.
I'll create a patch to remove the tags where they are not needed.

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