[Openipmi-developer] [PATCH] ipmi: Remove usage of the deprecated ida_simple_xx() API
ida_alloc() and ida_free() should be preferred to the deprecated
ida_simple_get() and ida_simple_remove().
This is less verbose.
Signed-off-by: Christophe JAILLET
---
drivers/char/ipmi/ipmi_msghandler.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_msghandler.c
b/drivers/char/ipmi/ipmi_msghandler.c
index d6f14279684d..b0eedc4595b3 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -3053,7 +3053,7 @@ static void cleanup_bmc_work(struct work_struct *work)
int id = bmc->pdev.id; /* Unregister overwrites id */
platform_device_unregister(>pdev);
- ida_simple_remove(_bmc_ida, id);
+ ida_free(_bmc_ida, id);
}
static void
@@ -3169,7 +3169,7 @@ static int __ipmi_bmc_register(struct ipmi_smi *intf,
bmc->pdev.name = "ipmi_bmc";
- rv = ida_simple_get(_bmc_ida, 0, 0, GFP_KERNEL);
+ rv = ida_alloc(_bmc_ida, GFP_KERNEL);
if (rv < 0) {
kfree(bmc);
goto out;
--
2.34.1
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
[Openipmi-developer] [PATCH] ipmi: ipmb: Fix the MODULE_PARM_DESC associated to 'retry_time_ms'
'This should be 'retry_time_ms' instead of 'max_retries'.
Fixes: 63c4eb347164 ("ipmi:ipmb: Add initial support for IPMI over IPMB")
Signed-off-by: Christophe JAILLET
---
drivers/char/ipmi/ipmi_ipmb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/char/ipmi/ipmi_ipmb.c b/drivers/char/ipmi/ipmi_ipmb.c
index 7c1aee5e11b7..3f1c9f1573e7 100644
--- a/drivers/char/ipmi/ipmi_ipmb.c
+++ b/drivers/char/ipmi/ipmi_ipmb.c
@@ -27,7 +27,7 @@ MODULE_PARM_DESC(bmcaddr, "Address to use for BMC.");
static unsigned int retry_time_ms = 250;
module_param(retry_time_ms, uint, 0644);
-MODULE_PARM_DESC(max_retries, "Timeout time between retries, in
milliseconds.");
+MODULE_PARM_DESC(retry_time_ms, "Timeout time between retries, in
milliseconds.");
static unsigned int max_retries = 1;
module_param(max_retries, uint, 0644);
--
2.34.1
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
[Openipmi-developer] [PATCH] ipmi/watchdog: Include when appropriate
The kstrto() functions have been moved from kernel.h to kstrtox.h. So, in order to eventually remove from , include the latter directly in the appropriate files. Signed-off-by: Christophe JAILLET --- drivers/char/ipmi/ipmi_watchdog.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/char/ipmi/ipmi_watchdog.c b/drivers/char/ipmi/ipmi_watchdog.c index 5b4e677929ca..47365150e431 100644 --- a/drivers/char/ipmi/ipmi_watchdog.c +++ b/drivers/char/ipmi/ipmi_watchdog.c @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include -- 2.34.1 ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
[Openipmi-developer] [PATCH] ipmi: kcs_bmc: Avoid wasting some memory.
KCS_MSG_BUFSIZ is 1000. When using devm_kmalloc(), there is a small memory overhead and, on most systems, this leads to 40 bytes of extra memory allocation. So 1040 bytes are expected to be allocated. The memory allocator works with fixed size hunks of memory. In this case, it will require 2048 bytes of memory because more than 1024 bytes are required. So, when requesting 3 x 1000 bytes, it ends up to 2048 x 3. In order to avoid wasting 3ko of memory, allocate buffers all at once. 3000+40 bytes will be required and 4ko allocated. This still wastes 1ko, but it is already better. Signed-off-by: Christophe JAILLET --- Looking at this code, I wonder why priv->miscdev.name is not freed in kcs_bmc_ipmi_remove_device()? If this make sense, this also mean that KCS_MSG_BUFSIZ can be increased at no cost. Or it could be slightly reduce to around 1024-40-1 bytes to keep the logic which is in place. Another solution would be to use just kmalloc and add a devm_add_action_or_reset() call and a function that frees the memory. If it make sense, KCS_MSG_BUFSIZ could be increased to 1024 and we would allocate just a little above 3x1024 bytes. --- drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 11 +-- 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c index 486834a962c3..15a4a39a6478 100644 --- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c +++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c @@ -485,14 +485,15 @@ static int kcs_bmc_ipmi_add_device(struct kcs_bmc_device *kcs_bmc) priv->client.dev = kcs_bmc; priv->client.ops = _bmc_ipmi_client_ops; - priv->data_in = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL); - priv->data_out = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL); - priv->kbuffer = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL); + /* Allocate buffers all at once */ + priv->data_in = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ * 3, GFP_KERNEL); + priv->data_out = priv->data_in + KCS_MSG_BUFSIZ; + priv->kbuffer = priv->data_in + KCS_MSG_BUFSIZ * 2; priv->miscdev.minor = MISC_DYNAMIC_MINOR; priv->miscdev.name = devm_kasprintf(kcs_bmc->dev, GFP_KERNEL, "%s%u", DEVICE_NAME, kcs_bmc->channel); - if (!priv->data_in || !priv->data_out || !priv->kbuffer || !priv->miscdev.name) + if (!priv->data_in || !priv->miscdev.name) return -EINVAL; priv->miscdev.fops = _bmc_ipmi_fops; @@ -531,8 +532,6 @@ static int kcs_bmc_ipmi_remove_device(struct kcs_bmc_device *kcs_bmc) misc_deregister(>miscdev); kcs_bmc_disable_device(priv->client.dev, >client); - devm_kfree(kcs_bmc->dev, priv->kbuffer); - devm_kfree(kcs_bmc->dev, priv->data_out); devm_kfree(kcs_bmc->dev, priv->data_in); devm_kfree(kcs_bmc->dev, priv); -- 2.34.1 ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v8 1/3] ipmi: ssif_bmc: Add SSIF BMC driver
Le 15/06/2022 à 11:02, Quan Nguyen a écrit :
The SMBus system interface (SSIF) IPMI BMC driver can be used to perform
in-band IPMI communication with their host in management (BMC) side.
Thanks Dan for the copy_from_user() fix in the link below.
Link:
https://lore.kernel.org/linux-arm-kernel/20220310114119.13736-4-quan-shex6MNQR2J/sfdzf78azzkzedxyl...@public.gmane.org/
Signed-off-by: Quan Nguyen
---
Hi,
a few nitpick below
[...]
diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c
new file mode 100644
index ..0bfd4b9bbaf1
--- /dev/null
+++ b/drivers/char/ipmi/ssif_bmc.c
@@ -0,0 +1,880 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * The driver for BMC side of SSIF interface
+ *
+ * Copyright (c) 2022, Ampere Computing LLC
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define DEVICE_NAME "ipmi-ssif-host"
+
+#define GET_8BIT_ADDR(addr_7bit)(((addr_7bit) << 1) & 0xff)
+
+/* A standard SMBus Transaction is limited to 32 data bytes */
+#define MAX_PAYLOAD_PER_TRANSACTION 32
+/* Transaction includes the address, the command, the length and the PEC byte
*/
+#define MAX_TRANSACTION (MAX_PAYLOAD_PER_TRANSACTION +
4)
+
+#define MAX_IPMI_DATA_PER_START_TRANSACTION 30
+#define MAX_IPMI_DATA_PER_MIDDLE_TRANSACTION31
+
+#define SSIF_IPMI_SINGLEPART_WRITE 0x2
+#define SSIF_IPMI_SINGLEPART_READ 0x3
+#define SSIF_IPMI_MULTIPART_WRITE_START 0x6
+#define SSIF_IPMI_MULTIPART_WRITE_MIDDLE0x7
+#define SSIF_IPMI_MULTIPART_WRITE_END 0x8
+#define SSIF_IPMI_MULTIPART_READ_START 0x3
+#define SSIF_IPMI_MULTIPART_READ_MIDDLE 0x9
+
+/*
+ * IPMI 2.0 Spec, section 12.7 SSIF Timing,
+ * Request-to-Response Time is T6max(250ms) - T1max(20ms) - 3ms = 227ms
+ * Recover ssif_bmc from busy state if it takes up to 500ms
+ */
+#define RESPONSE_TIMEOUT500 /* ms */
+
+struct ssif_part_buffer {
+ u8 address;
+ u8 smbus_cmd;
+ u8 length;
+ u8 payload[MAX_PAYLOAD_PER_TRANSACTION];
+ u8 pec;
+ u8 index;
+};
+
+/*
+ * SSIF internal states:
+ * SSIF_READY 0x00 : Ready state
+ * SSIF_START 0x01 : Start smbus transaction
+ * SSIF_SMBUS_CMD 0x02 : Received SMBus command
+ * SSIF_REQ_RECVING 0x03 : Receiving request
+ * SSIF_RES_SENDING 0x04 : Sending response
+ * SSIF_BAD_SMBUS 0x05 : Bad SMbus transaction
If these states are related to the enum just below,
s/SSIF_BAD_SMBUS/SSIF_ABORTING/ + description update?
+ */
+enum ssif_state {
+ SSIF_READY,
+ SSIF_START,
+ SSIF_SMBUS_CMD,
+ SSIF_REQ_RECVING,
+ SSIF_RES_SENDING,
+ SSIF_ABORTING,
+ SSIF_STATE_MAX
+};
+
[...]
+static int ssif_bmc_probe(struct i2c_client *client, const struct
i2c_device_id *id)
+{
+ struct ssif_bmc_ctx *ssif_bmc;
+ int ret;
+
+ ssif_bmc = devm_kzalloc(>dev, sizeof(*ssif_bmc), GFP_KERNEL);
+ if (!ssif_bmc)
+ return -ENOMEM;
+
+ spin_lock_init(_bmc->lock);
+
+ init_waitqueue_head(_bmc->wait_queue);
+ ssif_bmc->request_available = false;
+ ssif_bmc->response_in_progress = false;
+ ssif_bmc->busy = false;
+ ssif_bmc->response_timer_inited = false;
+
+ /* Register misc device interface */
+ ssif_bmc->miscdev.minor = MISC_DYNAMIC_MINOR;
+ ssif_bmc->miscdev.name = DEVICE_NAME;
+ ssif_bmc->miscdev.fops = _bmc_fops;
+ ssif_bmc->miscdev.parent = >dev;
+ ret = misc_register(_bmc->miscdev);
+ if (ret)
+ goto out;
Could be "return ret;"
(see below)
+
+ ssif_bmc->client = client;
+ ssif_bmc->client->flags |= I2C_CLIENT_SLAVE;
+
+ /* Register I2C slave */
+ i2c_set_clientdata(client, ssif_bmc);
+ ret = i2c_slave_register(client, ssif_bmc_cb);
+ if (ret) {
+ misc_deregister(_bmc->miscdev);
+ goto out;
+ }
+
+ return 0;
+out:
+ devm_kfree(>dev, ssif_bmc);
This looks useless to me. The whole error handling path could be
removed, or updated to only have the "misc_deregister()" above.
CJ
+ return ret;
+}
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi: Move remove_work to dedicated workqueue
Le 12/11/2021 à 17:54, Ioanna Alifieraki a écrit :
Currently when removing an ipmi_user the removal is deferred as a work on
the system's workqueue. Although this guarantees the free operation will
occur in non atomic context, it can race with the ipmi_msghandler module
removal (see [1]) . In case a remove_user work is scheduled for removal
and shortly after ipmi_msghandler module is removed we can end up in a
situation where the module is removed fist and when the work is executed
the system crashes with :
BUG: unable to handle page fault for address: c05c3450
PF: supervisor instruction fetch in kernel mode
PF: error_code(0x0010) - not-present page
because the pages of the module are gone. In cleanup_ipmi() there is no
easy way to detect if there are any pending works to flush them before
removing the module. This patch creates a separate workqueue and schedules
the remove_work works on it. When removing the module the workqueue is
flushed to avoid the race.
[1] https://bugs.launchpad.net/bugs/1950666
Cc: sta...@vger.kernel.org
Fixes: 3b9a907223d7 (ipmi: fix sleep-in-atomic in free_user at cleanup SRCU
user->release_barrier)
Signed-off-by: Ioanna Alifieraki
---
drivers/char/ipmi/ipmi_msghandler.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/char/ipmi/ipmi_msghandler.c
b/drivers/char/ipmi/ipmi_msghandler.c
index deed355422f4..9e0ad2ccd3e0 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -191,6 +191,8 @@ struct ipmi_user {
struct work_struct remove_work;
};
+struct workqueue_struct *remove_work_wq;
+
static struct ipmi_user *acquire_ipmi_user(struct ipmi_user *user, int *index)
__acquires(user->release_barrier)
{
@@ -1297,7 +1299,7 @@ static void free_user(struct kref *ref)
struct ipmi_user *user = container_of(ref, struct ipmi_user, refcount);
/* SRCU cleanup must happen in task context. */
- schedule_work(>remove_work);
+ queue_work(remove_work_wq, >remove_work);
}
static void _ipmi_destroy_user(struct ipmi_user *user)
@@ -5383,6 +5385,8 @@ static int ipmi_init_msghandler(void)
atomic_notifier_chain_register(_notifier_list, _block);
+ remove_work_wq = create_singlethread_workqueue("ipmi-msghandler-remove-wq");
+
initialized = true;
out:
@@ -5408,6 +5412,9 @@ static void __exit cleanup_ipmi(void)
int count;
if (initialized) {
+ flush_workqueue(remove_work_wq);
+ destroy_workqueue(remove_work_wq);
+
Hi,
there is no need to call 'flush_workqueue()' before 'destroy_workqueue()'.
'destroy_workqueue()' already drains the queue before destroying it, so
there is no need to flush it explicitly.
Just my 2c.
CJ
atomic_notifier_chain_unregister(_notifier_list,
_block);
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi: kcs_bmc: Fix a memory leak in the error handling path of 'kcs_bmc_serio_add_device()'
> Message du 08/09/21 08:28
> De : "Dan Carpenter"
> A : "Christophe JAILLET"
> Copie à : miny...@acm.org, zwe...@equinix.com, and...@aj.id.au,
> openipmi-developer@lists.sourceforge.net, linux-ker...@vger.kernel.org,
> kernel-janit...@vger.kernel.org
> Objet : Re: [PATCH] ipmi: kcs_bmc: Fix a memory leak in the error handling
> path of 'kcs_bmc_serio_add_device()'
>
> On Tue, Sep 07, 2021 at 11:06:32PM +0200, Christophe JAILLET wrote:
> > In the unlikely event where 'devm_kzalloc()' fails and 'kzalloc()'
> > succeeds, 'port' would be leaking.
> >
> > Test each allocation separately to avoid the leak.
> >
> > Fixes: 3a3d2f6a4c64 ("ipmi: kcs_bmc: Add serio adaptor")
> > Signed-off-by: Christophe JAILLET
> > ---
> > drivers/char/ipmi/kcs_bmc_serio.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/char/ipmi/kcs_bmc_serio.c
> > b/drivers/char/ipmi/kcs_bmc_serio.c
> > index 7948cabde50b..7e2067628a6c 100644
> > --- a/drivers/char/ipmi/kcs_bmc_serio.c
> > +++ b/drivers/char/ipmi/kcs_bmc_serio.c
> > @@ -73,10 +73,12 @@ static int kcs_bmc_serio_add_device(struct
> > kcs_bmc_device *kcs_bmc)
> > struct serio *port;
> >
> > priv = devm_kzalloc(kcs_bmc->dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> >
> > /* Use kzalloc() as the allocation is cleaned up with kfree() via
> > serio_unregister_port() */
>
> The serio_unregister_port() calls serio_destroy_port() which calls
> put_device(>dev). But I wasn't able to track it further than
> that to the actual kfree().
Hi Dan,
Checking this release path was not the goal of this patch.
It was only about the VERR unlikely memory leak.
However my understanding is:
kcs_bmc_serio_add_device
--> serio_register_port
--> __serio_register_port
--> serio_init_port
--> serio->dev.release = serio_release_port
And in serio_release_port:
struct serio *serio = to_serio_port(dev);
kfree(serio);
For me, this 'serio' looks to the one allocated by 'kcs_bmc_serio_add_device'.
I think that the comment is correct.
CJ
>
> Is there a trick to finding ->release() functions?
>
> regards,
> dan carpenter
>
>
>
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
[Openipmi-developer] [PATCH] ipmi: kcs_bmc: Fix a memory leak in the error handling path of 'kcs_bmc_serio_add_device()'
In the unlikely event where 'devm_kzalloc()' fails and 'kzalloc()'
succeeds, 'port' would be leaking.
Test each allocation separately to avoid the leak.
Fixes: 3a3d2f6a4c64 ("ipmi: kcs_bmc: Add serio adaptor")
Signed-off-by: Christophe JAILLET
---
drivers/char/ipmi/kcs_bmc_serio.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/char/ipmi/kcs_bmc_serio.c
b/drivers/char/ipmi/kcs_bmc_serio.c
index 7948cabde50b..7e2067628a6c 100644
--- a/drivers/char/ipmi/kcs_bmc_serio.c
+++ b/drivers/char/ipmi/kcs_bmc_serio.c
@@ -73,10 +73,12 @@ static int kcs_bmc_serio_add_device(struct kcs_bmc_device
*kcs_bmc)
struct serio *port;
priv = devm_kzalloc(kcs_bmc->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
/* Use kzalloc() as the allocation is cleaned up with kfree() via
serio_unregister_port() */
port = kzalloc(sizeof(*port), GFP_KERNEL);
- if (!(priv && port))
+ if (!port)
return -ENOMEM;
port->id.type = SERIO_8042;
--
2.30.2
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
