Messages in this thread Patch in this message | | | Date | Mon, 11 Sep 2023 18:18:37 -0700 | From | Ira Weiny <> | Subject | Re: [PATCH RFC v2 03/18] cxl/mem: Read Dynamic capacity configuration from the device |
| |
Alison Schofield wrote: > On Mon, Aug 28, 2023 at 10:20:54PM -0700, Ira Weiny wrote: > > From: Navneet Singh <navneet.singh@intel.com> > > > > Devices can optionally support Dynamic Capacity (DC). These devices are > > known as Dynamic Capacity Devices (DCD). > > snip > > > > > +static enum cxl_region_mode > > +cxl_decoder_to_region_mode(enum cxl_decoder_mode mode) > > +{ > > + switch (mode) { > > + case CXL_DECODER_NONE: > > + return CXL_REGION_NONE; > > + case CXL_DECODER_RAM: > > + return CXL_REGION_RAM; > > + case CXL_DECODER_PMEM: > > + return CXL_REGION_PMEM; > > + case CXL_DECODER_DEAD: > > + return CXL_REGION_DEAD; > > + case CXL_DECODER_MIXED: > > + default: > > + return CXL_REGION_MIXED; > > + } > > + > > + return CXL_REGION_MIXED; > > Can the paths to return _MIXED be simplified here?
I suppose:
... case CXL_DECODER_MIXED: default: break; } return CXL_REGION_MIXED; ...
I don't think that makes things any better.
> > > > +} > > + > snip > > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > > index cd4a9ffdacc7..ed282dcd5cf5 100644 > > --- a/drivers/cxl/cxl.h > > +++ b/drivers/cxl/cxl.h > > @@ -374,6 +374,28 @@ static inline const char *cxl_decoder_mode_name(enum cxl_decoder_mode mode) > > return "mixed"; > > } > > > > +enum cxl_region_mode { > > + CXL_REGION_NONE, > > + CXL_REGION_RAM, > > + CXL_REGION_PMEM, > > + CXL_REGION_MIXED, > > + CXL_REGION_DEAD, > > +}; > > I'm concerned about _DEAD. > At first I was going to say name these as CXL_REGION_MODE_*, but it's > pretty obvious that these are mode words...except for DEAD. Is that > an actual mode or is it some type of status? I don't think I see it > used yet.
My first reaction was to remove this. But I had to go back and look. It took me a minute to trace this.
'Dead' is not used directly. If a decoder happens to be dead (CXL_DECODER_DEAD) then it will eventually fail the creation of a region with CXL_REGION_DEAD as the mode. CXL_REGION_MIXED fails the same way but only because mixed mode is not yet supported. Therefore, decoder mode DEAD indicates something different and CXL_REGION_DEAD was added to convey this when converting.
The alternative is to be more explicit and check decoder mode to be !DEAD prior to trying to convert. I think I like that but I'm going to sleep on it.
> > > + > > +static inline const char *cxl_region_mode_name(enum cxl_region_mode mode) > > +{ > > + static const char * const names[] = { > > + [CXL_REGION_NONE] = "none", > > + [CXL_REGION_RAM] = "ram", > > + [CXL_REGION_PMEM] = "pmem", > > + [CXL_REGION_MIXED] = "mixed", > > + }; > > + > > + if (mode >= CXL_REGION_NONE && mode <= CXL_REGION_MIXED) > > + return names[mode]; > > + return "mixed"; > > +} > > snip > > > + > > /** > > * struct cxl_memdev_state - Generic Type-3 Memory Device Class driver data > > * > > @@ -449,6 +464,8 @@ struct cxl_dev_state { > > * @enabled_cmds: Hardware commands found enabled in CEL. > > * @exclusive_cmds: Commands that are kernel-internal only > > * @total_bytes: sum of all possible capacities > > + * @static_cap: Sum of RAM and PMEM capacities > > + * @dynamic_cap: Complete DPA range occupied by DC regions > > Wondering about renaming RAM and PMEM caps as 'static'. > They are changeable via set partition commands.
True but they are static compared to dynamic capacity. I'm open to other names but !dynamic is normally referred to as static. :-/
Thanks for the review! Ira
| |