From 83dea128567250ee8dee6a5f1f9913f617bfc153 Mon Sep 17 00:00:00 2001 From: Allen Pais Date: Mon, 17 Aug 2020 14:45:57 +0530 Subject: char: ipmi: convert tasklets to use new tasklet_setup() API In preparation for unconditionally passing the struct tasklet_struct pointer to all tasklet callbacks, switch to using the new tasklet_setup() and from_tasklet() to pass the tasklet pointer explicitly. Signed-off-by: Romain Perier Signed-off-by: Allen Pais Message-Id: <20200817091617.28119-3-allen.cryptic@gmail.com> Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_msghandler.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index 737c0b6b24ea..e1814b6a1225 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -39,7 +39,7 @@ static struct ipmi_recv_msg *ipmi_alloc_recv_msg(void); static int ipmi_init_msghandler(void); -static void smi_recv_tasklet(unsigned long); +static void smi_recv_tasklet(struct tasklet_struct *t); static void handle_new_recv_msgs(struct ipmi_smi *intf); static void need_waiter(struct ipmi_smi *intf); static int handle_one_recv_msg(struct ipmi_smi *intf, @@ -3430,9 +3430,8 @@ int ipmi_add_smi(struct module *owner, intf->curr_seq = 0; spin_lock_init(&intf->waiting_rcv_msgs_lock); INIT_LIST_HEAD(&intf->waiting_rcv_msgs); - tasklet_init(&intf->recv_tasklet, - smi_recv_tasklet, - (unsigned long) intf); + tasklet_setup(&intf->recv_tasklet, + smi_recv_tasklet); atomic_set(&intf->watchdog_pretimeouts_to_deliver, 0); spin_lock_init(&intf->xmit_msgs_lock); INIT_LIST_HEAD(&intf->xmit_msgs); @@ -4467,10 +4466,10 @@ static void handle_new_recv_msgs(struct ipmi_smi *intf) } } -static void smi_recv_tasklet(unsigned long val) +static void smi_recv_tasklet(struct tasklet_struct *t) { unsigned long flags = 0; /* keep us warning-free. */ - struct ipmi_smi *intf = (struct ipmi_smi *) val; + struct ipmi_smi *intf = from_tasklet(intf, t, recv_tasklet); int run_to_completion = intf->run_to_completion; struct ipmi_smi_msg *newmsg = NULL; @@ -4542,7 +4541,7 @@ void ipmi_smi_msg_received(struct ipmi_smi *intf, spin_unlock_irqrestore(&intf->xmit_msgs_lock, flags); if (run_to_completion) - smi_recv_tasklet((unsigned long) intf); + smi_recv_tasklet(&intf->recv_tasklet); else tasklet_schedule(&intf->recv_tasklet); } -- cgit v1.2.3 From 8a00e56a14a706cdca9b2a575e7a709b641f3ce0 Mon Sep 17 00:00:00 2001 From: Xiongfeng Wang Date: Thu, 3 Sep 2020 19:01:13 +0800 Subject: ipmi: add a newline when printing parameter 'panic_op' by sysfs When I cat ipmi_msghandler parameter 'panic_op' by sysfs, it displays as follows. It's better to add a newline for easy reading. root@(none):/# cat /sys/module/ipmi_msghandler/parameters/panic_op noneroot@(none):/# Signed-off-by: Xiongfeng Wang Message-Id: <1599130873-2402-1-git-send-email-wangxiongfeng2@huawei.com> Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_msghandler.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index e1814b6a1225..ed888d5a33ae 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -89,19 +89,19 @@ static int panic_op_read_handler(char *buffer, const struct kernel_param *kp) { switch (ipmi_send_panic_event) { case IPMI_SEND_PANIC_EVENT_NONE: - strcpy(buffer, "none"); + strcpy(buffer, "none\n"); break; case IPMI_SEND_PANIC_EVENT: - strcpy(buffer, "event"); + strcpy(buffer, "event\n"); break; case IPMI_SEND_PANIC_EVENT_STRING: - strcpy(buffer, "string"); + strcpy(buffer, "string\n"); break; default: - strcpy(buffer, "???"); + strcpy(buffer, "???\n"); break; } -- cgit v1.2.3 From 81e7571ea35e2b8b74b777ac8484818c8a984ea9 Mon Sep 17 00:00:00 2001 From: Markus Boehme Date: Mon, 7 Sep 2020 18:25:35 +0200 Subject: ipmi: Reset response handler when failing to send the command When failing to send a command we don't expect a response. Clear the `null_user_handler` like is done in the success path. Signed-off-by: Markus Boehme Message-Id: <1599495937-10654-1-git-send-email-markubo@amazon.com> Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_msghandler.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index ed888d5a33ae..9f61a1a30f2f 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -2433,7 +2433,7 @@ static int __get_device_id(struct ipmi_smi *intf, struct bmc_device *bmc) rv = send_get_device_id_cmd(intf); if (rv) - return rv; + goto out_reset_handler; wait_event(intf->waitq, bmc->dyn_id_set != 2); @@ -2443,6 +2443,7 @@ static int __get_device_id(struct ipmi_smi *intf, struct bmc_device *bmc) /* dyn_id_set makes the id data available. */ smp_rmb(); +out_reset_handler: intf->null_user_handler = NULL; return rv; @@ -3329,6 +3330,7 @@ static int __scan_channels(struct ipmi_smi *intf, struct ipmi_device_id *id) dev_warn(intf->si_dev, "Error sending channel information for channel 0, %d\n", rv); + intf->null_user_handler = NULL; return -EIO; } -- cgit v1.2.3 From c2b1e76d8c91166ca5f7aa8df02d67b619e24dc3 Mon Sep 17 00:00:00 2001 From: Xianting Tian Date: Tue, 15 Sep 2020 15:44:41 +0800 Subject: ipmi:sm: Print current state when the state is invalid Print current state before returning IPMI_NOT_IN_MY_STATE_ERR so we can know where this issue is coming from and possibly fix the state machine. Signed-off-by: Xianting Tian Message-Id: <20200915074441.4090-1-tian.xianting@h3c.com> [Converted printk() to pr_xxx().] Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_bt_sm.c | 4 +++- drivers/char/ipmi/ipmi_kcs_sm.c | 4 +++- drivers/char/ipmi/ipmi_smic_sm.c | 4 +++- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/drivers/char/ipmi/ipmi_bt_sm.c b/drivers/char/ipmi/ipmi_bt_sm.c index f3f216cdf686..2de0c6c304f3 100644 --- a/drivers/char/ipmi/ipmi_bt_sm.c +++ b/drivers/char/ipmi/ipmi_bt_sm.c @@ -213,8 +213,10 @@ static int bt_start_transaction(struct si_sm_data *bt, if (bt->state == BT_STATE_LONG_BUSY) return IPMI_NODE_BUSY_ERR; - if (bt->state != BT_STATE_IDLE) + if (bt->state != BT_STATE_IDLE) { + dev_warn(bt->io->dev, "BT is now in the state %d\n", bt->state); return IPMI_NOT_IN_MY_STATE_ERR; + } if (bt_debug & BT_DEBUG_MSG) { dev_dbg(bt->io->dev, "+++++++++++++++++ New command\n"); diff --git a/drivers/char/ipmi/ipmi_kcs_sm.c b/drivers/char/ipmi/ipmi_kcs_sm.c index 2e7cda08b079..5064b884ab67 100644 --- a/drivers/char/ipmi/ipmi_kcs_sm.c +++ b/drivers/char/ipmi/ipmi_kcs_sm.c @@ -268,8 +268,10 @@ static int start_kcs_transaction(struct si_sm_data *kcs, unsigned char *data, if (size > MAX_KCS_WRITE_SIZE) return IPMI_REQ_LEN_EXCEEDED_ERR; - if ((kcs->state != KCS_IDLE) && (kcs->state != KCS_HOSED)) + if ((kcs->state != KCS_IDLE) && (kcs->state != KCS_HOSED)) { + pr_warn("KCS is now in the state %d\n", kcs->state); return IPMI_NOT_IN_MY_STATE_ERR; + } if (kcs_debug & KCS_DEBUG_MSG) { printk(KERN_DEBUG "start_kcs_transaction -"); diff --git a/drivers/char/ipmi/ipmi_smic_sm.c b/drivers/char/ipmi/ipmi_smic_sm.c index b6225bba2532..5b8b6b0e463d 100644 --- a/drivers/char/ipmi/ipmi_smic_sm.c +++ b/drivers/char/ipmi/ipmi_smic_sm.c @@ -126,8 +126,10 @@ static int start_smic_transaction(struct si_sm_data *smic, if (size > MAX_SMIC_WRITE_SIZE) return IPMI_REQ_LEN_EXCEEDED_ERR; - if ((smic->state != SMIC_IDLE) && (smic->state != SMIC_HOSED)) + if ((smic->state != SMIC_IDLE) && (smic->state != SMIC_HOSED)) { + pr_warn("SMIC is now in the state %d\n", smic->state); return IPMI_NOT_IN_MY_STATE_ERR; + } if (smic_debug & SMIC_DEBUG_MSG) { printk(KERN_DEBUG "start_smic_transaction -"); -- cgit v1.2.3 From f8910ffa81b085030dc54814c85d338c26a3157e Mon Sep 17 00:00:00 2001 From: Xianting Tian Date: Tue, 15 Sep 2020 15:18:17 +0800 Subject: ipmi:msghandler: retry to get device id on an error We fail to get the BMCS's device id with low probability when loading the ipmi driver and it causes BMC device registration failed. When this issue occurs we got below kernel prints: [Wed Sep 9 19:52:03 2020] ipmi_si IPI0001:00: IPMI message handler: device id demangle failed: -22 [Wed Sep 9 19:52:03 2020] IPMI BT: using default values [Wed Sep 9 19:52:03 2020] IPMI BT: req2rsp=5 secs retries=2 [Wed Sep 9 19:52:03 2020] ipmi_si IPI0001:00: Unable to get the device id: -5 [Wed Sep 9 19:52:04 2020] ipmi_si IPI0001:00: Unable to register device: error -5 When this issue happens, we want to manually unload the driver and try to load it again, but it can't be unloaded by 'rmmod' as it is already 'in use'. We add a print in handle_one_recv_msg(), when this issue happens, the msg we received is "Recv: 1c 01 d5", which means the data_len is 1, data[0] is 0xd5 (completion code), which means "bmc cannot execute command. Command, or request parameter(s), not supported in present state". Debug code: static int handle_one_recv_msg(struct ipmi_smi *intf, struct ipmi_smi_msg *msg) { printk("Recv: %*ph\n", msg->rsp_size, msg->rsp); ... ... } Then in ipmi_demangle_device_id(), it returned '-EINVAL' as 'data_len < 7' and 'data[0] != 0'. We created this patch to retry the get device id when this error happens. We reproduced this issue again and the retry succeed on the first retry, we finally got the correct msg and then all is ok: Recv: 1c 01 00 01 81 05 84 02 af db 07 00 01 00 b9 00 10 00 So use a retry machanism in this patch to give bmc more opportunity to correctly response kernel when we received specific completion codes. Signed-off-by: Xianting Tian Message-Id: <20200915071817.4484-1-tian.xianting@h3c.com> [Cleaned up the verbage a bit in the header and prints.] Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_msghandler.c | 29 +++++++++++++++++++++++++---- include/uapi/linux/ipmi_msgdefs.h | 2 ++ 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index 9f61a1a30f2f..e56409659459 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -34,6 +34,7 @@ #include #include #include +#include #define IPMI_DRIVER_VERSION "39.2" @@ -60,6 +61,9 @@ enum ipmi_panic_event_op { #else #define IPMI_PANIC_DEFAULT IPMI_SEND_PANIC_EVENT_NONE #endif + +#define GET_DEVICE_ID_MAX_RETRY 5 + static enum ipmi_panic_event_op ipmi_send_panic_event = IPMI_PANIC_DEFAULT; static int panic_op_write_handler(const char *val, @@ -317,6 +321,7 @@ struct bmc_device { int dyn_guid_set; struct kref usecount; struct work_struct remove_work; + char cc; /* completion code */ }; #define to_bmc_device(x) container_of((x), struct bmc_device, pdev.dev) @@ -2381,6 +2386,8 @@ static void bmc_device_id_handler(struct ipmi_smi *intf, msg->msg.data, msg->msg.data_len, &intf->bmc->fetch_id); if (rv) { dev_warn(intf->si_dev, "device id demangle failed: %d\n", rv); + /* record completion code when error */ + intf->bmc->cc = msg->msg.data[0]; intf->bmc->dyn_id_set = 0; } else { /* @@ -2426,19 +2433,34 @@ send_get_device_id_cmd(struct ipmi_smi *intf) static int __get_device_id(struct ipmi_smi *intf, struct bmc_device *bmc) { int rv; - - bmc->dyn_id_set = 2; + unsigned int retry_count = 0; intf->null_user_handler = bmc_device_id_handler; +retry: + bmc->cc = 0; + bmc->dyn_id_set = 2; + rv = send_get_device_id_cmd(intf); if (rv) goto out_reset_handler; wait_event(intf->waitq, bmc->dyn_id_set != 2); - if (!bmc->dyn_id_set) + if (!bmc->dyn_id_set) { + if ((bmc->cc == IPMI_DEVICE_IN_FW_UPDATE_ERR + || bmc->cc == IPMI_DEVICE_IN_INIT_ERR + || bmc->cc == IPMI_NOT_IN_MY_STATE_ERR) + && ++retry_count <= GET_DEVICE_ID_MAX_RETRY) { + msleep(500); + dev_warn(intf->si_dev, + "BMC returned 0x%2.2x, retry get bmc device id\n", + bmc->cc); + goto retry; + } + rv = -EIO; /* Something went wrong in the fetch. */ + } /* dyn_id_set makes the id data available. */ smp_rmb(); @@ -3246,7 +3268,6 @@ channel_handler(struct ipmi_smi *intf, struct ipmi_recv_msg *msg) /* It's the one we want */ if (msg->msg.data[0] != 0) { /* Got an error from the channel, just go on. */ - if (msg->msg.data[0] == IPMI_INVALID_COMMAND_ERR) { /* * If the MC does not support this diff --git a/include/uapi/linux/ipmi_msgdefs.h b/include/uapi/linux/ipmi_msgdefs.h index c2b23a9fdf3d..0934af3b8037 100644 --- a/include/uapi/linux/ipmi_msgdefs.h +++ b/include/uapi/linux/ipmi_msgdefs.h @@ -69,6 +69,8 @@ #define IPMI_ERR_MSG_TRUNCATED 0xc6 #define IPMI_REQ_LEN_INVALID_ERR 0xc7 #define IPMI_REQ_LEN_EXCEEDED_ERR 0xc8 +#define IPMI_DEVICE_IN_FW_UPDATE_ERR 0xd1 +#define IPMI_DEVICE_IN_INIT_ERR 0xd2 #define IPMI_NOT_IN_MY_STATE_ERR 0xd5 /* IPMI 2.0 */ #define IPMI_LOST_ARBITRATION_ERR 0x81 #define IPMI_BUS_ERR 0x82 -- cgit v1.2.3 From a190db945bccdb8c51a02fc26a9f79b860dc586e Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Tue, 15 Sep 2020 08:48:56 -0500 Subject: ipmi: Clean up some printks Convert to dev_xxx() and fix some verbage. Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_bt_sm.c | 2 +- drivers/char/ipmi/ipmi_kcs_sm.c | 13 ++++++++----- drivers/char/ipmi/ipmi_smic_sm.c | 33 ++++++++++++++++++++------------- 3 files changed, 29 insertions(+), 19 deletions(-) diff --git a/drivers/char/ipmi/ipmi_bt_sm.c b/drivers/char/ipmi/ipmi_bt_sm.c index 2de0c6c304f3..f41f78972b9c 100644 --- a/drivers/char/ipmi/ipmi_bt_sm.c +++ b/drivers/char/ipmi/ipmi_bt_sm.c @@ -214,7 +214,7 @@ static int bt_start_transaction(struct si_sm_data *bt, return IPMI_NODE_BUSY_ERR; if (bt->state != BT_STATE_IDLE) { - dev_warn(bt->io->dev, "BT is now in the state %d\n", bt->state); + dev_warn(bt->io->dev, "BT in invalid state %d\n", bt->state); return IPMI_NOT_IN_MY_STATE_ERR; } diff --git a/drivers/char/ipmi/ipmi_kcs_sm.c b/drivers/char/ipmi/ipmi_kcs_sm.c index 5064b884ab67..efda90dcf5b3 100644 --- a/drivers/char/ipmi/ipmi_kcs_sm.c +++ b/drivers/char/ipmi/ipmi_kcs_sm.c @@ -17,6 +17,8 @@ * that document. */ +#define DEBUG /* So dev_dbg() is always available. */ + #include /* For printk. */ #include #include @@ -187,8 +189,8 @@ static inline void start_error_recovery(struct si_sm_data *kcs, char *reason) (kcs->error_retries)++; if (kcs->error_retries > MAX_ERROR_RETRIES) { if (kcs_debug & KCS_DEBUG_ENABLE) - printk(KERN_DEBUG "ipmi_kcs_sm: kcs hosed: %s\n", - reason); + dev_dbg(kcs->io->dev, "ipmi_kcs_sm: kcs hosed: %s\n", + reason); kcs->state = KCS_HOSED; } else { kcs->error0_timeout = jiffies + ERROR0_OBF_WAIT_JIFFIES; @@ -269,12 +271,12 @@ static int start_kcs_transaction(struct si_sm_data *kcs, unsigned char *data, return IPMI_REQ_LEN_EXCEEDED_ERR; if ((kcs->state != KCS_IDLE) && (kcs->state != KCS_HOSED)) { - pr_warn("KCS is now in the state %d\n", kcs->state); + dev_warn(kcs->io->dev, "KCS in invalid state %d\n", kcs->state); return IPMI_NOT_IN_MY_STATE_ERR; } if (kcs_debug & KCS_DEBUG_MSG) { - printk(KERN_DEBUG "start_kcs_transaction -"); + dev_dbg(kcs->io->dev, "%s -", __func__); for (i = 0; i < size; i++) pr_cont(" %02x", data[i]); pr_cont("\n"); @@ -333,7 +335,8 @@ static enum si_sm_result kcs_event(struct si_sm_data *kcs, long time) status = read_status(kcs); if (kcs_debug & KCS_DEBUG_STATES) - printk(KERN_DEBUG "KCS: State = %d, %x\n", kcs->state, status); + dev_dbg(kcs->io->dev, + "KCS: State = %d, %x\n", kcs->state, status); /* All states wait for ibf, so just do it here. */ if (!check_ibf(kcs, status, time)) diff --git a/drivers/char/ipmi/ipmi_smic_sm.c b/drivers/char/ipmi/ipmi_smic_sm.c index 5b8b6b0e463d..bfea500d6f5f 100644 --- a/drivers/char/ipmi/ipmi_smic_sm.c +++ b/drivers/char/ipmi/ipmi_smic_sm.c @@ -21,6 +21,8 @@ * 2001 Hewlett-Packard Company */ +#define DEBUG /* So dev_dbg() is always available. */ + #include /* For printk. */ #include #include @@ -127,12 +129,13 @@ static int start_smic_transaction(struct si_sm_data *smic, return IPMI_REQ_LEN_EXCEEDED_ERR; if ((smic->state != SMIC_IDLE) && (smic->state != SMIC_HOSED)) { - pr_warn("SMIC is now in the state %d\n", smic->state); + dev_warn(smic->io->dev, + "SMIC in invalid state %d\n", smic->state); return IPMI_NOT_IN_MY_STATE_ERR; } if (smic_debug & SMIC_DEBUG_MSG) { - printk(KERN_DEBUG "start_smic_transaction -"); + dev_dbg(smic->io->dev, "%s -", __func__); for (i = 0; i < size; i++) pr_cont(" %02x", data[i]); pr_cont("\n"); @@ -154,7 +157,7 @@ static int smic_get_result(struct si_sm_data *smic, int i; if (smic_debug & SMIC_DEBUG_MSG) { - printk(KERN_DEBUG "smic_get result -"); + dev_dbg(smic->io->dev, "smic_get result -"); for (i = 0; i < smic->read_pos; i++) pr_cont(" %02x", smic->read_data[i]); pr_cont("\n"); @@ -326,9 +329,9 @@ static enum si_sm_result smic_event(struct si_sm_data *smic, long time) } if (smic->state != SMIC_IDLE) { if (smic_debug & SMIC_DEBUG_STATES) - printk(KERN_DEBUG - "smic_event - smic->smic_timeout = %ld, time = %ld\n", - smic->smic_timeout, time); + dev_dbg(smic->io->dev, + "%s - smic->smic_timeout = %ld, time = %ld\n", + __func__, smic->smic_timeout, time); /* * FIXME: smic_event is sometimes called with time > * SMIC_RETRY_TIMEOUT @@ -347,8 +350,9 @@ static enum si_sm_result smic_event(struct si_sm_data *smic, long time) status = read_smic_status(smic); if (smic_debug & SMIC_DEBUG_STATES) - printk(KERN_DEBUG "smic_event - state = %d, flags = 0x%02x, status = 0x%02x\n", - smic->state, flags, status); + dev_dbg(smic->io->dev, + "%s - state = %d, flags = 0x%02x, status = 0x%02x\n", + __func__, smic->state, flags, status); switch (smic->state) { case SMIC_IDLE: @@ -438,8 +442,9 @@ static enum si_sm_result smic_event(struct si_sm_data *smic, long time) data = read_smic_data(smic); if (data != 0) { if (smic_debug & SMIC_DEBUG_ENABLE) - printk(KERN_DEBUG "SMIC_WRITE_END: data = %02x\n", - data); + dev_dbg(smic->io->dev, + "SMIC_WRITE_END: data = %02x\n", + data); start_error_recovery(smic, "state = SMIC_WRITE_END, " "data != SUCCESS"); @@ -518,8 +523,9 @@ static enum si_sm_result smic_event(struct si_sm_data *smic, long time) /* data register holds an error code */ if (data != 0) { if (smic_debug & SMIC_DEBUG_ENABLE) - printk(KERN_DEBUG "SMIC_READ_END: data = %02x\n", - data); + dev_dbg(smic->io->dev, + "SMIC_READ_END: data = %02x\n", + data); start_error_recovery(smic, "state = SMIC_READ_END, " "data != SUCCESS"); @@ -535,7 +541,8 @@ static enum si_sm_result smic_event(struct si_sm_data *smic, long time) default: if (smic_debug & SMIC_DEBUG_ENABLE) { - printk(KERN_DEBUG "smic->state = %d\n", smic->state); + dev_dbg(smic->io->dev, + "smic->state = %d\n", smic->state); start_error_recovery(smic, "state = UNKNOWN"); return SI_SM_CALL_WITH_DELAY; } -- cgit v1.2.3 From 42d8a346c5c06689f4f25aecfa287a5aca501a55 Mon Sep 17 00:00:00 2001 From: Xianting Tian Date: Wed, 16 Sep 2020 14:21:29 +0800 Subject: ipmi: add retry in try_get_dev_id() Use a retry machanism to give the BMC more opportunities to correctly respond when we receive specific completion codes. This is similar to what is done in __get_device_id(). Signed-off-by: Xianting Tian Message-Id: <20200916062129.26129-1-tian.xianting@h3c.com> [Moved GET_DEVICE_ID_MAX_RETRY to include/linux/ipmi.h, reworded some text.] Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_msghandler.c | 2 -- drivers/char/ipmi/ipmi_si_intf.c | 17 +++++++++++++++++ include/linux/ipmi.h | 2 ++ 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index e56409659459..555c3b1e4926 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -62,8 +62,6 @@ enum ipmi_panic_event_op { #define IPMI_PANIC_DEFAULT IPMI_SEND_PANIC_EVENT_NONE #endif -#define GET_DEVICE_ID_MAX_RETRY 5 - static enum ipmi_panic_event_op ipmi_send_panic_event = IPMI_PANIC_DEFAULT; static int panic_op_write_handler(const char *val, diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c index 77b8d551ae7f..164f85007080 100644 --- a/drivers/char/ipmi/ipmi_si_intf.c +++ b/drivers/char/ipmi/ipmi_si_intf.c @@ -1316,6 +1316,7 @@ static int try_get_dev_id(struct smi_info *smi_info) unsigned char *resp; unsigned long resp_len; int rv = 0; + unsigned int retry_count = 0; resp = kmalloc(IPMI_MAX_MSG_LENGTH, GFP_KERNEL); if (!resp) @@ -1327,6 +1328,8 @@ static int try_get_dev_id(struct smi_info *smi_info) */ msg[0] = IPMI_NETFN_APP_REQUEST << 2; msg[1] = IPMI_GET_DEVICE_ID_CMD; + +retry: smi_info->handlers->start_transaction(smi_info->si_sm, msg, 2); rv = wait_for_msg_done(smi_info); @@ -1339,6 +1342,20 @@ static int try_get_dev_id(struct smi_info *smi_info) /* Check and record info from the get device id, in case we need it. */ rv = ipmi_demangle_device_id(resp[0] >> 2, resp[1], resp + 2, resp_len - 2, &smi_info->device_id); + if (rv) { + /* record completion code */ + char cc = *(resp + 2); + + if ((cc == IPMI_DEVICE_IN_FW_UPDATE_ERR + || cc == IPMI_DEVICE_IN_INIT_ERR + || cc == IPMI_NOT_IN_MY_STATE_ERR) + && ++retry_count <= GET_DEVICE_ID_MAX_RETRY) { + dev_warn(smi_info->io.dev, + "BMC returned 0x%2.2x, retry get bmc device id\n", + cc); + goto retry; + } + } out: kfree(resp); diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h index ef61676cfe05..52850a02a3d0 100644 --- a/include/linux/ipmi.h +++ b/include/linux/ipmi.h @@ -333,4 +333,6 @@ struct ipmi_smi_info { /* This is to get the private info of struct ipmi_smi */ extern int ipmi_get_smi_info(int if_num, struct ipmi_smi_info *data); +#define GET_DEVICE_ID_MAX_RETRY 5 + #endif /* __LINUX_IPMI_H */ -- cgit v1.2.3 From c011410d9145695c460567fa5ba1ade6a7b16f06 Mon Sep 17 00:00:00 2001 From: Dan Carpenter Date: Fri, 18 Sep 2020 17:27:56 +0300 Subject: ipmi: msghandler: Fix a signedness bug The type for the completion codes should be unsigned char instead of char. If it is declared as a normal char then the conditions in __get_device_id() are impossible because the IPMI_DEVICE_IN_FW_UPDATE_ERR error codes are higher than 127. drivers/char/ipmi/ipmi_msghandler.c:2449 __get_device_id() warn: impossible condition '(bmc->cc == 209) => ((-128)-127 == 209)' Fixes: f8910ffa81b0 ("ipmi:msghandler: retry to get device id on an error") Signed-off-by: Dan Carpenter Message-Id: <20200918142756.GB909725@mwanda> Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_msghandler.c | 2 +- drivers/char/ipmi/ipmi_si_intf.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index 555c3b1e4926..8774a3b8ff95 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -319,7 +319,7 @@ struct bmc_device { int dyn_guid_set; struct kref usecount; struct work_struct remove_work; - char cc; /* completion code */ + unsigned char cc; /* completion code */ }; #define to_bmc_device(x) container_of((x), struct bmc_device, pdev.dev) diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c index 164f85007080..0b3dbc7e39fd 100644 --- a/drivers/char/ipmi/ipmi_si_intf.c +++ b/drivers/char/ipmi/ipmi_si_intf.c @@ -1344,7 +1344,7 @@ retry: resp + 2, resp_len - 2, &smi_info->device_id); if (rv) { /* record completion code */ - char cc = *(resp + 2); + unsigned char cc = *(resp + 2); if ((cc == IPMI_DEVICE_IN_FW_UPDATE_ERR || cc == IPMI_DEVICE_IN_INIT_ERR -- cgit v1.2.3 From 8fe7990ceda8597e407d06bffc4bdbe835a93ece Mon Sep 17 00:00:00 2001 From: Tianjia Zhang Date: Mon, 5 Oct 2020 22:52:12 +0800 Subject: ipmi_si: Fix wrong return value in try_smi_init() On an error exit path, a negative error code should be returned instead of a positive return value. Fixes: 90b2d4f15ff7 ("ipmi_si: Remove hacks for adding a dummy platform devices") Cc: Corey Minyard Signed-off-by: Tianjia Zhang Message-Id: <20201005145212.84435-1-tianjia.zhang@linux.alibaba.com> Signed-off-by: Corey Minyard --- 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 0b3dbc7e39fd..5eac94cf4ff8 100644 --- a/drivers/char/ipmi/ipmi_si_intf.c +++ b/drivers/char/ipmi/ipmi_si_intf.c @@ -1980,7 +1980,7 @@ static int try_smi_init(struct smi_info *new_smi) /* Do this early so it's available for logs. */ if (!new_smi->io.dev) { pr_err("IPMI interface added with no device\n"); - rv = EIO; + rv = -EIO; goto out_err; } -- cgit v1.2.3