[PATCH] ipmi_si: use smi_num for init_name

[PATCH] ipmi_si: use smi_num for init_name

[minyard]

From: Tony Camuso <tcamuso@redhat.com>

Commit 1abf71e moved the creation of new_smi->dev to earlier in the init
sequence in order to provide infrastructure for log printing.

However, the init_name was created with a hard-coded value of zero. This
presents a problem in systems with more than one interface, producing a
call trace in dmesg.

To correct the problem, simply use smi_num instead of the hard-coded
value of zero.

Tested on a lenovo x3950.

Signed-off-by: Tony Camuso <tcamuso@redhat.com>

There was actually a more general problem, the platform device wasn't
being set correctly, either, and there was a possible (though extremely
unlikely) race on smi_num.  Add locks to clean up the race and use the
proper value for the platform device, too.

Tested on qemu in various configurations.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---

I reworked this a bit, I think I've covered all the problems with
smi_num.  Tony, does this work for you?

Thanks

 drivers/char/ipmi/ipmi_si_intf.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 2a7c425..b2b618f 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -1954,7 +1954,9 @@ static int hotmod_handler(const char *val, struct kernel_param *kp)
 				kfree(info);
 				goto out;
 			}
+			mutex_lock(&smi_infos_lock);
 			rv = try_smi_init(info);
+			mutex_unlock(&smi_infos_lock);
 			if (rv) {
 				cleanup_one_si(info);
 				goto out;
@@ -2042,8 +2044,10 @@ static int hardcode_find_bmc(void)
 		info->slave_addr = slave_addrs[i];
 
 		if (!add_smi(info)) {
+			mutex_lock(&smi_infos_lock);
 			if (try_smi_init(info))
 				cleanup_one_si(info);
+			mutex_unlock(&smi_infos_lock);
 			ret = 0;
 		} else {
 			kfree(info);
@@ -3492,6 +3496,11 @@ static int add_smi(struct smi_info *new_smi)
 	return rv;
 }
 
+/*
+ * Try to start up an interface.  Must be called with smi_infos_lock
+ * held, primarily to keep smi_num consistent, we only one to do these
+ * one at a time.
+ */
 static int try_smi_init(struct smi_info *new_smi)
 {
 	int rv = 0;
@@ -3524,9 +3533,12 @@ static int try_smi_init(struct smi_info *new_smi)
 		goto out_err;
 	}
 
+	new_smi->intf_num = smi_num;
+
 	/* Do this early so it's available for logs. */
 	if (!new_smi->dev) {
-		init_name = kasprintf(GFP_KERNEL, "ipmi_si.%d", 0);
+		init_name = kasprintf(GFP_KERNEL, "ipmi_si.%d",
+				      new_smi->intf_num);
 
 		/*
 		 * If we don't already have a device from something
@@ -3593,8 +3605,6 @@ static int try_smi_init(struct smi_info *new_smi)
 
 	new_smi->interrupt_disabled = true;
 	atomic_set(&new_smi->need_watch, 0);
-	new_smi->intf_num = smi_num;
-	smi_num++;
 
 	rv = try_enable_event_buffer(new_smi);
 	if (rv == 0)
@@ -3661,6 +3671,9 @@ static int try_smi_init(struct smi_info *new_smi)
 		goto out_err_stop_timer;
 	}
 
+	/* Don't increment till we know we have succeeded. */
+	smi_num++;
+
 	dev_info(new_smi->dev, "IPMI %s interface initialized\n",
 		 si_to_str[new_smi->si_type]);
 
-- 
2.7.4

Re: [PATCH] ipmi_si: use smi_num for init_name

[Tony Camuso]

On 04/10/2017 01:43 PM, minyard@acm.org wrote:
> From: Tony Camuso <tcamuso@redhat.com>
>
> Commit 1abf71e moved the creation of new_smi->dev to earlier in the init
> sequence in order to provide infrastructure for log printing.
>
> However, the init_name was created with a hard-coded value of zero. This
> presents a problem in systems with more than one interface, producing a
> call trace in dmesg.
>
> To correct the problem, simply use smi_num instead of the hard-coded
> value of zero.
>
> Tested on a lenovo x3950.
>
> Signed-off-by: Tony Camuso <tcamuso@redhat.com>
>
> There was actually a more general problem, the platform device wasn't
> being set correctly, either, and there was a possible (though extremely
> unlikely) race on smi_num.  Add locks to clean up the race and use the
> proper value for the platform device, too.

Ouch! I thought smi_num was already locked by smi_infos. Now I see that
it wasn't.

Thanks!

Regards,
tony

>
> Tested on qemu in various configurations.
>
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>
> I reworked this a bit, I think I've covered all the problems with
> smi_num.  Tony, does this work for you?
>
> Thanks
>
>  drivers/char/ipmi/ipmi_si_intf.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index 2a7c425..b2b618f 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -1954,7 +1954,9 @@ static int hotmod_handler(const char *val, struct kernel_param *kp)
>  				kfree(info);
>  				goto out;
>  			}
> +			mutex_lock(&smi_infos_lock);
>  			rv = try_smi_init(info);
> +			mutex_unlock(&smi_infos_lock);
>  			if (rv) {
>  				cleanup_one_si(info);
>  				goto out;
> @@ -2042,8 +2044,10 @@ static int hardcode_find_bmc(void)
>  		info->slave_addr = slave_addrs[i];
>
>  		if (!add_smi(info)) {
> +			mutex_lock(&smi_infos_lock);
>  			if (try_smi_init(info))
>  				cleanup_one_si(info);
> +			mutex_unlock(&smi_infos_lock);
>  			ret = 0;
>  		} else {
>  			kfree(info);
> @@ -3492,6 +3496,11 @@ static int add_smi(struct smi_info *new_smi)
>  	return rv;
>  }
>
> +/*
> + * Try to start up an interface.  Must be called with smi_infos_lock
> + * held, primarily to keep smi_num consistent, we only one to do these
> + * one at a time.
> + */
>  static int try_smi_init(struct smi_info *new_smi)
>  {
>  	int rv = 0;
> @@ -3524,9 +3533,12 @@ static int try_smi_init(struct smi_info *new_smi)
>  		goto out_err;
>  	}
>
> +	new_smi->intf_num = smi_num;
> +
>  	/* Do this early so it's available for logs. */
>  	if (!new_smi->dev) {
> -		init_name = kasprintf(GFP_KERNEL, "ipmi_si.%d", 0);
> +		init_name = kasprintf(GFP_KERNEL, "ipmi_si.%d",
> +				      new_smi->intf_num);
>
>  		/*
>  		 * If we don't already have a device from something
> @@ -3593,8 +3605,6 @@ static int try_smi_init(struct smi_info *new_smi)
>
>  	new_smi->interrupt_disabled = true;
>  	atomic_set(&new_smi->need_watch, 0);
> -	new_smi->intf_num = smi_num;
> -	smi_num++;
>
>  	rv = try_enable_event_buffer(new_smi);
>  	if (rv == 0)
> @@ -3661,6 +3671,9 @@ static int try_smi_init(struct smi_info *new_smi)
>  		goto out_err_stop_timer;
>  	}
>
> +	/* Don't increment till we know we have succeeded. */
> +	smi_num++;
> +
>  	dev_info(new_smi->dev, "IPMI %s interface initialized\n",
>  		 si_to_str[new_smi->si_type]);
>
>

Re: [PATCH] ipmi_si: use smi_num for init_name

[Tony Camuso]

On 04/10/2017 01:43 PM, minyard@acm.org wrote:
> From: Tony Camuso <tcamuso@redhat.com>
>
> Commit 1abf71e moved the creation of new_smi->dev to earlier in the init
> sequence in order to provide infrastructure for log printing.
>
> However, the init_name was created with a hard-coded value of zero. This
> presents a problem in systems with more than one interface, producing a
> call trace in dmesg.
>
> To correct the problem, simply use smi_num instead of the hard-coded
> value of zero.
>
> Tested on a lenovo x3950.
>
> Signed-off-by: Tony Camuso <tcamuso@redhat.com>
>
> There was actually a more general problem, the platform device wasn't
> being set correctly, either, and there was a possible (though extremely
> unlikely) race on smi_num.  Add locks to clean up the race and use the
> proper value for the platform device, too.
>
> Tested on qemu in various configurations.
>
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>
> I reworked this a bit, I think I've covered all the problems with
> smi_num.  Tony, does this work for you?

Testing now ...


>
> Thanks
>
>  drivers/char/ipmi/ipmi_si_intf.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index 2a7c425..b2b618f 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -1954,7 +1954,9 @@ static int hotmod_handler(const char *val, struct kernel_param *kp)
>  				kfree(info);
>  				goto out;
>  			}
> +			mutex_lock(&smi_infos_lock);
>  			rv = try_smi_init(info);
> +			mutex_unlock(&smi_infos_lock);
>  			if (rv) {
>  				cleanup_one_si(info);
>  				goto out;
> @@ -2042,8 +2044,10 @@ static int hardcode_find_bmc(void)
>  		info->slave_addr = slave_addrs[i];
>
>  		if (!add_smi(info)) {
> +			mutex_lock(&smi_infos_lock);
>  			if (try_smi_init(info))
>  				cleanup_one_si(info);
> +			mutex_unlock(&smi_infos_lock);
>  			ret = 0;
>  		} else {
>  			kfree(info);
> @@ -3492,6 +3496,11 @@ static int add_smi(struct smi_info *new_smi)
>  	return rv;
>  }
>
> +/*
> + * Try to start up an interface.  Must be called with smi_infos_lock
> + * held, primarily to keep smi_num consistent, we only one to do these
> + * one at a time.
> + */
>  static int try_smi_init(struct smi_info *new_smi)
>  {
>  	int rv = 0;
> @@ -3524,9 +3533,12 @@ static int try_smi_init(struct smi_info *new_smi)
>  		goto out_err;
>  	}
>
> +	new_smi->intf_num = smi_num;
> +
>  	/* Do this early so it's available for logs. */
>  	if (!new_smi->dev) {
> -		init_name = kasprintf(GFP_KERNEL, "ipmi_si.%d", 0);
> +		init_name = kasprintf(GFP_KERNEL, "ipmi_si.%d",
> +				      new_smi->intf_num);
>
>  		/*
>  		 * If we don't already have a device from something
> @@ -3593,8 +3605,6 @@ static int try_smi_init(struct smi_info *new_smi)
>
>  	new_smi->interrupt_disabled = true;
>  	atomic_set(&new_smi->need_watch, 0);
> -	new_smi->intf_num = smi_num;
> -	smi_num++;
>
>  	rv = try_enable_event_buffer(new_smi);
>  	if (rv == 0)
> @@ -3661,6 +3671,9 @@ static int try_smi_init(struct smi_info *new_smi)
>  		goto out_err_stop_timer;
>  	}
>
> +	/* Don't increment till we know we have succeeded. */
> +	smi_num++;
> +
>  	dev_info(new_smi->dev, "IPMI %s interface initialized\n",
>  		 si_to_str[new_smi->si_type]);
>
>

Re: [PATCH] ipmi_si: use smi_num for init_name

[Tony Camuso]

On 04/10/2017 01:43 PM, minyard@acm.org wrote:
> From: Tony Camuso <tcamuso@redhat.com>
>
> Commit 1abf71e moved the creation of new_smi->dev to earlier in the init
> sequence in order to provide infrastructure for log printing.
>
> However, the init_name was created with a hard-coded value of zero. This
> presents a problem in systems with more than one interface, producing a
> call trace in dmesg.
>
> To correct the problem, simply use smi_num instead of the hard-coded
> value of zero.
>
> Tested on a lenovo x3950.
>
> Signed-off-by: Tony Camuso <tcamuso@redhat.com>
>
> There was actually a more general problem, the platform device wasn't
> being set correctly, either, and there was a possible (though extremely
> unlikely) race on smi_num.  Add locks to clean up the race and use the
> proper value for the platform device, too.
>
> Tested on qemu in various configurations.
>
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>
> I reworked this a bit, I think I've covered all the problems with
> smi_num.  Tony, does this work for you?
>
> Thanks

Works perfectly.

Regards,
Tony



>
>  drivers/char/ipmi/ipmi_si_intf.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index 2a7c425..b2b618f 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -1954,7 +1954,9 @@ static int hotmod_handler(const char *val, struct kernel_param *kp)
>  				kfree(info);
>  				goto out;
>  			}
> +			mutex_lock(&smi_infos_lock);
>  			rv = try_smi_init(info);
> +			mutex_unlock(&smi_infos_lock);
>  			if (rv) {
>  				cleanup_one_si(info);
>  				goto out;
> @@ -2042,8 +2044,10 @@ static int hardcode_find_bmc(void)
>  		info->slave_addr = slave_addrs[i];
>
>  		if (!add_smi(info)) {
> +			mutex_lock(&smi_infos_lock);
>  			if (try_smi_init(info))
>  				cleanup_one_si(info);
> +			mutex_unlock(&smi_infos_lock);
>  			ret = 0;
>  		} else {
>  			kfree(info);
> @@ -3492,6 +3496,11 @@ static int add_smi(struct smi_info *new_smi)
>  	return rv;
>  }
>
> +/*
> + * Try to start up an interface.  Must be called with smi_infos_lock
> + * held, primarily to keep smi_num consistent, we only one to do these
> + * one at a time.
> + */
>  static int try_smi_init(struct smi_info *new_smi)
>  {
>  	int rv = 0;
> @@ -3524,9 +3533,12 @@ static int try_smi_init(struct smi_info *new_smi)
>  		goto out_err;
>  	}
>
> +	new_smi->intf_num = smi_num;
> +
>  	/* Do this early so it's available for logs. */
>  	if (!new_smi->dev) {
> -		init_name = kasprintf(GFP_KERNEL, "ipmi_si.%d", 0);
> +		init_name = kasprintf(GFP_KERNEL, "ipmi_si.%d",
> +				      new_smi->intf_num);
>
>  		/*
>  		 * If we don't already have a device from something
> @@ -3593,8 +3605,6 @@ static int try_smi_init(struct smi_info *new_smi)
>
>  	new_smi->interrupt_disabled = true;
>  	atomic_set(&new_smi->need_watch, 0);
> -	new_smi->intf_num = smi_num;
> -	smi_num++;
>
>  	rv = try_enable_event_buffer(new_smi);
>  	if (rv == 0)
> @@ -3661,6 +3671,9 @@ static int try_smi_init(struct smi_info *new_smi)
>  		goto out_err_stop_timer;
>  	}
>
> +	/* Don't increment till we know we have succeeded. */
> +	smi_num++;
> +
>  	dev_info(new_smi->dev, "IPMI %s interface initialized\n",
>  		 si_to_str[new_smi->si_type]);
>
>

[PATCH] ipmi_si: use smi_num for init_name

[Tony Camuso]

Commit 1abf71e moved the creation of new_smi->dev to earlier in the init
sequence in order to provide infrastructure for log printing.

However, the init_name was created with a hard-coded value of zero. This
presents a problem in systems with more than one interface, producing a
call trace in dmesg.

To correct the problem, simply use smi_num instead of the hard-coded
value of zero.

Tested on a lenovo x3950.

Signed-off-by: Tony Camuso <tcamuso@redhat.com>
---
 drivers/char/ipmi/ipmi_si_intf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 2a7c425..9d8fc51 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -3526,7 +3526,7 @@ static int try_smi_init(struct smi_info *new_smi)
 
 	/* Do this early so it's available for logs. */
 	if (!new_smi->dev) {
-		init_name = kasprintf(GFP_KERNEL, "ipmi_si.%d", 0);
+		init_name = kasprintf(GFP_KERNEL, "ipmi_si.%d", smi_num);
 
 		/*
 		 * If we don't already have a device from something
-- 
1.8.3.1

Re: [PATCH] ipmi_si: use smi_num for init_name

[Corey Minyard]

On 04/10/2017 11:22 AM, Tony Camuso wrote:
> Commit 1abf71e moved the creation of new_smi->dev to earlier in the init
> sequence in order to provide infrastructure for log printing.
>
> However, the init_name was created with a hard-coded value of zero. This
> presents a problem in systems with more than one interface, producing a
> call trace in dmesg.
>
> To correct the problem, simply use smi_num instead of the hard-coded
> value of zero.

Yeah, you are right.  And there's more, the platform_device_alloc will
also be wrong, which is even worse.

I'll revise your patch a bit...

-corey

> Tested on a lenovo x3950.
>
> Signed-off-by: Tony Camuso <tcamuso@redhat.com>
> ---
>   drivers/char/ipmi/ipmi_si_intf.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index 2a7c425..9d8fc51 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -3526,7 +3526,7 @@ static int try_smi_init(struct smi_info *new_smi)
>   
>   	/* Do this early so it's available for logs. */
>   	if (!new_smi->dev) {
> -		init_name = kasprintf(GFP_KERNEL, "ipmi_si.%d", 0);
> +		init_name = kasprintf(GFP_KERNEL, "ipmi_si.%d", smi_num);
>   
>   		/*
>   		 * If we don't already have a device from something