| Message ID | 165603871491.551046.6682199179541194356.stgit@dwillia2-xfh (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | CXL PMEM Region Provisioning | expand |
On Thu, Jun 23, 2022 at 07:45:14PM -0700, Dan Williams wrote: > The upcoming region provisioning implementation has a need to > dereference port->uport during the port unregister flow. Specifically, > endpoint decoders need to be able to lookup their corresponding memdev > via port->uport. > > The existing ->dead flag was added for cases where the core was > committed to tearing down the port, but needed to drop locks before > calling device_unregister(). Reuse that flag to indicate to > delete_endpoint() that it has no "release action" work to do as > unregister_port() will handle it. > > Fixes: 8dd2bc0f8e02 ("cxl/mem: Add the cxl_mem driver") > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Reviewed-by: Alison Schofield <alison.schofield@intel.com> > --- > drivers/cxl/core/port.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > index dbce99bdffab..7810d1a8369b 100644 > --- a/drivers/cxl/core/port.c > +++ b/drivers/cxl/core/port.c > @@ -370,7 +370,7 @@ static void unregister_port(void *_port) > lock_dev = &parent->dev; > > device_lock_assert(lock_dev); > - port->uport = NULL; > + port->dead = true; > device_unregister(&port->dev); > } > > @@ -857,7 +857,7 @@ static void delete_endpoint(void *data) > parent = &parent_port->dev; > > device_lock(parent); > - if (parent->driver && endpoint->uport) { > + if (parent->driver && !endpoint->dead) { > devm_release_action(parent, cxl_unlink_uport, endpoint); > devm_release_action(parent, unregister_port, endpoint); > } >
On Thu, 23 Jun 2022 19:45:14 -0700 Dan Williams <dan.j.williams@intel.com> wrote: > The upcoming region provisioning implementation has a need to > dereference port->uport during the port unregister flow. Specifically, > endpoint decoders need to be able to lookup their corresponding memdev > via port->uport. > > The existing ->dead flag was added for cases where the core was > committed to tearing down the port, but needed to drop locks before > calling device_unregister(). Reuse that flag to indicate to > delete_endpoint() that it has no "release action" work to do as > unregister_port() will handle it. > > Fixes: 8dd2bc0f8e02 ("cxl/mem: Add the cxl_mem driver") From the explanation I'm not seeing why this has a fixes tag? Otherwise seems fine... > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/cxl/core/port.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > index dbce99bdffab..7810d1a8369b 100644 > --- a/drivers/cxl/core/port.c > +++ b/drivers/cxl/core/port.c > @@ -370,7 +370,7 @@ static void unregister_port(void *_port) > lock_dev = &parent->dev; > > device_lock_assert(lock_dev); > - port->uport = NULL; > + port->dead = true; > device_unregister(&port->dev); > } > > @@ -857,7 +857,7 @@ static void delete_endpoint(void *data) > parent = &parent_port->dev; > > device_lock(parent); > - if (parent->driver && endpoint->uport) { > + if (parent->driver && !endpoint->dead) { > devm_release_action(parent, cxl_unlink_uport, endpoint); > devm_release_action(parent, unregister_port, endpoint); > } >
Jonathan Cameron wrote: > On Thu, 23 Jun 2022 19:45:14 -0700 > Dan Williams <dan.j.williams@intel.com> wrote: > > > The upcoming region provisioning implementation has a need to > > dereference port->uport during the port unregister flow. Specifically, > > endpoint decoders need to be able to lookup their corresponding memdev > > via port->uport. > > > > The existing ->dead flag was added for cases where the core was > > committed to tearing down the port, but needed to drop locks before > > calling device_unregister(). Reuse that flag to indicate to > > delete_endpoint() that it has no "release action" work to do as > > unregister_port() will handle it. > > > > Fixes: 8dd2bc0f8e02 ("cxl/mem: Add the cxl_mem driver") > > From the explanation I'm not seeing why this has a fixes tag? True, that can be dropped as the crash scenario that found the need for this was not relevant at that older baseline.
On Thu, Jun 23, 2022 at 07:45:14PM -0700, Dan Williams wrote: > The upcoming region provisioning implementation has a need to > dereference port->uport during the port unregister flow. Specifically, > endpoint decoders need to be able to lookup their corresponding memdev > via port->uport. > > The existing ->dead flag was added for cases where the core was > committed to tearing down the port, but needed to drop locks before > calling device_unregister(). Reuse that flag to indicate to > delete_endpoint() that it has no "release action" work to do as > unregister_port() will handle it. > > Fixes: 8dd2bc0f8e02 ("cxl/mem: Add the cxl_mem driver") > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/cxl/core/port.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > index dbce99bdffab..7810d1a8369b 100644 > --- a/drivers/cxl/core/port.c > +++ b/drivers/cxl/core/port.c > @@ -370,7 +370,7 @@ static void unregister_port(void *_port) > lock_dev = &parent->dev; > > device_lock_assert(lock_dev); > - port->uport = NULL; > + port->dead = true; > device_unregister(&port->dev); > } > > @@ -857,7 +857,7 @@ static void delete_endpoint(void *data) > parent = &parent_port->dev; > > device_lock(parent); > - if (parent->driver && endpoint->uport) { > + if (parent->driver && !endpoint->dead) { > devm_release_action(parent, cxl_unlink_uport, endpoint); > devm_release_action(parent, unregister_port, endpoint); > } > > Looks good. Reviewed by: Adam Manzanares <a.manzanares@samsung.com>
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c index dbce99bdffab..7810d1a8369b 100644 --- a/drivers/cxl/core/port.c +++ b/drivers/cxl/core/port.c @@ -370,7 +370,7 @@ static void unregister_port(void *_port) lock_dev = &parent->dev; device_lock_assert(lock_dev); - port->uport = NULL; + port->dead = true; device_unregister(&port->dev); } @@ -857,7 +857,7 @@ static void delete_endpoint(void *data) parent = &parent_port->dev; device_lock(parent); - if (parent->driver && endpoint->uport) { + if (parent->driver && !endpoint->dead) { devm_release_action(parent, cxl_unlink_uport, endpoint); devm_release_action(parent, unregister_port, endpoint); }
The upcoming region provisioning implementation has a need to dereference port->uport during the port unregister flow. Specifically, endpoint decoders need to be able to lookup their corresponding memdev via port->uport. The existing ->dead flag was added for cases where the core was committed to tearing down the port, but needed to drop locks before calling device_unregister(). Reuse that flag to indicate to delete_endpoint() that it has no "release action" work to do as unregister_port() will handle it. Fixes: 8dd2bc0f8e02 ("cxl/mem: Add the cxl_mem driver") Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/cxl/core/port.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)