Re: [patch] PCI Express Enhanced Config Patch - 2.6.0-test11

[Andi Kleen <ak@muc.de>]

"Durairaj, Sundarapandian" <sundarapandian.durairaj@intel.com> writes:

> +u32 pcie_last_accessed_device;
> +
> +unsigned long pci_exp_set_dev_base (int bus, int dev, int fn)
> +{
> +	u32 dev_base = 
> +		mmcfg_base_address | (bus << 20) | ((PCI_DEVFN (dev,fn))
> <<12);
> +	if (dev_base != pcie_last_accessed_device){
> +		pcie_last_accessed_device = dev_base;
> +		set_fixmap (FIX_PCIE_MCFG, dev_base);
> +	}

Can you please put the details on how the fixmap is managed into
an asm-i386/* file. x86-64 shares the i386 PCI subsystem and I would
like to use a different implement a different method there (just mapping 
everything statically) 

> +}
> +
> +static int pci_express_conf_read(int seg, int bus, int devfn, int reg,
> int len, u32 *value)
> +{
> +	 unsigned long flags;
> +	 unsigned long base_address;
> +	 char * virt_addr;
> +	 int dev = PCI_SLOT (devfn);
> +	 int fn  = PCI_FUNC (devfn);
> + 
> +  if (!value || ((u32)bus > 255) || ((u32)dev > 31) || ((u32)fn > 7) ||
> ((u32)reg > 4095))
> +  	return -EINVAL;
> +
> +	/* Shoot misalligned transaction now */
> +	if (reg & (len-1))
> +  	return -EINVAL;

It would be better to printk here because nobody will check the return
value of this function

Same for _write.

> +			writew(value,(unsigned long)virt_addr+reg);
> +       		        break;
> +	        case 4:
> +			writel(value,(unsigned long)virt_addr+reg);
> +	                break;
> +     	}
> + 	/* Dummy read to flush PCI write */
> +	readl (virt_addr);

I thought the consensus was that reading the same address is dangerous
because some registers react when they are only read.
Also you cannot readl on a w or b registers, if you do that you would
need to use the same size.

Also are you sure that the old port based config read/write functions
actually flushed their writes? I don't think they did.  It probably
would be best to just drop that read.

Otherwise it should read some save register in the same mapping.


> +	 if( is_pci_exp_platform() != 0){

Shouldn't there a pci_probe option for this here?

It would be probably better to add some way to turn this off at runtime,
since it's still experimental and most drivers will probably work
with the old methods for now.

>  
>  	if (capable(CAP_SYS_ADMIN))
> -		size = PCI_CFG_SPACE_SIZE;
> +#ifdef CONFIG_PCI_EXP_ENHANCED
> +		if (pci_find_capability(dev,PCI_CAP_ID_EXP))
> +			size = PCI_CFG_SPACE_EXP_SIZE;
> +		else
> +#endif /*CONFIG_PCI_EXP_ENHANCED */
> + 		size = PCI_CFG_SPACE_SIZE;

This would be somewhat cleaner in an standard function that returns
the cfg space size for a device, especially since this code is often duplicated
in your patch.


> +#ifdef CONFIG_PCI_EXP_ENHANCED
> +struct acpi_table_mcfg {
> +	struct acpi_table_header 	header;
> +	u8	reserved[8];
> +	u64	base_address;
> +} __attribute__ ((packed));
> +#endif

Declarations don't need to be #ifdefed.

-Andi