Date: Thu, 2 Apr 2009 12:54:15 +0200
From: Joerg Roedel <joerg.roedel@amd.com>
To: Stephen Hemminger <shemminger@vyatta.com>
CC: Dave Jones <davej@redhat.com>, <tglx@linuxtronix.de>, <mingo@redhat.com>,
       <hpa@zytor.com>, <linux-kernel@vger.kernel.org>, <x86@kernel.org>
Subject: Re: [Bug 487894] New: sky2 0000:06:00.0: DMA-API: device driver
	frees DMA memory with different size
Message-ID: <20090402105415.GD21083@amd.com>
References: <20090401172708.GA11941@redhat.com> <20090401110245.4d1ddf72@nehalam>
MIME-Version: 1.0
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
In-Reply-To: <20090401110245.4d1ddf72@nehalam>
User-Agent: Mutt/1.5.18 (2008-05-17)
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3194
Lines: 71

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/