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

From: Joerg Roedel
Date: Thu Apr 02 2009 - 06:54:20 EST


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@xxxxxxxxxx>
>
>
> --- 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/