[PATCH 1/5] ipmi: ssif_bmc: cancel response timer on remove

[PATCH 1/5] ipmi: ssif_bmc: cancel response timer on remove

[Jian Zhang]

The response timer can stay armed across device teardown. If it fires after
remove, the callback dereferences the SSIF context and the i2c client after
teardown has started.

Cancel the timer in remove so the callback cannot run after the device is
unregistered.

Fixes: dd2bc5cc9e25 ("ipmi: ssif_bmc: Add SSIF BMC driver")
Signed-off-by: Jian Zhang <zhangjian.3032@bytedance.com>
---
 drivers/char/ipmi/ssif_bmc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c
index 7a52e3ea49ed..348e29490945 100644
--- a/drivers/char/ipmi/ssif_bmc.c
+++ b/drivers/char/ipmi/ssif_bmc.c
@@ -843,6 +843,7 @@ static void ssif_bmc_remove(struct i2c_client *client)
 {
 	struct ssif_bmc_ctx *ssif_bmc = i2c_get_clientdata(client);
 
+	del_timer_sync(&ssif_bmc->response_timer);
 	i2c_slave_unregister(client);
 	misc_deregister(&ssif_bmc->miscdev);
 }
-- 
2.20.1

[PATCH 2/5] ipmi: ssif_bmc: fix missing check for copy_to_user() partial failure

[Jian Zhang]

copy_to_user() returns the number of bytes that could not be copied,
with a non-zero value indicating a partial or complete failure. The
current code only checks for negative return values and treats all
non-negative results as success.

Treating any positive return value from copy_to_user() as
an error and returning -EFAULT.

Fixes: dd2bc5cc9e25 ("ipmi: ssif_bmc: Add SSIF BMC driver")
Signed-off-by: Jian Zhang <zhangjian.3032@bytedance.com>
---
 drivers/char/ipmi/ssif_bmc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c
index 348e29490945..4037615ace12 100644
--- a/drivers/char/ipmi/ssif_bmc.c
+++ b/drivers/char/ipmi/ssif_bmc.c
@@ -163,6 +163,8 @@ static ssize_t ssif_bmc_read(struct file *file, char __user *buf, size_t count,
 		spin_unlock_irqrestore(&ssif_bmc->lock, flags);
 
 		ret = copy_to_user(buf, &msg, count);
+		if (ret > 0)
+			ret = -EFAULT;
 	}
 
 	return (ret < 0) ? ret : count;
-- 
2.20.1

[PATCH 3/5] ipmi: ssif_bmc: fix message desynchronization after truncated response

[Jian Zhang]

A truncated response, caused by host power-off, or other conditions,
can lead to message desynchronization.

Raw trace data (STOP loss scenario, add state transition comment):

1. T-1: Read response phase (SSIF_RES_SENDING)
8271.955342  WR_RCV [03]                          <- Read polling cmd
8271.955348  RD_REQ [04]  <== SSIF_RES_SENDING    <- start sending response
8271.955436  RD_PRO [b4]
8271.955527  RD_PRO [00]
8271.955618  RD_PRO [c1]
8271.955707  RD_PRO [00]
8271.955814  RD_PRO [ad]  <== SSIF_RES_SENDING     <- last byte
	<- !! STOP lost (truncated response)

2. T: New Write request arrives, BMC still in SSIF_RES_SENDING
8271.967973  WR_REQ []    <== SSIF_RES_SENDING >> SSIF_ABORTING  <- log: unexpected WR_REQ in RES_SENDING
8271.968447  WR_RCV [02]  <== SSIF_ABORTING  <- do nothing
8271.968452  WR_RCV [02]  <== SSIF_ABORTING  <- do nothing
8271.968454  WR_RCV [18]  <== SSIF_ABORTING  <- do nothing
8271.968456  WR_RCV [01]  <== SSIF_ABORTING  <- do nothing
8271.968458  WR_RCV [66]  <== SSIF_ABORTING  <- do nothing
8271.978714  STOP []      <== SSIF_ABORTING >> SSIF_READY  <- log: unexpected SLAVE STOP in state=SSIF_ABORTING

3. T+1: Next Read polling, treated as a fresh transaction
8271.979125  WR_REQ []    <== SSIF_READY >> SSIF_START
8271.979326  WR_RCV [03]  <== SSIF_START >> SSIF_SMBUS_CMD        <- smbus_cmd=0x03
8271.979331  RD_REQ [04]  <== SSIF_RES_SENDING      <- sending response
8271.979427  RD_PRO [b4]                            <- !! this is T's stale response -> desynchronization

When in SSIF_ABORTING state, a newly arrived command should still be
handled to avoid dropping the request or causing message
desynchronization.

Fixes: dd2bc5cc9e25 ("ipmi: ssif_bmc: Add SSIF BMC driver")
Signed-off-by: Jian Zhang <zhangjian.3032@bytedance.com>
---
 drivers/char/ipmi/ssif_bmc.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c
index 4037615ace12..d0543be0af80 100644
--- a/drivers/char/ipmi/ssif_bmc.c
+++ b/drivers/char/ipmi/ssif_bmc.c
@@ -458,6 +458,15 @@ static bool supported_write_cmd(u8 cmd)
 	return false;
 }
 
+static bool supported_write_start_cmd(u8 cmd)
+{
+	if (cmd == SSIF_IPMI_SINGLEPART_WRITE ||
+	    cmd == SSIF_IPMI_MULTIPART_WRITE_START)
+		return true;
+
+	return false;
+}
+
 /* Process the IPMI response that will be read by master */
 static void handle_read_processed(struct ssif_bmc_ctx *ssif_bmc, u8 *val)
 {
@@ -709,6 +718,11 @@ static void on_write_received_event(struct ssif_bmc_ctx *ssif_bmc, u8 *val)
 			ssif_bmc->state = SSIF_ABORTING;
 		else
 			ssif_bmc->state = SSIF_REQ_RECVING;
+	} else if (ssif_bmc->state == SSIF_ABORTING) {
+		if (supported_write_start_cmd(*val)) {
+			ssif_bmc->state = SSIF_SMBUS_CMD;
+			ssif_bmc->aborting = false;
+		}
 	}
 
 	/* This is response sending state */
-- 
2.20.1

[PATCH 4/5] ipmi: ssif_bmc: change log level to dbg in irq callback

[Jian Zhang]

Long-running tests indicate that this logging can occasionally disrupt
timing and lead to request/response corruption.

Irq handler need to be executed as fast as possible,
most I2C slave IRQ implementations are byte-level, logging here
can significantly affect transfer behavior and timing. It is recommended
to use dev_dbg() for these messages.

Fixes: dd2bc5cc9e25 ("ipmi: ssif_bmc: Add SSIF BMC driver")
Signed-off-by: Jian Zhang <zhangjian.3032@bytedance.com>
---
 drivers/char/ipmi/ssif_bmc.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c
index d0543be0af80..691e95a67f94 100644
--- a/drivers/char/ipmi/ssif_bmc.c
+++ b/drivers/char/ipmi/ssif_bmc.c
@@ -569,7 +569,7 @@ static void process_request_part(struct ssif_bmc_ctx *ssif_bmc)
 		len = ssif_bmc->request.len + part->length;
 		/* Do the bound check here, not allow the request len exceed 254 bytes */
 		if (len > IPMI_SSIF_PAYLOAD_MAX) {
-			dev_warn(&ssif_bmc->client->dev,
+			dev_dbg(&ssif_bmc->client->dev,
 				 "Warn: Request exceeded 254 bytes, aborting");
 			/* Request too long, aborting */
 			ssif_bmc->aborting =  true;
@@ -615,7 +615,7 @@ static void on_read_requested_event(struct ssif_bmc_ctx *ssif_bmc, u8 *val)
 	    ssif_bmc->state == SSIF_START ||
 	    ssif_bmc->state == SSIF_REQ_RECVING ||
 	    ssif_bmc->state == SSIF_RES_SENDING) {
-		dev_warn(&ssif_bmc->client->dev,
+		dev_dbg(&ssif_bmc->client->dev,
 			 "Warn: %s unexpected READ REQUESTED in state=%s\n",
 			 __func__, state_to_string(ssif_bmc->state));
 		ssif_bmc->state = SSIF_ABORTING;
@@ -624,7 +624,7 @@ static void on_read_requested_event(struct ssif_bmc_ctx *ssif_bmc, u8 *val)
 
 	} else if (ssif_bmc->state == SSIF_SMBUS_CMD) {
 		if (!supported_read_cmd(ssif_bmc->part_buf.smbus_cmd)) {
-			dev_warn(&ssif_bmc->client->dev, "Warn: Unknown SMBus read command=0x%x",
+			dev_dbg(&ssif_bmc->client->dev, "Warn: Unknown SMBus read command=0x%x",
 				 ssif_bmc->part_buf.smbus_cmd);
 			ssif_bmc->aborting = true;
 		}
@@ -659,7 +659,7 @@ static void on_read_processed_event(struct ssif_bmc_ctx *ssif_bmc, u8 *val)
 	    ssif_bmc->state == SSIF_START ||
 	    ssif_bmc->state == SSIF_REQ_RECVING ||
 	    ssif_bmc->state == SSIF_SMBUS_CMD) {
-		dev_warn(&ssif_bmc->client->dev,
+		dev_dbg(&ssif_bmc->client->dev,
 			 "Warn: %s unexpected READ PROCESSED in state=%s\n",
 			 __func__, state_to_string(ssif_bmc->state));
 		ssif_bmc->state = SSIF_ABORTING;
@@ -684,7 +684,7 @@ static void on_write_requested_event(struct ssif_bmc_ctx *ssif_bmc, u8 *val)
 	} else if (ssif_bmc->state == SSIF_START ||
 		   ssif_bmc->state == SSIF_REQ_RECVING ||
 		   ssif_bmc->state == SSIF_RES_SENDING) {
-		dev_warn(&ssif_bmc->client->dev,
+		dev_dbg(&ssif_bmc->client->dev,
 			 "Warn: %s unexpected WRITE REQUEST in state=%s\n",
 			 __func__, state_to_string(ssif_bmc->state));
 		ssif_bmc->state = SSIF_ABORTING;
@@ -699,7 +699,7 @@ static void on_write_received_event(struct ssif_bmc_ctx *ssif_bmc, u8 *val)
 {
 	if (ssif_bmc->state == SSIF_READY ||
 	    ssif_bmc->state == SSIF_RES_SENDING) {
-		dev_warn(&ssif_bmc->client->dev,
+		dev_dbg(&ssif_bmc->client->dev,
 			 "Warn: %s unexpected WRITE RECEIVED in state=%s\n",
 			 __func__, state_to_string(ssif_bmc->state));
 		ssif_bmc->state = SSIF_ABORTING;
@@ -709,7 +709,7 @@ static void on_write_received_event(struct ssif_bmc_ctx *ssif_bmc, u8 *val)
 
 	} else if (ssif_bmc->state == SSIF_SMBUS_CMD) {
 		if (!supported_write_cmd(ssif_bmc->part_buf.smbus_cmd)) {
-			dev_warn(&ssif_bmc->client->dev, "Warn: Unknown SMBus write command=0x%x",
+			dev_dbg(&ssif_bmc->client->dev, "Warn: Unknown SMBus write command=0x%x",
 				 ssif_bmc->part_buf.smbus_cmd);
 			ssif_bmc->aborting = true;
 		}
@@ -738,7 +738,7 @@ static void on_stop_event(struct ssif_bmc_ctx *ssif_bmc, u8 *val)
 	    ssif_bmc->state == SSIF_START ||
 	    ssif_bmc->state == SSIF_SMBUS_CMD ||
 	    ssif_bmc->state == SSIF_ABORTING) {
-		dev_warn(&ssif_bmc->client->dev,
+		dev_dbg(&ssif_bmc->client->dev,
 			 "Warn: %s unexpected SLAVE STOP in state=%s\n",
 			 __func__, state_to_string(ssif_bmc->state));
 		ssif_bmc->state = SSIF_READY;
@@ -805,7 +805,7 @@ static int ssif_bmc_cb(struct i2c_client *client, enum i2c_slave_event event, u8
 		break;
 
 	default:
-		dev_warn(&ssif_bmc->client->dev, "Warn: Unknown i2c slave event\n");
+		dev_dbg(&ssif_bmc->client->dev, "Warn: Unknown i2c slave event\n");
 		break;
 	}
 
-- 
2.20.1

[PATCH 5/5] ipmi: ssif_bmc: add unit test for state machine

[Jian Zhang]

Add some unit test for state machine when in SSIF_ABORTING state.

Signed-off-by: Jian Zhang <zhangjian.3032@bytedance.com>
---
 drivers/char/ipmi/Kconfig    |  10 +
 drivers/char/ipmi/ssif_bmc.c | 405 +++++++++++++++++++++++++++++++++++
 2 files changed, 415 insertions(+)

diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
index 92bed266d07c..709820f80266 100644
--- a/drivers/char/ipmi/Kconfig
+++ b/drivers/char/ipmi/Kconfig
@@ -187,6 +187,16 @@ config SSIF_IPMI_BMC
 	  The driver implements the BMC side of the SMBus system
 	  interface (SSIF).
 
+config SSIF_IPMI_BMC_KUNIT_TEST
+	bool "KUnit tests for SSIF IPMI BMC driver" if !KUNIT_ALL_TESTS
+	depends on KUNIT=y
+	depends on SSIF_IPMI_BMC=y
+	default KUNIT_ALL_TESTS
+	help
+	  This option builds unit tests that exercise the SSIF BMC state
+	  machine, including request handling, response transmission,
+	  and error paths such as aborted or truncated transfers.
+
 config IPMB_DEVICE_INTERFACE
 	tristate 'IPMB Interface handler'
 	depends on I2C
diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c
index 691e95a67f94..69266c109cb9 100644
--- a/drivers/char/ipmi/ssif_bmc.c
+++ b/drivers/char/ipmi/ssif_bmc.c
@@ -18,6 +18,9 @@
 #include <linux/timer.h>
 #include <linux/jiffies.h>
 #include <linux/ipmi_ssif_bmc.h>
+#if IS_ENABLED(CONFIG_SSIF_IPMI_BMC_KUNIT_TEST)
+#include <kunit/test.h>
+#endif
 
 #define DEVICE_NAME                             "ipmi-ssif-host"
 
@@ -886,6 +889,408 @@ static struct i2c_driver ssif_bmc_driver = {
 	.id_table       = ssif_bmc_id,
 };
 
+#if IS_ENABLED(CONFIG_SSIF_IPMI_BMC_KUNIT_TEST)
+struct ssif_bmc_test_ctx {
+	struct ssif_bmc_ctx ssif_bmc;
+	struct i2c_client client;
+	struct i2c_adapter adapter;
+	struct i2c_algorithm algo;
+};
+
+static int ssif_bmc_test_init(struct kunit *test)
+{
+	struct ssif_bmc_test_ctx *test_ctx;
+
+	test_ctx = kunit_kzalloc(test, sizeof(*test_ctx), GFP_KERNEL);
+	if (!test_ctx)
+		return -ENOMEM;
+
+	test_ctx->adapter.algo = &test_ctx->algo;
+	test_ctx->client.addr = 0x20;
+	test_ctx->client.adapter = &test_ctx->adapter;
+
+	spin_lock_init(&test_ctx->ssif_bmc.lock);
+	init_waitqueue_head(&test_ctx->ssif_bmc.wait_queue);
+	test_ctx->ssif_bmc.client = &test_ctx->client;
+	i2c_set_clientdata(&test_ctx->client, &test_ctx->ssif_bmc);
+
+	test->priv = test_ctx;
+
+	return 0;
+}
+
+static void ssif_bmc_test_exit(struct kunit *test)
+{
+	struct ssif_bmc_test_ctx *test_ctx = test->priv;
+
+	if (test_ctx->ssif_bmc.response_timer_inited)
+		del_timer_sync(&test_ctx->ssif_bmc.response_timer);
+}
+
+static int ssif_bmc_test_run_event_val(struct ssif_bmc_test_ctx *test_ctx,
+					       enum i2c_slave_event event,
+					       u8 *value)
+{
+	return ssif_bmc_cb(&test_ctx->client, event, value);
+}
+
+static int ssif_bmc_test_run_event(struct ssif_bmc_test_ctx *test_ctx,
+					   enum i2c_slave_event event, u8 value)
+{
+	return ssif_bmc_test_run_event_val(test_ctx, event, &value);
+}
+
+static void ssif_bmc_test_singlepart_req(struct kunit *test)
+{
+	struct ssif_bmc_test_ctx *test_ctx = test->priv;
+	struct ssif_bmc_ctx *ssif_bmc = &test_ctx->ssif_bmc;
+
+	ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_REQUESTED,
+				GET_8BIT_ADDR(test_ctx->client.addr));
+	ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_RECEIVED,
+				SSIF_IPMI_SINGLEPART_WRITE);
+	ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_RECEIVED, 2);
+
+	ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_RECEIVED, 0xaa);
+
+	ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_RECEIVED, 0x55);
+	KUNIT_EXPECT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_STOP, 0), -EBUSY);
+
+	KUNIT_EXPECT_EQ(test, ssif_bmc->state, SSIF_READY);
+	KUNIT_EXPECT_TRUE(test, ssif_bmc->request_available);
+	KUNIT_EXPECT_TRUE(test, ssif_bmc->busy);
+	KUNIT_EXPECT_FALSE(test, ssif_bmc->aborting);
+	KUNIT_EXPECT_EQ(test, ssif_bmc->request.len, 2);
+	KUNIT_EXPECT_EQ(test, ssif_bmc->request.payload[0], 0xaa);
+	KUNIT_EXPECT_EQ(test, ssif_bmc->request.payload[1], 0x55);
+	KUNIT_EXPECT_TRUE(test, ssif_bmc->response_timer_inited);
+}
+
+static void ssif_bmc_test_restart_write_without_stop(struct kunit *test)
+{
+	struct ssif_bmc_test_ctx *test_ctx = test->priv;
+	struct ssif_bmc_ctx *ssif_bmc = &test_ctx->ssif_bmc;
+
+	KUNIT_ASSERT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_REQUESTED,
+						GET_8BIT_ADDR(test_ctx->client.addr)), 0);
+	KUNIT_ASSERT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_RECEIVED,
+						SSIF_IPMI_SINGLEPART_WRITE), 0);
+	KUNIT_ASSERT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_RECEIVED, 2), 0);
+	KUNIT_ASSERT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_RECEIVED, 0xde), 0);
+
+	KUNIT_EXPECT_EQ(test, ssif_bmc->state, SSIF_REQ_RECVING);
+
+	/* Write transaction, without stop, and new request coming */
+	ssif_bmc_test_singlepart_req(test);
+}
+
+
+static void ssif_bmc_test_restart_after_invalid_command(struct kunit *test)
+{
+	struct ssif_bmc_test_ctx *test_ctx = test->priv;
+	struct ssif_bmc_ctx *ssif_bmc = &test_ctx->ssif_bmc;
+
+	KUNIT_ASSERT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_REQUESTED,
+						GET_8BIT_ADDR(test_ctx->client.addr)), 0);
+	KUNIT_ASSERT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_RECEIVED, 0xff), 0);
+	KUNIT_ASSERT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_RECEIVED, 1), 0);
+
+	KUNIT_EXPECT_EQ(test, ssif_bmc->state, SSIF_ABORTING);
+	KUNIT_EXPECT_TRUE(test, ssif_bmc->aborting);
+
+	/* After An Invalid Command, expect could handle new request */
+	ssif_bmc_test_singlepart_req(test);
+}
+
+static void ssif_bmc_test_restart_after_invalid_length(struct kunit *test)
+{
+	struct ssif_bmc_test_ctx *test_ctx = test->priv;
+	struct ssif_bmc_ctx *ssif_bmc = &test_ctx->ssif_bmc;
+	int i;
+
+	KUNIT_ASSERT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_REQUESTED,
+						GET_8BIT_ADDR(test_ctx->client.addr)), 0);
+	KUNIT_ASSERT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_RECEIVED,
+						SSIF_IPMI_SINGLEPART_WRITE), 0);
+	KUNIT_ASSERT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_RECEIVED,
+						MAX_PAYLOAD_PER_TRANSACTION + 1), 0);
+
+	for (i = 0; i < MAX_PAYLOAD_PER_TRANSACTION + 1; i++)
+		KUNIT_ASSERT_EQ(test,
+				ssif_bmc_test_run_event(test_ctx,
+							I2C_SLAVE_WRITE_RECEIVED, i), 0);
+
+	KUNIT_EXPECT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_STOP, 0), 0);
+	KUNIT_EXPECT_TRUE(test, ssif_bmc->aborting);
+	KUNIT_EXPECT_FALSE(test, ssif_bmc->request_available);
+	KUNIT_EXPECT_EQ(test, ssif_bmc->state, SSIF_REQ_RECVING);
+
+	ssif_bmc_test_singlepart_req(test);
+}
+
+static void ssif_bmc_test_singlepart_read_response_completion(struct kunit *test)
+{
+	struct ssif_bmc_test_ctx *test_ctx = test->priv;
+	struct ssif_bmc_ctx *ssif_bmc = &test_ctx->ssif_bmc;
+	u8 value;
+
+	ssif_bmc->state = SSIF_SMBUS_CMD;
+	ssif_bmc->part_buf.smbus_cmd = SSIF_IPMI_SINGLEPART_READ;
+	ssif_bmc->response.len = 2;
+	ssif_bmc->response.payload[0] = 0x11;
+	ssif_bmc->response.payload[1] = 0x22;
+	ssif_bmc->response_in_progress = true;
+	ssif_bmc->is_singlepart_read = true;
+	ssif_bmc->pec_support = true;
+
+	value = 0;
+	KUNIT_ASSERT_EQ(test,
+			ssif_bmc_test_run_event_val(test_ctx, I2C_SLAVE_READ_REQUESTED,
+						    &value), 0);
+	KUNIT_EXPECT_EQ(test, value, 2);
+	KUNIT_EXPECT_EQ(test, ssif_bmc->state, SSIF_RES_SENDING);
+	KUNIT_EXPECT_TRUE(test, ssif_bmc->response_in_send);
+
+	value = 0;
+	KUNIT_EXPECT_EQ(test,
+			ssif_bmc_test_run_event_val(test_ctx, I2C_SLAVE_READ_PROCESSED,
+						    &value), 0);
+	KUNIT_EXPECT_EQ(test, value, 0x11);
+
+	value = 0;
+	KUNIT_EXPECT_EQ(test,
+			ssif_bmc_test_run_event_val(test_ctx, I2C_SLAVE_READ_PROCESSED,
+						    &value), 0);
+	KUNIT_EXPECT_EQ(test, value, 0x22);
+
+	value = 0;
+	KUNIT_EXPECT_EQ(test,
+			ssif_bmc_test_run_event_val(test_ctx, I2C_SLAVE_READ_PROCESSED,
+						    &value), 0);
+	KUNIT_EXPECT_EQ(test, value, ssif_bmc->part_buf.pec);
+
+	KUNIT_EXPECT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_STOP, 0), 0);
+	KUNIT_EXPECT_EQ(test, ssif_bmc->state, SSIF_READY);
+	KUNIT_EXPECT_FALSE(test, ssif_bmc->response_in_progress);
+	KUNIT_EXPECT_FALSE(test, ssif_bmc->response_in_send);
+	KUNIT_EXPECT_EQ(test, ssif_bmc->response.len, 0);
+}
+
+static void ssif_bmc_test_stop_during_start_discards_partial_request(struct kunit *test)
+{
+	struct ssif_bmc_test_ctx *test_ctx = test->priv;
+	struct ssif_bmc_ctx *ssif_bmc = &test_ctx->ssif_bmc;
+
+	KUNIT_ASSERT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_REQUESTED,
+						GET_8BIT_ADDR(test_ctx->client.addr)), 0);
+	KUNIT_EXPECT_EQ(test, ssif_bmc->state, SSIF_START);
+
+	KUNIT_EXPECT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_STOP, 0), 0);
+	KUNIT_EXPECT_EQ(test, ssif_bmc->state, SSIF_READY);
+	KUNIT_EXPECT_FALSE(test, ssif_bmc->request_available);
+	KUNIT_EXPECT_EQ(test, ssif_bmc->msg_idx, 0);
+
+	KUNIT_EXPECT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_REQUESTED,
+						GET_8BIT_ADDR(test_ctx->client.addr)), 0);
+	KUNIT_EXPECT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_RECEIVED,
+						SSIF_IPMI_SINGLEPART_WRITE), 0);
+	KUNIT_EXPECT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_RECEIVED, 1), 0);
+	KUNIT_EXPECT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_RECEIVED, 0x77), 0);
+	KUNIT_EXPECT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_STOP, 0), -EBUSY);
+
+	KUNIT_EXPECT_TRUE(test, ssif_bmc->request_available);
+	KUNIT_EXPECT_EQ(test, ssif_bmc->request.len, 1);
+	KUNIT_EXPECT_EQ(test, ssif_bmc->request.payload[0], 0x77);
+}
+
+static void ssif_bmc_test_read_interrupts_partial_write(struct kunit *test)
+{
+	struct ssif_bmc_test_ctx *test_ctx = test->priv;
+	struct ssif_bmc_ctx *ssif_bmc = &test_ctx->ssif_bmc;
+	u8 value = 0xff;
+
+	KUNIT_ASSERT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_REQUESTED,
+						GET_8BIT_ADDR(test_ctx->client.addr)), 0);
+	KUNIT_ASSERT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_RECEIVED,
+						SSIF_IPMI_SINGLEPART_WRITE), 0);
+	KUNIT_ASSERT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_RECEIVED, 2), 0);
+	KUNIT_ASSERT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_RECEIVED, 0xab), 0);
+
+	KUNIT_EXPECT_EQ(test, ssif_bmc->state, SSIF_REQ_RECVING);
+	KUNIT_EXPECT_EQ(test,
+			ssif_bmc_test_run_event_val(test_ctx, I2C_SLAVE_READ_REQUESTED,
+						    &value), 0);
+	KUNIT_EXPECT_EQ(test, value, 0);
+	KUNIT_EXPECT_EQ(test, ssif_bmc->state, SSIF_ABORTING);
+
+	KUNIT_EXPECT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_STOP, 0), 0);
+	KUNIT_EXPECT_EQ(test, ssif_bmc->state, SSIF_READY);
+	KUNIT_EXPECT_FALSE(test, ssif_bmc->request_available);
+	KUNIT_EXPECT_EQ(test, ssif_bmc->request.len, 0);
+
+	KUNIT_EXPECT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_REQUESTED,
+						GET_8BIT_ADDR(test_ctx->client.addr)), 0);
+	KUNIT_EXPECT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_RECEIVED,
+						SSIF_IPMI_SINGLEPART_WRITE), 0);
+	KUNIT_EXPECT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_RECEIVED, 1), 0);
+	KUNIT_EXPECT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_RECEIVED, 0xcd), 0);
+	KUNIT_EXPECT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_STOP, 0), -EBUSY);
+
+	KUNIT_EXPECT_TRUE(test, ssif_bmc->request_available);
+	KUNIT_EXPECT_EQ(test, ssif_bmc->request.len, 1);
+	KUNIT_EXPECT_EQ(test, ssif_bmc->request.payload[0], 0xcd);
+}
+
+static void ssif_bmc_test_write_interrupts_response_send(struct kunit *test)
+{
+	struct ssif_bmc_test_ctx *test_ctx = test->priv;
+	struct ssif_bmc_ctx *ssif_bmc = &test_ctx->ssif_bmc;
+	u8 value = 0;
+
+	ssif_bmc->state = SSIF_SMBUS_CMD;
+	ssif_bmc->part_buf.smbus_cmd = SSIF_IPMI_SINGLEPART_READ;
+	ssif_bmc->response.len = 1;
+	ssif_bmc->response.payload[0] = 0x66;
+	ssif_bmc->response_in_progress = true;
+	ssif_bmc->is_singlepart_read = true;
+
+	KUNIT_ASSERT_EQ(test,
+			ssif_bmc_test_run_event_val(test_ctx, I2C_SLAVE_READ_REQUESTED,
+						    &value), 0);
+	KUNIT_EXPECT_EQ(test, ssif_bmc->state, SSIF_RES_SENDING);
+	KUNIT_EXPECT_TRUE(test, ssif_bmc->response_in_send);
+
+	/* READ_REQUESTED transaction */
+	ssif_bmc_test_singlepart_req(test);
+}
+
+static void ssif_bmc_test_write_interrupts_response_sending(struct kunit *test)
+{
+	struct ssif_bmc_test_ctx *test_ctx = test->priv;
+	struct ssif_bmc_ctx *ssif_bmc = &test_ctx->ssif_bmc;
+	u8 value = 0;
+
+	ssif_bmc->state = SSIF_SMBUS_CMD;
+	ssif_bmc->part_buf.smbus_cmd = SSIF_IPMI_SINGLEPART_READ;
+	ssif_bmc->response.len = 1;
+	ssif_bmc->response.payload[0] = 0x66;
+	ssif_bmc->response_in_progress = true;
+	ssif_bmc->is_singlepart_read = true;
+
+	KUNIT_ASSERT_EQ(test,
+			ssif_bmc_test_run_event_val(test_ctx, I2C_SLAVE_READ_REQUESTED,
+						    &value), 0);
+	KUNIT_EXPECT_EQ(test, ssif_bmc->state, SSIF_RES_SENDING);
+	KUNIT_EXPECT_TRUE(test, ssif_bmc->response_in_send);
+
+	KUNIT_ASSERT_EQ(test,
+			ssif_bmc_test_run_event_val(test_ctx, I2C_SLAVE_READ_PROCESSED,
+						    &value), 0);
+	KUNIT_EXPECT_EQ(test, value, 0x66);
+
+	/* READ_REQUESTED transaction */
+	ssif_bmc_test_singlepart_req(test);
+}
+
+static void ssif_bmc_test_timeout_interrupt_allows_retry(struct kunit *test)
+{
+	struct ssif_bmc_test_ctx *test_ctx = test->priv;
+	struct ssif_bmc_ctx *ssif_bmc = &test_ctx->ssif_bmc;
+
+	KUNIT_ASSERT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_REQUESTED,
+						GET_8BIT_ADDR(test_ctx->client.addr)), 0);
+	KUNIT_ASSERT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_RECEIVED,
+						SSIF_IPMI_SINGLEPART_WRITE), 0);
+	KUNIT_ASSERT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_RECEIVED, 1), 0);
+	KUNIT_ASSERT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_RECEIVED, 0x21), 0);
+	KUNIT_ASSERT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_STOP, 0), -EBUSY);
+
+	KUNIT_ASSERT_TRUE(test, timer_pending(&ssif_bmc->response_timer));
+	del_timer_sync(&ssif_bmc->response_timer);
+	response_timeout(&ssif_bmc->response_timer);
+
+	KUNIT_EXPECT_FALSE(test, ssif_bmc->busy);
+	KUNIT_EXPECT_TRUE(test, ssif_bmc->aborting);
+	KUNIT_EXPECT_FALSE(test, ssif_bmc->response_timer_inited);
+
+	KUNIT_EXPECT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_REQUESTED,
+						GET_8BIT_ADDR(test_ctx->client.addr)), 0);
+	KUNIT_EXPECT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_RECEIVED,
+						SSIF_IPMI_SINGLEPART_WRITE), 0);
+	KUNIT_EXPECT_FALSE(test, ssif_bmc->aborting);
+	KUNIT_EXPECT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_RECEIVED, 1), 0);
+	KUNIT_EXPECT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_WRITE_RECEIVED, 0x22), 0);
+	KUNIT_EXPECT_EQ(test,
+			ssif_bmc_test_run_event(test_ctx, I2C_SLAVE_STOP, 0), -EBUSY);
+
+	KUNIT_EXPECT_TRUE(test, ssif_bmc->request_available);
+	KUNIT_EXPECT_EQ(test, ssif_bmc->request.len, 1);
+	KUNIT_EXPECT_EQ(test, ssif_bmc->request.payload[0], 0x22);
+}
+
+static struct kunit_case ssif_bmc_test_cases[] = {
+	KUNIT_CASE(ssif_bmc_test_singlepart_req),
+	KUNIT_CASE(ssif_bmc_test_restart_write_without_stop),
+	KUNIT_CASE(ssif_bmc_test_restart_after_invalid_command),
+	KUNIT_CASE(ssif_bmc_test_restart_after_invalid_length),
+	KUNIT_CASE(ssif_bmc_test_singlepart_read_response_completion),
+	KUNIT_CASE(ssif_bmc_test_stop_during_start_discards_partial_request),
+	KUNIT_CASE(ssif_bmc_test_read_interrupts_partial_write),
+	KUNIT_CASE(ssif_bmc_test_write_interrupts_response_send),
+	KUNIT_CASE(ssif_bmc_test_write_interrupts_response_sending),
+	KUNIT_CASE(ssif_bmc_test_timeout_interrupt_allows_retry),
+	{}
+};
+
+static struct kunit_suite ssif_bmc_test_suite = {
+	.name = "ssif_bmc_test",
+	.init = ssif_bmc_test_init,
+	.exit = ssif_bmc_test_exit,
+	.test_cases = ssif_bmc_test_cases,
+};
+
+kunit_test_suite(ssif_bmc_test_suite);
+#endif
+
 module_i2c_driver(ssif_bmc_driver);
 
 MODULE_AUTHOR("Quan Nguyen <quan@os.amperecomputing.com>");
-- 
2.20.1

Re: [Openipmi-developer] [PATCH 1/5] ipmi: ssif_bmc: cancel response timer on remove

[Corey Minyard]

On Thu, Apr 02, 2026 at 07:04:31PM +0800, Jian Zhang via Openipmi-developer wrote:
> The response timer can stay armed across device teardown. If it fires after
> remove, the callback dereferences the SSIF context and the i2c client after
> teardown has started.
> 
> Cancel the timer in remove so the callback cannot run after the device is
> unregistered.

This whole series looks good to me, and I have added it to my next tree.
I'd like to hear from Quan, though.

-corey

> 
> Fixes: dd2bc5cc9e25 ("ipmi: ssif_bmc: Add SSIF BMC driver")
> Signed-off-by: Jian Zhang <zhangjian.3032@bytedance.com>
> ---
>  drivers/char/ipmi/ssif_bmc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c
> index 7a52e3ea49ed..348e29490945 100644
> --- a/drivers/char/ipmi/ssif_bmc.c
> +++ b/drivers/char/ipmi/ssif_bmc.c
> @@ -843,6 +843,7 @@ static void ssif_bmc_remove(struct i2c_client *client)
>  {
>  	struct ssif_bmc_ctx *ssif_bmc = i2c_get_clientdata(client);
>  
> +	del_timer_sync(&ssif_bmc->response_timer);
>  	i2c_slave_unregister(client);
>  	misc_deregister(&ssif_bmc->miscdev);
>  }
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> Openipmi-developer mailing list
> Openipmi-developer@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openipmi-developer

Re: [PATCH 3/5] ipmi: ssif_bmc: fix message desynchronization after truncated response

[Quan Nguyen]

On 2/4/26 18:04, Jian Zhang wrote:
> A truncated response, caused by host power-off, or other conditions,
> can lead to message desynchronization.
> 
> Raw trace data (STOP loss scenario, add state transition comment):
> 
> 1. T-1: Read response phase (SSIF_RES_SENDING)
> 8271.955342  WR_RCV [03]                          <- Read polling cmd
> 8271.955348  RD_REQ [04]  <== SSIF_RES_SENDING    <- start sending response
> 8271.955436  RD_PRO [b4]
> 8271.955527  RD_PRO [00]
> 8271.955618  RD_PRO [c1]
> 8271.955707  RD_PRO [00]
> 8271.955814  RD_PRO [ad]  <== SSIF_RES_SENDING     <- last byte
> 	<- !! STOP lost (truncated response)

Honestly, I have a little concern about if there is the case. What I 
think is that if there was no ACK (SCA is not pull low by Controller) on 
the 9th clock pulse while Target sending data to bus. Target will 
release SDA line, eventually, there will be a STOP condition and a 
SLAVE_STOP event should be emitted.

- Quan

Re: [Openipmi-developer] [PATCH 1/5] ipmi: ssif_bmc: cancel response timer on remove

[Quan Nguyen]

On 2/4/26 22:42, Corey Minyard wrote:
> On Thu, Apr 02, 2026 at 07:04:31PM +0800, Jian Zhang via Openipmi-developer wrote:
>> The response timer can stay armed across device teardown. If it fires after
>> remove, the callback dereferences the SSIF context and the i2c client after
>> teardown has started.
>>
>> Cancel the timer in remove so the callback cannot run after the device is
>> unregistered.
> 
> This whole series looks good to me, and I have added it to my next tree.
> I'd like to hear from Quan, though.
> 

The whole series looks good to me too. Just a little concern on the 3/5 
part as we have not yet encountered.

Thanks for the patches
-- Quan

Re: [PATCH 3/5] ipmi: ssif_bmc: fix message desynchronization after truncated response

[Jian Zhang]

> 2026年4月3日 15:22，Quan Nguyen <quan@os.amperecomputing.com> 写道：
> 
> 
> 
> On 2/4/26 18:04, Jian Zhang wrote:
>> A truncated response, caused by host power-off, or other conditions,
>> can lead to message desynchronization.
>> Raw trace data (STOP loss scenario, add state transition comment):
>> 1. T-1: Read response phase (SSIF_RES_SENDING)
>> 8271.955342  WR_RCV [03]                          <- Read polling cmd
>> 8271.955348  RD_REQ [04]  <== SSIF_RES_SENDING    <- start sending response
>> 8271.955436  RD_PRO [b4]
>> 8271.955527  RD_PRO [00]
>> 8271.955618  RD_PRO [c1]
>> 8271.955707  RD_PRO [00]
>> 8271.955814  RD_PRO [ad]  <== SSIF_RES_SENDING     <- last byte
>> <- !! STOP lost (truncated response)
> 
> Honestly, I have a little concern about if there is the case. What I think is that if there was no ACK (SCA is not pull low by Controller) on the 9th clock pulse while Target sending data to bus. Target will release SDA line, eventually, there will be a STOP condition and a SLAVE_STOP event should be emitted.

The concern is valid under normal I2C operation where the controller
continues to drive the clock and issues a NACK, which guarantees a STOP
condition.

However, the scenario described here is different:

- The controller may be powered off or reset abruptly
- SCL may stop toggling before the 9th clock
- As a result, no ACK/NACK phase is completed
- Therefore, no STOP condition is generated on the bus

In such cases, the target state machine may remain in
SSIF_RES_SENDING without receiving a SLAVE_STOP event,
leading to desynchronization.

Additionally, based on my previous experience with MCTP over I2C,
during events like I2C_SLAVE_READ_REQUESTED, I2C_SLAVE_WRITE_REQUESTED, I2C_SLAVE_READ_PROCESSED, and I2C_SLAVE_WRITE_RECEIVED,
various abnormal conditions—such as power loss, firmware bugs, or I2C bus hangs—can interrupt the signaling.

Therefore, we cannot assume that a STOP condition will always be observed.

The current change addresses this by allowing the state machine, when in SSIF_ABORTING, 
to handle a newly detected command request and proceed accordingly.

- Jian

> 
> - Quan