Re: [Bug 487894] New: sky2 0000:06:00.0: DMA-API: device driver frees DMA memory with different size

[Joerg Roedel <joerg.roedel@amd.com>]

On Wed, Apr 01, 2009 at 11:02:45AM -0700, Stephen Hemminger wrote:
> 
> The sky2 driver uses pci_unmap_len and pci_unmap_len_set which on 32 bit
> platforms are meaningless so they are stubbed out. 
> Basically, DMA-API checks are wrong/bogus to enforce on 32bit x86 as is.

As far as I know the VT-d driver is available on 32 bit x86 too. So this should
not always be a nop.
The other question is why pci_unmap_* functions are not defined as a nop too
and call dma_unmap_* instead? This looks inconsisent to me.

> ========================================================
> Unstub pci_unmap macros if doing DMA-API checks
> 
> If doing device driver DMA-API tests then need to keep track of address/length
> even on 32-bit x86 where the information is not normally needed.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> 
> --- a/arch/x86/include/asm/pci_32.h	2009-04-01 10:52:07.117504355 -0700
> +++ b/arch/x86/include/asm/pci_32.h	2009-04-01 10:56:02.249066174 -0700
> @@ -17,16 +17,25 @@ struct pci_dev;
>   */
>  #define PCI_DMA_BUS_IS_PHYS	(1)
>  
> +#ifdef CONFIG_DMA_API_DEBUG
> +/* keep real values */
> +#define DECLARE_PCI_UNMAP_ADDR(ADDR_NAME)	dma_addr_t ADDR_NAME;
> +#define DECLARE_PCI_UNMAP_LEN(LEN_NAME)		__u32 LEN_NAME;
> +#define pci_unmap_addr(PTR, ADDR_NAME)		((PTR)->ADDR_NAME)
> +#define pci_unmap_addr_set(PTR, ADDR_NAME, VAL)	(((PTR)->ADDR_NAME) = (VAL))
> +#define pci_unmap_len(PTR, LEN_NAME)		((PTR)->LEN_NAME)
> +#define pci_unmap_len_set(PTR, LEN_NAME, VAL)	(((PTR)->LEN_NAME) = (VAL))
> +#else
>  /* pci_unmap_{page,single} is a nop so... */
>  #define DECLARE_PCI_UNMAP_ADDR(ADDR_NAME)	dma_addr_t ADDR_NAME[0];
> -#define DECLARE_PCI_UNMAP_LEN(LEN_NAME)	unsigned LEN_NAME[0];
> -#define pci_unmap_addr(PTR, ADDR_NAME)	sizeof((PTR)->ADDR_NAME)
> +#define DECLARE_PCI_UNMAP_LEN(LEN_NAME)		__u32 LEN_NAME[0];
> +#define pci_unmap_addr(PTR, ADDR_NAME)		sizeof((PTR)->ADDR_NAME)
>  #define pci_unmap_addr_set(PTR, ADDR_NAME, VAL) \
>  	do { break; } while (pci_unmap_addr(PTR, ADDR_NAME))
>  #define pci_unmap_len(PTR, LEN_NAME)		sizeof((PTR)->LEN_NAME)
>  #define pci_unmap_len_set(PTR, LEN_NAME, VAL) \
>  	do { break; } while (pci_unmap_len(PTR, LEN_NAME))
> -
> +#endif

I think we need another solution which takes into account that there might be
VT-d enabled and which defines the pci_unmap_* functions also as a nop when it
is not. I suggest to make a CONFIG option for these functions nit being a nop
and let DMA_API_DEBUG select it on x86. So we get proper checking for drivers
on 32bit x86 too which helps other architectures where these drivers can be
used.

	Joerg

-- 
           | Advanced Micro Devices GmbH
 Operating | Karl-Hammerschmidt-Str. 34, 85609 Dornach bei München
 System    | 
 Research  | Geschäftsführer: Jochen Polster, Thomas M. McCoy, Giuliano Meroni
 Center    | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
           | Registergericht München, HRB Nr. 43632