[PATCH] cxl/region: reset the cxl endpoint decoder HPA range on teardown

[PATCH] cxl/region: reset the cxl endpoint decoder HPA range on teardown

[Vishal Verma]

The endpoint decoder HPA range gets set up during cxl_region_attach, but
cxl_region_detach() neglects to reset it back to a 'free' state. As a
result, a create-destroy-create cycle with the same region parameters
results in the second create-region failing with an EBUSY when trying to
reserver DPA.

  # cxl create-region -d decoder3.2 -m mem4 -g 1024
  cxl region: create_region: decoder11.1: set_dpa_size failed: Device or resource busy
  cxl region: cmd_create_region: created 0 regions

Set the endpoint decoder's hpa_range to [0, -1] to mark it as free in
cxl_region_detach().

Fixes: 292bdc6af8f2 ("cxl/region: Move HPA setup to cxl_region_attach()")
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 drivers/cxl/core/region.c | 4 ++++
 1 file changed, 4 insertions(+)

Hi Dan,

This is a fixup-to-a-fixup, and the commit in Fixes: is only in
'preview'. Feel free to squash this into the original fix if you so
prefer.

Another question - should we be resetting the endpoint decoder's mode to
CXL_DECODER_DEAD while in here? I notice that a freed decoder still
lists mode:pmem in cxl-list.

---

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index a68e4e0cf169..f0c8966a69c1 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1369,6 +1369,10 @@ static int cxl_region_detach(struct cxl_endpoint_decoder *cxled)
 	}
 	p->targets[cxled->pos] = NULL;
 	p->nr_targets--;
+	cxled->cxld.hpa_range = (struct range) {
+		.start = 0,
+		.end = -1,
+	};
 
 	/* notify the region driver that one of its targets has departed */
 	up_write(&cxl_region_rwsem);

base-commit: 65fc1c3d26b96002a5aa1f4012fae4dc98fd5683
prerequisite-patch-id: 151ffe5b355fcc040b69b651037b239b28ce459d
prerequisite-patch-id: 5c3f0d23262513eb991f7db058736e20b601a897
prerequisite-patch-id: 63825d8f66bf5eda9fe23ab3d995de15a516754c
prerequisite-patch-id: d5ea0349921af5ce2670b4a0f9cc09662e8d6163
prerequisite-patch-id: 6f974183c2c44c4d5b32f2496af7935ec59bc402
-- 
2.37.1

RE: [PATCH] cxl/region: reset the cxl endpoint decoder HPA range on teardown

[Dan Williams]

Vishal Verma wrote:
> The endpoint decoder HPA range gets set up during cxl_region_attach, but
> cxl_region_detach() neglects to reset it back to a 'free' state. As a
> result, a create-destroy-create cycle with the same region parameters
> results in the second create-region failing with an EBUSY when trying to
> reserver DPA.
> 
>   # cxl create-region -d decoder3.2 -m mem4 -g 1024
>   cxl region: create_region: decoder11.1: set_dpa_size failed: Device or resource busy
>   cxl region: cmd_create_region: created 0 regions
> 
> Set the endpoint decoder's hpa_range to [0, -1] to mark it as free in
> cxl_region_detach().
> 
> Fixes: 292bdc6af8f2 ("cxl/region: Move HPA setup to cxl_region_attach()")
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  drivers/cxl/core/region.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> Hi Dan,
> 
> This is a fixup-to-a-fixup, and the commit in Fixes: is only in
> 'preview'. Feel free to squash this into the original fix if you so
> prefer.

Will do, I think this also needs to kill the cxld_clear_hpa() helper and
call in cxl_decoder_reset().

> Another question - should we be resetting the endpoint decoder's mode to
> CXL_DECODER_DEAD while in here? I notice that a freed decoder still
> lists mode:pmem in cxl-list.

The DEAD state is a internal-only hack while the decoder is being torn
down, it's not visible to userspace. Perhaps just hide the none state
and default the mode to 'ram' when no DPA is allocated?

Re: [PATCH] cxl/region: reset the cxl endpoint decoder HPA range on teardown

[Verma, Vishal L]

On Fri, 2022-08-05 at 01:07 -0700, Dan Williams wrote:
> Vishal Verma wrote:
> > The endpoint decoder HPA range gets set up during cxl_region_attach, but
> > cxl_region_detach() neglects to reset it back to a 'free' state. As a
> > result, a create-destroy-create cycle with the same region parameters
> > results in the second create-region failing with an EBUSY when trying to
> > reserver DPA.
> > 
> >   # cxl create-region -d decoder3.2 -m mem4 -g 1024
> >   cxl region: create_region: decoder11.1: set_dpa_size failed: Device or resource busy
> >   cxl region: cmd_create_region: created 0 regions
> > 
> > Set the endpoint decoder's hpa_range to [0, -1] to mark it as free in
> > cxl_region_detach().
> > 
> > Fixes: 292bdc6af8f2 ("cxl/region: Move HPA setup to cxl_region_attach()")
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  drivers/cxl/core/region.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > Hi Dan,
> > 
> > This is a fixup-to-a-fixup, and the commit in Fixes: is only in
> > 'preview'. Feel free to squash this into the original fix if you so
> > prefer.
> 
> Will do, I think this also needs to kill the cxld_clear_hpa() helper and
> call in cxl_decoder_reset().
> 
> > Another question - should we be resetting the endpoint decoder's mode to
> > CXL_DECODER_DEAD while in here? I notice that a freed decoder still
> > lists mode:pmem in cxl-list.
> 
> The DEAD state is a internal-only hack while the decoder is being torn
> down, it's not visible to userspace. Perhaps just hide the none state
> and default the mode to 'ram' when no DPA is allocated?

Yeah that seems reasonable.