On 17/06/2022 03:47, Christophe JAILLET wrote:
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/SfDzf78azzKzEDxYleXD@xxxxxxxxxxxxxxxx/
Signed-off-by: Quan Nguyen
<quan-shex6MNQR2J/SfDzf78azzKzEDxYleXD@xxxxxxxxxxxxxxxx>
---
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 000000000000..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 <linux/i2c.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/poll.h>
+#include <linux/sched.h>
+#include <linux/mutex.h>
+#include <linux/spinlock.h>
+#include <linux/timer.h>
+#include <linux/jiffies.h>
+#include <linux/ipmi_ssif_bmc.h>
+
+#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_TRANSACTION 31
+
+#define SSIF_IPMI_SINGLEPART_WRITE 0x2
+#define SSIF_IPMI_SINGLEPART_READ 0x3
+#define SSIF_IPMI_MULTIPART_WRITE_START 0x6
+#define SSIF_IPMI_MULTIPART_WRITE_MIDDLE 0x7
+#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_TIMEOUT 500 /* 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?
Thank you for this catch.
Will fix in new version.
+ */
+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(&client->dev, sizeof(*ssif_bmc),
GFP_KERNEL);
+ if (!ssif_bmc)
+ return -ENOMEM;
+
+ spin_lock_init(&ssif_bmc->lock);
+
+ init_waitqueue_head(&ssif_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 = &ssif_bmc_fops;
+ ssif_bmc->miscdev.parent = &client->dev;
+ ret = misc_register(&ssif_bmc->miscdev);
+ if (ret)
+ goto out;
Could be "return ret;"
(see below)
Will change to "return ret;" in next version
+
+ 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(&ssif_bmc->miscdev);
+ goto out;
+ }
+
+ return 0;
+out:
+ devm_kfree(&client->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.
Will rewrite as:
/* Register I2C slave */
i2c_set_clientdata(client, ssif_bmc);
ret = i2c_slave_register(client, ssif_bmc_cb);
if (ret)
misc_deregister(&ssif_bmc->miscdev);
return ret;
Thanks for the review.
- Quan
