Re: [PATCH ndctl 3/7] cxl: add core plumbing for creation of ram regions

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

Verma, Vishal L wrote:
> On Tue, 2023-02-07 at 19:55 -0800, Ira Weiny wrote:
> > Vishal Verma wrote:
> <..>
> > > 
> > > diff --git a/cxl/region.c b/cxl/region.c
> > > index 38aa142..0945a14 100644
> > > --- a/cxl/region.c
> > > +++ b/cxl/region.c
> > > @@ -380,7 +380,22 @@ static void collect_minsize(struct cxl_ctx *ctx, struct parsed_params *p)
> > >                 struct json_object *jobj =
> > >                         json_object_array_get_idx(p->memdevs, i);
> > >                 struct cxl_memdev *memdev = json_object_get_userdata(jobj);
> > > -               u64 size = cxl_memdev_get_pmem_size(memdev);
> > > +               u64 size;
> > > +
> > > +               switch(p->mode) {
> > > +               case CXL_DECODER_MODE_RAM:
> > > +                       size = cxl_memdev_get_ram_size(memdev);
> > > +                       break;
> > > +               case CXL_DECODER_MODE_PMEM:
> > > +                       size = cxl_memdev_get_pmem_size(memdev);
> > > +                       break;
> > > +               default:
> > > +                       /*
> > > +                        * This will 'poison' ep_min_size with a 0, and
> > > +                        * subsequently cause the region creation to fail.
> > > +                        */
> > > +                       size = 0;
> > 
> > Why not change collect_minsize() to return int and propagate the error up
> > through create_region_validate_config()?
> > 
> > It seems more confusing to hide a special value in size like this.
> > 
> Hm, true, though the default case should never get hit. In fact I was
> planning to leave it out entirely until gcc warned that I can't skip
> cases if switching for an enum. I think the comment is maybe misleading
> in that it makes one think that this is some special handling. It would
> probably be clearer to make size = 0 in the initial declaration, and
> make the default case a no-op (maybe with a comment that we would never
> get here). Does that sound better?
> > 

To me the question is whether size == 0 is an error or just a valid size
which something at the higher level determines is an error.  To me the
default case here is that mode is incorrectly set to get a valid size.
Your comment implied that as well.

Having size == 0 is ok generally as this is a 'min size'.  So I will not
argue the point too much.

Ira