[1/9] x86/intel_rdt: Fix MBA parsing callback
diff mbox series

Message ID 1536957129-70380-2-git-send-email-fenghua.yu@intel.com
State New
Headers show
Series
  • x86/intel_rdt: MBA integration fixes
Related show

Commit Message

Fenghua Yu Sept. 14, 2018, 8:32 p.m. UTC
From: Xiaochen Shen <xiaochen.shen@intel.com>

Each resource is associated with a parsing callback to parse the
data provided from user space when writing schemata file.

Commit 9ab9aa15c309 ("x86/intel_rdt: Ensure requested schemata
respects mode") changes the first parameter 'data' of parse_ctrlval()
from string to a pointer to a structure. parse_ctrlval() could point
to two functions parse_cbm() and parse_bw() and both of the
functions should be changed to take the different type of parameter
'data'. But the commit only changes parse_cbm(), not parse_bw().
Thus, parse_bw() takes wrong 'data' parameter and causes failure
of parsing MBA throttle value.

To fix the issue, parse_bw() should handle its first parameter as
right structure instead of a string.

Fixes: 9ab9aa15c309 ("x86/intel_rdt: Ensure requested schemata respects mode")

Signed-off-by: Xiaochen Shen <xiaochen.shen@intel.com>
Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/x86/kernel/cpu/intel_rdt.h             |  2 +-
 arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c | 24 ++++++++++-----------
 2 files changed, 13 insertions(+), 13 deletions(-)

Comments

Thomas Gleixner Sept. 15, 2018, 10:13 a.m. UTC | #1
On Fri, 14 Sep 2018, Fenghua Yu wrote:
>  int parse_cbm(void *_data, struct rdt_resource *r, struct rdt_domain *d);
> -int parse_bw(void *_buf, struct rdt_resource *r,  struct rdt_domain *d);
> +int parse_bw(void *_data, struct rdt_resource *r, struct rdt_domain *d);

Sorry no. This keeps the code equally error prone as it was. Why is that
argument a void pointer in the first place? 

>  extern struct mutex rdtgroup_mutex;
>  
> diff --git a/arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c b/arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c
> index af358ca05160..d427c86e7cd0 100644
> --- a/arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c
> +++ b/arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c
> @@ -28,6 +28,11 @@
>  #include <linux/slab.h>
>  #include "intel_rdt.h"
>  
> +struct rdt_parse_data {
> +	struct rdtgroup		*rdtgrp;
> +	char			*buf;
> +};

This is a copy of rdt_cbm_parse_data. Sigh.

The right thing to do here is

    1) rename struct rdt_cbm_parse_data to struct rdt_parse_data

    2) Move it to a header file

    3) Change the argument of parse_ctrlval from void * to struct
       rdt_parse_data *

Everything else is just proliferating the initial underlying problem of
having a void pointer in those callbacks for no reason at all.

Thanks,

	tglx
Fenghua Yu Sept. 15, 2018, 10:27 p.m. UTC | #2
On Sat, Sep 15, 2018 at 12:13:53PM +0200, Thomas Gleixner wrote:
> On Fri, 14 Sep 2018, Fenghua Yu wrote:
> > +int parse_bw(void *_data, struct rdt_resource *r, struct rdt_domain *d);
> 
> Sorry no. This keeps the code equally error prone as it was. Why is that
> argument a void pointer in the first place? 
> 
> >  extern struct mutex rdtgroup_mutex;
> 
> This is a copy of rdt_cbm_parse_data. Sigh.
> 
> The right thing to do here is
> 
>     1) rename struct rdt_cbm_parse_data to struct rdt_parse_data
> 
>     2) Move it to a header file
> 
>     3) Change the argument of parse_ctrlval from void * to struct
>        rdt_parse_data *
> 
> Everything else is just proliferating the initial underlying problem of
> having a void pointer in those callbacks for no reason at all.

Sure. I have updated this patch and patch 2, 4, 5 based on your comments.

Thanks.

-Fenghua

Patch
diff mbox series

diff --git a/arch/x86/kernel/cpu/intel_rdt.h b/arch/x86/kernel/cpu/intel_rdt.h
index 4e588f36228f..8c30772e37ed 100644
--- a/arch/x86/kernel/cpu/intel_rdt.h
+++ b/arch/x86/kernel/cpu/intel_rdt.h
@@ -432,7 +432,7 @@  struct rdt_resource {
 };
 
 int parse_cbm(void *_data, struct rdt_resource *r, struct rdt_domain *d);
-int parse_bw(void *_buf, struct rdt_resource *r,  struct rdt_domain *d);
+int parse_bw(void *_data, struct rdt_resource *r, struct rdt_domain *d);
 
 extern struct mutex rdtgroup_mutex;
 
diff --git a/arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c b/arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c
index af358ca05160..d427c86e7cd0 100644
--- a/arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c
+++ b/arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c
@@ -28,6 +28,11 @@ 
 #include <linux/slab.h>
 #include "intel_rdt.h"
 
+struct rdt_parse_data {
+	struct rdtgroup		*rdtgrp;
+	char			*buf;
+};
+
 /*
  * Check whether MBA bandwidth percentage value is correct. The value is
  * checked against the minimum and max bandwidth values specified by the
@@ -64,19 +69,19 @@  static bool bw_validate(char *buf, unsigned long *data, struct rdt_resource *r)
 	return true;
 }
 
-int parse_bw(void *_buf, struct rdt_resource *r, struct rdt_domain *d)
+int parse_bw(void *_data, struct rdt_resource *r, struct rdt_domain *d)
 {
-	unsigned long data;
-	char *buf = _buf;
+	struct rdt_parse_data *data = _data;
+	unsigned long bw_val;
 
 	if (d->have_new_ctrl) {
 		rdt_last_cmd_printf("duplicate domain %d\n", d->id);
 		return -EINVAL;
 	}
 
-	if (!bw_validate(buf, &data, r))
+	if (!bw_validate(data->buf, &bw_val, r))
 		return -EINVAL;
-	d->new_ctrl = data;
+	d->new_ctrl = bw_val;
 	d->have_new_ctrl = true;
 
 	return 0;
@@ -123,18 +128,13 @@  static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
 	return true;
 }
 
-struct rdt_cbm_parse_data {
-	struct rdtgroup		*rdtgrp;
-	char			*buf;
-};
-
 /*
  * Read one cache bit mask (hex). Check that it is valid for the current
  * resource type.
  */
 int parse_cbm(void *_data, struct rdt_resource *r, struct rdt_domain *d)
 {
-	struct rdt_cbm_parse_data *data = _data;
+	struct rdt_parse_data *data = _data;
 	struct rdtgroup *rdtgrp = data->rdtgrp;
 	u32 cbm_val;
 
@@ -195,7 +195,7 @@  int parse_cbm(void *_data, struct rdt_resource *r, struct rdt_domain *d)
 static int parse_line(char *line, struct rdt_resource *r,
 		      struct rdtgroup *rdtgrp)
 {
-	struct rdt_cbm_parse_data data;
+	struct rdt_parse_data data;
 	char *dom = NULL, *id;
 	struct rdt_domain *d;
 	unsigned long dom_id;