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? 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.
>
> But I don't think this is wrong. So:
>
> Reviewed-by: Ira Weiny <[email protected]>
