| Message ID | 167564538227.847146.16305045998592488364.stgit@dwillia2-xfh.jf.intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | CXL RAM and the 'Soft Reserved' => 'System RAM' default | expand |
On Sun, 05 Feb 2023 17:03:02 -0800 Dan Williams <dan.j.williams@intel.com> wrote: > Region autodiscovery is the process of kernel creating 'struct > cxl_region' object to represent active CXL memory ranges it finds > already active in hardware when the driver loads. Typically this happens > when platform firmware establishes CXL memory regions and then publishes > them in the memory map. However, this can also happen in the case of > kexec-reboot after the kernel has created regions. > > In the autodiscovery case the region creation process starts with a > known endpoint decoder. Refactor attach_target() into a helper that is > suitable to be called from either sysfs, for runtime region creation, or > from cxl_port_probe() after it has enumerated all endpoint decoders. > > The cxl_port_probe() context is an async device-core probing context, so > it is not appropriate to allow SIGTERM to interrupt the assembly > process. Refactor attach_target() to take @cxled and @state as arguments > where @state indicates whether waiting from the region rwsem is > interruptible or not. As below, I'd have broken this change out as a follow up patch - that way reviewer could check for strict noop refactor, then look in ioslation at that case. I don't care that much though as second patch would only have about 4 lines of diff. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > No behavior change is intended. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/cxl/core/region.c | 47 +++++++++++++++++++++++++++------------------ > 1 file changed, 28 insertions(+), 19 deletions(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 8dea49c021b8..97eafdd75675 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -1418,31 +1418,25 @@ void cxl_decoder_kill_region(struct cxl_endpoint_decoder *cxled) > up_write(&cxl_region_rwsem); > } > > -static int attach_target(struct cxl_region *cxlr, const char *decoder, int pos) > +static int attach_target(struct cxl_region *cxlr, > + struct cxl_endpoint_decoder *cxled, int pos, > + unsigned int state) > { > - struct device *dev; > - int rc; > - > - dev = bus_find_device_by_name(&cxl_bus_type, NULL, decoder); > - if (!dev) > - return -ENODEV; > - > - if (!is_endpoint_decoder(dev)) { > - put_device(dev); > - return -EINVAL; > - } > + int rc = 0; > > - rc = down_write_killable(&cxl_region_rwsem); > + if (state == TASK_INTERRUPTIBLE) > + rc = down_write_killable(&cxl_region_rwsem); > + else > + down_write(&cxl_region_rwsem); I'd be tempted to do this in two hops for patch readability. First make the code reorg then follow up with this bit before the use of it in the next patch. > if (rc) > - goto out; > + return rc; > + > down_read(&cxl_dpa_rwsem); > - rc = cxl_region_attach(cxlr, to_cxl_endpoint_decoder(dev), pos); > + rc = cxl_region_attach(cxlr, cxled, pos); > if (rc == 0) > set_bit(CXL_REGION_F_INCOHERENT, &cxlr->flags); > up_read(&cxl_dpa_rwsem); > up_write(&cxl_region_rwsem); > -out: > - put_device(dev); > return rc; > } > > @@ -1480,8 +1474,23 @@ static size_t store_targetN(struct cxl_region *cxlr, const char *buf, int pos, > > if (sysfs_streq(buf, "\n")) > rc = detach_target(cxlr, pos); > - else > - rc = attach_target(cxlr, buf, pos); > + else { > + struct device *dev; > + > + dev = bus_find_device_by_name(&cxl_bus_type, NULL, buf); > + if (!dev) > + return -ENODEV; > + > + if (!is_endpoint_decoder(dev)) { > + rc = -EINVAL; > + goto out; > + } > + > + rc = attach_target(cxlr, to_cxl_endpoint_decoder(dev), pos, > + TASK_INTERRUPTIBLE); > +out: > + put_device(dev); > + } > > if (rc < 0) > return rc; > >
Jonathan Cameron wrote: > On Sun, 05 Feb 2023 17:03:02 -0800 > Dan Williams <dan.j.williams@intel.com> wrote: > > > Region autodiscovery is the process of kernel creating 'struct > > cxl_region' object to represent active CXL memory ranges it finds > > already active in hardware when the driver loads. Typically this happens > > when platform firmware establishes CXL memory regions and then publishes > > them in the memory map. However, this can also happen in the case of > > kexec-reboot after the kernel has created regions. > > > > In the autodiscovery case the region creation process starts with a > > known endpoint decoder. Refactor attach_target() into a helper that is > > suitable to be called from either sysfs, for runtime region creation, or > > from cxl_port_probe() after it has enumerated all endpoint decoders. > > > > The cxl_port_probe() context is an async device-core probing context, so > > it is not appropriate to allow SIGTERM to interrupt the assembly > > process. Refactor attach_target() to take @cxled and @state as arguments > > where @state indicates whether waiting from the region rwsem is > > interruptible or not. > [..] > > @@ -1418,31 +1418,25 @@ void cxl_decoder_kill_region(struct cxl_endpoint_decoder *cxled) > > up_write(&cxl_region_rwsem); > > } > > > > -static int attach_target(struct cxl_region *cxlr, const char *decoder, int pos) > > +static int attach_target(struct cxl_region *cxlr, > > + struct cxl_endpoint_decoder *cxled, int pos, > > + unsigned int state) > > { > > - struct device *dev; > > - int rc; > > - > > - dev = bus_find_device_by_name(&cxl_bus_type, NULL, decoder); > > - if (!dev) > > - return -ENODEV; > > - > > - if (!is_endpoint_decoder(dev)) { > > - put_device(dev); > > - return -EINVAL; > > - } > > + int rc = 0; > > > > - rc = down_write_killable(&cxl_region_rwsem); > > + if (state == TASK_INTERRUPTIBLE) > > + rc = down_write_killable(&cxl_region_rwsem); > > + else > > + down_write(&cxl_region_rwsem); > > I'd be tempted to do this in two hops for patch readability. First > make the code reorg then follow up with this bit before the use > of it in the next patch. > Sure.
Dan Williams wrote: > Region autodiscovery is the process of kernel creating 'struct > cxl_region' object to represent active CXL memory ranges it finds > already active in hardware when the driver loads. Typically this happens > when platform firmware establishes CXL memory regions and then publishes > them in the memory map. However, this can also happen in the case of > kexec-reboot after the kernel has created regions. > > In the autodiscovery case the region creation process starts with a > known endpoint decoder. Refactor attach_target() into a helper that is > suitable to be called from either sysfs, for runtime region creation, or > from cxl_port_probe() after it has enumerated all endpoint decoders. > > The cxl_port_probe() context is an async device-core probing context, so > it is not appropriate to allow SIGTERM to interrupt the assembly > process. Refactor attach_target() to take @cxled and @state as arguments > where @state indicates whether waiting from the region rwsem is > interruptible or not. > > No behavior change is intended. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
On 2/5/23 6:03 PM, Dan Williams wrote: > Region autodiscovery is the process of kernel creating 'struct > cxl_region' object to represent active CXL memory ranges it finds > already active in hardware when the driver loads. Typically this happens > when platform firmware establishes CXL memory regions and then publishes > them in the memory map. However, this can also happen in the case of > kexec-reboot after the kernel has created regions. > > In the autodiscovery case the region creation process starts with a > known endpoint decoder. Refactor attach_target() into a helper that is > suitable to be called from either sysfs, for runtime region creation, or > from cxl_port_probe() after it has enumerated all endpoint decoders. > > The cxl_port_probe() context is an async device-core probing context, so > it is not appropriate to allow SIGTERM to interrupt the assembly > process. Refactor attach_target() to take @cxled and @state as arguments > where @state indicates whether waiting from the region rwsem is > interruptible or not. > > No behavior change is intended. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/cxl/core/region.c | 47 +++++++++++++++++++++++++++------------------ > 1 file changed, 28 insertions(+), 19 deletions(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 8dea49c021b8..97eafdd75675 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -1418,31 +1418,25 @@ void cxl_decoder_kill_region(struct cxl_endpoint_decoder *cxled) > up_write(&cxl_region_rwsem); > } > > -static int attach_target(struct cxl_region *cxlr, const char *decoder, int pos) > +static int attach_target(struct cxl_region *cxlr, > + struct cxl_endpoint_decoder *cxled, int pos, > + unsigned int state) > { > - struct device *dev; > - int rc; > - > - dev = bus_find_device_by_name(&cxl_bus_type, NULL, decoder); > - if (!dev) > - return -ENODEV; > - > - if (!is_endpoint_decoder(dev)) { > - put_device(dev); > - return -EINVAL; > - } > + int rc = 0; > > - rc = down_write_killable(&cxl_region_rwsem); > + if (state == TASK_INTERRUPTIBLE) > + rc = down_write_killable(&cxl_region_rwsem); > + else > + down_write(&cxl_region_rwsem); > if (rc) > - goto out; > + return rc; > + > down_read(&cxl_dpa_rwsem); > - rc = cxl_region_attach(cxlr, to_cxl_endpoint_decoder(dev), pos); > + rc = cxl_region_attach(cxlr, cxled, pos); > if (rc == 0) > set_bit(CXL_REGION_F_INCOHERENT, &cxlr->flags); > up_read(&cxl_dpa_rwsem); > up_write(&cxl_region_rwsem); > -out: > - put_device(dev); > return rc; > } > > @@ -1480,8 +1474,23 @@ static size_t store_targetN(struct cxl_region *cxlr, const char *buf, int pos, > > if (sysfs_streq(buf, "\n")) > rc = detach_target(cxlr, pos); > - else > - rc = attach_target(cxlr, buf, pos); > + else { > + struct device *dev; > + > + dev = bus_find_device_by_name(&cxl_bus_type, NULL, buf); > + if (!dev) > + return -ENODEV; > + > + if (!is_endpoint_decoder(dev)) { > + rc = -EINVAL; > + goto out; > + } > + > + rc = attach_target(cxlr, to_cxl_endpoint_decoder(dev), pos, > + TASK_INTERRUPTIBLE); > +out: > + put_device(dev); > + } > > if (rc < 0) > return rc; >
On Sun, 2023-02-05 at 17:03 -0800, Dan Williams wrote: > Region autodiscovery is the process of kernel creating 'struct > cxl_region' object to represent active CXL memory ranges it finds > already active in hardware when the driver loads. Typically this happens > when platform firmware establishes CXL memory regions and then publishes > them in the memory map. However, this can also happen in the case of > kexec-reboot after the kernel has created regions. > > In the autodiscovery case the region creation process starts with a > known endpoint decoder. Refactor attach_target() into a helper that is > suitable to be called from either sysfs, for runtime region creation, or > from cxl_port_probe() after it has enumerated all endpoint decoders. > > The cxl_port_probe() context is an async device-core probing context, so > it is not appropriate to allow SIGTERM to interrupt the assembly > process. Refactor attach_target() to take @cxled and @state as arguments > where @state indicates whether waiting from the region rwsem is > interruptible or not. > > No behavior change is intended. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/cxl/core/region.c | 47 +++++++++++++++++++++++++++------------------ > 1 file changed, 28 insertions(+), 19 deletions(-) Looks good, Reviewed-by: Vishal Verma <vishal.l.verma@intel.com> > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 8dea49c021b8..97eafdd75675 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -1418,31 +1418,25 @@ void cxl_decoder_kill_region(struct cxl_endpoint_decoder *cxled) > up_write(&cxl_region_rwsem); > } > > -static int attach_target(struct cxl_region *cxlr, const char *decoder, int pos) > +static int attach_target(struct cxl_region *cxlr, > + struct cxl_endpoint_decoder *cxled, int pos, > + unsigned int state) > { > - struct device *dev; > - int rc; > - > - dev = bus_find_device_by_name(&cxl_bus_type, NULL, decoder); > - if (!dev) > - return -ENODEV; > - > - if (!is_endpoint_decoder(dev)) { > - put_device(dev); > - return -EINVAL; > - } > + int rc = 0; > > - rc = down_write_killable(&cxl_region_rwsem); > + if (state == TASK_INTERRUPTIBLE) > + rc = down_write_killable(&cxl_region_rwsem); > + else > + down_write(&cxl_region_rwsem); > if (rc) > - goto out; > + return rc; > + > down_read(&cxl_dpa_rwsem); > - rc = cxl_region_attach(cxlr, to_cxl_endpoint_decoder(dev), pos); > + rc = cxl_region_attach(cxlr, cxled, pos); > if (rc == 0) > set_bit(CXL_REGION_F_INCOHERENT, &cxlr->flags); > up_read(&cxl_dpa_rwsem); > up_write(&cxl_region_rwsem); > -out: > - put_device(dev); > return rc; > } > > @@ -1480,8 +1474,23 @@ static size_t store_targetN(struct cxl_region *cxlr, const char *buf, int pos, > > if (sysfs_streq(buf, "\n")) > rc = detach_target(cxlr, pos); > - else > - rc = attach_target(cxlr, buf, pos); > + else { > + struct device *dev; > + > + dev = bus_find_device_by_name(&cxl_bus_type, NULL, buf); > + if (!dev) > + return -ENODEV; > + > + if (!is_endpoint_decoder(dev)) { > + rc = -EINVAL; > + goto out; > + } > + > + rc = attach_target(cxlr, to_cxl_endpoint_decoder(dev), pos, > + TASK_INTERRUPTIBLE); > +out: > + put_device(dev); > + } > > if (rc < 0) > return rc; >
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 8dea49c021b8..97eafdd75675 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -1418,31 +1418,25 @@ void cxl_decoder_kill_region(struct cxl_endpoint_decoder *cxled) up_write(&cxl_region_rwsem); } -static int attach_target(struct cxl_region *cxlr, const char *decoder, int pos) +static int attach_target(struct cxl_region *cxlr, + struct cxl_endpoint_decoder *cxled, int pos, + unsigned int state) { - struct device *dev; - int rc; - - dev = bus_find_device_by_name(&cxl_bus_type, NULL, decoder); - if (!dev) - return -ENODEV; - - if (!is_endpoint_decoder(dev)) { - put_device(dev); - return -EINVAL; - } + int rc = 0; - rc = down_write_killable(&cxl_region_rwsem); + if (state == TASK_INTERRUPTIBLE) + rc = down_write_killable(&cxl_region_rwsem); + else + down_write(&cxl_region_rwsem); if (rc) - goto out; + return rc; + down_read(&cxl_dpa_rwsem); - rc = cxl_region_attach(cxlr, to_cxl_endpoint_decoder(dev), pos); + rc = cxl_region_attach(cxlr, cxled, pos); if (rc == 0) set_bit(CXL_REGION_F_INCOHERENT, &cxlr->flags); up_read(&cxl_dpa_rwsem); up_write(&cxl_region_rwsem); -out: - put_device(dev); return rc; } @@ -1480,8 +1474,23 @@ static size_t store_targetN(struct cxl_region *cxlr, const char *buf, int pos, if (sysfs_streq(buf, "\n")) rc = detach_target(cxlr, pos); - else - rc = attach_target(cxlr, buf, pos); + else { + struct device *dev; + + dev = bus_find_device_by_name(&cxl_bus_type, NULL, buf); + if (!dev) + return -ENODEV; + + if (!is_endpoint_decoder(dev)) { + rc = -EINVAL; + goto out; + } + + rc = attach_target(cxlr, to_cxl_endpoint_decoder(dev), pos, + TASK_INTERRUPTIBLE); +out: + put_device(dev); + } if (rc < 0) return rc;
Region autodiscovery is the process of kernel creating 'struct cxl_region' object to represent active CXL memory ranges it finds already active in hardware when the driver loads. Typically this happens when platform firmware establishes CXL memory regions and then publishes them in the memory map. However, this can also happen in the case of kexec-reboot after the kernel has created regions. In the autodiscovery case the region creation process starts with a known endpoint decoder. Refactor attach_target() into a helper that is suitable to be called from either sysfs, for runtime region creation, or from cxl_port_probe() after it has enumerated all endpoint decoders. The cxl_port_probe() context is an async device-core probing context, so it is not appropriate to allow SIGTERM to interrupt the assembly process. Refactor attach_target() to take @cxled and @state as arguments where @state indicates whether waiting from the region rwsem is interruptible or not. No behavior change is intended. Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/cxl/core/region.c | 47 +++++++++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 19 deletions(-)