Messages in this thread | ![/](/images/icornerl.gif) | | Date | Sun, 5 May 2024 20:46:59 -0700 | From | Ira Weiny <> | Subject | Re: [PATCH 05/26] cxl/core: Simplify cxl_dpa_set_mode() |
| |
Dan Williams wrote: > Ira Weiny wrote: > > Alison Schofield wrote: > > > On Sun, Mar 24, 2024 at 04:18:08PM -0700, Ira Weiny wrote: > > > > cxl_dpa_set_mode() checks the mode for validity two times, once outside > > > > of the DPA RW semaphore and again within. > > > > > > Not true. > > > > Sorry for not being clear. It does check the mode 2x but not for > > validity. I'll clarify. > > > > > It only checks mode once before the lock. It checks for > > > capacity after the lock. If it didn't check mode before the lock, > > > then unsupported modes would fall through. > > > > But we can check the mode 1 time and either check the size or fail. > > > > > > > > > The function is not in a critical path. > > > > > > Implying what here? OK to check twice (even though it wasn't) > > > or OK to expand scope of locking. > > > > Implying that checking the mode outside the lock is not required. > > > > > > > > > Prior to Dynamic Capacity the extra check was not much > > > > of an issue. The addition of DC modes increases the complexity of > > > > the check. > > > > > > > > Simplify the mode check before adding the more complex DC modes. > > > > > > > > > > The addition of the DC mode check doesn't seem complex. > > > > It is if you have to check it 2 times. > > > > > > > > Pardon my picking at the words, but if you'd like to refactor the > > > function, just say so. The final result is a bit more readable, but > > > also adding the DC mode checks without refactoring would read fine > > > also. > > > > When I added the DC mode to this function without this refactoring it was > > quite a bit more code and ugly IMO. So this cleanup helped. If I were > > not adding the DC code there would be much less reason to change this > > function. > > Where did the "quite a bit more code" come from? A change that moves > unnecessary code under a lock and is larger than just incrementally > extending the status quo does not feel like a cleanup.
I'll drop the patch.
Ira
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > index 7d97790b893d..0dc886bc22c6 100644 > --- a/drivers/cxl/core/hdm.c > +++ b/drivers/cxl/core/hdm.c > @@ -411,11 +411,12 @@ int cxl_dpa_set_mode(struct cxl_endpoint_decoder *cxled, > struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); > struct cxl_dev_state *cxlds = cxlmd->cxlds; > struct device *dev = &cxled->cxld.dev; > - int rc; > + int rc, dcd; > > switch (mode) { > case CXL_DECODER_RAM: > case CXL_DECODER_PMEM: > + case CXL_DECODER_DC0 ... CXL_DECODER_DC7: > break; > default: > dev_dbg(dev, "unsupported mode: %d\n", mode); > @@ -442,6 +443,11 @@ int cxl_dpa_set_mode(struct cxl_endpoint_decoder *cxled, > rc = -ENXIO; > goto out; > } > + dcd = dc_mode_to_region_index(mode); > + if (resource_size(&cxlds->dc_res[dcd]) == 0) { > + dev_dbg(dev, "no available dynamic capacity\n"); > + goto out; > + } > > cxled->mode = mode; > rc = 0;
| ![\](/images/icornerr.gif) |