pci_set_dac helper
Subsystems (2)
PATCH 1/1 pci_set_dac helper
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.
===== drivers/net/tg3.c 1.112 vs edited =====
drivers/net/tg3.c +13 -17
@@ -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 =====
drivers/pci/pci.c +27 -1
@@ -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 =====
include/linux/pci.h +8 -0
@@ -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) { }
Quoted 8 lines · On Wed, 31 Dec 2003 11:12:42 -0500 Jeff Garzik <[email protected]> wrote:
It seems to me like a lot of drivers wind up getting their
pci_set_dma_maskstuff 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 thepci_set_dma_masksucceeds butpci_set_consistent_dma_maskfails.
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.
Quoted 3 lines · Jeff Garzik
It seems to me like a lot of drivers wind up getting their
pci_set_dma_maskstuff 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:
Quoted 12 lines
drivers/net/tg3.c +0
-8@@ -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 {
drivers/net/tg3.c line 7536
Quoted 10 lines
drivers/net/tg3.c +3
-7- 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)
Review line 7537
Quoted 1 line
(quoted diff) +1
-0+ goto err_out_free_res;
+ dac_flags = err;
drivers/net/tg3.c line 7546
Quoted 9 lines
drivers/net/tg3.c +6
-0+ +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.
Signature · 6 lines
"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
Quoted 7 lines · Matthew Wilcox
Quoted 1 line · Jeff Garzik
It seems to me like a lot of drivers wind up getting their
pci_set_dma_maskstuff 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 ;-)
Quoted 4 lines
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
Quoted 8 lines · Jeff Garzik
Quoted 4 lines · Matthew Wilcox
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