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",
> > 1<<afu->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, 0xFFFFFFFFFFFFFFFFULL);
> > @@ -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 mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
