Re: [PATCH] powerpc: dma-mapping: Return dma_direct_ops variable when dev == NULL

Re: [PATCH] powerpc: dma-mapping: Return dma_direct_ops variable when dev == NULL

[Benjamin Herrenschmidt]

On Tue, 2014-01-14 at 17:44 +0800, Chunhe Lan wrote:
> Without this patch, kind of below error will be dumped if
> 'insmod ixgbevf.ko' is executed:
> 
>     ixgbevf: Intel(R) 10 Gigabit PCI Express Virtual Function
>              Network Driver - version 2.7.12-k
>     ixgbevf: Copyright (c) 2009 - 2012 Intel Corporation.
>     ixgbevf 0000:01:10.0: enabling device (0000 -> 0002)
>     ixgbevf 0000:01:10.0: No usable DMA configuration, aborting
>     ixgbevf: probe of 0000:01:10.0 failed with error -5
>     ......
>     ......

That's not right. The DMA ops must be set properly for the VF somewhere
in the arch code instead. When creating VFs, is there a hook allowing
the arch to fix things up ?

(Also adding linux-pci on CC)

Ben.

> Signed-off-by: Chunhe Lan <Chunhe.Lan@freescale.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Tested-by: Chunhe Lan <Chunhe.Lan@freescale.com>
> ---
>  arch/powerpc/include/asm/dma-mapping.h |   13 +++++++++----
>  1 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/dma-mapping.h b/arch/powerpc/include/asm/dma-mapping.h
> index e27e9ad..b8c10de 100644
> --- a/arch/powerpc/include/asm/dma-mapping.h
> +++ b/arch/powerpc/include/asm/dma-mapping.h
> @@ -84,10 +84,15 @@ static inline struct dma_map_ops *get_dma_ops(struct device *dev)
>  	 * only ISA DMA device we support is the floppy and we have a hack
>  	 * in the floppy driver directly to get a device for us.
>  	 */
> -	if (unlikely(dev == NULL))
> -		return NULL;
> -
> -	return dev->archdata.dma_ops;
> +	if (dev && dev->archdata.dma_ops)
> +		return dev->archdata.dma_ops;
> +	/*
> +	 * In some cases (for example, use the Intel(R) 10 Gigabit PCI
> +	 * expression Virtual Function Network Driver -- ixgbevf.ko),
> +	 * their value of dev is the NULL. If return NULL, the driver is
> +	 * aborting. So return dma_direct_ops variable when dev == NULL.
> +	 */
> +	return &dma_direct_ops;
>  }
>  
>  static inline void set_dma_ops(struct device *dev, struct dma_map_ops *ops)

Re: [PATCH] powerpc: dma-mapping: Return dma_direct_ops variable when dev == NULL

[Chunhe Lan]

On 01/14/2014 06:14 PM, Benjamin Herrenschmidt wrote:
> On Tue, 2014-01-14 at 17:44 +0800, Chunhe Lan wrote:
>> Without this patch, kind of below error will be dumped if
>> 'insmod ixgbevf.ko' is executed:
>>
>>      ixgbevf: Intel(R) 10 Gigabit PCI Express Virtual Function
>>               Network Driver - version 2.7.12-k
>>      ixgbevf: Copyright (c) 2009 - 2012 Intel Corporation.
>>      ixgbevf 0000:01:10.0: enabling device (0000 -> 0002)
>>      ixgbevf 0000:01:10.0: No usable DMA configuration, aborting
>>      ixgbevf: probe of 0000:01:10.0 failed with error -5
>>      ......
>>      ......
> That's not right. The DMA ops must be set properly for the VF somewhere
> in the arch code instead. When creating VFs, is there a hook allowing
> the arch to fix things up ?
>
> (Also adding linux-pci on CC)
>
> Ben.
>
>> Signed-off-by: Chunhe Lan <Chunhe.Lan@freescale.com>
>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Tested-by: Chunhe Lan <Chunhe.Lan@freescale.com>
>> ---
>>   arch/powerpc/include/asm/dma-mapping.h |   13 +++++++++----
>>   1 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/dma-mapping.h b/arch/powerpc/include/asm/dma-mapping.h
>> index e27e9ad..b8c10de 100644
>> --- a/arch/powerpc/include/asm/dma-mapping.h
>> +++ b/arch/powerpc/include/asm/dma-mapping.h
>> @@ -84,10 +84,15 @@ static inline struct dma_map_ops *get_dma_ops(struct device *dev)
         I see the get_dma_ops function in 
arch/*x86*/include/asm/dma-mapping.h as the following:

  32 static inline struct dma_map_ops *get_dma_ops(struct device *dev)
  33 {
  34 #ifndef CONFIG_X86_DEV_DMA_OPS
  35         return dma_ops;
  36 #else
  37         if (unlikely(!dev) || !dev->archdata.dma_ops)
  38                 return dma_ops;
  39         else
  40                 return dev->archdata.dma_ops;
  41 #endif
  42 }

         And also  see the get_dma_ops function in 
arch/*arm*/include/asm/dma-mapping.h as the following:

  18 static inline struct dma_map_ops *get_dma_ops(struct device *dev)
  19 {
  20         if (dev && dev->archdata.dma_ops)
  21                 return dev->archdata.dma_ops;
  22         return &arm_dma_ops;
  23 }

       Why not powerpc use this method to process dev == NULL ?

Thanks,
-Chunhe

>>   	 * only ISA DMA device we support is the floppy and we have a hack
>>   	 * in the floppy driver directly to get a device for us.
>>   	 */
>> -	if (unlikely(dev == NULL))
>> -		return NULL;
>> -
>> -	return dev->archdata.dma_ops;
>> +	if (dev && dev->archdata.dma_ops)
>> +		return dev->archdata.dma_ops;
>> +	/*
>> +	 * In some cases (for example, use the Intel(R) 10 Gigabit PCI
>> +	 * expression Virtual Function Network Driver -- ixgbevf.ko),
>> +	 * their value of dev is the NULL. If return NULL, the driver is
>> +	 * aborting. So return dma_direct_ops variable when dev == NULL.
>> +	 */
>> +	return &dma_direct_ops;
>>   }
>>   
>>   static inline void set_dma_ops(struct device *dev, struct dma_map_ops *ops)
>
>
>

Re: [PATCH] powerpc: dma-mapping: Return dma_direct_ops variable when dev == NULL

[Benjamin Herrenschmidt]

On Wed, 2014-01-15 at 11:36 +0800, Chunhe Lan wrote:

> >
> >> Signed-off-by: Chunhe Lan <Chunhe.Lan@freescale.com>
> >> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >> Tested-by: Chunhe Lan <Chunhe.Lan@freescale.com>
> >> ---
> >>   arch/powerpc/include/asm/dma-mapping.h |   13 +++++++++----
> >>   1 files changed, 9 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/powerpc/include/asm/dma-mapping.h b/arch/powerpc/include/asm/dma-mapping.h
> >> index e27e9ad..b8c10de 100644
> >> --- a/arch/powerpc/include/asm/dma-mapping.h
> >> +++ b/arch/powerpc/include/asm/dma-mapping.h
> >> @@ -84,10 +84,15 @@ static inline struct dma_map_ops *get_dma_ops(struct device *dev)
>          I see the get_dma_ops function in 
> arch/*x86*/include/asm/dma-mapping.h as the following:
> 
>   32 static inline struct dma_map_ops *get_dma_ops(struct device *dev)
>   33 {
>   34 #ifndef CONFIG_X86_DEV_DMA_OPS
>   35         return dma_ops;
>   36 #else
>   37         if (unlikely(!dev) || !dev->archdata.dma_ops)
>   38                 return dma_ops;
>   39         else
>   40                 return dev->archdata.dma_ops;
>   41 #endif
>   42 }
> 
>          And also  see the get_dma_ops function in  
> arch/*arm*/include/asm/dma-mapping.h as the following:
> 
>   18 static inline struct dma_map_ops *get_dma_ops(struct device *dev)
>   19 {
>   20         if (dev && dev->archdata.dma_ops)
>   21                 return dev->archdata.dma_ops;
>   22         return &arm_dma_ops;
>   23 }
> 
>        Why not powerpc use this method to process dev == NULL ?

Because we don't :-) We used to and removed this. Due to how our HW
works it might not be correct. When an iommu is enabled for example
you simply cannot use the direct ops.

So the right fix is to properly establish the iommu for the VFs like
we do for the PFs.

> Thanks,
> -Chunhe
> 
> >>   	 * only ISA DMA device we support is the floppy and we have a hack
> >>   	 * in the floppy driver directly to get a device for us.
> >>   	 */
> >> -	if (unlikely(dev == NULL))
> >> -		return NULL;
> >> -
> >> -	return dev->archdata.dma_ops;
> >> +	if (dev && dev->archdata.dma_ops)
> >> +		return dev->archdata.dma_ops;
> >> +	/*
> >> +	 * In some cases (for example, use the Intel(R) 10 Gigabit PCI
> >> +	 * expression Virtual Function Network Driver -- ixgbevf.ko),
> >> +	 * their value of dev is the NULL. If return NULL, the driver is
> >> +	 * aborting. So return dma_direct_ops variable when dev == NULL.
> >> +	 */
> >> +	return &dma_direct_ops;
> >>   }
> >>   
> >>   static inline void set_dma_ops(struct device *dev, struct dma_map_ops *ops)
> >
> >
> >
> 
>