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 <[email protected]>
My tag stands.
Ira
