| Message ID | 20241209185429.54054-25-alejandro.lucero-palau@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl: add type2 device basic support | expand |
On 09/12/2024 18:54, alejandro.lucero-palau@amd.com wrote: > From: Alejandro Lucero <alucerop@amd.com> > > By definition a type2 cxl device will use the host managed memory for > specific functionality, therefore it should not be available to other > uses. However, a dax interface could be just good enough in some cases. > > Add a flag to a cxl region for specifically state to not create a dax > device. Allow a Type2 driver to set that flag at region creation time. > > Signed-off-by: Alejandro Lucero <alucerop@amd.com> > Reviewed-by: Zhi Wang <zhiw@nvidia.com> > --- > drivers/cxl/core/region.c | 10 +++++++++- > drivers/cxl/cxl.h | 3 +++ > drivers/cxl/cxlmem.h | 3 ++- > include/cxl/cxl.h | 3 ++- > 4 files changed, 16 insertions(+), 3 deletions(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index b014f2fab789..b39086356d74 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -3562,7 +3562,8 @@ __construct_new_region(struct cxl_root_decoder *cxlrd, > * cxl_region driver. > */ > struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd, > - struct cxl_endpoint_decoder *cxled) > + struct cxl_endpoint_decoder *cxled, > + bool no_dax) Won't this break bisectability? sfc won't build as of this commit because it tries to call cxl_create_region with the old signature. You could do the whole dance of having an interim API during the conversion, but seems simpler just to reorder the patches so that the no_dax parameter is added first before the caller is introduced.
On 12/11/24 02:31, Edward Cree wrote: > On 09/12/2024 18:54, alejandro.lucero-palau@amd.com wrote: >> From: Alejandro Lucero <alucerop@amd.com> >> >> By definition a type2 cxl device will use the host managed memory for >> specific functionality, therefore it should not be available to other >> uses. However, a dax interface could be just good enough in some cases. >> >> Add a flag to a cxl region for specifically state to not create a dax >> device. Allow a Type2 driver to set that flag at region creation time. >> >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >> Reviewed-by: Zhi Wang <zhiw@nvidia.com> >> --- >> drivers/cxl/core/region.c | 10 +++++++++- >> drivers/cxl/cxl.h | 3 +++ >> drivers/cxl/cxlmem.h | 3 ++- >> include/cxl/cxl.h | 3 ++- >> 4 files changed, 16 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c >> index b014f2fab789..b39086356d74 100644 >> --- a/drivers/cxl/core/region.c >> +++ b/drivers/cxl/core/region.c >> @@ -3562,7 +3562,8 @@ __construct_new_region(struct cxl_root_decoder *cxlrd, >> * cxl_region driver. >> */ >> struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd, >> - struct cxl_endpoint_decoder *cxled) >> + struct cxl_endpoint_decoder *cxled, >> + bool no_dax) > Won't this break bisectability? sfc won't build as of this commit > because it tries to call cxl_create_region with the old signature. > You could do the whole dance of having an interim API during the > conversion, but seems simpler just to reorder the patches so that > the no_dax parameter is added first before the caller is introduced. Oh. That's true. I wonder why the robot did not catch this! I thought it was building things after each patch in a patchset. I will change the order for properly using this in the sfc driver. Thanks!
On Mon, Dec 09, 2024 at 06:54:25PM +0000, alejandro.lucero-palau@amd.com wrote: > From: Alejandro Lucero <alucerop@amd.com> > > By definition a type2 cxl device will use the host managed memory for > specific functionality, therefore it should not be available to other > uses. However, a dax interface could be just good enough in some cases. > > Add a flag to a cxl region for specifically state to not create a dax > device. Allow a Type2 driver to set that flag at region creation time. > > Signed-off-by: Alejandro Lucero <alucerop@amd.com> > Reviewed-by: Zhi Wang <zhiw@nvidia.com> > --- > drivers/cxl/core/region.c | 10 +++++++++- > drivers/cxl/cxl.h | 3 +++ > drivers/cxl/cxlmem.h | 3 ++- > include/cxl/cxl.h | 3 ++- > 4 files changed, 16 insertions(+), 3 deletions(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index b014f2fab789..b39086356d74 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -3562,7 +3562,8 @@ __construct_new_region(struct cxl_root_decoder *cxlrd, > * cxl_region driver. > */ > struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd, > - struct cxl_endpoint_decoder *cxled) > + struct cxl_endpoint_decoder *cxled, > + bool no_dax) nit: no_dax should be added to the Kernel doc for this function. Also, I think you need to squash the following patch, which updates the caller to use pass the extra argument, into this patch. Or otherwise rework things slightly to avoid breaking bisection. > { > struct cxl_region *cxlr; > ...
On 12/12/24 18:44, Simon Horman wrote: > On Mon, Dec 09, 2024 at 06:54:25PM +0000, alejandro.lucero-palau@amd.com wrote: >> From: Alejandro Lucero <alucerop@amd.com> >> >> By definition a type2 cxl device will use the host managed memory for >> specific functionality, therefore it should not be available to other >> uses. However, a dax interface could be just good enough in some cases. >> >> Add a flag to a cxl region for specifically state to not create a dax >> device. Allow a Type2 driver to set that flag at region creation time. >> >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >> Reviewed-by: Zhi Wang <zhiw@nvidia.com> >> --- >> drivers/cxl/core/region.c | 10 +++++++++- >> drivers/cxl/cxl.h | 3 +++ >> drivers/cxl/cxlmem.h | 3 ++- >> include/cxl/cxl.h | 3 ++- >> 4 files changed, 16 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c >> index b014f2fab789..b39086356d74 100644 >> --- a/drivers/cxl/core/region.c >> +++ b/drivers/cxl/core/region.c >> @@ -3562,7 +3562,8 @@ __construct_new_region(struct cxl_root_decoder *cxlrd, >> * cxl_region driver. >> */ >> struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd, >> - struct cxl_endpoint_decoder *cxled) >> + struct cxl_endpoint_decoder *cxled, >> + bool no_dax) > nit: no_dax should be added to the Kernel doc for this function. Yes, I'll do. > > Also, I think you need to squash the following patch, which updates > the caller to use pass the extra argument, into this patch. Or otherwise > rework things slightly to avoid breaking bisection. Correct. Ed raised this concern as well, and I'll change the patches order in v8 for avoiding the problem. Thanks! >> { >> struct cxl_region *cxlr; >> > ... >
On Fri, Dec 13, 2024 at 09:47:42AM +0000, Alejandro Lucero Palau wrote: > > On 12/12/24 18:44, Simon Horman wrote: > > On Mon, Dec 09, 2024 at 06:54:25PM +0000, alejandro.lucero-palau@amd.com wrote: > > > From: Alejandro Lucero <alucerop@amd.com> > > > > > > By definition a type2 cxl device will use the host managed memory for > > > specific functionality, therefore it should not be available to other > > > uses. However, a dax interface could be just good enough in some cases. > > > > > > Add a flag to a cxl region for specifically state to not create a dax > > > device. Allow a Type2 driver to set that flag at region creation time. > > > > > > Signed-off-by: Alejandro Lucero <alucerop@amd.com> > > > Reviewed-by: Zhi Wang <zhiw@nvidia.com> > > > --- > > > drivers/cxl/core/region.c | 10 +++++++++- > > > drivers/cxl/cxl.h | 3 +++ > > > drivers/cxl/cxlmem.h | 3 ++- > > > include/cxl/cxl.h | 3 ++- > > > 4 files changed, 16 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > > > index b014f2fab789..b39086356d74 100644 > > > --- a/drivers/cxl/core/region.c > > > +++ b/drivers/cxl/core/region.c > > > @@ -3562,7 +3562,8 @@ __construct_new_region(struct cxl_root_decoder *cxlrd, > > > * cxl_region driver. > > > */ > > > struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd, > > > - struct cxl_endpoint_decoder *cxled) > > > + struct cxl_endpoint_decoder *cxled, > > > + bool no_dax) > > nit: no_dax should be added to the Kernel doc for this function. > > > Yes, I'll do. > > > > > > Also, I think you need to squash the following patch, which updates > > the caller to use pass the extra argument, into this patch. Or otherwise > > rework things slightly to avoid breaking bisection. > > > Correct. Ed raised this concern as well, and I'll change the patches order > in v8 for avoiding the problem. Thanks, and sorry for missing that Ed had already raised this. Likewise, thanks for responding to my other feedback on this patchset.
On Wed, 11 Dec 2024 09:23:10 +0000 Alejandro Lucero Palau <alucerop@amd.com> wrote: > On 12/11/24 02:31, Edward Cree wrote: > > On 09/12/2024 18:54, alejandro.lucero-palau@amd.com wrote: > >> From: Alejandro Lucero <alucerop@amd.com> > >> > >> By definition a type2 cxl device will use the host managed memory for > >> specific functionality, therefore it should not be available to other > >> uses. However, a dax interface could be just good enough in some cases. > >> > >> Add a flag to a cxl region for specifically state to not create a dax > >> device. Allow a Type2 driver to set that flag at region creation time. > >> > >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> > >> Reviewed-by: Zhi Wang <zhiw@nvidia.com> > >> --- > >> drivers/cxl/core/region.c | 10 +++++++++- > >> drivers/cxl/cxl.h | 3 +++ > >> drivers/cxl/cxlmem.h | 3 ++- > >> include/cxl/cxl.h | 3 ++- > >> 4 files changed, 16 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > >> index b014f2fab789..b39086356d74 100644 > >> --- a/drivers/cxl/core/region.c > >> +++ b/drivers/cxl/core/region.c > >> @@ -3562,7 +3562,8 @@ __construct_new_region(struct cxl_root_decoder *cxlrd, > >> * cxl_region driver. > >> */ > >> struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd, > >> - struct cxl_endpoint_decoder *cxled) > >> + struct cxl_endpoint_decoder *cxled, > >> + bool no_dax) > > Won't this break bisectability? sfc won't build as of this commit > > because it tries to call cxl_create_region with the old signature. > > You could do the whole dance of having an interim API during the > > conversion, but seems simpler just to reorder the patches so that > > the no_dax parameter is added first before the caller is introduced. > > > Oh. That's true. I wonder why the robot did not catch this! I thought it > was building things after each patch in a patchset. That would be fantastically more expensive. There were some talks on 0-day magic a while back. If I recall correctly it even merges what it thinks are unrelated trees on basis if the merge is fine, both trees probably are as well ;) The whole game of that system is maximum catching of bugs for minimum compile times! Jonathan > > I will change the order for properly using this in the sfc driver. > > Thanks! > > > >
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index b014f2fab789..b39086356d74 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -3562,7 +3562,8 @@ __construct_new_region(struct cxl_root_decoder *cxlrd, * cxl_region driver. */ struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd, - struct cxl_endpoint_decoder *cxled) + struct cxl_endpoint_decoder *cxled, + bool no_dax) { struct cxl_region *cxlr; @@ -3578,6 +3579,10 @@ struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd, drop_region(cxlr); return ERR_PTR(-ENODEV); } + + if (no_dax) + set_bit(CXL_REGION_F_NO_DAX, &cxlr->flags); + return cxlr; } EXPORT_SYMBOL_NS_GPL(cxl_create_region, "CXL"); @@ -3713,6 +3718,9 @@ static int cxl_region_probe(struct device *dev) if (rc) return rc; + if (test_bit(CXL_REGION_F_NO_DAX, &cxlr->flags)) + return 0; + switch (cxlr->mode) { case CXL_DECODER_PMEM: return devm_cxl_add_pmem_region(cxlr); diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 57d6dda3fb4a..cc9e3d859fa6 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -521,6 +521,9 @@ struct cxl_region_params { */ #define CXL_REGION_F_NEEDS_RESET 1 +/* Allow Type2 drivers to specify if a dax region should not be created. */ +#define CXL_REGION_F_NO_DAX 2 + /** * struct cxl_region - CXL region * @dev: This region's device diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index 9d874f1cb3bf..712f25f494e0 100644 --- a/drivers/cxl/cxlmem.h +++ b/drivers/cxl/cxlmem.h @@ -875,5 +875,6 @@ struct seq_file; struct dentry *cxl_debugfs_create_dir(const char *dir); void cxl_dpa_debug(struct seq_file *file, struct cxl_dev_state *cxlds); struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd, - struct cxl_endpoint_decoder *cxled); + struct cxl_endpoint_decoder *cxled, + bool no_dax); #endif /* __CXL_MEM_H__ */ diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h index e0ea5b801a2e..14be26358f9c 100644 --- a/include/cxl/cxl.h +++ b/include/cxl/cxl.h @@ -61,7 +61,8 @@ struct cxl_endpoint_decoder *cxl_request_dpa(struct cxl_memdev *cxlmd, resource_size_t max); int cxl_dpa_free(struct cxl_endpoint_decoder *cxled); struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd, - struct cxl_endpoint_decoder *cxled); + struct cxl_endpoint_decoder *cxled, + bool no_dax); int cxl_accel_region_detach(struct cxl_endpoint_decoder *cxled); #endif