Some minor fixes I had queued up. The last one came in recently (patch 4)
and it and patch 2 are candidates for stable-kernel.
When the interrupt enable message returns an error, the messages are
not entirely accurate nor helpful. So improve them.
Signed-off-by: Corey Minyard <[email protected]>
Cc: Andy Lutomirski <[email protected]>
---
drivers/char/ipmi/ipmi_si_intf.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 313538a..af4b23f 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -663,8 +663,10 @@ static void handle_transaction_done(struct smi_info *smi_info)
/* We got the flags from the SMI, now handle them. */
smi_info->handlers->get_result(smi_info->si_sm, msg, 4);
if (msg[2] != 0) {
- dev_warn(smi_info->dev, "Could not enable interrupts"
- ", failed get, using polled mode.\n");
+ dev_warn(smi_info->dev,
+ "Couldn't get irq info: %x.\n", msg[2]);
+ dev_warn(smi_info->dev,
+ "Maybe ok, but ipmi might run very slowly.\n");
smi_info->si_state = SI_NORMAL;
} else {
msg[0] = (IPMI_NETFN_APP_REQUEST << 2);
@@ -685,10 +687,12 @@ static void handle_transaction_done(struct smi_info *smi_info)
/* We got the flags from the SMI, now handle them. */
smi_info->handlers->get_result(smi_info->si_sm, msg, 4);
- if (msg[2] != 0)
- dev_warn(smi_info->dev, "Could not enable interrupts"
- ", failed set, using polled mode.\n");
- else
+ if (msg[2] != 0) {
+ dev_warn(smi_info->dev,
+ "Couldn't set irq info: %x.\n", msg[2]);
+ dev_warn(smi_info->dev,
+ "Maybe ok, but ipmi might run very slowly.\n");
+ } else
smi_info->interrupt_disabled = 0;
smi_info->si_state = SI_NORMAL;
break;
--
1.7.9.5
From: Alexandru Gheorghiu <[email protected]>
Replaced calls to kmalloc followed by strcpy with a sincle call to kstrdup.
Patch found using coccinelle.
Signed-off-by: Alexandru Gheorghiu <[email protected]>
Signed-off-by: Corey Minyard <[email protected]>
---
drivers/char/ipmi/ipmi_msghandler.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index 4d439d2..4445fa1 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -2037,12 +2037,11 @@ int ipmi_smi_add_proc_entry(ipmi_smi_t smi, char *name,
entry = kmalloc(sizeof(*entry), GFP_KERNEL);
if (!entry)
return -ENOMEM;
- entry->name = kmalloc(strlen(name)+1, GFP_KERNEL);
+ entry->name = kstrdup(name, GFP_KERNEL);
if (!entry->name) {
kfree(entry);
return -ENOMEM;
}
- strcpy(entry->name, name);
file = proc_create_data(name, 0, smi->proc_dir, proc_ops, data);
if (!file) {
--
1.7.9.5
From: Chen Gang <[email protected]>
when calling memcpy, read_data and write_data need additional 2 bytes.
write_data:
for checking: "if (size > IPMI_MAX_MSG_LENGTH)"
for operating: "memcpy(bt->write_data + 3, data + 1, size - 1)"
read_data:
for checking: "if (msg_len < 3 || msg_len > IPMI_MAX_MSG_LENGTH)"
for operating: "memcpy(data + 2, bt->read_data + 4, msg_len - 2)"
Signed-off-by: Chen Gang <[email protected]>
Signed-off-by: Corey Minyard <[email protected]>
Cc: [email protected]
---
drivers/char/ipmi/ipmi_bt_sm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_bt_sm.c b/drivers/char/ipmi/ipmi_bt_sm.c
index cdd4c09f..a22a7a5 100644
--- a/drivers/char/ipmi/ipmi_bt_sm.c
+++ b/drivers/char/ipmi/ipmi_bt_sm.c
@@ -95,9 +95,9 @@ struct si_sm_data {
enum bt_states state;
unsigned char seq; /* BT sequence number */
struct si_sm_io *io;
- unsigned char write_data[IPMI_MAX_MSG_LENGTH];
+ unsigned char write_data[IPMI_MAX_MSG_LENGTH + 2]; /* +2 for memcpy */
int write_count;
- unsigned char read_data[IPMI_MAX_MSG_LENGTH];
+ unsigned char read_data[IPMI_MAX_MSG_LENGTH + 2]; /* +2 for memcpy */
int read_count;
int truncated;
long timeout; /* microseconds countdown */
--
1.7.9.5
From: Benjamin LaHaise <[email protected]>
When a 32 bit version of ipmitool is used on a 64 bit kernel, the
ipmi_devintf code fails to correctly acquire ipmi_mutex. This results in
incomplete data being retrieved in some cases, or other possible failures.
Add a wrapper around compat_ipmi_ioctl() to take ipmi_mutex to fix this.
Signed-off-by: Benjamin LaHaise <[email protected]>
Signed-off-by: Corey Minyard <[email protected]>
Cc: [email protected]
---
drivers/char/ipmi/ipmi_devintf.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/char/ipmi/ipmi_devintf.c b/drivers/char/ipmi/ipmi_devintf.c
index 9eb360f..d5a5f02 100644
--- a/drivers/char/ipmi/ipmi_devintf.c
+++ b/drivers/char/ipmi/ipmi_devintf.c
@@ -837,13 +837,25 @@ static long compat_ipmi_ioctl(struct file *filep, unsigned int cmd,
return ipmi_ioctl(filep, cmd, arg);
}
}
+
+static long unlocked_compat_ipmi_ioctl(struct file *filep, unsigned int cmd,
+ unsigned long arg)
+{
+ int ret;
+
+ mutex_lock(&ipmi_mutex);
+ ret = compat_ipmi_ioctl(filep, cmd, arg);
+ mutex_unlock(&ipmi_mutex);
+
+ return ret;
+}
#endif
static const struct file_operations ipmi_fops = {
.owner = THIS_MODULE,
.unlocked_ioctl = ipmi_unlocked_ioctl,
#ifdef CONFIG_COMPAT
- .compat_ioctl = compat_ipmi_ioctl,
+ .compat_ioctl = unlocked_compat_ipmi_ioctl,
#endif
.open = ipmi_open,
.release = ipmi_release,
--
1.7.9.5
On Thu, May 16, 2013 at 12:04 PM, Corey Minyard <[email protected]> wrote:
> When the interrupt enable message returns an error, the messages are
> not entirely accurate nor helpful. So improve them.
>
> Signed-off-by: Corey Minyard <[email protected]>
> Cc: Andy Lutomirski <[email protected]>
> ---
> drivers/char/ipmi/ipmi_si_intf.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index 313538a..af4b23f 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -663,8 +663,10 @@ static void handle_transaction_done(struct smi_info *smi_info)
> /* We got the flags from the SMI, now handle them. */
> smi_info->handlers->get_result(smi_info->si_sm, msg, 4);
> if (msg[2] != 0) {
> - dev_warn(smi_info->dev, "Could not enable interrupts"
> - ", failed get, using polled mode.\n");
> + dev_warn(smi_info->dev,
> + "Couldn't get irq info: %x.\n", msg[2]);
> + dev_warn(smi_info->dev,
> + "Maybe ok, but ipmi might run very slowly.\n");
> smi_info->si_state = SI_NORMAL;
> } else {
> msg[0] = (IPMI_NETFN_APP_REQUEST << 2);
> @@ -685,10 +687,12 @@ static void handle_transaction_done(struct smi_info *smi_info)
>
> /* We got the flags from the SMI, now handle them. */
> smi_info->handlers->get_result(smi_info->si_sm, msg, 4);
> - if (msg[2] != 0)
> - dev_warn(smi_info->dev, "Could not enable interrupts"
> - ", failed set, using polled mode.\n");
> - else
> + if (msg[2] != 0) {
> + dev_warn(smi_info->dev,
> + "Couldn't set irq info: %x.\n", msg[2]);
> + dev_warn(smi_info->dev,
> + "Maybe ok, but ipmi might run very slowly.\n");
> + } else
Minor nit: it would be nice if these warnings were collapsed into a
single printk -- that would save me a whitelist entry of acceptable
KERN_WARNING messages :)
My Dell 12g server says:
[97627.407724] ipmi_si ipmi_si.0: Using irq 10
[97627.421369] ipmi_si ipmi_si.0: Couldn't set irq info: cc.
[97627.427389] ipmi_si ipmi_si.0: Maybe ok, but ipmi might run very slowly.
Tested-by: Andy Lutomirski <[email protected]>
--Andy
On 05/16/2013 05:23 PM, Andy Lutomirski wrote:
>
> /* We got the flags from the SMI, now handle them. */
> smi_info->handlers->get_result(smi_info->si_sm, msg, 4);
> - if (msg[2] != 0)
> - dev_warn(smi_info->dev, "Could not enable interrupts"
> - ", failed set, using polled mode.\n");
> - else
> + if (msg[2] != 0) {
> + dev_warn(smi_info->dev,
> + "Couldn't set irq info: %x.\n", msg[2]);
> + dev_warn(smi_info->dev,
> + "Maybe ok, but ipmi might run very slowly.\n");
> + } else
> Minor nit: it would be nice if these warnings were collapsed into a
> single printk -- that would save me a whitelist entry of acceptable
> KERN_WARNING messages :)
Yeah, the trouble is that checkpatch will give a warning if you split a
string
between two lines or if a line is longer than 80 characters. I'm not
creative
enough to fit it into a single line. Maybe I'm trying to be too literal
here,
but I split it into two prints to avoid the warning.
>
> My Dell 12g server says:
>
> [97627.407724] ipmi_si ipmi_si.0: Using irq 10
> [97627.421369] ipmi_si ipmi_si.0: Couldn't set irq info: cc.
> [97627.427389] ipmi_si ipmi_si.0: Maybe ok, but ipmi might run very slowly.
>
> Tested-by: Andy Lutomirski <[email protected]>
Thanks a bunch.
-corey
> --Andy
On Thu, 2013-05-16 at 22:47 -0500, Corey Minyard wrote:
> On 05/16/2013 05:23 PM, Andy Lutomirski wrote:
> >
> > /* We got the flags from the SMI, now handle them. */
> > smi_info->handlers->get_result(smi_info->si_sm, msg, 4);
> > - if (msg[2] != 0)
> > - dev_warn(smi_info->dev, "Could not enable interrupts"
> > - ", failed set, using polled mode.\n");
> > - else
> > + if (msg[2] != 0) {
> > + dev_warn(smi_info->dev,
> > + "Couldn't set irq info: %x.\n", msg[2]);
> > + dev_warn(smi_info->dev,
> > + "Maybe ok, but ipmi might run very slowly.\n");
> > + } else
> > Minor nit: it would be nice if these warnings were collapsed into a
> > single printk -- that would save me a whitelist entry of acceptable
> > KERN_WARNING messages :)
>
> Yeah, the trouble is that checkpatch will give a warning if you split a
> string
> between two lines or if a line is longer than 80 characters.
Hi Corey.
Yes it will and no it won't.
dev_<level>(struct device *, "some really really long format string that makes the line longer than 80 chars\n");
passes checkpatch without warning just fine.
I'd use something like:
dev_warn(smi_info->dev,
"Couldn't set irq info: %x - this may be OK, but ipmi might run very slowly\n",
msg[2]);