Skip to contentSkip to conversation

ssb, b43, b43legacy, b44: Rewrite SSB DMA API

PATCH In review Michael Buesch 4 participants 9 messages Jun 20, 2008 – Jul 8, 2008

Subsystems (3)
Network drivers Networking drivers (wireless) Sonics silicon backplane driver (ssb)
lore
Share & tools
lore

Michael Buesch

author
#0 lore raw

PATCH 1/1 ssb, b43, b43legacy, b44: Rewrite SSB DMA API

This is a rewrite of the DMA API for SSB devices.
This is needed, because the old (non-existing) "API" made too many bad assumptions on the API of the host-bus (PCI).
This introduces an almost complete SSB-DMA-API that maps to the lowlevel bus-API based on the bustype.

Signed-off-by Michael Buesch <[email protected]>

---

John, please merge for 2.6.27
This is one huge patch, because it's not really possible to split this without breaking bisect.

drivers/net/b44.c +71 -69
Index: wireless-testing/drivers/net/b44.c
===================================================================
@@ -145,25 +145,25 @@ B44_STAT_REG_DECLARE
 
 static inline void b44_sync_dma_desc_for_device(struct ssb_device *sdev,
 						dma_addr_t dma_base,
 						unsigned long offset,
 						enum dma_data_direction dir)
 {
-	dma_sync_single_range_for_device(sdev->dma_dev, dma_base,
-					 offset & dma_desc_align_mask,
-					 dma_desc_sync_size, dir);
+	ssb_dma_sync_single_range_for_device(sdev, dma_base,
+					     offset & dma_desc_align_mask,
+					     dma_desc_sync_size, dir);
 }
 
 static inline void b44_sync_dma_desc_for_cpu(struct ssb_device *sdev,
 					     dma_addr_t dma_base,
 					     unsigned long offset,
 					     enum dma_data_direction dir)
 {
-	dma_sync_single_range_for_cpu(sdev->dma_dev, dma_base,
-				      offset & dma_desc_align_mask,
-				      dma_desc_sync_size, dir);
+	ssb_dma_sync_single_range_for_cpu(sdev, dma_base,
+					  offset & dma_desc_align_mask,
+					  dma_desc_sync_size, dir);
 }
 
 static inline unsigned long br32(const struct b44 *bp, unsigned long reg)
 {
 	return ssb_read32(bp->sdev, reg);
 }

@@ -610,16 +610,16 @@ static void b44_tx(struct b44 *bp)
 	for (cons = bp->tx_cons; cons != cur; cons = NEXT_TX(cons)) {
 		struct ring_info *rp = &bp->tx_buffers[cons];
 		struct sk_buff *skb = rp->skb;
 
 		BUG_ON(skb == NULL);
 
-		dma_unmap_single(bp->sdev->dma_dev,
-				 rp->mapping,
-				 skb->len,
-				 DMA_TO_DEVICE);
+		ssb_dma_unmap_single(bp->sdev,
+				     rp->mapping,
+				     skb->len,
+				     DMA_TO_DEVICE);
 		rp->skb = NULL;
 		dev_kfree_skb_irq(skb);
 	}
 
 	bp->tx_cons = cons;
 	if (netif_queue_stopped(bp->dev) &&

@@ -650,35 +650,35 @@ static int b44_alloc_rx_skb(struct b44 *
 	dest_idx = dest_idx_unmasked & (B44_RX_RING_SIZE - 1);
 	map = &bp->rx_buffers[dest_idx];
 	skb = netdev_alloc_skb(bp->dev, RX_PKT_BUF_SZ);
 	if (skb == NULL)
 		return -ENOMEM;
 
-	mapping = dma_map_single(bp->sdev->dma_dev, skb->data,
-				 RX_PKT_BUF_SZ,
-				 DMA_FROM_DEVICE);
+	mapping = ssb_dma_map_single(bp->sdev, skb->data,
+				     RX_PKT_BUF_SZ,
+				     DMA_FROM_DEVICE);
 
 	/* Hardware bug work-around, the chip is unable to do PCI DMA
 	   to/from anything above 1GB :-( */
-	if (dma_mapping_error(mapping) ||
+	if (ssb_dma_mapping_error(bp->sdev, mapping) ||
 		mapping + RX_PKT_BUF_SZ > DMA_30BIT_MASK) {
 		/* Sigh... */
-		if (!dma_mapping_error(mapping))
-			dma_unmap_single(bp->sdev->dma_dev, mapping,
-					RX_PKT_BUF_SZ, DMA_FROM_DEVICE);
+		if (!ssb_dma_mapping_error(bp->sdev, mapping))
+			ssb_dma_unmap_single(bp->sdev, mapping,
+					     RX_PKT_BUF_SZ, DMA_FROM_DEVICE);
 		dev_kfree_skb_any(skb);
 		skb = __netdev_alloc_skb(bp->dev, RX_PKT_BUF_SZ, GFP_ATOMIC|GFP_DMA);
 		if (skb == NULL)
 			return -ENOMEM;
-		mapping = dma_map_single(bp->sdev->dma_dev, skb->data,
-					 RX_PKT_BUF_SZ,
-					 DMA_FROM_DEVICE);
-		if (dma_mapping_error(mapping) ||
+		mapping = ssb_dma_map_single(bp->sdev, skb->data,
+					     RX_PKT_BUF_SZ,
+					     DMA_FROM_DEVICE);
+		if (ssb_dma_mapping_error(bp->sdev, mapping) ||
 			mapping + RX_PKT_BUF_SZ > DMA_30BIT_MASK) {
-			if (!dma_mapping_error(mapping))
-				dma_unmap_single(bp->sdev->dma_dev, mapping, RX_PKT_BUF_SZ,DMA_FROM_DEVICE);
+			if (!ssb_dma_mapping_error(bp->sdev, mapping))
+				ssb_dma_unmap_single(bp->sdev, mapping, RX_PKT_BUF_SZ,DMA_FROM_DEVICE);
 			dev_kfree_skb_any(skb);
 			return -ENOMEM;
 		}
 	}
 
 	rh = (struct rx_header *) skb->data;

@@ -747,15 +747,15 @@ static void b44_recycle_rx(struct b44 *b
 
 	if (bp->flags & B44_FLAG_RX_RING_HACK)
 		b44_sync_dma_desc_for_device(bp->sdev, bp->rx_ring_dma,
 					     dest_idx * sizeof(dest_desc),
 					     DMA_BIDIRECTIONAL);
 
-	dma_sync_single_for_device(bp->sdev->dma_dev, le32_to_cpu(src_desc->addr),
-				   RX_PKT_BUF_SZ,
-				   DMA_FROM_DEVICE);
+	ssb_dma_sync_single_for_device(bp->sdev, le32_to_cpu(src_desc->addr),
+				       RX_PKT_BUF_SZ,
+				       DMA_FROM_DEVICE);
 }
 
 static int b44_rx(struct b44 *bp, int budget)
 {
 	int received;
 	u32 cons, prod;

@@ -769,13 +769,13 @@ static int b44_rx(struct b44 *bp, int bu
 		struct ring_info *rp = &bp->rx_buffers[cons];
 		struct sk_buff *skb = rp->skb;
 		dma_addr_t map = rp->mapping;
 		struct rx_header *rh;
 		u16 len;
 
-		dma_sync_single_for_cpu(bp->sdev->dma_dev, map,
+		ssb_dma_sync_single_for_cpu(bp->sdev, map,
 					    RX_PKT_BUF_SZ,
 					    DMA_FROM_DEVICE);
 		rh = (struct rx_header *) skb->data;
 		len = le16_to_cpu(rh->len);
 		if ((len > (RX_PKT_BUF_SZ - RX_PKT_OFFSET)) ||
 		    (rh->flags & cpu_to_le16(RX_FLAG_ERRORS))) {

@@ -803,14 +803,14 @@ static int b44_rx(struct b44 *bp, int bu
 
 		if (len > RX_COPY_THRESHOLD) {
 			int skb_size;
 			skb_size = b44_alloc_rx_skb(bp, cons, bp->rx_prod);
 			if (skb_size < 0)
 				goto drop_it;
-			dma_unmap_single(bp->sdev->dma_dev, map,
-					 skb_size, DMA_FROM_DEVICE);
+			ssb_dma_unmap_single(bp->sdev, map,
+					     skb_size, DMA_FROM_DEVICE);
 			/* Leave out rx_header */
                 	skb_put(skb, len + RX_PKT_OFFSET);
             	        skb_pull(skb, RX_PKT_OFFSET);
 		} else {
 			struct sk_buff *copy_skb;
 

@@ -963,31 +963,31 @@ static int b44_start_xmit(struct sk_buff
 		netif_stop_queue(dev);
 		printk(KERN_ERR PFX "%s: BUG! Tx Ring full when queue awake!\n",
 		       dev->name);
 		goto err_out;
 	}
 
-	mapping = dma_map_single(bp->sdev->dma_dev, skb->data, len, DMA_TO_DEVICE);
-	if (dma_mapping_error(mapping) || mapping + len > DMA_30BIT_MASK) {
+	mapping = ssb_dma_map_single(bp->sdev, skb->data, len, DMA_TO_DEVICE);
+	if (ssb_dma_mapping_error(bp->sdev, mapping) || mapping + len > DMA_30BIT_MASK) {
 		struct sk_buff *bounce_skb;
 
 		/* Chip can't handle DMA to/from >1GB, use bounce buffer */
-		if (!dma_mapping_error(mapping))
-			dma_unmap_single(bp->sdev->dma_dev, mapping, len,
-					DMA_TO_DEVICE);
+		if (!ssb_dma_mapping_error(bp->sdev, mapping))
+			ssb_dma_unmap_single(bp->sdev, mapping, len,
+					     DMA_TO_DEVICE);
 
 		bounce_skb = __dev_alloc_skb(len, GFP_ATOMIC | GFP_DMA);
 		if (!bounce_skb)
 			goto err_out;
 
-		mapping = dma_map_single(bp->sdev->dma_dev, bounce_skb->data,
-					 len, DMA_TO_DEVICE);
-		if (dma_mapping_error(mapping) || mapping + len > DMA_30BIT_MASK) {
-			if (!dma_mapping_error(mapping))
-				dma_unmap_single(bp->sdev->dma_dev, mapping,
-					 len, DMA_TO_DEVICE);
+		mapping = ssb_dma_map_single(bp->sdev, bounce_skb->data,
+					     len, DMA_TO_DEVICE);
+		if (ssb_dma_mapping_error(bp->sdev, mapping) || mapping + len > DMA_30BIT_MASK) {
+			if (!ssb_dma_mapping_error(bp->sdev, mapping))
+				ssb_dma_unmap_single(bp->sdev, mapping,
+						     len, DMA_TO_DEVICE);
 			dev_kfree_skb_any(bounce_skb);
 			goto err_out;
 		}
 
 		skb_copy_from_linear_data(skb, skb_put(bounce_skb, len), len);
 		dev_kfree_skb_any(skb);

@@ -1079,26 +1079,26 @@ static void b44_free_rings(struct b44 *b
 
 	for (i = 0; i < B44_RX_RING_SIZE; i++) {
 		rp = &bp->rx_buffers[i];
 
 		if (rp->skb == NULL)
 			continue;
-		dma_unmap_single(bp->sdev->dma_dev, rp->mapping, RX_PKT_BUF_SZ,
-					DMA_FROM_DEVICE);
+		ssb_dma_unmap_single(bp->sdev, rp->mapping, RX_PKT_BUF_SZ,
+				     DMA_FROM_DEVICE);
 		dev_kfree_skb_any(rp->skb);
 		rp->skb = NULL;
 	}
 
 	/* XXX needs changes once NETIF_F_SG is set... */
 	for (i = 0; i < B44_TX_RING_SIZE; i++) {
 		rp = &bp->tx_buffers[i];
 
 		if (rp->skb == NULL)
 			continue;
-		dma_unmap_single(bp->sdev->dma_dev, rp->mapping, rp->skb->len,
-					DMA_TO_DEVICE);
+		ssb_dma_unmap_single(bp->sdev, rp->mapping, rp->skb->len,
+				     DMA_TO_DEVICE);
 		dev_kfree_skb_any(rp->skb);
 		rp->skb = NULL;
 	}
 }
 
 /* Initialize tx/rx rings for packet processing.

@@ -1114,20 +1114,20 @@ static void b44_init_rings(struct b44 *b
 	b44_free_rings(bp);
 
 	memset(bp->rx_ring, 0, B44_RX_RING_BYTES);
 	memset(bp->tx_ring, 0, B44_TX_RING_BYTES);
 
 	if (bp->flags & B44_FLAG_RX_RING_HACK)
-		dma_sync_single_for_device(bp->sdev->dma_dev, bp->rx_ring_dma,
-			                  DMA_TABLE_BYTES,
-			                  DMA_BIDIRECTIONAL);
+		ssb_dma_sync_single_for_device(bp->sdev, bp->rx_ring_dma,
+					       DMA_TABLE_BYTES,
+					       DMA_BIDIRECTIONAL);
 
 	if (bp->flags & B44_FLAG_TX_RING_HACK)
-		dma_sync_single_for_device(bp->sdev->dma_dev, bp->tx_ring_dma,
-			                  DMA_TABLE_BYTES,
-			                  DMA_TO_DEVICE);
+		ssb_dma_sync_single_for_device(bp->sdev, bp->tx_ring_dma,
+					       DMA_TABLE_BYTES,
+					       DMA_TO_DEVICE);
 
 	for (i = 0; i < bp->rx_pending; i++) {
 		if (b44_alloc_rx_skb(bp, -1, i) < 0)
 			break;
 	}
 }

@@ -1141,31 +1141,33 @@ static void b44_free_consistent(struct b
 	kfree(bp->rx_buffers);
 	bp->rx_buffers = NULL;
 	kfree(bp->tx_buffers);
 	bp->tx_buffers = NULL;
 	if (bp->rx_ring) {
 		if (bp->flags & B44_FLAG_RX_RING_HACK) {
-			dma_unmap_single(bp->sdev->dma_dev, bp->rx_ring_dma,
-					DMA_TABLE_BYTES,
-					DMA_BIDIRECTIONAL);
+			ssb_dma_unmap_single(bp->sdev, bp->rx_ring_dma,
+					     DMA_TABLE_BYTES,
+					     DMA_BIDIRECTIONAL);
 			kfree(bp->rx_ring);
 		} else
-			dma_free_coherent(bp->sdev->dma_dev, DMA_TABLE_BYTES,
-					    bp->rx_ring, bp->rx_ring_dma);
+			ssb_dma_free_consistent(bp->sdev, DMA_TABLE_BYTES,
+						bp->rx_ring, bp->rx_ring_dma,
+						GFP_KERNEL);
 		bp->rx_ring = NULL;
 		bp->flags &= ~B44_FLAG_RX_RING_HACK;
 	}
 	if (bp->tx_ring) {
 		if (bp->flags & B44_FLAG_TX_RING_HACK) {
-			dma_unmap_single(bp->sdev->dma_dev, bp->tx_ring_dma,
-					DMA_TABLE_BYTES,
-					DMA_TO_DEVICE);
+			ssb_dma_unmap_single(bp->sdev, bp->tx_ring_dma,
+					     DMA_TABLE_BYTES,
+					     DMA_TO_DEVICE);
 			kfree(bp->tx_ring);
 		} else
-			dma_free_coherent(bp->sdev->dma_dev, DMA_TABLE_BYTES,
-					    bp->tx_ring, bp->tx_ring_dma);
+			ssb_dma_free_consistent(bp->sdev, DMA_TABLE_BYTES,
+						bp->tx_ring, bp->tx_ring_dma,
+						GFP_KERNEL);
 		bp->tx_ring = NULL;
 		bp->flags &= ~B44_FLAG_TX_RING_HACK;
 	}
 }
 
 /*

@@ -1184,56 +1186,56 @@ static int b44_alloc_consistent(struct b
 	size = B44_TX_RING_SIZE * sizeof(struct ring_info);
 	bp->tx_buffers = kzalloc(size, gfp);
 	if (!bp->tx_buffers)
 		goto out_err;
 
 	size = DMA_TABLE_BYTES;
-	bp->rx_ring = dma_alloc_coherent(bp->sdev->dma_dev, size, &bp->rx_ring_dma, gfp);
+	bp->rx_ring = ssb_dma_alloc_consistent(bp->sdev, size, &bp->rx_ring_dma, gfp);
 	if (!bp->rx_ring) {
 		/* Allocation may have failed due to pci_alloc_consistent
 		   insisting on use of GFP_DMA, which is more restrictive
 		   than necessary...  */
 		struct dma_desc *rx_ring;
 		dma_addr_t rx_ring_dma;
 
 		rx_ring = kzalloc(size, gfp);
 		if (!rx_ring)
 			goto out_err;
 
-		rx_ring_dma = dma_map_single(bp->sdev->dma_dev, rx_ring,
-			                    DMA_TABLE_BYTES,
-			                    DMA_BIDIRECTIONAL);
+		rx_ring_dma = ssb_dma_map_single(bp->sdev, rx_ring,
+						 DMA_TABLE_BYTES,
+						 DMA_BIDIRECTIONAL);
 
-		if (dma_mapping_error(rx_ring_dma) ||
+		if (ssb_dma_mapping_error(bp->sdev, rx_ring_dma) ||
 			rx_ring_dma + size > DMA_30BIT_MASK) {
 			kfree(rx_ring);
 			goto out_err;
 		}
 
 		bp->rx_ring = rx_ring;
 		bp->rx_ring_dma = rx_ring_dma;
 		bp->flags |= B44_FLAG_RX_RING_HACK;
 	}
 
-	bp->tx_ring = dma_alloc_coherent(bp->sdev->dma_dev, size, &bp->tx_ring_dma, gfp);
+	bp->tx_ring = ssb_dma_alloc_consistent(bp->sdev, size, &bp->tx_ring_dma, gfp);
 	if (!bp->tx_ring) {
-		/* Allocation may have failed due to dma_alloc_coherent
+		/* Allocation may have failed due to ssb_dma_alloc_consistent
 		   insisting on use of GFP_DMA, which is more restrictive
 		   than necessary...  */
 		struct dma_desc *tx_ring;
 		dma_addr_t tx_ring_dma;
 
 		tx_ring = kzalloc(size, gfp);
 		if (!tx_ring)
 			goto out_err;
 
-		tx_ring_dma = dma_map_single(bp->sdev->dma_dev, tx_ring,
+		tx_ring_dma = ssb_dma_map_single(bp->sdev, tx_ring,
 			                    DMA_TABLE_BYTES,
 			                    DMA_TO_DEVICE);
 
-		if (dma_mapping_error(tx_ring_dma) ||
+		if (ssb_dma_mapping_error(bp->sdev, tx_ring_dma) ||
 			tx_ring_dma + size > DMA_30BIT_MASK) {
 			kfree(tx_ring);
 			goto out_err;
 		}
 
 		bp->tx_ring = tx_ring;
drivers/net/wireless/b43/dma.c +35 -30

186 lines not on this page · raw message

drivers/net/wireless/b43legacy/dma.c +31 -32

174 lines not on this page · raw message

drivers/ssb/Kconfig +1 -1
Index: wireless-testing/drivers/ssb/Kconfig
===================================================================
@@ -1,11 +1,11 @@
 menu "Sonics Silicon Backplane"
 
 config SSB_POSSIBLE
 	bool
-	depends on HAS_IOMEM
+	depends on HAS_IOMEM && HAS_DMA
 	default y
 
 config SSB
 	tristate "Sonics Silicon Backplane support"
 	depends on SSB_POSSIBLE
 	help
drivers/ssb/main.c +59 -16

125 lines not on this page · raw message

include/linux/ssb/ssb.h +139 -4

173 lines not on this page · raw message

author
#1 lore raw
Quoted 6 lines · On Friday 20 June 2008 11:50:29 Michael Buesch wrote:

This is a rewrite of the DMA API for SSB devices.
This is needed, because the old (non-existing) "API" made too many bad assumptions on the API of the host-bus (PCI).
This introduces an almost complete SSB-DMA-API that maps to the lowlevel bus-API based on the bustype.

Quoted 7 lines
Signed-off-by Michael Buesch <[email protected]>

---

John, please merge for 2.6.27
This is one huge patch, because it's not really possible to split this without breaking bisect.

Note that this patch will slightly clash with another DMA update that's going into linux-next.
This is trivial to resolve, however. wiggle will probably be able to resolve most, if not all, of them.

drivers/net/wireless/b43/dma.c static int b43_dmacontroller_tx_reset(st · line 527

Quoted 7 lines

static bool b43_dma_mapping_error(struct b43_dmaring *ring,

drivers/net/wireless/b43/dma.c +1 -1
 				  dma_addr_t addr,
 				  size_t buffersize, bool dma_to_device)
 {
-	if (unlikely(dma_mapping_error(addr)))
+	if (unlikely(ssb_dma_mapping_error(ring->dev->dev, addr)))
 		return 1;

These calls will clash. The patch in linux-next introduced an additional dev parameter to dma_mapping_error.
The fix is obvious. Remove the clashing line and add the ssb_dma_mapping_error call exactly as shown above.

include/linux/ssb/ssb.h static inline void ssb_block_write(struc · line 436

Quoted 12 lines
include/linux/ssb/ssb.h +12 -0
+static inline int ssb_dma_mapping_error(struct ssb_device *dev, dma_addr_t addr)
+{
+	switch (dev->bus->bustype) {
+	case SSB_BUSTYPE_PCI:
+		return pci_dma_mapping_error(addr);
+	case SSB_BUSTYPE_SSB:
+		return dma_mapping_error(addr);
+	default:
+		__ssb_dma_not_implemented(dev);
+	}
+	return -ENOSYS;
+}

The patch in linux-next will require a dev parameter to the lowlevel functions. Simply do it this way:

pci_dma_mapping_error(dev->bus->host_pci, addr);

and

dma_mapping_error(dev->dev, addr);

Signature · 1 line
Greetings Michael.
#2 lore raw
In reply to Michael Buesch
Quoted 8 lines · Michael Buesch

This is a rewrite of the DMA API for SSB devices.
This is needed, because the old (non-existing) "API" made too many bad assumptions on the API of the host-bus (PCI).
This introduces an almost complete SSB-DMA-API that maps to the lowlevel bus-API based on the bustype.

Signed-off-by Michael Buesch <[email protected]>

Is there a pre-requisite for this patch? I tried to apply it to wireless-testing with version v2.6.26-rc6-10526-ga0d3022. Hunk 4 of the patch for drivers/net/wireless/b43legacy/dma.c failed due to a line missing from the original source:

Review struct b43legacy_dmaring *b43legacy_setu

Quoted 1 line
(quoted diff) +0 -0
@@ -891,39 +890,39 @@ struct b43legacy_dmaring *b43legacy_setu

--snip--

drivers/net/wireless/b43legacy/dma.c struct b43legacy_dmaring *b43legacy_setu · line 924

Quoted 5 lines
drivers/net/wireless/b43legacy/dma.c +3 -0
+		ssb_dma_unmap_single(dev->dev, dma_test,
+				     sizeof(struct b43legacy_txhdr_fw3),
+				     DMA_TO_DEVICE);
 	}
 

	ring->dev = dev;     <-------------- This line missing.

drivers/net/wireless/b43legacy/dma.c struct b43legacy_dmaring *b43legacy_setu · line 928

Quoted 4 lines
drivers/net/wireless/b43legacy/dma.c +0 -0
 	ring->nr_slots = nr_slots;
 	ring->mmio_base = b43legacy_dmacontroller_base(type, controller_index);
 	ring->index = controller_index;
 	if (type == B43legacy_DMA_64BIT)

Larry

author
#3 lore raw
Quoted 1 line · On Saturday 21 June 2008 02:05:38 Larry Finger wrote:

Is there a pre-requisite for this patch?

[PATCH] b43legacy: Fix possible NULL pointer dereference in DMA code

Signature · 1 line
Greetings Michael.
#4 lore raw
In reply to Michael Buesch

Hi, I have applied this patch and i tested on a wl500gpv2 (BCM5354). The following part disable the ethernet device. If this part of the patch is restored the device work ok.

Quoted 32 lines
drivers/ssb/main.c +11 -12
-int ssb_dma_set_mask(struct ssb_device *ssb_dev, u64 mask)
+int ssb_dma_set_mask(struct ssb_device *dev, u64 mask)
 {
-	struct device *dma_dev = ssb_dev->dma_dev;
-	int err = 0;
+	int err;
 
-#ifdef CONFIG_SSB_PCIHOST
-	if (ssb_dev->bus->bustype == SSB_BUSTYPE_PCI) {
-		err = pci_set_dma_mask(ssb_dev->bus->host_pci, mask);
+	switch (dev->bus->bustype) {
+	case SSB_BUSTYPE_PCI:
+		err = pci_set_dma_mask(dev->bus->host_pci, mask);
 		if (err)
 			return err;
-		err = pci_set_consistent_dma_mask(ssb_dev->bus->host_pci, mask);
+		err = pci_set_consistent_dma_mask(dev->bus->host_pci, mask);
 		return err;
+	case SSB_BUSTYPE_SSB:
+		return dma_set_mask(dev->dev, mask);
+	default:
+		__ssb_dma_not_implemented(dev);
 	}
-#endif
-	dma_dev->coherent_dma_mask = mask;
-	dma_dev->dma_mask = &dma_dev->coherent_dma_mask;
-
-	return err;
+	return -ENOSYS;
 }
 EXPORT_SYMBOL(ssb_dma_set_mask);
author
#5 lore raw
Quoted 3 lines · On Thursday 03 July 2008 18:30:18 Felipe Maya wrote:

Hi, I have applied this patch and i tested on a wl500gpv2 (BCM5354). The following part disable the ethernet device. If this part of the patch is restored the device work ok.

Well, I would say your architecture is pretty broken then. This patch replaces an incorrect manual DMA mask setting by a call to the official API.
Please report this to your archtitecture maintainer.

Quoted 35 lines
Quoted 31 lines
drivers/ssb/main.c +11 -12
-int ssb_dma_set_mask(struct ssb_device *ssb_dev, u64 mask)
+int ssb_dma_set_mask(struct ssb_device *dev, u64 mask)
 {
-	struct device *dma_dev = ssb_dev->dma_dev;
-	int err = 0;
+	int err;
 
-#ifdef CONFIG_SSB_PCIHOST
-	if (ssb_dev->bus->bustype == SSB_BUSTYPE_PCI) {
-		err = pci_set_dma_mask(ssb_dev->bus->host_pci, mask);
+	switch (dev->bus->bustype) {
+	case SSB_BUSTYPE_PCI:
+		err = pci_set_dma_mask(dev->bus->host_pci, mask);
 		if (err)
 			return err;
-		err = pci_set_consistent_dma_mask(ssb_dev->bus->host_pci, mask);
+		err = pci_set_consistent_dma_mask(dev->bus->host_pci, mask);
 		return err;
+	case SSB_BUSTYPE_SSB:
+		return dma_set_mask(dev->dev, mask);
+	default:
+		__ssb_dma_not_implemented(dev);
 	}
-#endif
-	dma_dev->coherent_dma_mask = mask;
-	dma_dev->dma_mask = &dma_dev->coherent_dma_mask;
-
-	return err;
+	return -ENOSYS;
 }
 EXPORT_SYMBOL(ssb_dma_set_mask);
Signature · 1 line
Greetings Michael.
author
#6 lore raw
Quoted 10 lines · On Thursday 03 July 2008 22:52:35 Michael Buesch wrote:
Quoted 3 lines · On Thursday 03 July 2008 18:30:18 Felipe Maya wrote:

Hi, I have applied this patch and i tested on a wl500gpv2 (BCM5354). The following part disable the ethernet device. If this part of the patch is restored the device work ok.

Well, I would say your architecture is pretty broken then. This patch replaces an incorrect manual DMA mask setting by a call to the official API.
Please report this to your archtitecture maintainer.

Ah and btw, the bug most likely is that dma_set_mask does not set the dev->coherent_dma_mask or something like that
while the dma remapping function require it to be set (I could be wrong, this is just guesswork). In any case, I think the bug really is in the arch DMA implementation and we must not workaround that in the ssb code. So I will reject a patch that adds the

	dev->coherent_dma_mask = mask;
	dma_dev->dma_mask = &dma_dev->coherent_dma_mask;

stuff back. dma_set_mask should set all required masks for the dma_... remapping functions. If it doesn't, it's either a bug in dma_set_mask or in the remapping functions using the wrong mask.
We need to fix it correctly this time, as the whole purpose of my SSB patch was the get the DMA remapping _right_ so it doesn't break every two months if somebody changes some arch internal assumption. So please do not to send me any SSB patch that works around the bug. ;)

Quoted 39 lines
Quoted 36 lines
Quoted 32 lines
drivers/ssb/main.c +11 -12
-int ssb_dma_set_mask(struct ssb_device *ssb_dev, u64 mask)
+int ssb_dma_set_mask(struct ssb_device *dev, u64 mask)
 {
-	struct device *dma_dev = ssb_dev->dma_dev;
-	int err = 0;
+	int err;
 
-#ifdef CONFIG_SSB_PCIHOST
-	if (ssb_dev->bus->bustype == SSB_BUSTYPE_PCI) {
-		err = pci_set_dma_mask(ssb_dev->bus->host_pci, mask);
+	switch (dev->bus->bustype) {
+	case SSB_BUSTYPE_PCI:
+		err = pci_set_dma_mask(dev->bus->host_pci, mask);
 		if (err)
 			return err;
-		err = pci_set_consistent_dma_mask(ssb_dev->bus->host_pci, mask);
+		err = pci_set_consistent_dma_mask(dev->bus->host_pci, mask);
 		return err;
+	case SSB_BUSTYPE_SSB:
+		return dma_set_mask(dev->dev, mask);
+	default:
+		__ssb_dma_not_implemented(dev);
 	}
-#endif
-	dma_dev->coherent_dma_mask = mask;
-	dma_dev->dma_mask = &dma_dev->coherent_dma_mask;
-
-	return err;
+	return -ENOSYS;
 }
 EXPORT_SYMBOL(ssb_dma_set_mask);
Signature · 1 line
Greetings Michael.
#7 lore raw
In reply to Michael Buesch
Quoted 8 lines · Michael Buesch
Quoted 4 lines · On Thursday 03 July 2008 18:30:18 Felipe Maya wrote:

Hi, I have applied this patch and i tested on a wl500gpv2 (BCM5354). The following part disable the ethernet device. If this part of the patch is restored the device work ok.

Quoted 7 lines

Well, I would say your architecture is pretty broken then. This patch replaces an incorrect manual DMA mask setting by a call to the official API.
Please report this to your archtitecture maintainer.

This also breaks at least b44 on my wl500gpv1.

with wireless-testing rc9-wl tree:
b44.c:v2.0
b44 ssb0:0: Required 30BIT DMA mask unsupported by the system.
b44: probe of ssb0:0 failed with error -5
b44 ssb0:1: Required 30BIT DMA mask unsupported by the system.
b44: probe of ssb0:1 failed with error -5

and reverting the patch:
b44.c:v2.0
eth0: Broadcom 44xx/47xx 10/100BaseT Ethernet 00:17:31:ba:ec:35
eth1: Broadcom 44xx/47xx 10/100BaseT Ethernet 40:10:18:00:00:2d

The problem appears to be a missing non-pci equivalent of pci_set_consistent_dma_mask. Maybe that's why the mask was set manually. I can not find anything that sets coherent_dma_mask for other than a pci device. If somebody can confirm my (mis)understanding of this, I'll post the problem to linux-mips.

Steve

author
#8 lore raw
Quoted 1 line · On Tuesday 08 July 2008 17:58:58 Steve Brown wrote:

I'll post the problem to linux-mips.

Please do so. This really is a DMA API issue. It's either using the masks incorrectly, or ot should provide a function to set the coherent mask.

SSB does use The Right (TM) API. So it should not break, in my opinion. I'm fully aware, that there is no non-PCI function that sets the coherent mask. But I really expected that nobody is using it for non-PCI devices either.

Signature · 1 line
Greetings Michael.