| Message ID | 20230120-vv-volatile-regions-v1-1-b42b21ee8d0b@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl: add support for listing and creating volatile regions | expand |
On Tue, Feb 07, 2023 at 12:16:27PM -0700, Vishal Verma wrote: > Commit 3d6cd829ec08 ("cxl/region: Use cxl_filter_walk() to gather create-region targets") > removed the early return for create-region, and this caused a > create-region operation to unnecessarily loop through buses and root > decoders only to EINVAL out because ACTION_CREATE is handled outside of > the other actions. This results in confising messages such as: s/confising/confusing/ > > # cxl create-region -t ram -d 0.0 -m 0,4 > { > "region":"region7", > "resource":"0xf030000000", > "size":"512.00 MiB (536.87 MB)", > ... > } > cxl region: decoder_region_action: region0: failed: Invalid argument > cxl region: region_action: one or more failures, last failure: Invalid argument > cxl region: cmd_create_region: created 1 region > > Since there's no need to walk through the topology after creating a > region, and especially not to perform an invalid 'action', switch > back to retuening early for create-region. s/retuening/returning/ > > Fixes: 3d6cd829ec08 ("cxl/region: Use cxl_filter_walk() to gather create-region targets") > Cc: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > --- > cxl/region.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/cxl/region.c b/cxl/region.c > index efe05aa..38aa142 100644 > --- a/cxl/region.c > +++ b/cxl/region.c > @@ -789,7 +789,7 @@ static int region_action(int argc, const char **argv, struct cxl_ctx *ctx, > return rc; > > if (action == ACTION_CREATE) > - rc = create_region(ctx, count, p); > + return create_region(ctx, count, p); > > cxl_bus_foreach(ctx, bus) { > struct cxl_decoder *decoder; > > -- > 2.39.1 > >
On Tue, 2023-02-07 at 22:07 +0000, Fan Ni wrote: > On Tue, Feb 07, 2023 at 12:16:27PM -0700, Vishal Verma wrote: > > Commit 3d6cd829ec08 ("cxl/region: Use cxl_filter_walk() to gather create-region targets") > > removed the early return for create-region, and this caused a > > create-region operation to unnecessarily loop through buses and root > > decoders only to EINVAL out because ACTION_CREATE is handled outside of > > the other actions. This results in confising messages such as: > s/confising/confusing/ > > > > # cxl create-region -t ram -d 0.0 -m 0,4 > > { > > "region":"region7", > > "resource":"0xf030000000", > > "size":"512.00 MiB (536.87 MB)", > > ... > > } > > cxl region: decoder_region_action: region0: failed: Invalid argument > > cxl region: region_action: one or more failures, last failure: Invalid argument > > cxl region: cmd_create_region: created 1 region > > > > Since there's no need to walk through the topology after creating a > > region, and especially not to perform an invalid 'action', switch > > back to retuening early for create-region. > s/retuening/returning/ Thanks Fan! Fixed for v2. > > > > Fixes: 3d6cd829ec08 ("cxl/region: Use cxl_filter_walk() to gather create-region targets") > > Cc: Dan Williams <dan.j.williams@intel.com> > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > > --- > > cxl/region.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/cxl/region.c b/cxl/region.c > > index efe05aa..38aa142 100644 > > --- a/cxl/region.c > > +++ b/cxl/region.c > > @@ -789,7 +789,7 @@ static int region_action(int argc, const char **argv, struct cxl_ctx *ctx, > > return rc; > > > > if (action == ACTION_CREATE) > > - rc = create_region(ctx, count, p); > > + return create_region(ctx, count, p); > > > > cxl_bus_foreach(ctx, bus) { > > struct cxl_decoder *decoder; > > > > -- > > 2.39.1 > >
Vishal Verma wrote: > Commit 3d6cd829ec08 ("cxl/region: Use cxl_filter_walk() to gather create-region targets") > removed the early return for create-region, and this caused a > create-region operation to unnecessarily loop through buses and root > decoders only to EINVAL out because ACTION_CREATE is handled outside of > the other actions. This results in confising messages such as: > > # cxl create-region -t ram -d 0.0 -m 0,4 > { > "region":"region7", > "resource":"0xf030000000", > "size":"512.00 MiB (536.87 MB)", > ... > } > cxl region: decoder_region_action: region0: failed: Invalid argument > cxl region: region_action: one or more failures, last failure: Invalid argument > cxl region: cmd_create_region: created 1 region > > Since there's no need to walk through the topology after creating a > region, and especially not to perform an invalid 'action', switch > back to retuening early for create-region. > > Fixes: 3d6cd829ec08 ("cxl/region: Use cxl_filter_walk() to gather create-region targets") > Cc: Dan Williams <dan.j.williams@intel.com> Reviewed-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > --- > cxl/region.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/cxl/region.c b/cxl/region.c > index efe05aa..38aa142 100644 > --- a/cxl/region.c > +++ b/cxl/region.c > @@ -789,7 +789,7 @@ static int region_action(int argc, const char **argv, struct cxl_ctx *ctx, > return rc; > > if (action == ACTION_CREATE) > - rc = create_region(ctx, count, p); > + return create_region(ctx, count, p); > > cxl_bus_foreach(ctx, bus) { > struct cxl_decoder *decoder; > > -- > 2.39.1 > >
Vishal Verma wrote: > Commit 3d6cd829ec08 ("cxl/region: Use cxl_filter_walk() to gather create-region targets") > removed the early return for create-region, and this caused a > create-region operation to unnecessarily loop through buses and root > decoders only to EINVAL out because ACTION_CREATE is handled outside of > the other actions. This results in confising messages such as: > > # cxl create-region -t ram -d 0.0 -m 0,4 > { > "region":"region7", > "resource":"0xf030000000", > "size":"512.00 MiB (536.87 MB)", > ... > } > cxl region: decoder_region_action: region0: failed: Invalid argument > cxl region: region_action: one or more failures, last failure: Invalid argument > cxl region: cmd_create_region: created 1 region > > Since there's no need to walk through the topology after creating a > region, and especially not to perform an invalid 'action', switch > back to retuening early for create-region. > > Fixes: 3d6cd829ec08 ("cxl/region: Use cxl_filter_walk() to gather create-region targets") > Cc: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > --- > cxl/region.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/cxl/region.c b/cxl/region.c > index efe05aa..38aa142 100644 > --- a/cxl/region.c > +++ b/cxl/region.c > @@ -789,7 +789,7 @@ static int region_action(int argc, const char **argv, struct cxl_ctx *ctx, > return rc; > > if (action == ACTION_CREATE) > - rc = create_region(ctx, count, p); > + return create_region(ctx, count, p); Looks good, can't remember the motivation for changing that. Reviewed-by: Dan Williams <dan.j.williams@intel.com> ...modulo the spelling fixes that Fan noted.
diff --git a/cxl/region.c b/cxl/region.c index efe05aa..38aa142 100644 --- a/cxl/region.c +++ b/cxl/region.c @@ -789,7 +789,7 @@ static int region_action(int argc, const char **argv, struct cxl_ctx *ctx, return rc; if (action == ACTION_CREATE) - rc = create_region(ctx, count, p); + return create_region(ctx, count, p); cxl_bus_foreach(ctx, bus) { struct cxl_decoder *decoder;
Commit 3d6cd829ec08 ("cxl/region: Use cxl_filter_walk() to gather create-region targets") removed the early return for create-region, and this caused a create-region operation to unnecessarily loop through buses and root decoders only to EINVAL out because ACTION_CREATE is handled outside of the other actions. This results in confising messages such as: # cxl create-region -t ram -d 0.0 -m 0,4 { "region":"region7", "resource":"0xf030000000", "size":"512.00 MiB (536.87 MB)", ... } cxl region: decoder_region_action: region0: failed: Invalid argument cxl region: region_action: one or more failures, last failure: Invalid argument cxl region: cmd_create_region: created 1 region Since there's no need to walk through the topology after creating a region, and especially not to perform an invalid 'action', switch back to retuening early for create-region. Fixes: 3d6cd829ec08 ("cxl/region: Use cxl_filter_walk() to gather create-region targets") Cc: Dan Williams <dan.j.williams@intel.com> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> --- cxl/region.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)