Re: [PATCH ndctl 5/7] cxl/region: determine region type based on root decoder capability

[Ira Weiny <ira.weiny@intel.com>]

Verma, Vishal L wrote:
> On Tue, 2023-02-07 at 20:07 -0800, Ira Weiny wrote:
> > Vishal Verma wrote:
> > > 
> > > diff --git a/cxl/region.c b/cxl/region.c
> > > index 9079b2d..1c8ccc7 100644
> > > --- a/cxl/region.c
> > > +++ b/cxl/region.c
> > > @@ -448,6 +448,31 @@ static int validate_decoder(struct cxl_decoder *decoder,
> > >         return 0;
> > >  }
> > >  
> > > +static void set_type_from_decoder(struct cxl_ctx *ctx, struct parsed_params *p)
> > > +{
> > > +       int num_cap = 0;
> > > +
> > > +       /* if param.type was explicitly specified, nothing to do here */
> > > +       if (param.type)
> > > +               return;
> > > +
> > > +       /*
> > > +        * if the root decoder only has one type of capability, default
> > > +        * to that mode for the region.
> > > +        */
> > > +       if (cxl_decoder_is_pmem_capable(p->root_decoder))
> > > +               num_cap++;
> > > +       if (cxl_decoder_is_volatile_capable(p->root_decoder))
> > > +               num_cap++;
> > > +
> > > +       if (num_cap == 1) {
> > > +               if (cxl_decoder_is_volatile_capable(p->root_decoder))
> > > +                       p->mode = CXL_DECODER_MODE_RAM;
> > > +               else if (cxl_decoder_is_pmem_capable(p->root_decoder))
> > > +                       p->mode = CXL_DECODER_MODE_PMEM;
> > > +       }
> > 
> > I feel like the default for p->mode should be moved here from
> > parse_create_options().  But I'm not sure what the flows might be like in
> > that case.  That means p->mode would default to NONE until here.
> > 
> > That would make the man page behavior and this function match up nicely
> > for future maintenance.
> 
> Hm, do they not match now?

As I said it is correct.

> I can see the appeal in collecting the
> default mode setup in one place, but in my mind the early checks /
> defaults in parse_create_options() are a simple, initial pass for
> canned defaults, and conflicting option checks. Things like
> set_type_from_decoder() are more of a 'second pass' thing where we may
> do more involved porcelain type decision making based on the full
> topology.

Yea I can see it that way too.

> 
> > 
> > But I don't think this is wrong.  So:
> > 
> > Reviewed-by: Ira Weiny <ira.weiny@intel.com>
 
My tag stands.
Ira