In error cases a NULL can be passed to memcpy. The length will always
be zero, so it doesn't really matter, but go ahead and check for NULL,
anyway, to be more precise and avoid static analysis errors.
Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Corey Minyard <cminyard@mvista.com>
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 <kamlakantp@marvell.com>
Message-Id: <1566389064-27356-1-git-send-email-kamlakantp@marvell.com>
Signed-off-by: Corey Minyard <cminyard@mvista.com>
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 <cminyard@mvista.com>
If platform_driver_register() fails from init_ipmi_ssif(),
platform_driver_unregister() called unconditionally will
trigger following warning,
ipmi_ssif: Unable to register driver: -12
------------[ cut here ]------------
Unexpected driver unregister!
WARNING: CPU: 1 PID: 6305 at drivers/base/driver.c:193 driver_unregister+0x60/0x70 drivers/base/driver.c:193
Fix it by adding platform_registered variable, only unregister platform
driver when it is already successfully registered.
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
Message-Id: <20190524143724.43218-1-wangkefeng.wang@huawei.com>
Signed-off-by: Corey Minyard <cminyard@mvista.com>
It's just noise, really, lots of systems don't have it.
Reported-by: Kamlakant Patel <kamlakantp@marvell.com>
Signed-off-by: Corey Minyard <cminyard@mvista.com>
According to ipmi spec, block number is a number that is incremented,
starting with 0, for each new block of message data returned using the
middle transaction.
Here, the 'blocknum' is data[0] which always starts from zero(0) and
'ssif_info->multi_pos' starts from 1.
So, we need to add +1 to blocknum while comparing with multi_pos.
Fixes: 7d6380cd40 ("ipmi:ssif: Fix handling of multi-part return messages").
Reported-by: Kiran Kolukuluru <kirank@ami.com>
Signed-off-by: Kamlakant Patel <kamlakantp@marvell.com>
Message-Id: <1556106615-18722-1-git-send-email-kamlakantp@marvell.com>
[Also added a debug log if the block numbers don't match.]
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: stable@vger.kernel.org # 4.4
Trivial fix to clean up an indentation issue, remove space
Signed-off-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: Corey Minyard <cminyard@mvista.com>
The code to tell the lower layer to enable or disable watching for
certain things was lazy in disabling, it waited until a timer tick
to see if a disable was necessary. Not a really big deal, but it
could be improved.
Modify the code to enable and disable watching immediately and don't
do it from the background timer any more.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Tested-by: Kamlakant Patel <kamlakant.patel@cavium.com>
The IPMI driver has a mechanism to tell the lower layers it needs
to watch for messages, commands, and watchdogs (so it doesn't
needlessly poll). However, it needed some extensions, it needed
a way to tell what is being waited for so it could set the timeout
appropriately.
The update to the lower layer was also being done once a second
at best because it was done in the main timeout handler. However,
if a command is sent and a response message is coming back,
it needed to be started immediately. So modify the code to
update immediately if it needs to be enabled. Disable is still
lazy.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Tested-by: Kamlakant Patel <kamlakant.patel@cavium.com>
Commit 89986496de ("ipmi: Turn off all activity on an idle ipmi
interface") modified the IPMI code to only request events when the
driver had somethine waiting for events. The SSIF code, however,
was using the event fetch request to also fetch the flags.
Add a timer and the proper handling for the upper layer telling
whether flags fetches are required.
Reported-by: Kamlakant Patel <Kamlakant.Patel@cavium.com>
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Tested-by: Kamlakant Patel <kamlakant.patel@cavium.com>
The block number was not being compared right, it was off by one
when checking the response.
Some statistics wouldn't be incremented properly in some cases.
Check to see if that middle-part messages always have 31 bytes of
data.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: stable@vger.kernel.org # 4.4
The spec was fairly confusing about how multi-part transmit messages
worked, so the original implementation only added support for two
part messages. But after talking about it with others and finding
something I missed, I think it makes more sense.
The spec mentions smbus command 8 in a table at the end of the
section on SSIF support as the end transaction. If that works,
then all is good and as it should be. However, some implementations
seem to use a middle transaction <32 bytes tomark the end because of the
confusion in the spec, even though that is an SMBus violation if
the number of bytes is zero.
So this change adds some tests, if command=8 works, it uses that,
otherwise if an empty end transaction works, it uses a middle
transaction <32 bytes to mark the end. If neither works, then
it limits the size to 63 bytes as it is now.
Cc: Harri Hakkarainen <harri@cavium.com>
Cc: Bazhenov, Dmitry <dmitry.bazhenov@auriga.com>
Cc: Mach, Dat <Dat.Mach@cavium.com>
Signed-off-by: Corey Minyard <cminyard@mvista.com>
The IPMI DMI code was adding platform overrides, which is not
really an ideal solution. Switch to using the id_table in
the drivers to identify the devices.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Add and use #define pr_fmt/dev_fmt, and remove #define PFX
This also prefixes some messages that were not previously prefixed.
Miscellanea:
o Convert printk(KERN_<level> to pr_<level>(
o Use %s, __func__ where appropriate
Signed-off-by: Joe Perches <joe@perches.com>
Signed-off-by: Corey Minyard <cminyard@mvista.com>
There is a potential execution path in which function ssif_info_find()
returns NULL, hence there is a NULL pointer dereference when accessing
pointer *addr_info*
Fix this by null checking *addr_info* before dereferencing it.
Addresses-Coverity-ID: 1473145 ("Explicit null dereferenced")
Fixes: e333054a91d1 ("ipmi: Fix I2C client removal in the SSIF driver")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
Signed-off-by: Corey Minyard <cminyard@mvista.com>
There were certain situations where ipmi_register_smi() would
return a failure, but the interface would still be registered
and would need to be unregistered. This is obviously a bad
design and resulted in an oops in certain failure cases.
If the interface is started up in ipmi_register_smi(), then
an error occurs, shut down the interface there so the
cleanup can be done properly.
Fix the various smi users, too.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Reported-by: Justin Ernst <justin.ernst@hpe.com>
Tested-by: Justin Ernst <justin.ernst@hpe.com>
Cc: Andrew Banman <abanman@hpe.com>
Cc: Russ Anderson <russ.anderson@hpe.com>
Cc: <stable@vger.kernel.org> # 4.18.x
Currently, function ssif_remove returns _rv_, which is a variable that
is never initialized.
Fix this by removing variable _rv_ and return 0 instead.
Addresses-Coverity-ID: 1467999 ("Uninitialized scalar variable")
Fixes: 6a0d23ed33 ("ipmi: ipmi_unregister_smi() cannot fail, have it
return void")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Due to changes in the way shutdown is done, it is no longer
required to check that the interface is set.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Move the shutdown handling to a shutdown function called from
the IPMI core code. That makes for a cleaner shutdown.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
This happens when BMC doesn't return any data and the code is trying
to print the value of data[2].
Getting following crash:
[ 484.728410] Unable to handle kernel NULL pointer dereference at virtual address 00000002
[ 484.736496] pgd = ffff0000094a2000
[ 484.739885] [00000002] *pgd=00000047fcffe003, *pud=00000047fcffd003, *pmd=0000000000000000
[ 484.748158] Internal error: Oops: 96000005 [#1] SMP
[...]
[ 485.101451] Call trace:
[...]
[ 485.188473] [<ffff000000a46e68>] msg_done_handler+0x668/0x700 [ipmi_ssif]
[ 485.195249] [<ffff000000a456b8>] ipmi_ssif_thread+0x110/0x128 [ipmi_ssif]
[ 485.202038] [<ffff0000080f1430>] kthread+0x108/0x138
[ 485.206994] [<ffff0000080838e0>] ret_from_fork+0x10/0x30
[ 485.212294] Code: aa1903e1 aa1803e0 b900227f 95fef6a5 (39400aa3)
Adding a check to validate the data len before printing data[2] to fix this issue.
Signed-off-by: Kamlakant Patel <kamlakant.patel@cavium.com>
Signed-off-by: Corey Minyard <cminyard@mvista.com>
The IPMI spec states:
The purpose of the SPMI Table is to provide a mechanism that can
be used by the OSPM (an ACPI term for “OS Operating System-directed
configuration and Power Management” essentially meaning an ACPI-aware
OS or OS loader) very early in the boot process, e.g., before the
ability to execute ACPI control methods in the OS is available.
When we are probing IPMI in Linux, ACPI control methods are available,
so we shouldn't be probing using SPMI. It could cause some confusion
during the probing process.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Tested-by: Jiandi An <anjiandi@codeaurora.org>
And get rid of the license text that is no longer necessary.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Alistair Popple <alistair@popple.id.au>
Cc: Jeremy Kerr <jk@ozlabs.org>
Cc: Joel Stanley <joel@jms.id.au>
Cc: Rocky Craig <rocky.craig@hp.com>
Since i2c_unregister_device() became NULL-aware we may remove duplicate
NULL check.
Cc: Corey Minyard <minyard@acm.org>
Cc: openipmi-developer@lists.sourceforge.net
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Rework the DMI probe function to be a generic platform probe, and
then rework the DMI code (and a few other things) to use the more
generic information. This is so other things can declare platform
IPMI devices.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Create a device attribute for everything we show in proc, getting
ready for removing the proc stuff.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Currently, ipmi_demagle_device_id requires a full response buffer in its
data argument. This means we can't use it to parse a response in a
struct ipmi_recv_msg, which has the netfn and cmd as separate bytes.
This change alters the definition and users of ipmi_demangle_device_id
to use a split netfn, cmd and data buffer, so it can be used with
non-sequential responses.
Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Fixed the ipmi_ssif.c and ipmi_si_intf.c changes to use data from the
response, not the data from the message, when passing info to the
ipmi_demangle_device_id() function.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
When getting flags, a response to a different message would
result in a deadlock because of a missing unlock. Add that
unlock and a comment. Found by static analysis.
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Cc: stable@vger.kernel.org # 3.19
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Now that the IPMI DMI code creates a platform device for IPMI devices
in the firmware, use that instead of handling all the DMI work
in the IPMI drivers themselves.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Andy Lutomirski <luto@kernel.org>
The null check on client->adapter->name is redundant as name is an
array of I2C_NAME_SIZE chars and hence can never be null. We may as
well remove this redundant check.
Detected by CoverityScan, CID#1375918 ("Array compared against 0")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: Corey Minyard <cminyard@mvista.com>
We should unlock and re-enable IRQs if this allocation fails.
Fixes: 259307074b ("ipmi: Add SMBus interface driver (SSIF) ")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Use setup_timer() instead of init_timer() to simplify the code.
Signed-off-by: Geliang Tang <geliangtang@gmail.com>
Signed-off-by: Corey Minyard <cminyard@mvista.com>
msg_written_handler() may set ssif_info->multi_data to NULL
when using ipmitool to write fru.
Before setting ssif_info->multi_data to NULL, add new local
pointer "data_to_send" and store correct i2c data pointer to
it to fix NULL pointer kernel panic and incorrect ssif_info->multi_pos.
Signed-off-by: Joeseph Chang <joechang@codeaurora.org>
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: stable@vger.kernel.org # 3.19-
When added by ACPI, the information does not contain the slave address
of the BMC. However, that information is available from SMBIOS. So
if we add a device that doesn't have a slave address, look at the other
devices that are duplicate interfaces and see if they have a slave
address.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Corey Minyard