[PATCH] pci_set_dac helper

[PATCH] pci_set_dac helper

[Jeff Garzik]

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


It seems to me like a lot of drivers wind up getting their 
pci_set_dma_mask stuff wrong, occasionally in subtle ways.  So, I 
created a "give me 64-bit PCI DMA" helper function.

The attached patch demonstrates its use in tg3, from which the logic 
originated.  It also fixes a tiny bug in tg3, whereby it might return 
zero on error (rather than -EFOO) if the pci_set_dma_mask succeeds but 
pci_set_consistent_dma_mask fails.



[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 4134 bytes --]

===== drivers/net/tg3.c 1.112 vs edited =====
--- 1.112/drivers/net/tg3.c	Thu Nov 20 12:46:05 2003
+++ edited/drivers/net/tg3.c	Wed Dec 31 11:06:43 2003
@@ -7493,7 +7493,8 @@
 	unsigned long tg3reg_base, tg3reg_len;
 	struct net_device *dev;
 	struct tg3 *tp;
-	int i, err, pci_using_dac, pm_cap;
+	int i, err, pm_cap;
+	u32 dac_flags;
 
 	if (tg3_version_printed++ == 0)
 		printk(KERN_INFO "%s", version);
@@ -7530,21 +7531,16 @@
 	}
 
 	/* Configure DMA attributes. */
-	if (!pci_set_dma_mask(pdev, 0xffffffffffffffffULL)) {
-		pci_using_dac = 1;
-		if (pci_set_consistent_dma_mask(pdev, 0xffffffffffffffffULL)) {
-			printk(KERN_ERR PFX "Unable to obtain 64 bit DMA "
-			       "for consistent allocations\n");
-			goto err_out_free_res;
-		}
-	} else {
-		err = pci_set_dma_mask(pdev, 0xffffffffULL);
-		if (err) {
-			printk(KERN_ERR PFX "No usable DMA configuration, "
-			       "aborting.\n");
-			goto err_out_free_res;
-		}
-		pci_using_dac = 0;
+	dac_flags = 0;
+	err = pci_set_dac(pdev, &dac_flags);
+	if (err)
+		goto err_out_free_res;
+
+	if ((dac_flags & (PCI_DAC_EN | PCI_DAC_CONS_EN)) == PCI_DAC_EN) {
+		printk(KERN_ERR PFX "Unable to obtain 64 bit DMA "
+		       "for consistent allocations\n");
+		err = -EIO;
+		goto err_out_free_res;
 	}
 
 	tg3reg_base = pci_resource_start(pdev, 0);
@@ -7560,7 +7556,7 @@
 	SET_MODULE_OWNER(dev);
 	SET_NETDEV_DEV(dev, &pdev->dev);
 
-	if (pci_using_dac)
+	if (dac_flags)
 		dev->features |= NETIF_F_HIGHDMA;
 #if TG3_VLAN_TAG_USED
 	dev->features |= NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX;
===== drivers/pci/pci.c 1.60 vs edited =====
--- 1.60/drivers/pci/pci.c	Sun Oct  5 04:07:55 2003
+++ edited/drivers/pci/pci.c	Wed Dec 31 11:00:44 2003
@@ -713,7 +713,32 @@
 
 	return 0;
 }
-     
+
+int
+pci_set_dac(struct pci_dev *dev, u32 *flags_out)
+{
+	int err = 0;
+	u32 flags = 0;
+
+	if (!pci_set_dma_mask(dev, 0xffffffffffffffffULL)) {
+		flags |= PCI_DAC_EN;
+		if (pci_set_consistent_dma_mask(dev, 0xffffffffffffffffULL))
+			printk(KERN_WARNING
+			       "PCI(%s): Unable to obtain 64 bit DMA "
+			       "for consistent allocations\n", pci_name(dev));
+		else
+			flags |= PCI_DAC_CONS_EN;
+	} else {
+		err = pci_set_dma_mask(dev, 0xffffffffULL);
+		if (err)
+			printk(KERN_ERR "PCI(%s): No usable DMA configuration",
+			       pci_name(dev));
+	}
+
+	*flags_out = flags;
+	return err;
+}
+
 static int __devinit pci_init(void)
 {
 	struct pci_dev *dev = NULL;
@@ -765,6 +790,7 @@
 EXPORT_SYMBOL(pci_clear_mwi);
 EXPORT_SYMBOL(pci_set_dma_mask);
 EXPORT_SYMBOL(pci_dac_set_dma_mask);
+EXPORT_SYMBOL(pci_set_dac);
 EXPORT_SYMBOL(pci_set_consistent_dma_mask);
 EXPORT_SYMBOL(pci_assign_resource);
 EXPORT_SYMBOL(pci_find_parent_resource);
===== include/linux/pci.h 1.109 vs edited =====
--- 1.109/include/linux/pci.h	Mon Dec 29 16:37:23 2003
+++ edited/include/linux/pci.h	Wed Dec 31 10:54:16 2003
@@ -340,6 +340,10 @@
 #define PCIIOC_MMAP_IS_MEM	(PCIIOC_BASE | 0x02)	/* Set mmap state to MEM space. */
 #define PCIIOC_WRITE_COMBINE	(PCIIOC_BASE | 0x03)	/* Enable/disable write-combining. */
 
+/* pci_set_dac flags */
+#define PCI_DAC_EN		(1 << 0)
+#define PCI_DAC_CONS_EN		(1 << 1)
+
 #ifdef __KERNEL__
 
 #include <linux/types.h>
@@ -653,6 +657,8 @@
 void pci_clear_mwi(struct pci_dev *dev);
 int pci_set_dma_mask(struct pci_dev *dev, u64 mask);
 int pci_dac_set_dma_mask(struct pci_dev *dev, u64 mask);
+int pci_set_dac(struct pci_dev *dev, u32 *flags);
+
 int pci_set_consistent_dma_mask(struct pci_dev *dev, u64 mask);
 int pci_assign_resource(struct pci_dev *dev, int i);
 
@@ -759,6 +765,8 @@
 static inline int pci_module_init(struct pci_driver *drv) { return -ENODEV; }
 static inline int pci_set_dma_mask(struct pci_dev *dev, u64 mask) { return -EIO; }
 static inline int pci_dac_set_dma_mask(struct pci_dev *dev, u64 mask) { return -EIO; }
+static inline int pci_set_dac(struct pci_dev *dev, u32 *flags) { return -EIO; }
+
 static inline int pci_assign_resource(struct pci_dev *dev, int i) { return -EBUSY;}
 static inline int pci_register_driver(struct pci_driver *drv) { return 0;}
 static inline void pci_unregister_driver(struct pci_driver *drv) { }

Re: [PATCH] pci_set_dac helper

[David S. Miller]

On Wed, 31 Dec 2003 11:12:42 -0500
Jeff Garzik <jgarzik@pobox.com> wrote:

> It seems to me like a lot of drivers wind up getting their 
> pci_set_dma_mask stuff wrong, occasionally in subtle ways.  So, I 
> created a "give me 64-bit PCI DMA" helper function.
> 
> The attached patch demonstrates its use in tg3, from which the logic 
> originated.  It also fixes a tiny bug in tg3, whereby it might return 
> zero on error (rather than -EFOO) if the pci_set_dma_mask succeeds but 
> pci_set_consistent_dma_mask fails.

I'm fine with the helper.

The tg3 error return bug you noticed is already fixed in the tg3
updates I'm sending to Linus today.

Re: [PATCH] pci_set_dac helper

[Matthew Wilcox]

On Wed, Dec 31, 2003 at 11:12:42AM -0500, Jeff Garzik wrote:
> It seems to me like a lot of drivers wind up getting their 
> pci_set_dma_mask stuff wrong, occasionally in subtle ways.  So, I 
> created a "give me 64-bit PCI DMA" helper function.

I like it, but I think it could be better.  A lot of drivers want
64-bit streaming DMA but 32-bit consistent DMA.  So how about this:

> @@ -7530,21 +7531,16 @@
>  	}
>  
>  	/* Configure DMA attributes. */
> -	if (!pci_set_dma_mask(pdev, 0xffffffffffffffffULL)) {
> -		pci_using_dac = 1;
> -		if (pci_set_consistent_dma_mask(pdev, 0xffffffffffffffffULL)) {
> -			printk(KERN_ERR PFX "Unable to obtain 64 bit DMA "
> -			       "for consistent allocations\n");
> -			goto err_out_free_res;
> -		}
> -	} else {
> -		err = pci_set_dma_mask(pdev, 0xffffffffULL);
> -		if (err) {
> -			printk(KERN_ERR PFX "No usable DMA configuration, "
> -			       "aborting.\n");
> -			goto err_out_free_res;
> -		}
> -		pci_using_dac = 0;
> +	dac_flags = 0;
> +	err = pci_set_dac(pdev, &dac_flags);
> +	if (err)

-	dac_flags = 0;
-	err = pci_set_dac(pdev, &dac_flags);
-	if (err)
+	dac_flags = PCI_DAC_EN | PCI_DAC_CONS_EN;
+	err = pci_set_dac(pdev, dac_flags);
+	if (err < 0)

> +		goto err_out_free_res;

+	dac_flags = err;

> +
> +	if ((dac_flags & (PCI_DAC_EN | PCI_DAC_CONS_EN)) == PCI_DAC_EN) {
> +		printk(KERN_ERR PFX "Unable to obtain 64 bit DMA "
> +		       "for consistent allocations\n");
> +		err = -EIO;
> +		goto err_out_free_res;
>  	}
>  
>  	tg3reg_base = pci_resource_start(pdev, 0);

/**
 * pci_set_dac - Sets the Dual-Address-Cycle capabilities for this device
 * @dev: The device
 * @flags: Indicates the desired capabilities
 *
 * Returns a negative errno on error, otherwise returns the flags that were
 * successfully set.
 */
int pci_set_dac(struct pci_dev *dev, int flags)
{
	int err;

	if (flags == 0)
		goto 32bit;

	if (flags & PCI_DAC_EN) {
		if (pci_set_dma_mask(dev, 0xffffffffffffffffULL)) {
			flags = 0;
			goto 32bit;
		}
	}

	if (flags & PCI_DAC_CONS_EN) {
		if (pci_set_consistent_dma_mask(dev, 0xffffffffffffffffULL)) {
			printk(KERN_WARNING
			       "PCI(%s): Unable to obtain 64 bit DMA "
			       "for consistent allocations\n", pci_name(dev));
			flags &= ~PCI_DAC_CONS_EN;
		}
	}

	return flags;

 32bit:
	err = pci_set_dma_mask(dev, 0xffffffffULL);
	if (err)
		printk(KERN_ERR "PCI(%s): No usable DMA configuration",
		       pci_name(dev));
	return err;
}

I note ithat both this and your patch will lead to two errors being
printed on 64-bit consistent failure; one by tg3 and one by the PCI
layer; this seems suboptimal.  I suspect you want to do away with the
error printk in the tg3 driver.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [PATCH] pci_set_dac helper

[Jeff Garzik]

Matthew Wilcox wrote:
> On Wed, Dec 31, 2003 at 11:12:42AM -0500, Jeff Garzik wrote:
> 
>>It seems to me like a lot of drivers wind up getting their 
>>pci_set_dma_mask stuff wrong, occasionally in subtle ways.  So, I 
>>created a "give me 64-bit PCI DMA" helper function.
> 
> 
> I like it, but I think it could be better.  A lot of drivers want
> 64-bit streaming DMA but 32-bit consistent DMA.  So how about this:

I like the direction of your patch:  the driver informs us ahead of time 
what it wants (even though this isn't necessarily true with PCI_DAC_EN, 
which falls back instead of fails).

However, the one big failing of your version is that the driver _must_ 
know if PCI DAC succeeded or not.  Therefore, two pieces of information 
must be returned (error value, DAC flag(s)), which lends itself more to 
leaving my version as-is ;-)


> I note ithat both this and your patch will lead to two errors being
> printed on 64-bit consistent failure; one by tg3 and one by the PCI
> layer; this seems suboptimal.  I suspect you want to do away with the
> error printk in the tg3 driver.

That was intentional in my patch, as it's a warning not an error in my 
pci_set_dac.  In your version I would agree.

	Jeff

Re: [PATCH] pci_set_dac helper

[Grant Grundler]

On Wed, Dec 31, 2003 at 02:05:31PM -0500, Jeff Garzik wrote:
> Matthew Wilcox wrote:
> > I note that both this and your patch will lead to two errors being
> > printed on 64-bit consistent failure; one by tg3 and one by the PCI
> > layer; this seems suboptimal.  I suspect you want to do away with the
> > error printk in the tg3 driver.
> 
> That was intentional in my patch, as it's a warning not an error in my 
> pci_set_dac.  In your version I would agree.

It's perfectly ok for some platforms to not support 64-bit DMA mask
for either type of DMA. The warning suggests it's not OK.
I don't see why we either a warning or error printed unless it would
lead to incorrect operation of the device.

grant