* [PATCH] ipmi_si: use smi_num for init_name
@ 2017-04-10 16:22 Tony Camuso
2017-04-10 16:59 ` Corey Minyard
0 siblings, 1 reply; 6+ messages in thread
From: Tony Camuso @ 2017-04-10 16:22 UTC (permalink / raw)
To: minyard, openipmi-developer, linux-kernel; +Cc: tcamuso
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ipmi_si: use smi_num for init_name
2017-04-10 16:22 [PATCH] ipmi_si: use smi_num for init_name Tony Camuso
@ 2017-04-10 16:59 ` Corey Minyard
0 siblings, 0 replies; 6+ messages in thread
From: Corey Minyard @ 2017-04-10 16:59 UTC (permalink / raw)
To: Tony Camuso, openipmi-developer, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ipmi_si: use smi_num for init_name
2017-04-10 17:43 minyard
2017-04-10 17:48 ` Tony Camuso
2017-04-10 17:57 ` Tony Camuso
@ 2017-04-10 18:17 ` Tony Camuso
2 siblings, 0 replies; 6+ messages in thread
From: Tony Camuso @ 2017-04-10 18:17 UTC (permalink / raw)
To: minyard; +Cc: openipmi-developer, linux-kernel, Corey Minyard
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]);
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ipmi_si: use smi_num for init_name
2017-04-10 17:43 minyard
2017-04-10 17:48 ` Tony Camuso
@ 2017-04-10 17:57 ` Tony Camuso
2017-04-10 18:17 ` Tony Camuso
2 siblings, 0 replies; 6+ messages in thread
From: Tony Camuso @ 2017-04-10 17:57 UTC (permalink / raw)
To: minyard; +Cc: openipmi-developer, linux-kernel, Corey Minyard
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]);
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ipmi_si: use smi_num for init_name
2017-04-10 17:43 minyard
@ 2017-04-10 17:48 ` Tony Camuso
2017-04-10 17:57 ` Tony Camuso
2017-04-10 18:17 ` Tony Camuso
2 siblings, 0 replies; 6+ messages in thread
From: Tony Camuso @ 2017-04-10 17:48 UTC (permalink / raw)
To: minyard; +Cc: openipmi-developer, linux-kernel, Corey Minyard
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]);
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ipmi_si: use smi_num for init_name
@ 2017-04-10 17:43 minyard
2017-04-10 17:48 ` Tony Camuso
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: minyard @ 2017-04-10 17:43 UTC (permalink / raw)
To: Tony Camuso; +Cc: openipmi-developer, linux-kernel, Corey 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-04-10 18:17 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-10 16:22 [PATCH] ipmi_si: use smi_num for init_name Tony Camuso
2017-04-10 16:59 ` Corey Minyard
2017-04-10 17:43 minyard
2017-04-10 17:48 ` Tony Camuso
2017-04-10 17:57 ` Tony Camuso
2017-04-10 18:17 ` Tony Camuso
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.