From cbb19cb1eef050b193c823502bf920097c0f0459 Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Wed, 31 Jul 2019 15:33:36 -0500 Subject: ipmi_si: Convert timespec64 to timespec There is no need for timespec64, and it will cause issues in the future with i386 and 64-bit division not being available. Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_si_intf.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) (limited to 'drivers/char') diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c index da5b6723329a..488979cc2579 100644 --- a/drivers/char/ipmi/ipmi_si_intf.c +++ b/drivers/char/ipmi/ipmi_si_intf.c @@ -261,10 +261,10 @@ static void cleanup_ipmi_si(void); #ifdef DEBUG_TIMING void debug_timestamp(char *msg) { - struct timespec64 t; + struct timespec t; - ktime_get_ts64(&t); - pr_debug("**%s: %lld.%9.9ld\n", msg, (long long) t.tv_sec, t.tv_nsec); + ktime_get_ts(&t); + pr_debug("**%s: %ld.%9.9ld\n", msg, (long) t.tv_sec, t.tv_nsec); } #else #define debug_timestamp(x) @@ -935,18 +935,18 @@ static void set_run_to_completion(void *send_info, bool i_run_to_completion) * we are spinning in kipmid looking for something and not delaying * between checks */ -static inline void ipmi_si_set_not_busy(struct timespec64 *ts) +static inline void ipmi_si_set_not_busy(struct timespec *ts) { ts->tv_nsec = -1; } -static inline int ipmi_si_is_busy(struct timespec64 *ts) +static inline int ipmi_si_is_busy(struct timespec *ts) { return ts->tv_nsec != -1; } -static inline int ipmi_thread_busy_wait(enum si_sm_result smi_result, - const struct smi_info *smi_info, - struct timespec64 *busy_until) +static inline bool ipmi_thread_busy_wait(enum si_sm_result smi_result, + const struct smi_info *smi_info, + struct timespec *busy_until) { unsigned int max_busy_us = 0; @@ -955,18 +955,18 @@ static inline int ipmi_thread_busy_wait(enum si_sm_result smi_result, if (max_busy_us == 0 || smi_result != SI_SM_CALL_WITH_DELAY) ipmi_si_set_not_busy(busy_until); else if (!ipmi_si_is_busy(busy_until)) { - ktime_get_ts64(busy_until); - timespec64_add_ns(busy_until, max_busy_us*NSEC_PER_USEC); + ktime_get_ts(busy_until); + timespec_add_ns(busy_until, max_busy_us * NSEC_PER_USEC); } else { - struct timespec64 now; + struct timespec now; - ktime_get_ts64(&now); - if (unlikely(timespec64_compare(&now, busy_until) > 0)) { + ktime_get_ts(&now); + if (unlikely(timespec_compare(&now, busy_until) > 0)) { ipmi_si_set_not_busy(busy_until); - return 0; + return false; } } - return 1; + return true; } @@ -984,7 +984,7 @@ static int ipmi_thread(void *data) struct smi_info *smi_info = data; unsigned long flags; enum si_sm_result smi_result; - struct timespec64 busy_until; + struct timespec busy_until = { 0, 0 }; ipmi_si_set_not_busy(&busy_until); set_user_nice(current, MAX_NICE); -- cgit v1.2.3 From 104fb25f60077e4696145bcea51ca56f0959d7e3 Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Wed, 31 Jul 2019 19:18:25 -0500 Subject: ipmi_si: Rework some include files ipmi_si_sm.h was getting included in lots of places it didn't belong. Rework things a bit to remove all the dependencies, mostly just moving things between include files that were in the wrong place and removing bogus includes. Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_dmi.c | 1 - drivers/char/ipmi/ipmi_dmi.h | 1 + drivers/char/ipmi/ipmi_si.h | 57 ++++++++++++++++++++++++++++++++++-- drivers/char/ipmi/ipmi_si_intf.c | 5 ++-- drivers/char/ipmi/ipmi_si_mem_io.c | 2 +- drivers/char/ipmi/ipmi_si_pci.c | 2 +- drivers/char/ipmi/ipmi_si_platform.c | 2 +- drivers/char/ipmi/ipmi_si_port_io.c | 2 +- drivers/char/ipmi/ipmi_si_sm.h | 54 ++++------------------------------ drivers/char/ipmi/ipmi_ssif.c | 1 - 10 files changed, 68 insertions(+), 59 deletions(-) (limited to 'drivers/char') diff --git a/drivers/char/ipmi/ipmi_dmi.c b/drivers/char/ipmi/ipmi_dmi.c index f38e651dd1b5..bbf7029e224b 100644 --- a/drivers/char/ipmi/ipmi_dmi.c +++ b/drivers/char/ipmi/ipmi_dmi.c @@ -12,7 +12,6 @@ #include #include #include -#include "ipmi_si_sm.h" #include "ipmi_dmi.h" #include "ipmi_plat_data.h" diff --git a/drivers/char/ipmi/ipmi_dmi.h b/drivers/char/ipmi/ipmi_dmi.h index 2dbec0461d0c..e16a9dbdcc29 100644 --- a/drivers/char/ipmi/ipmi_dmi.h +++ b/drivers/char/ipmi/ipmi_dmi.h @@ -2,6 +2,7 @@ /* * DMI defines for use by IPMI */ +#include "ipmi_si.h" #ifdef CONFIG_IPMI_DMI_DECODE int ipmi_dmi_get_slave_addr(enum si_type si_type, unsigned int space, diff --git a/drivers/char/ipmi/ipmi_si.h b/drivers/char/ipmi/ipmi_si.h index 357a229c9012..bac0ff86e48e 100644 --- a/drivers/char/ipmi/ipmi_si.h +++ b/drivers/char/ipmi/ipmi_si.h @@ -6,14 +6,65 @@ * etc) to the base ipmi system interface code. */ +#ifndef __IPMI_SI_H__ +#define __IPMI_SI_H__ + +#include #include #include -#include "ipmi_si_sm.h" + +#define SI_DEVICE_NAME "ipmi_si" #define DEFAULT_REGSPACING 1 #define DEFAULT_REGSIZE 1 -#define DEVICE_NAME "ipmi_si" +enum si_type { + SI_TYPE_INVALID, SI_KCS, SI_SMIC, SI_BT +}; + +enum ipmi_addr_space { + IPMI_IO_ADDR_SPACE, IPMI_MEM_ADDR_SPACE +}; + +/* + * The structure for doing I/O in the state machine. The state + * machine doesn't have the actual I/O routines, they are done through + * this interface. + */ +struct si_sm_io { + unsigned char (*inputb)(const struct si_sm_io *io, unsigned int offset); + void (*outputb)(const struct si_sm_io *io, + unsigned int offset, + unsigned char b); + + /* + * Generic info used by the actual handling routines, the + * state machine shouldn't touch these. + */ + void __iomem *addr; + unsigned int regspacing; + unsigned int regsize; + unsigned int regshift; + enum ipmi_addr_space addr_space; + unsigned long addr_data; + enum ipmi_addr_src addr_source; /* ACPI, PCI, SMBIOS, hardcode, etc. */ + void (*addr_source_cleanup)(struct si_sm_io *io); + void *addr_source_data; + union ipmi_smi_info_union addr_info; + + int (*io_setup)(struct si_sm_io *info); + void (*io_cleanup)(struct si_sm_io *info); + unsigned int io_size; + + int irq; + int (*irq_setup)(struct si_sm_io *io); + void *irq_handler_data; + void (*irq_cleanup)(struct si_sm_io *io); + + u8 slave_addr; + enum si_type si_type; + struct device *dev; +}; int ipmi_si_add_smi(struct si_sm_io *io); irqreturn_t ipmi_si_irq_handler(int irq, void *data); @@ -50,3 +101,5 @@ static inline void ipmi_si_parisc_shutdown(void) { } int ipmi_si_port_setup(struct si_sm_io *io); int ipmi_si_mem_setup(struct si_sm_io *io); + +#endif /* __IPMI_SI_H__ */ diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c index 488979cc2579..d728682236ea 100644 --- a/drivers/char/ipmi/ipmi_si_intf.c +++ b/drivers/char/ipmi/ipmi_si_intf.c @@ -40,6 +40,7 @@ #include #include #include "ipmi_si.h" +#include "ipmi_si_sm.h" #include #include @@ -1266,12 +1267,12 @@ int ipmi_std_irq_setup(struct si_sm_io *io) rv = request_irq(io->irq, ipmi_si_irq_handler, IRQF_SHARED, - DEVICE_NAME, + SI_DEVICE_NAME, io->irq_handler_data); if (rv) { dev_warn(io->dev, "%s unable to claim interrupt %d," " running polled\n", - DEVICE_NAME, io->irq); + SI_DEVICE_NAME, io->irq); io->irq = 0; } else { io->irq_cleanup = std_irq_cleanup; diff --git a/drivers/char/ipmi/ipmi_si_mem_io.c b/drivers/char/ipmi/ipmi_si_mem_io.c index 75583612ab10..86b92e93a70d 100644 --- a/drivers/char/ipmi/ipmi_si_mem_io.c +++ b/drivers/char/ipmi/ipmi_si_mem_io.c @@ -118,7 +118,7 @@ int ipmi_si_mem_setup(struct si_sm_io *io) */ for (idx = 0; idx < io->io_size; idx++) { if (request_mem_region(addr + idx * io->regspacing, - io->regsize, DEVICE_NAME) == NULL) { + io->regsize, SI_DEVICE_NAME) == NULL) { /* Undo allocations */ mem_region_cleanup(io, idx); return -EIO; diff --git a/drivers/char/ipmi/ipmi_si_pci.c b/drivers/char/ipmi/ipmi_si_pci.c index ce93fc7a1e36..95bbcfba5408 100644 --- a/drivers/char/ipmi/ipmi_si_pci.c +++ b/drivers/char/ipmi/ipmi_si_pci.c @@ -150,7 +150,7 @@ static const struct pci_device_id ipmi_pci_devices[] = { MODULE_DEVICE_TABLE(pci, ipmi_pci_devices); static struct pci_driver ipmi_pci_driver = { - .name = DEVICE_NAME, + .name = SI_DEVICE_NAME, .id_table = ipmi_pci_devices, .probe = ipmi_pci_probe, .remove = ipmi_pci_remove, diff --git a/drivers/char/ipmi/ipmi_si_platform.c b/drivers/char/ipmi/ipmi_si_platform.c index 22f6c9b20e9a..c78127ccbc0d 100644 --- a/drivers/char/ipmi/ipmi_si_platform.c +++ b/drivers/char/ipmi/ipmi_si_platform.c @@ -457,7 +457,7 @@ static const struct platform_device_id si_plat_ids[] = { struct platform_driver ipmi_platform_driver = { .driver = { - .name = DEVICE_NAME, + .name = SI_DEVICE_NAME, .of_match_table = of_ipmi_match, .acpi_match_table = ACPI_PTR(acpi_ipmi_match), }, diff --git a/drivers/char/ipmi/ipmi_si_port_io.c b/drivers/char/ipmi/ipmi_si_port_io.c index 03924c32b6e9..7d66f68eb1f1 100644 --- a/drivers/char/ipmi/ipmi_si_port_io.c +++ b/drivers/char/ipmi/ipmi_si_port_io.c @@ -99,7 +99,7 @@ int ipmi_si_port_setup(struct si_sm_io *io) */ for (idx = 0; idx < io->io_size; idx++) { if (request_region(addr + idx * io->regspacing, - io->regsize, DEVICE_NAME) == NULL) { + io->regsize, SI_DEVICE_NAME) == NULL) { /* Undo allocations */ while (idx--) release_region(addr + idx * io->regspacing, diff --git a/drivers/char/ipmi/ipmi_si_sm.h b/drivers/char/ipmi/ipmi_si_sm.h index 499db820fadb..c3cdbcab0f77 100644 --- a/drivers/char/ipmi/ipmi_si_sm.h +++ b/drivers/char/ipmi/ipmi_si_sm.h @@ -14,7 +14,10 @@ * Copyright 2002 MontaVista Software Inc. */ -#include +#ifndef __IPMI_SI_SM_H__ +#define __IPMI_SI_SM_H__ + +#include "ipmi_si.h" /* * This is defined by the state machines themselves, it is an opaque @@ -22,54 +25,6 @@ */ struct si_sm_data; -enum si_type { - SI_TYPE_INVALID, SI_KCS, SI_SMIC, SI_BT -}; - -enum ipmi_addr_space { - IPMI_IO_ADDR_SPACE, IPMI_MEM_ADDR_SPACE -}; - -/* - * The structure for doing I/O in the state machine. The state - * machine doesn't have the actual I/O routines, they are done through - * this interface. - */ -struct si_sm_io { - unsigned char (*inputb)(const struct si_sm_io *io, unsigned int offset); - void (*outputb)(const struct si_sm_io *io, - unsigned int offset, - unsigned char b); - - /* - * Generic info used by the actual handling routines, the - * state machine shouldn't touch these. - */ - void __iomem *addr; - unsigned int regspacing; - unsigned int regsize; - unsigned int regshift; - enum ipmi_addr_space addr_space; - unsigned long addr_data; - enum ipmi_addr_src addr_source; /* ACPI, PCI, SMBIOS, hardcode, etc. */ - void (*addr_source_cleanup)(struct si_sm_io *io); - void *addr_source_data; - union ipmi_smi_info_union addr_info; - - int (*io_setup)(struct si_sm_io *info); - void (*io_cleanup)(struct si_sm_io *info); - unsigned int io_size; - - int irq; - int (*irq_setup)(struct si_sm_io *io); - void *irq_handler_data; - void (*irq_cleanup)(struct si_sm_io *io); - - u8 slave_addr; - enum si_type si_type; - struct device *dev; -}; - /* Results of SMI events. */ enum si_sm_result { SI_SM_CALL_WITHOUT_DELAY, /* Call the driver again immediately */ @@ -146,3 +101,4 @@ extern const struct si_sm_handlers kcs_smi_handlers; extern const struct si_sm_handlers smic_smi_handlers; extern const struct si_sm_handlers bt_smi_handlers; +#endif /* __IPMI_SI_SM_H__ */ diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c index 305fa5054274..6e070ef4391b 100644 --- a/drivers/char/ipmi/ipmi_ssif.c +++ b/drivers/char/ipmi/ipmi_ssif.c @@ -52,7 +52,6 @@ #include #include #include -#include "ipmi_si_sm.h" #include "ipmi_dmi.h" #define DEVICE_NAME "ipmi_ssif" -- cgit v1.2.3 From a6f4c33187d038dc2ca4ab885eb5e9c44940760f Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Wed, 31 Jul 2019 19:37:43 -0500 Subject: ipmi_si: Convert device attr permissions to octal Kernel preferences are for octal values instead of symbols. Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_si_intf.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'drivers/char') diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c index d728682236ea..41586ddbb3df 100644 --- a/drivers/char/ipmi/ipmi_si_intf.c +++ b/drivers/char/ipmi/ipmi_si_intf.c @@ -1595,7 +1595,7 @@ static ssize_t ipmi_##name##_show(struct device *dev, \ \ return snprintf(buf, 10, "%u\n", smi_get_stat(smi_info, name)); \ } \ -static DEVICE_ATTR(name, S_IRUGO, ipmi_##name##_show, NULL) +static DEVICE_ATTR(name, 0444, ipmi_##name##_show, NULL) static ssize_t ipmi_type_show(struct device *dev, struct device_attribute *attr, @@ -1605,7 +1605,7 @@ static ssize_t ipmi_type_show(struct device *dev, return snprintf(buf, 10, "%s\n", si_to_str[smi_info->io.si_type]); } -static DEVICE_ATTR(type, S_IRUGO, ipmi_type_show, NULL); +static DEVICE_ATTR(type, 0444, ipmi_type_show, NULL); static ssize_t ipmi_interrupts_enabled_show(struct device *dev, struct device_attribute *attr, @@ -1616,7 +1616,7 @@ static ssize_t ipmi_interrupts_enabled_show(struct device *dev, return snprintf(buf, 10, "%d\n", enabled); } -static DEVICE_ATTR(interrupts_enabled, S_IRUGO, +static DEVICE_ATTR(interrupts_enabled, 0444, ipmi_interrupts_enabled_show, NULL); IPMI_SI_ATTR(short_timeouts); @@ -1648,7 +1648,7 @@ static ssize_t ipmi_params_show(struct device *dev, smi_info->io.irq, smi_info->io.slave_addr); } -static DEVICE_ATTR(params, S_IRUGO, ipmi_params_show, NULL); +static DEVICE_ATTR(params, 0444, ipmi_params_show, NULL); static struct attribute *ipmi_si_dev_attrs[] = { &dev_attr_type.attr, -- cgit v1.2.3 From 93b6984b31182cfc340495af17691c0b9d53f6b2 Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Wed, 31 Jul 2019 19:46:53 -0500 Subject: ipmi_si: Remove ipmi_ from the device attr names Better conform with kernel style. Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_si_intf.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) (limited to 'drivers/char') diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c index 41586ddbb3df..8c1d1b8327aa 100644 --- a/drivers/char/ipmi/ipmi_si_intf.c +++ b/drivers/char/ipmi/ipmi_si_intf.c @@ -1587,29 +1587,29 @@ out: } #define IPMI_SI_ATTR(name) \ -static ssize_t ipmi_##name##_show(struct device *dev, \ - struct device_attribute *attr, \ - char *buf) \ +static ssize_t name##_show(struct device *dev, \ + struct device_attribute *attr, \ + char *buf) \ { \ struct smi_info *smi_info = dev_get_drvdata(dev); \ \ return snprintf(buf, 10, "%u\n", smi_get_stat(smi_info, name)); \ } \ -static DEVICE_ATTR(name, 0444, ipmi_##name##_show, NULL) +static DEVICE_ATTR(name, 0444, name##_show, NULL) -static ssize_t ipmi_type_show(struct device *dev, - struct device_attribute *attr, - char *buf) +static ssize_t type_show(struct device *dev, + struct device_attribute *attr, + char *buf) { struct smi_info *smi_info = dev_get_drvdata(dev); return snprintf(buf, 10, "%s\n", si_to_str[smi_info->io.si_type]); } -static DEVICE_ATTR(type, 0444, ipmi_type_show, NULL); +static DEVICE_ATTR(type, 0444, type_show, NULL); -static ssize_t ipmi_interrupts_enabled_show(struct device *dev, - struct device_attribute *attr, - char *buf) +static ssize_t interrupts_enabled_show(struct device *dev, + struct device_attribute *attr, + char *buf) { struct smi_info *smi_info = dev_get_drvdata(dev); int enabled = smi_info->io.irq && !smi_info->interrupt_disabled; @@ -1617,7 +1617,7 @@ static ssize_t ipmi_interrupts_enabled_show(struct device *dev, return snprintf(buf, 10, "%d\n", enabled); } static DEVICE_ATTR(interrupts_enabled, 0444, - ipmi_interrupts_enabled_show, NULL); + interrupts_enabled_show, NULL); IPMI_SI_ATTR(short_timeouts); IPMI_SI_ATTR(long_timeouts); @@ -1631,9 +1631,9 @@ IPMI_SI_ATTR(events); IPMI_SI_ATTR(watchdog_pretimeouts); IPMI_SI_ATTR(incoming_messages); -static ssize_t ipmi_params_show(struct device *dev, - struct device_attribute *attr, - char *buf) +static ssize_t params_show(struct device *dev, + struct device_attribute *attr, + char *buf) { struct smi_info *smi_info = dev_get_drvdata(dev); @@ -1648,7 +1648,7 @@ static ssize_t ipmi_params_show(struct device *dev, smi_info->io.irq, smi_info->io.slave_addr); } -static DEVICE_ATTR(params, 0444, ipmi_params_show, NULL); +static DEVICE_ATTR(params, 0444, params_show, NULL); static struct attribute *ipmi_si_dev_attrs[] = { &dev_attr_type.attr, -- cgit v1.2.3 From 340ff31ab00bca5c15915e70ad9ada3030c98cf8 Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Fri, 2 Aug 2019 07:31:36 -0500 Subject: ipmi_si: Only schedule continuously in the thread in maintenance mode ipmi_thread() uses back-to-back schedule() to poll for command completion which, on some machines, can push up CPU consumption and heavily tax the scheduler locks leading to noticeable overall performance degradation. This was originally added so firmware updates through IPMI would complete in a timely manner. But we can't kill the scheduler locks for that one use case. Instead, only run schedule() continuously in maintenance mode, where firmware updates should run. Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_si_intf.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) (limited to 'drivers/char') diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c index 8c1d1b8327aa..d58253432a9a 100644 --- a/drivers/char/ipmi/ipmi_si_intf.c +++ b/drivers/char/ipmi/ipmi_si_intf.c @@ -222,6 +222,9 @@ struct smi_info { */ bool irq_enable_broken; + /* Is the driver in maintenance mode? */ + bool in_maintenance_mode; + /* * Did we get an attention that we did not handle? */ @@ -1008,11 +1011,20 @@ static int ipmi_thread(void *data) spin_unlock_irqrestore(&(smi_info->si_lock), flags); busy_wait = ipmi_thread_busy_wait(smi_result, smi_info, &busy_until); - if (smi_result == SI_SM_CALL_WITHOUT_DELAY) + if (smi_result == SI_SM_CALL_WITHOUT_DELAY) { ; /* do nothing */ - else if (smi_result == SI_SM_CALL_WITH_DELAY && busy_wait) - schedule(); - else if (smi_result == SI_SM_IDLE) { + } else if (smi_result == SI_SM_CALL_WITH_DELAY && busy_wait) { + /* + * In maintenance mode we run as fast as + * possible to allow firmware updates to + * complete as fast as possible, but normally + * don't bang on the scheduler. + */ + if (smi_info->in_maintenance_mode) + schedule(); + else + usleep_range(100, 200); + } else if (smi_result == SI_SM_IDLE) { if (atomic_read(&smi_info->need_watch)) { schedule_timeout_interruptible(100); } else { @@ -1020,8 +1032,9 @@ static int ipmi_thread(void *data) __set_current_state(TASK_INTERRUPTIBLE); schedule(); } - } else + } else { schedule_timeout_interruptible(1); + } } return 0; } @@ -1199,6 +1212,7 @@ static void set_maintenance_mode(void *send_info, bool enable) if (!enable) atomic_set(&smi_info->req_events, 0); + smi_info->in_maintenance_mode = enable; } static void shutdown_smi(void *send_info); -- cgit v1.2.3 From 2033f6858970b98c18bed4d5ae68f43d17400abc Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Fri, 16 Aug 2019 16:13:42 -0500 Subject: ipmi: Free receive messages when in an oops If the driver handles a response in an oops, it was just ignoring the message. However, the IPMI watchdog timer was counting on the free happening to know when panic-time messages were complete. So free it in all cases. Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_msghandler.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'drivers/char') diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index 6707659cffd6..3548aceed4a9 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -904,12 +904,14 @@ static int deliver_response(struct ipmi_smi *intf, struct ipmi_recv_msg *msg) rv = -EINVAL; } ipmi_free_recv_msg(msg); - } else if (!oops_in_progress) { + } else if (oops_in_progress) { /* * If we are running in the panic context, calling the * receive handler doesn't much meaning and has a deadlock * risk. At this moment, simply skip it in that case. */ + ipmi_free_recv_msg(msg); + } else { int index; struct ipmi_user *user = acquire_ipmi_user(msg->user, &index); @@ -2220,7 +2222,8 @@ static int i_ipmi_request(struct ipmi_user *user, else { smi_msg = ipmi_alloc_smi_msg(); if (smi_msg == NULL) { - ipmi_free_recv_msg(recv_msg); + if (!supplied_recv) + ipmi_free_recv_msg(recv_msg); rv = -ENOMEM; goto out; } -- cgit v1.2.3 From c4436c9149c5d2bc0c49ab57ec85c75ea1c4d61c Mon Sep 17 00:00:00 2001 From: Kamlakant Patel Date: Wed, 21 Aug 2019 12:04:33 +0000 Subject: ipmi_ssif: avoid registering duplicate ssif interface It is possible that SSIF interface entry is present in both DMI and ACPI tables. In SMP systems, in such cases it is possible that ssif_probe could be called simultaneously from i2c interface (from ACPI) and from DMI on different CPUs at kernel boot. Both try to register same SSIF interface simultaneously and result in race. In such cases where ACPI and SMBIOS both IPMI entries are available, we need to prefer ACPI over SMBIOS so that ACPI functions work properly if they use IPMI. So, if we get an ACPI interface and have already registered an SMBIOS at the same address, we need to remove the SMBIOS one and add the ACPI. Log: [ 38.774743] ipmi device interface [ 38.805006] ipmi_ssif: IPMI SSIF Interface driver [ 38.861979] ipmi_ssif i2c-IPI0001:06: ssif_probe CPU 99 *** [ 38.863655] ipmi_ssif 0-000e: ssif_probe CPU 14 *** [ 38.863658] ipmi_ssif: Trying SMBIOS-specified SSIF interface at i2c address 0xe, adapter xlp9xx-i2c, slave address 0x0 [ 38.869500] ipmi_ssif: Trying ACPI-specified SSIF interface at i2c address 0xe, adapter xlp9xx-i2c, slave address 0x0 [ 38.914530] ipmi_ssif: Unable to clear message flags: -22 7 c7 [ 38.952429] ipmi_ssif: Unable to clear message flags: -22 7 00 [ 38.994734] ipmi_ssif: Error getting global enables: -22 7 00 [ 39.015877] ipmi_ssif 0-000e: IPMI message handler: Found new BMC (man_id: 0x00b3d1, prod_id: 0x0001, dev_id: 0x20) [ 39.377645] ipmi_ssif i2c-IPI0001:06: IPMI message handler: Found new BMC (man_id: 0x00b3d1, prod_id: 0x0001, dev_id: 0x20) [ 39.387863] ipmi_ssif 0-000e: IPMI message handler: BMC returned incorrect response, expected netfn 7 cmd 42, got netfn 7 cmd 1 ... [NOTE] : Added custom prints to explain the problem. In the above log, ssif_probe is executed simultaneously on two different CPUs. This patch fixes this issue in following way: - Adds ACPI entry also to the 'ssif_infos' list. - Checks the list if SMBIOS is already registered, removes it and adds ACPI. - If ACPI is already registered, it ignores SMBIOS. - Adds mutex lock throughout the probe process to avoid race. Signed-off-by: Kamlakant Patel Message-Id: <1566389064-27356-1-git-send-email-kamlakantp@marvell.com> Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_ssif.c | 78 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 77 insertions(+), 1 deletion(-) (limited to 'drivers/char') diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c index 6e070ef4391b..22c6a2e61236 100644 --- a/drivers/char/ipmi/ipmi_ssif.c +++ b/drivers/char/ipmi/ipmi_ssif.c @@ -1427,6 +1427,10 @@ static struct ssif_addr_info *ssif_info_find(unsigned short addr, restart: list_for_each_entry(info, &ssif_infos, link) { if (info->binfo.addr == addr) { + if (info->addr_src == SI_SMBIOS) + info->adapter_name = kstrdup(adapter_name, + GFP_KERNEL); + if (info->adapter_name || adapter_name) { if (!info->adapter_name != !adapter_name) { /* One is NULL and one is not */ @@ -1602,6 +1606,60 @@ out_no_multi_part: #define GLOBAL_ENABLES_MASK (IPMI_BMC_EVT_MSG_BUFF | IPMI_BMC_RCV_MSG_INTR | \ IPMI_BMC_EVT_MSG_INTR) +static void ssif_remove_dup(struct i2c_client *client) +{ + struct ssif_info *ssif_info = i2c_get_clientdata(client); + + ipmi_unregister_smi(ssif_info->intf); + kfree(ssif_info); +} + +static int ssif_add_infos(struct i2c_client *client) +{ + struct ssif_addr_info *info; + + info = kzalloc(sizeof(*info), GFP_KERNEL); + if (!info) + return -ENOMEM; + info->addr_src = SI_ACPI; + info->client = client; + info->adapter_name = kstrdup(client->adapter->name, GFP_KERNEL); + info->binfo.addr = client->addr; + list_add_tail(&info->link, &ssif_infos); + return 0; +} + +/* + * Prefer ACPI over SMBIOS, if both are available. + * So if we get an ACPI interface and have already registered a SMBIOS + * interface at the same address, remove the SMBIOS and add the ACPI one. + */ +static int ssif_check_and_remove(struct i2c_client *client, + struct ssif_info *ssif_info) +{ + struct ssif_addr_info *info; + + list_for_each_entry(info, &ssif_infos, link) { + if (!info->client) + return 0; + if (!strcmp(info->adapter_name, client->adapter->name) && + info->binfo.addr == client->addr) { + if (info->addr_src == SI_ACPI) + return -EEXIST; + + if (ssif_info->addr_source == SI_ACPI && + info->addr_src == SI_SMBIOS) { + dev_info(&client->dev, + "Removing %s-specified SSIF interface in favor of ACPI\n", + ipmi_addr_src_to_str(info->addr_src)); + ssif_remove_dup(info->client); + return 0; + } + } + } + return 0; +} + static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id) { unsigned char msg[3]; @@ -1613,13 +1671,17 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id) u8 slave_addr = 0; struct ssif_addr_info *addr_info = NULL; + mutex_lock(&ssif_infos_mutex); resp = kmalloc(IPMI_MAX_MSG_LENGTH, GFP_KERNEL); - if (!resp) + if (!resp) { + mutex_unlock(&ssif_infos_mutex); return -ENOMEM; + } ssif_info = kzalloc(sizeof(*ssif_info), GFP_KERNEL); if (!ssif_info) { kfree(resp); + mutex_unlock(&ssif_infos_mutex); return -ENOMEM; } @@ -1638,6 +1700,19 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id) } } + rv = ssif_check_and_remove(client, ssif_info); + /* If rv is 0 and addr source is not SI_ACPI, continue probing */ + if (!rv && ssif_info->addr_source == SI_ACPI) { + rv = ssif_add_infos(client); + if (rv) { + dev_err(&client->dev, "Out of memory!, exiting ..\n"); + goto out; + } + } else if (rv) { + dev_err(&client->dev, "Not probing, Interface already present\n"); + goto out; + } + slave_addr = find_slave_address(client, slave_addr); dev_info(&client->dev, @@ -1850,6 +1925,7 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id) kfree(ssif_info); } kfree(resp); + mutex_unlock(&ssif_infos_mutex); return rv; out_remove_attr: -- cgit v1.2.3 From 383035211c79d4d98481a09ad429b31c7dbf22bd Mon Sep 17 00:00:00 2001 From: Tony Camuso Date: Thu, 22 Aug 2019 08:24:53 -0400 Subject: ipmi: move message error checking to avoid deadlock V1->V2: in handle_one_rcv_msg, if data_size > 2, set requeue to zero and goto out instead of calling ipmi_free_msg. Kosuke Tatsukawa In the source stack trace below, function set_need_watch tries to take out the same si_lock that was taken earlier by ipmi_thread. ipmi_thread() [drivers/char/ipmi/ipmi_si_intf.c:995] smi_event_handler() [drivers/char/ipmi/ipmi_si_intf.c:765] handle_transaction_done() [drivers/char/ipmi/ipmi_si_intf.c:555] deliver_recv_msg() [drivers/char/ipmi/ipmi_si_intf.c:283] ipmi_smi_msg_received() [drivers/char/ipmi/ipmi_msghandler.c:4503] intf_err_seq() [drivers/char/ipmi/ipmi_msghandler.c:1149] smi_remove_watch() [drivers/char/ipmi/ipmi_msghandler.c:999] set_need_watch() [drivers/char/ipmi/ipmi_si_intf.c:1066] Upstream commit e1891cffd4c4896a899337a243273f0e23c028df adds code to ipmi_smi_msg_received() to call smi_remove_watch() via intf_err_seq() and this seems to be causing the deadlock. commit e1891cffd4c4896a899337a243273f0e23c028df Author: Corey Minyard Date: Wed Oct 24 15:17:04 2018 -0500 ipmi: Make the smi watcher be disabled immediately when not needed The fix is to put all messages in the queue and move the message checking code out of ipmi_smi_msg_received and into handle_one_recv_msg, which processes the message checking after ipmi_thread releases its locks. Additionally,Kosuke Tatsukawa reported that handle_new_recv_msgs calls ipmi_free_msg when handle_one_rcv_msg returns zero, so that the call to ipmi_free_msg in handle_one_rcv_msg introduced another panic when "ipmitool sensor list" was run in a loop. He submitted this part of the patch. +free_msg: + requeue = 0; + goto out; Reported by: Osamu Samukawa Characterized by: Kosuke Tatsukawa Signed-off-by: Tony Camuso Fixes: e1891cffd4c4 ("ipmi: Make the smi watcher be disabled immediately when not needed") Cc: stable@vger.kernel.org # 5.1 Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_msghandler.c | 114 ++++++++++++++++++------------------ 1 file changed, 57 insertions(+), 57 deletions(-) (limited to 'drivers/char') diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index 3548aceed4a9..2aab80e19ae0 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -4218,7 +4218,53 @@ static int handle_one_recv_msg(struct ipmi_smi *intf, int chan; ipmi_debug_msg("Recv:", msg->rsp, msg->rsp_size); - if (msg->rsp_size < 2) { + + if ((msg->data_size >= 2) + && (msg->data[0] == (IPMI_NETFN_APP_REQUEST << 2)) + && (msg->data[1] == IPMI_SEND_MSG_CMD) + && (msg->user_data == NULL)) { + + if (intf->in_shutdown) + goto free_msg; + + /* + * This is the local response to a command send, start + * the timer for these. The user_data will not be + * NULL if this is a response send, and we will let + * response sends just go through. + */ + + /* + * Check for errors, if we get certain errors (ones + * that mean basically we can try again later), we + * ignore them and start the timer. Otherwise we + * report the error immediately. + */ + if ((msg->rsp_size >= 3) && (msg->rsp[2] != 0) + && (msg->rsp[2] != IPMI_NODE_BUSY_ERR) + && (msg->rsp[2] != IPMI_LOST_ARBITRATION_ERR) + && (msg->rsp[2] != IPMI_BUS_ERR) + && (msg->rsp[2] != IPMI_NAK_ON_WRITE_ERR)) { + int ch = msg->rsp[3] & 0xf; + struct ipmi_channel *chans; + + /* Got an error sending the message, handle it. */ + + chans = READ_ONCE(intf->channel_list)->c; + if ((chans[ch].medium == IPMI_CHANNEL_MEDIUM_8023LAN) + || (chans[ch].medium == IPMI_CHANNEL_MEDIUM_ASYNC)) + ipmi_inc_stat(intf, sent_lan_command_errs); + else + ipmi_inc_stat(intf, sent_ipmb_command_errs); + intf_err_seq(intf, msg->msgid, msg->rsp[2]); + } else + /* The message was sent, start the timer. */ + intf_start_seq_timer(intf, msg->msgid); +free_msg: + requeue = 0; + goto out; + + } else if (msg->rsp_size < 2) { /* Message is too small to be correct. */ dev_warn(intf->si_dev, "BMC returned too small a message for netfn %x cmd %x, got %d bytes\n", @@ -4475,62 +4521,16 @@ void ipmi_smi_msg_received(struct ipmi_smi *intf, unsigned long flags = 0; /* keep us warning-free. */ int run_to_completion = intf->run_to_completion; - if ((msg->data_size >= 2) - && (msg->data[0] == (IPMI_NETFN_APP_REQUEST << 2)) - && (msg->data[1] == IPMI_SEND_MSG_CMD) - && (msg->user_data == NULL)) { - - if (intf->in_shutdown) - goto free_msg; - - /* - * This is the local response to a command send, start - * the timer for these. The user_data will not be - * NULL if this is a response send, and we will let - * response sends just go through. - */ - - /* - * Check for errors, if we get certain errors (ones - * that mean basically we can try again later), we - * ignore them and start the timer. Otherwise we - * report the error immediately. - */ - if ((msg->rsp_size >= 3) && (msg->rsp[2] != 0) - && (msg->rsp[2] != IPMI_NODE_BUSY_ERR) - && (msg->rsp[2] != IPMI_LOST_ARBITRATION_ERR) - && (msg->rsp[2] != IPMI_BUS_ERR) - && (msg->rsp[2] != IPMI_NAK_ON_WRITE_ERR)) { - int ch = msg->rsp[3] & 0xf; - struct ipmi_channel *chans; - - /* Got an error sending the message, handle it. */ - - chans = READ_ONCE(intf->channel_list)->c; - if ((chans[ch].medium == IPMI_CHANNEL_MEDIUM_8023LAN) - || (chans[ch].medium == IPMI_CHANNEL_MEDIUM_ASYNC)) - ipmi_inc_stat(intf, sent_lan_command_errs); - else - ipmi_inc_stat(intf, sent_ipmb_command_errs); - intf_err_seq(intf, msg->msgid, msg->rsp[2]); - } else - /* The message was sent, start the timer. */ - intf_start_seq_timer(intf, msg->msgid); - -free_msg: - ipmi_free_smi_msg(msg); - } else { - /* - * To preserve message order, we keep a queue and deliver from - * a tasklet. - */ - if (!run_to_completion) - spin_lock_irqsave(&intf->waiting_rcv_msgs_lock, flags); - list_add_tail(&msg->link, &intf->waiting_rcv_msgs); - if (!run_to_completion) - spin_unlock_irqrestore(&intf->waiting_rcv_msgs_lock, - flags); - } + /* + * To preserve message order, we keep a queue and deliver from + * a tasklet. + */ + if (!run_to_completion) + spin_lock_irqsave(&intf->waiting_rcv_msgs_lock, flags); + list_add_tail(&msg->link, &intf->waiting_rcv_msgs); + if (!run_to_completion) + spin_unlock_irqrestore(&intf->waiting_rcv_msgs_lock, + flags); if (!run_to_completion) spin_lock_irqsave(&intf->xmit_msgs_lock, flags); -- cgit v1.2.3 From c9acc3c4f8e42ae538aea7f418fddc16f257ba75 Mon Sep 17 00:00:00 2001 From: Jes Sorensen Date: Wed, 28 Aug 2019 16:36:25 -0400 Subject: ipmi_si_intf: Fix race in timer shutdown handling smi_mod_timer() enables the timer before setting timer_running. This means the timer can be running when we get to stop_timer_and_thread() without timer_running having been set, resulting in del_timer_sync() not being called and the timer being left to cause havoc during shutdown. Instead just call del_timer_sync() unconditionally Signed-off-by: Jes Sorensen Message-Id: <20190828203625.32093-2-Jes.Sorensen@gmail.com> Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_si_intf.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'drivers/char') diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c index d58253432a9a..6b9a0593d2eb 100644 --- a/drivers/char/ipmi/ipmi_si_intf.c +++ b/drivers/char/ipmi/ipmi_si_intf.c @@ -1843,8 +1843,7 @@ static inline void stop_timer_and_thread(struct smi_info *smi_info) } smi_info->timer_can_start = false; - if (smi_info->timer_running) - del_timer_sync(&smi_info->si_timer); + del_timer_sync(&smi_info->si_timer); } static struct smi_info *find_dup_si(struct smi_info *info) -- cgit v1.2.3