| Message ID | 20200422115814.22205-3-rrichter@marvell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | EDAC/mc/ghes: Fixes, cleanup and reworks | expand |
On Wed, Apr 22, 2020 at 01:58:06PM +0200, Robert Richter wrote: > Most iterators use int type as index. mci->mc_idx is also type int. So > use int type for parameters of edac_mc_alloc(). Extend the range check > to check for negative values. There is a type cast now when assigning > variable n_layers to mci->n_layer, it is safe due to the range check. > > While at it, rename the users of edac_mc_alloc() to mc_idx as this > fits better here. > > Signed-off-by: Robert Richter <rrichter@marvell.com> > --- > drivers/edac/edac_mc.c | 7 +++---- > drivers/edac/edac_mc.h | 6 +++--- > 2 files changed, 6 insertions(+), 7 deletions(-) > > diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c > index 107d7c4de933..57d1d356d69c 100644 > --- a/drivers/edac/edac_mc.c > +++ b/drivers/edac/edac_mc.c > @@ -444,8 +444,7 @@ static int edac_mc_alloc_dimms(struct mem_ctl_info *mci) > return 0; > } > > -struct mem_ctl_info *edac_mc_alloc(unsigned int mc_num, > - unsigned int n_layers, > +struct mem_ctl_info *edac_mc_alloc(int mc_idx, int n_layers, > struct edac_mc_layer *layers, > unsigned int sz_pvt) > { > @@ -456,7 +455,7 @@ struct mem_ctl_info *edac_mc_alloc(unsigned int mc_num, > void *pvt, *ptr = NULL; > bool per_rank = false; > > - if (WARN_ON(n_layers > EDAC_MAX_LAYERS || n_layers == 0)) > + if (WARN_ON(mc_idx < 0 || n_layers < 1 || n_layers > EDAC_MAX_LAYERS)) > return NULL; Yeah, no, this doesn't make sense to me. The memory controller number and the number of layers can never ever be negative and thus signed. And some drivers supply unsigned types and some signed. So if anything, this should be fixing all the callers to supply unsigned quantities. Thx.
On 23.04.20 19:49:34, Borislav Petkov wrote: > On Wed, Apr 22, 2020 at 01:58:06PM +0200, Robert Richter wrote: > > Most iterators use int type as index. mci->mc_idx is also type int. So > > use int type for parameters of edac_mc_alloc(). Extend the range check > > to check for negative values. There is a type cast now when assigning > > variable n_layers to mci->n_layer, it is safe due to the range check. > > > > While at it, rename the users of edac_mc_alloc() to mc_idx as this > > fits better here. > > > > Signed-off-by: Robert Richter <rrichter@marvell.com> > > --- > > drivers/edac/edac_mc.c | 7 +++---- > > drivers/edac/edac_mc.h | 6 +++--- > > 2 files changed, 6 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c > > index 107d7c4de933..57d1d356d69c 100644 > > --- a/drivers/edac/edac_mc.c > > +++ b/drivers/edac/edac_mc.c > > @@ -444,8 +444,7 @@ static int edac_mc_alloc_dimms(struct mem_ctl_info *mci) > > return 0; > > } > > > > -struct mem_ctl_info *edac_mc_alloc(unsigned int mc_num, > > - unsigned int n_layers, > > +struct mem_ctl_info *edac_mc_alloc(int mc_idx, int n_layers, > > struct edac_mc_layer *layers, > > unsigned int sz_pvt) > > { > > @@ -456,7 +455,7 @@ struct mem_ctl_info *edac_mc_alloc(unsigned int mc_num, > > void *pvt, *ptr = NULL; > > bool per_rank = false; > > > > - if (WARN_ON(n_layers > EDAC_MAX_LAYERS || n_layers == 0)) > > + if (WARN_ON(mc_idx < 0 || n_layers < 1 || n_layers > EDAC_MAX_LAYERS)) > > return NULL; > > Yeah, no, this doesn't make sense to me. The memory controller number > and the number of layers can never ever be negative and thus signed. > > And some drivers supply unsigned types and some signed. So if anything, > this should be fixing all the callers to supply unsigned quantities. mci->mc_idx is of type int and there is a cast here that should be fixed. IMO that should be a signed int as some interfaces (esp. if you search for an index) that require a negative value to report errors or something could not be found. So let's take this patch out of this series if you want have it different. Thanks, -Robert
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c index 107d7c4de933..57d1d356d69c 100644 --- a/drivers/edac/edac_mc.c +++ b/drivers/edac/edac_mc.c @@ -444,8 +444,7 @@ static int edac_mc_alloc_dimms(struct mem_ctl_info *mci) return 0; } -struct mem_ctl_info *edac_mc_alloc(unsigned int mc_num, - unsigned int n_layers, +struct mem_ctl_info *edac_mc_alloc(int mc_idx, int n_layers, struct edac_mc_layer *layers, unsigned int sz_pvt) { @@ -456,7 +455,7 @@ struct mem_ctl_info *edac_mc_alloc(unsigned int mc_num, void *pvt, *ptr = NULL; bool per_rank = false; - if (WARN_ON(n_layers > EDAC_MAX_LAYERS || n_layers == 0)) + if (WARN_ON(mc_idx < 0 || n_layers < 1 || n_layers > EDAC_MAX_LAYERS)) return NULL; /* @@ -505,7 +504,7 @@ struct mem_ctl_info *edac_mc_alloc(unsigned int mc_num, pvt = sz_pvt ? (((char *)mci) + ((unsigned long)pvt)) : NULL; /* setup index and various internal pointers */ - mci->mc_idx = mc_num; + mci->mc_idx = mc_idx; mci->tot_dimms = tot_dimms; mci->pvt_info = pvt; mci->n_layers = n_layers; diff --git a/drivers/edac/edac_mc.h b/drivers/edac/edac_mc.h index 881b00eadf7a..4815f50afea0 100644 --- a/drivers/edac/edac_mc.h +++ b/drivers/edac/edac_mc.h @@ -98,7 +98,7 @@ do { \ /** * edac_mc_alloc() - Allocate and partially fill a struct &mem_ctl_info. * - * @mc_num: Memory controller number + * @mc_idx: Memory controller number * @n_layers: Number of MC hierarchy layers * @layers: Describes each layer as seen by the Memory Controller * @sz_pvt: size of private storage needed @@ -122,8 +122,8 @@ do { \ * On success, return a pointer to struct mem_ctl_info pointer; * %NULL otherwise */ -struct mem_ctl_info *edac_mc_alloc(unsigned int mc_num, - unsigned int n_layers, +struct mem_ctl_info *edac_mc_alloc(int mc_idx, + int n_layers, struct edac_mc_layer *layers, unsigned int sz_pvt);
Most iterators use int type as index. mci->mc_idx is also type int. So use int type for parameters of edac_mc_alloc(). Extend the range check to check for negative values. There is a type cast now when assigning variable n_layers to mci->n_layer, it is safe due to the range check. While at it, rename the users of edac_mc_alloc() to mc_idx as this fits better here. Signed-off-by: Robert Richter <rrichter@marvell.com> --- drivers/edac/edac_mc.c | 7 +++---- drivers/edac/edac_mc.h | 6 +++--- 2 files changed, 6 insertions(+), 7 deletions(-)