Re: [PATCH 3/6] edac, mce_amd_inj: Modify flags attrigute to use string arguments

[Borislav Petkov <bp@alien8.de>]

On Wed, May 27, 2015 at 02:03:35PM -0500, Aravind Gopalakrishnan wrote:
> Use char values such as "hw" or "sw" to indicate the type of error
> injection to be performed.
> 
> Current flags attribute derives the meanings of values that can be
> programmed into it from asm/mce.h. Moving to defined strings for the
> atribute allows this module to be self sufficient and removes the
> dependency. Also, we can introduce new flags as and when needed without
> having to worry about conflicting with the flags already defined
> in asm/mce.h
> 
> Also, modify do_inject() to use the newly defined injection_type enum
> to figure out the injection mechanism we need to use
> 
> Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> ---
>  drivers/edac/mce_amd_inj.c | 85 +++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 76 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/edac/mce_amd_inj.c b/drivers/edac/mce_amd_inj.c
> index 15f6aa1..daec4af 100644
> --- a/drivers/edac/mce_amd_inj.c
> +++ b/drivers/edac/mce_amd_inj.c
> @@ -15,6 +15,8 @@
>  #include <linux/device.h>
>  #include <linux/module.h>
>  #include <linux/cpu.h>
> +#include <linux/string.h>
> +#include <linux/uaccess.h>
>  #include <asm/mce.h>
>  
>  #include "mce_amd.h"
> @@ -27,6 +29,23 @@ static struct dentry *dfs_inj;
>  
>  static u8 n_banks;
>  
> +#define MAX_FLAG_OPT_SIZE	10

Why 10?

This should be 2 and increased when another, longer injection type
string gets introduced.

> +
> +enum injection_type {
> +	SW_INJ = 0,	/* SW injection, simply decode the error */
> +	HW_INJ,		/* Trigger a #MC */
> +	N_INJ_TYPES,
> +};
> +
> +static const char * const flags_options[] = {
> +	[SW_INJ] = "sw",
> +	[HW_INJ] = "hw",
> +	NULL
> +};


> +
> +/* Set default injection to SW_INJ */
> +enum injection_type inj_type = SW_INJ;
> +
>  #define MCE_INJECT_SET(reg)						\
>  static int inj_##reg##_set(void *data, u64 val)				\
>  {									\
> @@ -81,24 +100,72 @@ static int toggle_hw_mce_inject(unsigned int cpu, bool enable)
>  	return err;
>  }
>  
> -static int flags_get(void *data, u64 *val)
> +static int __set_inj(const char *buf, size_t cnt)
>  {
> -	struct mce *m = (struct mce *)data;
> +	int i;
> +	const char *inj_op = NULL;
>  
> -	*val = m->inject_flags;
> +	for (i = 0; i <= N_INJ_TYPES; i++) {
> +		inj_op = flags_options[i];
> +		if (inj_op && strncmp(inj_op, buf, cnt) == 0)
> +			break;
> +	}

What's wrong with:

	for (i = 0; i < N_INJ_TYPES; i++) {
		if (!strncmp(flags_options[i], buf, strlen(flags_options[i]))) {
			inj_type = i;
			return 0;
		}
	}

	return -EINVAL;

> +
> +	if (!inj_op)
> +		return -EINVAL;
> +
> +	inj_type = i;
>  
>  	return 0;
>  }
>  
> -static int flags_set(void *data, u64 val)
> +static ssize_t flags_read(struct file *filp, char __user *ubuf,
> +			  size_t cnt, loff_t *ppos)
>  {
> -	struct mce *m = (struct mce *)data;
> +	char buf[MAX_FLAG_OPT_SIZE + 1];
> +	int n;
>  
> -	m->inject_flags = (u8)val;
> -	return 0;
> +	n = sprintf(buf, "%s\n", flags_options[inj_type]);
> +
> +	return simple_read_from_buffer(ubuf, cnt, ppos, buf, n);
>  }
>  
> -DEFINE_SIMPLE_ATTRIBUTE(flags_fops, flags_get, flags_set, "%llu\n");
> +static ssize_t flags_write(struct file *filp, const char __user *ubuf,
> +			   size_t cnt, loff_t *ppos)
> +{
> +	char buf[MAX_FLAG_OPT_SIZE + 1];
> +	int err;
> +	size_t ret;
> +
> +	ret = cnt;

You're assigning cnt to ret here...

> +
> +	if (cnt > MAX_FLAG_OPT_SIZE)
> +		cnt = MAX_FLAG_OPT_SIZE;

... but correcting cnt afterwards. The assignment should be *after* that
correction.

> +
> +	if (copy_from_user(&buf, ubuf, cnt))
> +		return -EFAULT;
> +
> +	buf[cnt] = 0;
> +
> +	/* strip whitespaces.. */
> +	strstrip(buf);
> +
> +	err = __set_inj(buf, cnt - 1);
> +	if (err) {
> +		pr_err("%s: Invalid flags value: %s\n", __func__, buf);
> +		return err;
> +	}
> +
> +	*ppos += ret;
> +
> +	return ret;
> +}
> +
> +static const struct file_operations flags_fops = {
> +	.read           = flags_read,
> +	.write          = flags_write,
> +	.llseek         = generic_file_llseek,
> +};
>  
>  /*
>   * On which CPU to inject?
> @@ -130,7 +197,7 @@ static void do_inject(void)
>  	unsigned int cpu = i_mce.extcpu;
>  	u8 b = i_mce.bank;
>  
> -	if (!(i_mce.inject_flags & MCJ_EXCEPTION)) {
> +	if (inj_type == SW_INJ) {
>  		amd_decode_mce(NULL, 0, &i_mce);
>  		return;
>  	}
> -- 
> 2.4.0
> 
> 

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--