Re: [PATCH kernel 1/2] powerpc/iommu: Get rid of default group_release()

2016-04-20 Thread Alexey Kardashevskiy 

On 04/21/2016 10:02 AM, Gavin Shan wrote:

On Fri, Apr 08, 2016 at 04:36:43PM +1000, Alexey Kardashevskiy wrote:

IBM PPC IOMMU API users always set IOMMU data and IOMMU release callback
to an IOMMU group. At the moment the callback clears one pointer in
iommu_table_group and that's it.

The platform code calls iommu_group_put() and counts on _put() being
called last so they check for table_group->group being reset which
is conceptually wrong as there may be another user holding a reference.

This removes the default IOMMU group release() callback and adds it
as a parameter to iommu_register_group(). As we are changing the prototype
anyway, this also changes the function name to more distinctive
iommu_register_table_group().

This should cause no behavioral change as it leaves BUG_ON for IODA2
(where it was reported) and removes BUG_ON for pseries/IODA1 as they
do not support IOV anyway and this BUG_ON has never been reported for
these platforms.

Signed-off-by: Alexey Kardashevskiy 


Reviewed-by: Gavin Shan 

There is one question at end of the thread...


---
arch/powerpc/include/asm/iommu.h  | 12 +++-
arch/powerpc/kernel/iommu.c   | 14 --
arch/powerpc/platforms/powernv/pci-ioda.c | 15 +++
arch/powerpc/platforms/pseries/iommu.c| 17 +
4 files changed, 31 insertions(+), 27 deletions(-)

diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h
index 7b87bab..d7ba3b4 100644
--- a/arch/powerpc/include/asm/iommu.h
+++ b/arch/powerpc/include/asm/iommu.h
@@ -201,17 +201,19 @@ struct iommu_table_group {

#ifdef CONFIG_IOMMU_API

-extern void iommu_register_group(struct iommu_table_group *table_group,
-int pci_domain_number, unsigned long pe_num);
+extern void iommu_register_table_group(struct iommu_table_group *table_group,
+   int pci_domain_number, unsigned long pe_num,
+   void (*release)(void *iommu_data));
extern int iommu_add_device(struct device *dev);
extern void iommu_del_device(struct device *dev);
extern int __init tce_iommu_bus_notifier_init(void);
extern long iommu_tce_xchg(struct iommu_table *tbl, unsigned long entry,
unsigned long *hpa, enum dma_data_direction *direction);
#else
-static inline void iommu_register_group(struct iommu_table_group *table_group,
-   int pci_domain_number,
-   unsigned long pe_num)
+static inline void iommu_register_table_group(
+   struct iommu_table_group *table_group,
+   int pci_domain_number, unsigned long pe_num,
+   void (*release)(void *iommu_data))
{
}

diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
index a8e3490..8eed2fa 100644
--- a/arch/powerpc/kernel/iommu.c
+++ b/arch/powerpc/kernel/iommu.c
@@ -887,15 +887,9 @@ EXPORT_SYMBOL_GPL(iommu_direction_to_tce_perm);
/*
  * SPAPR TCE API
  */
-static void group_release(void *iommu_data)
-{
-   struct iommu_table_group *table_group = iommu_data;
-
-   table_group->group = NULL;
-}
-
-void iommu_register_group(struct iommu_table_group *table_group,
-   int pci_domain_number, unsigned long pe_num)
+void iommu_register_table_group(struct iommu_table_group *table_group,
+   int pci_domain_number, unsigned long pe_num,
+   void (*release)(void *iommu_data))
{
struct iommu_group *grp;
char *name;
@@ -907,7 +901,7 @@ void iommu_register_group(struct iommu_table_group 
*table_group,
return;
}
table_group->group = grp;
-   iommu_group_set_iommudata(grp, table_group, group_release);
+   iommu_group_set_iommudata(grp, table_group, release);
name = kasprintf(GFP_KERNEL, "domain%d-pe%lx",
pci_domain_number, pe_num);
if (!name)
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c 
b/arch/powerpc/platforms/powernv/pci-ioda.c
index c5baaf3..ce9f2bf 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -1330,6 +1330,13 @@ static long pnv_pci_ioda2_unset_window(struct 
iommu_table_group *table_group,
int num);
static void pnv_pci_ioda2_set_bypass(struct pnv_ioda_pe *pe, bool enable);

+static void pnv_pci_ioda2_group_release(void *iommu_data)
+{
+   struct iommu_table_group *table_group = iommu_data;
+
+   table_group->group = NULL;
+}
+
static void pnv_pci_ioda2_release_dma_pe(struct pci_dev *dev, struct 
pnv_ioda_pe *pe)
{
struct iommu_table*tbl;
@@ -1965,8 +1972,8 @@ static void pnv_pci_ioda_setup_dma_pe(struct pnv_phb *phb,
return;

tbl = pnv_pci_table_alloc(phb->hose->node);
-   iommu_register_group(>table_group, phb->hose->global_number,
-   pe->pe_number);
+   iommu_register_table_group(>table_group,

Re: [PATCH kernel 1/2] powerpc/iommu: Get rid of default group_release()

2016-04-20 Thread Gavin Shan 
On Fri, Apr 08, 2016 at 04:36:43PM +1000, Alexey Kardashevskiy wrote:
>IBM PPC IOMMU API users always set IOMMU data and IOMMU release callback
>to an IOMMU group. At the moment the callback clears one pointer in
>iommu_table_group and that's it.
>
>The platform code calls iommu_group_put() and counts on _put() being
>called last so they check for table_group->group being reset which
>is conceptually wrong as there may be another user holding a reference.
>
>This removes the default IOMMU group release() callback and adds it
>as a parameter to iommu_register_group(). As we are changing the prototype
>anyway, this also changes the function name to more distinctive
>iommu_register_table_group().
>
>This should cause no behavioral change as it leaves BUG_ON for IODA2
>(where it was reported) and removes BUG_ON for pseries/IODA1 as they
>do not support IOV anyway and this BUG_ON has never been reported for
>these platforms.
>
>Signed-off-by: Alexey Kardashevskiy 

Reviewed-by: Gavin Shan 

There is one question at end of the thread...

>---
> arch/powerpc/include/asm/iommu.h  | 12 +++-
> arch/powerpc/kernel/iommu.c   | 14 --
> arch/powerpc/platforms/powernv/pci-ioda.c | 15 +++
> arch/powerpc/platforms/pseries/iommu.c| 17 +
> 4 files changed, 31 insertions(+), 27 deletions(-)
>
>diff --git a/arch/powerpc/include/asm/iommu.h 
>b/arch/powerpc/include/asm/iommu.h
>index 7b87bab..d7ba3b4 100644
>--- a/arch/powerpc/include/asm/iommu.h
>+++ b/arch/powerpc/include/asm/iommu.h
>@@ -201,17 +201,19 @@ struct iommu_table_group {
>
> #ifdef CONFIG_IOMMU_API
>
>-extern void iommu_register_group(struct iommu_table_group *table_group,
>-   int pci_domain_number, unsigned long pe_num);
>+extern void iommu_register_table_group(struct iommu_table_group *table_group,
>+  int pci_domain_number, unsigned long pe_num,
>+  void (*release)(void *iommu_data));
> extern int iommu_add_device(struct device *dev);
> extern void iommu_del_device(struct device *dev);
> extern int __init tce_iommu_bus_notifier_init(void);
> extern long iommu_tce_xchg(struct iommu_table *tbl, unsigned long entry,
>   unsigned long *hpa, enum dma_data_direction *direction);
> #else
>-static inline void iommu_register_group(struct iommu_table_group *table_group,
>-  int pci_domain_number,
>-  unsigned long pe_num)
>+static inline void iommu_register_table_group(
>+  struct iommu_table_group *table_group,
>+  int pci_domain_number, unsigned long pe_num,
>+  void (*release)(void *iommu_data))
> {
> }
>
>diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
>index a8e3490..8eed2fa 100644
>--- a/arch/powerpc/kernel/iommu.c
>+++ b/arch/powerpc/kernel/iommu.c
>@@ -887,15 +887,9 @@ EXPORT_SYMBOL_GPL(iommu_direction_to_tce_perm);
> /*
>  * SPAPR TCE API
>  */
>-static void group_release(void *iommu_data)
>-{
>-  struct iommu_table_group *table_group = iommu_data;
>-
>-  table_group->group = NULL;
>-}
>-
>-void iommu_register_group(struct iommu_table_group *table_group,
>-  int pci_domain_number, unsigned long pe_num)
>+void iommu_register_table_group(struct iommu_table_group *table_group,
>+  int pci_domain_number, unsigned long pe_num,
>+  void (*release)(void *iommu_data))
> {
>   struct iommu_group *grp;
>   char *name;
>@@ -907,7 +901,7 @@ void iommu_register_group(struct iommu_table_group 
>*table_group,
>   return;
>   }
>   table_group->group = grp;
>-  iommu_group_set_iommudata(grp, table_group, group_release);
>+  iommu_group_set_iommudata(grp, table_group, release);
>   name = kasprintf(GFP_KERNEL, "domain%d-pe%lx",
>   pci_domain_number, pe_num);
>   if (!name)
>diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c 
>b/arch/powerpc/platforms/powernv/pci-ioda.c
>index c5baaf3..ce9f2bf 100644
>--- a/arch/powerpc/platforms/powernv/pci-ioda.c
>+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
>@@ -1330,6 +1330,13 @@ static long pnv_pci_ioda2_unset_window(struct 
>iommu_table_group *table_group,
>   int num);
> static void pnv_pci_ioda2_set_bypass(struct pnv_ioda_pe *pe, bool enable);
>
>+static void pnv_pci_ioda2_group_release(void *iommu_data)
>+{
>+  struct iommu_table_group *table_group = iommu_data;
>+
>+  table_group->group = NULL;
>+}
>+
> static void pnv_pci_ioda2_release_dma_pe(struct pci_dev *dev, struct 
> pnv_ioda_pe *pe)
> {
>   struct iommu_table*tbl;
>@@ -1965,8 +1972,8 @@ static void pnv_pci_ioda_setup_dma_pe(struct pnv_phb 
>*phb,
>   return;
>
>   tbl = pnv_pci_table_alloc(phb->hose->node);
>-  iommu_register_group(>table_group, phb->hose->global_number,
>-  pe->pe_number);

Re: [PATCH kernel 1/2] powerpc/iommu: Get rid of default group_release()

2016-04-13 Thread David Gibson 
On Fri, Apr 08, 2016 at 04:36:43PM +1000, Alexey Kardashevskiy wrote:
> IBM PPC IOMMU API users always set IOMMU data and IOMMU release callback
> to an IOMMU group. At the moment the callback clears one pointer in
> iommu_table_group and that's it.
> 
> The platform code calls iommu_group_put() and counts on _put() being
> called last so they check for table_group->group being reset which
> is conceptually wrong as there may be another user holding a reference.
> 
> This removes the default IOMMU group release() callback and adds it
> as a parameter to iommu_register_group(). As we are changing the prototype
> anyway, this also changes the function name to more distinctive
> iommu_register_table_group().
> 
> This should cause no behavioral change as it leaves BUG_ON for IODA2
> (where it was reported) and removes BUG_ON for pseries/IODA1 as they
> do not support IOV anyway and this BUG_ON has never been reported for
> these platforms.
> 
> Signed-off-by: Alexey Kardashevskiy 

Reviewed-by: David Gibson 

> ---
>  arch/powerpc/include/asm/iommu.h  | 12 +++-
>  arch/powerpc/kernel/iommu.c   | 14 --
>  arch/powerpc/platforms/powernv/pci-ioda.c | 15 +++
>  arch/powerpc/platforms/pseries/iommu.c| 17 +
>  4 files changed, 31 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/iommu.h 
> b/arch/powerpc/include/asm/iommu.h
> index 7b87bab..d7ba3b4 100644
> --- a/arch/powerpc/include/asm/iommu.h
> +++ b/arch/powerpc/include/asm/iommu.h
> @@ -201,17 +201,19 @@ struct iommu_table_group {
>  
>  #ifdef CONFIG_IOMMU_API
>  
> -extern void iommu_register_group(struct iommu_table_group *table_group,
> -  int pci_domain_number, unsigned long pe_num);
> +extern void iommu_register_table_group(struct iommu_table_group *table_group,
> + int pci_domain_number, unsigned long pe_num,
> + void (*release)(void *iommu_data));
>  extern int iommu_add_device(struct device *dev);
>  extern void iommu_del_device(struct device *dev);
>  extern int __init tce_iommu_bus_notifier_init(void);
>  extern long iommu_tce_xchg(struct iommu_table *tbl, unsigned long entry,
>   unsigned long *hpa, enum dma_data_direction *direction);
>  #else
> -static inline void iommu_register_group(struct iommu_table_group 
> *table_group,
> - int pci_domain_number,
> - unsigned long pe_num)
> +static inline void iommu_register_table_group(
> + struct iommu_table_group *table_group,
> + int pci_domain_number, unsigned long pe_num,
> + void (*release)(void *iommu_data))
>  {
>  }
>  
> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> index a8e3490..8eed2fa 100644
> --- a/arch/powerpc/kernel/iommu.c
> +++ b/arch/powerpc/kernel/iommu.c
> @@ -887,15 +887,9 @@ EXPORT_SYMBOL_GPL(iommu_direction_to_tce_perm);
>  /*
>   * SPAPR TCE API
>   */
> -static void group_release(void *iommu_data)
> -{
> - struct iommu_table_group *table_group = iommu_data;
> -
> - table_group->group = NULL;
> -}
> -
> -void iommu_register_group(struct iommu_table_group *table_group,
> - int pci_domain_number, unsigned long pe_num)
> +void iommu_register_table_group(struct iommu_table_group *table_group,
> + int pci_domain_number, unsigned long pe_num,
> + void (*release)(void *iommu_data))
>  {
>   struct iommu_group *grp;
>   char *name;
> @@ -907,7 +901,7 @@ void iommu_register_group(struct iommu_table_group 
> *table_group,
>   return;
>   }
>   table_group->group = grp;
> - iommu_group_set_iommudata(grp, table_group, group_release);
> + iommu_group_set_iommudata(grp, table_group, release);
>   name = kasprintf(GFP_KERNEL, "domain%d-pe%lx",
>   pci_domain_number, pe_num);
>   if (!name)
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c 
> b/arch/powerpc/platforms/powernv/pci-ioda.c
> index c5baaf3..ce9f2bf 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -1330,6 +1330,13 @@ static long pnv_pci_ioda2_unset_window(struct 
> iommu_table_group *table_group,
>   int num);
>  static void pnv_pci_ioda2_set_bypass(struct pnv_ioda_pe *pe, bool enable);
>  
> +static void pnv_pci_ioda2_group_release(void *iommu_data)
> +{
> + struct iommu_table_group *table_group = iommu_data;
> +
> + table_group->group = NULL;
> +}
> +
>  static void pnv_pci_ioda2_release_dma_pe(struct pci_dev *dev, struct 
> pnv_ioda_pe *pe)
>  {
>   struct iommu_table*tbl;
> @@ -1965,8 +1972,8 @@ static void pnv_pci_ioda_setup_dma_pe(struct pnv_phb 
> *phb,
>   return;
>  
>   tbl = pnv_pci_table_alloc(phb->hose->node);
> - iommu_register_group(>table_group,

Re: [PATCH kernel 1/2] powerpc/iommu: Get rid of default group_release()

2016-04-08 Thread kbuild test robot 
Hi Alexey,

[auto build test ERROR on powerpc/next]
[also build test ERROR on v4.6-rc2 next-20160408]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improving the system]

url:
https://github.com/0day-ci/linux/commits/Alexey-Kardashevskiy/powerpc-powernv-Fix-crash-on-PF-unbind-when-VF-is-passed/20160408-144325
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-defconfig (attached as .config)
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=powerpc 

All errors (new ones prefixed by >>):

   arch/powerpc/platforms/powernv/pci-ioda.c: In function 
'pnv_pci_ioda2_setup_dma_pe':
>> arch/powerpc/platforms/powernv/pci-ioda.c:2461:19: error: 
>> 'pnv_pci_ioda2_group_release' undeclared (first use in this function)
   pe->pe_number, pnv_pci_ioda2_group_release);
  ^
   arch/powerpc/platforms/powernv/pci-ioda.c:2461:19: note: each undeclared 
identifier is reported only once for each function it appears in

vim +/pnv_pci_ioda2_group_release +2461 
arch/powerpc/platforms/powernv/pci-ioda.c

  2455  return;
  2456  
  2457  /* TVE #1 is selected by PCI address bit 59 */
  2458  pe->tce_bypass_base = 1ull << 59;
  2459  
  2460  iommu_register_table_group(>table_group, 
phb->hose->global_number,
> 2461  pe->pe_number, pnv_pci_ioda2_group_release);
  2462  
  2463  /* The PE will reserve all possible 32-bits space */
  2464  pe->tce32_seg = 0;

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH kernel 1/2] powerpc/iommu: Get rid of default group_release()

2016-04-08 Thread Alexey Kardashevskiy 
IBM PPC IOMMU API users always set IOMMU data and IOMMU release callback
to an IOMMU group. At the moment the callback clears one pointer in
iommu_table_group and that's it.

The platform code calls iommu_group_put() and counts on _put() being
called last so they check for table_group->group being reset which
is conceptually wrong as there may be another user holding a reference.

This removes the default IOMMU group release() callback and adds it
as a parameter to iommu_register_group(). As we are changing the prototype
anyway, this also changes the function name to more distinctive
iommu_register_table_group().

This should cause no behavioral change as it leaves BUG_ON for IODA2
(where it was reported) and removes BUG_ON for pseries/IODA1 as they
do not support IOV anyway and this BUG_ON has never been reported for
these platforms.

Signed-off-by: Alexey Kardashevskiy 
---
 arch/powerpc/include/asm/iommu.h  | 12 +++-
 arch/powerpc/kernel/iommu.c   | 14 --
 arch/powerpc/platforms/powernv/pci-ioda.c | 15 +++
 arch/powerpc/platforms/pseries/iommu.c| 17 +
 4 files changed, 31 insertions(+), 27 deletions(-)

diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h
index 7b87bab..d7ba3b4 100644
--- a/arch/powerpc/include/asm/iommu.h
+++ b/arch/powerpc/include/asm/iommu.h
@@ -201,17 +201,19 @@ struct iommu_table_group {
 
 #ifdef CONFIG_IOMMU_API
 
-extern void iommu_register_group(struct iommu_table_group *table_group,
-int pci_domain_number, unsigned long pe_num);
+extern void iommu_register_table_group(struct iommu_table_group *table_group,
+   int pci_domain_number, unsigned long pe_num,
+   void (*release)(void *iommu_data));
 extern int iommu_add_device(struct device *dev);
 extern void iommu_del_device(struct device *dev);
 extern int __init tce_iommu_bus_notifier_init(void);
 extern long iommu_tce_xchg(struct iommu_table *tbl, unsigned long entry,
unsigned long *hpa, enum dma_data_direction *direction);
 #else
-static inline void iommu_register_group(struct iommu_table_group *table_group,
-   int pci_domain_number,
-   unsigned long pe_num)
+static inline void iommu_register_table_group(
+   struct iommu_table_group *table_group,
+   int pci_domain_number, unsigned long pe_num,
+   void (*release)(void *iommu_data))
 {
 }
 
diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
index a8e3490..8eed2fa 100644
--- a/arch/powerpc/kernel/iommu.c
+++ b/arch/powerpc/kernel/iommu.c
@@ -887,15 +887,9 @@ EXPORT_SYMBOL_GPL(iommu_direction_to_tce_perm);
 /*
  * SPAPR TCE API
  */
-static void group_release(void *iommu_data)
-{
-   struct iommu_table_group *table_group = iommu_data;
-
-   table_group->group = NULL;
-}
-
-void iommu_register_group(struct iommu_table_group *table_group,
-   int pci_domain_number, unsigned long pe_num)
+void iommu_register_table_group(struct iommu_table_group *table_group,
+   int pci_domain_number, unsigned long pe_num,
+   void (*release)(void *iommu_data))
 {
struct iommu_group *grp;
char *name;
@@ -907,7 +901,7 @@ void iommu_register_group(struct iommu_table_group 
*table_group,
return;
}
table_group->group = grp;
-   iommu_group_set_iommudata(grp, table_group, group_release);
+   iommu_group_set_iommudata(grp, table_group, release);
name = kasprintf(GFP_KERNEL, "domain%d-pe%lx",
pci_domain_number, pe_num);
if (!name)
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c 
b/arch/powerpc/platforms/powernv/pci-ioda.c
index c5baaf3..ce9f2bf 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -1330,6 +1330,13 @@ static long pnv_pci_ioda2_unset_window(struct 
iommu_table_group *table_group,
int num);
 static void pnv_pci_ioda2_set_bypass(struct pnv_ioda_pe *pe, bool enable);
 
+static void pnv_pci_ioda2_group_release(void *iommu_data)
+{
+   struct iommu_table_group *table_group = iommu_data;
+
+   table_group->group = NULL;
+}
+
 static void pnv_pci_ioda2_release_dma_pe(struct pci_dev *dev, struct 
pnv_ioda_pe *pe)
 {
struct iommu_table*tbl;
@@ -1965,8 +1972,8 @@ static void pnv_pci_ioda_setup_dma_pe(struct pnv_phb *phb,
return;
 
tbl = pnv_pci_table_alloc(phb->hose->node);
-   iommu_register_group(>table_group, phb->hose->global_number,
-   pe->pe_number);
+   iommu_register_table_group(>table_group, phb->hose->global_number,
+   pe->pe_number, NULL);
pnv_pci_link_table_and_group(phb->hose->node, 0, tbl, >table_group);
 
/* Grab a 32-bit TCE table */
@@ -2450,8