diff mbox series

[02/46] cxl/port: Keep port->uport valid for the entire life of a port

Message ID 165603871491.551046.6682199179541194356.stgit@dwillia2-xfh
State New
Headers show
Series CXL PMEM Region Provisioning | expand

Commit Message

Dan Williams June 24, 2022, 2:45 a.m. UTC 
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(-)

Comments

Alison Schofield June 24, 2022, 3:37 a.m. UTC | #1 
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);
>  	}
>
Jonathan Cameron June 28, 2022, 11:47 a.m. UTC | #2 
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);
>  	}
>
Dan Williams June 28, 2022, 2:27 p.m. UTC | #3 
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.
Adam Manzanares June 29, 2022, 5:46 p.m. UTC | #4 
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 mbox series

Patch

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);
 	}