Re: [PATCH v2 02/10] cxl: Allocate and release the SPA with the AFU
On Tue, 28 Jul 2015 15:28:35 +1000
Daniel Axtens d...@axtens.net wrote:
Previously the SPA was allocated and freed upon entering and leaving
AFU-directed mode. This causes some issues for error recovery - contexts
hold a pointer inside the SPA, and they may persist after the AFU has
been detached.
We would ideally like to allocate the SPA when the AFU is allocated, and
release it until the AFU is released. However, we don't know how big the
SPA needs to be until we read the AFU descriptor.
Therefore, restructure the code:
- Allocate the SPA only once, on the first attach.
- Release the SPA only when the entire AFU is being released (not
detached). Guard the release with a NULL check, so we don't free
if it was never allocated (e.g. dedicated mode)
I'm sure you tested this :), looks fine to me, from an outsiders perspective
code appears to do what the commit message says.
Just one super minor question, you do the NULL check in the caller. How obvious
is the error if/when a caller forgets?
Acked-by: Cyril Bur cyril...@gmail.com
Signed-off-by: Daniel Axtens d...@axtens.net
---
drivers/misc/cxl/cxl.h| 3 +++
drivers/misc/cxl/native.c | 28 ++--
drivers/misc/cxl/pci.c| 3 +++
3 files changed, 24 insertions(+), 10 deletions(-)
diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
index 47eadbcfd379..88a88c445e2a 100644
--- a/drivers/misc/cxl/cxl.h
+++ b/drivers/misc/cxl/cxl.h
@@ -603,6 +603,9 @@ void unregister_cxl_calls(struct cxl_calls *calls);
int cxl_alloc_adapter_nr(struct cxl *adapter);
void cxl_remove_adapter_nr(struct cxl *adapter);
+int cxl_alloc_spa(struct cxl_afu *afu);
+void cxl_release_spa(struct cxl_afu *afu);
+
int cxl_file_init(void);
void cxl_file_exit(void);
int cxl_register_adapter(struct cxl *adapter);
diff --git a/drivers/misc/cxl/native.c b/drivers/misc/cxl/native.c
index 16948915eb0d..debd97147b58 100644
--- a/drivers/misc/cxl/native.c
+++ b/drivers/misc/cxl/native.c
@@ -182,10 +182,8 @@ static int spa_max_procs(int spa_size)
return ((spa_size / 8) - 96) / 17;
}
-static int alloc_spa(struct cxl_afu *afu)
+int cxl_alloc_spa(struct cxl_afu *afu)
{
- u64 spap;
-
/* Work out how many pages to allocate */
afu-spa_order = 0;
do {
@@ -204,6 +202,13 @@ static int alloc_spa(struct cxl_afu *afu)
pr_devel(spa pages: %i afu-spa_max_procs: %i afu-num_procs: %i\n,
1afu-spa_order, afu-spa_max_procs, afu-num_procs);
+ return 0;
+}
+
+static void attach_spa(struct cxl_afu *afu)
+{
+ u64 spap;
+
afu-sw_command_status = (__be64 *)((char *)afu-spa +
((afu-spa_max_procs + 3) * 128));
@@ -212,13 +217,15 @@ static int alloc_spa(struct cxl_afu *afu)
spap |= CXL_PSL_SPAP_V;
pr_devel(cxl: SPA allocated at 0x%p. Max processes: %i,
sw_command_status: 0x%p CXL_PSL_SPAP_An=0x%016llx\n, afu-spa,
afu-spa_max_procs, afu-sw_command_status, spap);
cxl_p1n_write(afu, CXL_PSL_SPAP_An, spap);
-
- return 0;
}
-static void release_spa(struct cxl_afu *afu)
+static inline void detach_spa(struct cxl_afu *afu)
{
cxl_p1n_write(afu, CXL_PSL_SPAP_An, 0);
+}
+
+void cxl_release_spa(struct cxl_afu *afu)
+{
free_pages((unsigned long) afu-spa, afu-spa_order);
}
@@ -446,8 +453,11 @@ static int activate_afu_directed(struct cxl_afu *afu)
dev_info(afu-dev, Activating AFU directed mode\n);
- if (alloc_spa(afu))
- return -ENOMEM;
+ if (afu-spa == NULL) {
+ if (cxl_alloc_spa(afu))
+ return -ENOMEM;
+ }
+ attach_spa(afu);
cxl_p1n_write(afu, CXL_PSL_SCNTL_An, CXL_PSL_SCNTL_An_PM_AFU);
cxl_p1n_write(afu, CXL_PSL_AMOR_An, 0xULL);
@@ -560,8 +570,6 @@ static int deactivate_afu_directed(struct cxl_afu *afu)
cxl_afu_disable(afu);
cxl_psl_purge(afu);
- release_spa(afu);
-
return 0;
}
diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index 32ad09705949..1849c1785b49 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c
@@ -551,6 +551,9 @@ static void cxl_release_afu(struct device *dev)
pr_devel(cxl_release_afu\n);
+ if (afu-spa)
+ cxl_release_spa(afu);
+
kfree(afu);
}
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 02/10] cxl: Allocate and release the SPA with the AFU
On Tue, 2015-08-11 at 13:42 +1000, Cyril Bur wrote:
On Tue, 28 Jul 2015 15:28:35 +1000
Daniel Axtens d...@axtens.net wrote:
Previously the SPA was allocated and freed upon entering and leaving
AFU-directed mode. This causes some issues for error recovery - contexts
hold a pointer inside the SPA, and they may persist after the AFU has
been detached.
We would ideally like to allocate the SPA when the AFU is allocated, and
release it until the AFU is released. However, we don't know how big the
SPA needs to be until we read the AFU descriptor.
Therefore, restructure the code:
- Allocate the SPA only once, on the first attach.
- Release the SPA only when the entire AFU is being released (not
detached). Guard the release with a NULL check, so we don't free
if it was never allocated (e.g. dedicated mode)
I'm sure you tested this :), looks fine to me, from an outsiders perspective
code appears to do what the commit message says.
Just one super minor question, you do the NULL check in the caller. How
obvious
is the error if/when a caller forgets?
Good point. The SPA is only released when releasing the entire AFU, so
it doesn't really matter at this point, but in case we ever move around
the assignment/release of the SPA (dynamic reprogramming comes to mind)
I've moved the check and added an explicit NULLing of the SPA pointer.
Acked-by: Cyril Bur cyril...@gmail.com
Thanks!
Signed-off-by: Daniel Axtens d...@axtens.net
---
drivers/misc/cxl/cxl.h| 3 +++
drivers/misc/cxl/native.c | 28 ++--
drivers/misc/cxl/pci.c| 3 +++
3 files changed, 24 insertions(+), 10 deletions(-)
diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
index 47eadbcfd379..88a88c445e2a 100644
--- a/drivers/misc/cxl/cxl.h
+++ b/drivers/misc/cxl/cxl.h
@@ -603,6 +603,9 @@ void unregister_cxl_calls(struct cxl_calls *calls);
int cxl_alloc_adapter_nr(struct cxl *adapter);
void cxl_remove_adapter_nr(struct cxl *adapter);
+int cxl_alloc_spa(struct cxl_afu *afu);
+void cxl_release_spa(struct cxl_afu *afu);
+
int cxl_file_init(void);
void cxl_file_exit(void);
int cxl_register_adapter(struct cxl *adapter);
diff --git a/drivers/misc/cxl/native.c b/drivers/misc/cxl/native.c
index 16948915eb0d..debd97147b58 100644
--- a/drivers/misc/cxl/native.c
+++ b/drivers/misc/cxl/native.c
@@ -182,10 +182,8 @@ static int spa_max_procs(int spa_size)
return ((spa_size / 8) - 96) / 17;
}
-static int alloc_spa(struct cxl_afu *afu)
+int cxl_alloc_spa(struct cxl_afu *afu)
{
- u64 spap;
-
/* Work out how many pages to allocate */
afu-spa_order = 0;
do {
@@ -204,6 +202,13 @@ static int alloc_spa(struct cxl_afu *afu)
pr_devel(spa pages: %i afu-spa_max_procs: %i afu-num_procs: %i\n,
1afu-spa_order, afu-spa_max_procs, afu-num_procs);
+ return 0;
+}
+
+static void attach_spa(struct cxl_afu *afu)
+{
+ u64 spap;
+
afu-sw_command_status = (__be64 *)((char *)afu-spa +
((afu-spa_max_procs + 3) * 128));
@@ -212,13 +217,15 @@ static int alloc_spa(struct cxl_afu *afu)
spap |= CXL_PSL_SPAP_V;
pr_devel(cxl: SPA allocated at 0x%p. Max processes: %i,
sw_command_status: 0x%p CXL_PSL_SPAP_An=0x%016llx\n, afu-spa,
afu-spa_max_procs, afu-sw_command_status, spap);
cxl_p1n_write(afu, CXL_PSL_SPAP_An, spap);
-
- return 0;
}
-static void release_spa(struct cxl_afu *afu)
+static inline void detach_spa(struct cxl_afu *afu)
{
cxl_p1n_write(afu, CXL_PSL_SPAP_An, 0);
+}
+
+void cxl_release_spa(struct cxl_afu *afu)
+{
free_pages((unsigned long) afu-spa, afu-spa_order);
}
@@ -446,8 +453,11 @@ static int activate_afu_directed(struct cxl_afu *afu)
dev_info(afu-dev, Activating AFU directed mode\n);
- if (alloc_spa(afu))
- return -ENOMEM;
+ if (afu-spa == NULL) {
+ if (cxl_alloc_spa(afu))
+ return -ENOMEM;
+ }
+ attach_spa(afu);
cxl_p1n_write(afu, CXL_PSL_SCNTL_An, CXL_PSL_SCNTL_An_PM_AFU);
cxl_p1n_write(afu, CXL_PSL_AMOR_An, 0xULL);
@@ -560,8 +570,6 @@ static int deactivate_afu_directed(struct cxl_afu *afu)
cxl_afu_disable(afu);
cxl_psl_purge(afu);
- release_spa(afu);
-
return 0;
}
diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index 32ad09705949..1849c1785b49 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c
@@ -551,6 +551,9 @@ static void cxl_release_afu(struct device *dev)
pr_devel(cxl_release_afu\n);
+ if (afu-spa)
+ cxl_release_spa(afu);
+
kfree(afu);
}
--
Regards,
Daniel
signature.asc
Description: This is a digitally signed message part
___
Linuxppc-dev
