[PATCH v2 0/4] i2c: SMBus ARP support

[PATCH v2 0/4] i2c: SMBus ARP support

[Heikki Krogerus]

Hi,

Changed in v2:

- Using kzalloc instead of kzalloc_obj. kzalloc_obj() is clearly not ready yet
  (scripts/checkpatch.pl should not promote it yet).

Original cover letter:

I know there has been a couple of PoCs for ARP over the years, but for
what ever reason none of them were ever made part of the kernel. I
don't know how common ARP-capable devices are, but since they do
exist, we really should enumerate them as intended. I'm guessing that
in Linux ARP-capable devices have so far been left mostly undetected.

The problem I'm trying to tackle with these is to detect SMBus devices
on discrete components like PCIe cards. We obviously won't have any
kind ACPI or DT description for those, so we would always have to
create the device for them in the drivers. Unfortunately in some cases
it's actually very difficult to know what exactly is attached to the
I2C on those cards because the vendors can put what ever they like
there - this is the case at least with some of the GPUs it seems. But
luckily those I2C/SMBus devices are at least in some cases fully
ARP-capable.

I'm including a patch that binds the detected ARP-capable MCTP devices
to the driver. There is also a target mode test driver.

thanks,


Heikki Krogerus (4):
  i2c: SMBus Address Resolution Protocol implementation for host side
  i2c: Sysfs attribute files for the Unique Device Identifier fields
  mctp i2c: Enable SMBus ARP discovery
  i2c: Add SMBus ARP target mode test driver

 Documentation/ABI/testing/sysfs-bus-i2c |  53 ++++
 drivers/i2c/Kconfig                     |   6 +
 drivers/i2c/Makefile                    |   3 +-
 drivers/i2c/i2c-core-arp.c              | 334 ++++++++++++++++++++++++
 drivers/i2c/i2c-core-base.c             | 154 ++++++++++-
 drivers/i2c/i2c-core.h                  |   8 +
 drivers/i2c/i2c-target-arp.c            | 201 ++++++++++++++
 drivers/net/mctp/mctp-i2c.c             |   8 +
 include/linux/i2c-smbus.h               |  67 +++++
 include/linux/i2c.h                     |  10 +
 include/linux/mod_devicetable.h         |  13 +
 scripts/mod/devicetable-offsets.c       |   8 +
 scripts/mod/file2alias.c                |  24 ++
 13 files changed, 878 insertions(+), 11 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c
 create mode 100644 drivers/i2c/i2c-core-arp.c
 create mode 100644 drivers/i2c/i2c-target-arp.c

-- 
2.50.1

[PATCH v2 1/4] i2c: SMBus Address Resolution Protocol implementation for host side

[Heikki Krogerus]

SMBus Address Resolution Protocol (ARP) is a standard (but
optional) method to detect and identify devices on SMBus.
The ARP is higher level message protocol that can be fully
emulated. ARP will therefore always be supported, and the
ARP-capable devices will be scanned automatically for every
adapter that is registered.

The ARP devices are identified with the Unique Device
Identifier (UDID). The UDID can be used for device matching
in drivers with a new struct smbus_device_id.

The function i2c_handle_smbus_host_notify() is also modified
so that the hotplugged ARP devices that send Notify ARP
Controller command can be detected.

Link: https://www.smbus.org/specs/smbus20.pdf
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/i2c/Makefile              |   2 +-
 drivers/i2c/i2c-core-arp.c        | 334 ++++++++++++++++++++++++++++++
 drivers/i2c/i2c-core-base.c       |  47 ++++-
 drivers/i2c/i2c-core.h            |   8 +
 include/linux/i2c-smbus.h         |  67 ++++++
 include/linux/i2c.h               |  10 +
 include/linux/mod_devicetable.h   |  13 ++
 scripts/mod/devicetable-offsets.c |   8 +
 scripts/mod/file2alias.c          |  24 +++
 9 files changed, 503 insertions(+), 10 deletions(-)
 create mode 100644 drivers/i2c/i2c-core-arp.c

diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
index d27de18de46f..4fdb37b94aa4 100644
--- a/drivers/i2c/Makefile
+++ b/drivers/i2c/Makefile
@@ -5,7 +5,7 @@
 
 obj-$(CONFIG_I2C_BOARDINFO)	+= i2c-boardinfo.o
 obj-$(CONFIG_I2C)		+= i2c-core.o
-i2c-core-y			:= i2c-core-base.o i2c-core-smbus.o
+i2c-core-y			:= i2c-core-base.o i2c-core-smbus.o i2c-core-arp.o
 i2c-core-$(CONFIG_ACPI)		+= i2c-core-acpi.o
 i2c-core-$(CONFIG_I2C_SLAVE)	+= i2c-core-slave.o
 i2c-core-$(CONFIG_OF)		+= i2c-core-of.o
diff --git a/drivers/i2c/i2c-core-arp.c b/drivers/i2c/i2c-core-arp.c
new file mode 100644
index 000000000000..d9ea033c97fc
--- /dev/null
+++ b/drivers/i2c/i2c-core-arp.c
@@ -0,0 +1,334 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * SMBus Address Resolution Protocol
+ *
+ * Copyright (C) 2026 Intel Corporation
+ */
+
+#include <linux/bits.h>
+#include <linux/bitfield.h>
+#include <linux/bitmap.h>
+#include <linux/dev_printk.h>
+#include <linux/device/devres.h>
+#include <linux/idr.h>
+#include <linux/i2c.h>
+#include <linux/i2c-smbus.h>
+#include <linux/list.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/sprintf.h>
+#include <linux/string.h>
+#include <linux/types.h>
+#include <linux/workqueue.h>
+
+#include "i2c-core.h"
+
+#define I2C_SMBUS_DEFAULT_ADDR	0x61
+
+#define ARP_CMD_PREPARE_TO_ARP	0x01
+#define ARP_CMD_RESET_DEVICE	0x02
+#define ARP_CMD_GET_UDID	0x03
+#define ARP_CMD_ASSIGN_ADDRESS	0x04
+
+struct get_udid_data {
+	struct i2c_arp_udid	udid;
+	u8			target_address;
+} __packed;
+
+struct arp_work {
+	struct work_struct	work;
+	struct i2c_adapter	*adapter;
+	u8			target;
+};
+
+struct i2c_smbus_arp {
+	struct list_head	clients;
+	struct mutex		lock; /* ARP controller lock */
+};
+
+/* SMBus Specification Appendix C. */
+static const DECLARE_BITMAP(reserved_addrs, (1 << 7)) = {
+	[BIT_WORD(0)] = GENMASK(12, 0) |	/* SMBus v1.1 */
+			BIT_MASK(0x28) |	/* PMBus ZONE READ */
+			BIT_MASK(0x2c) |	/* Reserved */
+			BIT_MASK(0x2d) |	/* Reserved */
+			BIT_MASK(0x55),		/* PMBus ZONE WRITE */
+
+	[BIT_WORD(64)] = BIT_MASK(0x40) |	/* Reserved */
+			 BIT_MASK(0x41) |	/* Reserved */
+			 BIT_MASK(0x42) |	/* Reserved */
+			 BIT_MASK(0x43) |	/* Reserved */
+			 BIT_MASK(0x44) |	/* Reserved */
+			 BIT_MASK(0x48) |	/* Prototype Address */
+			 BIT_MASK(0x49) |	/* Prototype Address */
+			 BIT_MASK(0x4a) |	/* Prototype Address */
+			 BIT_MASK(0x4b) |	/* Prototype Address */
+			 BIT_MASK(0x61) |	/* SMBus Default Address */
+			 BIT_MASK(0x78) |	/* 10-bit target addressing */
+			 BIT_MASK(0x79) |	/* 10-bit target addressing */
+			 BIT_MASK(0x7a) |	/* 10-bit target addressing */
+			 BIT_MASK(0x7b) |	/* 10-bit target addressing */
+			 BIT_MASK(0x7c) |	/* Reserved */
+			 BIT_MASK(0x7d) |	/* Reserved */
+			 BIT_MASK(0x7e) |	/* Reserved */
+			 BIT_MASK(0x7f)		/* Reserved */
+};
+
+const struct smbus_device_id *i2c_smbus_match_id(const struct i2c_client *client,
+						 const struct smbus_device_id *id)
+{
+	struct i2c_arp_udid *udid;
+
+	if (!(id && client && client->udid))
+		return NULL;
+
+	udid = client->udid;
+
+	while (id->vendor) {
+		if ((id->vendor == SMBUS_ANY_ID || id->vendor == udid->vendor) &&
+		    (id->device == SMBUS_ANY_ID || id->device == udid->device) &&
+		    /* The interface is split into "version" and "protocol". */
+		    (id->interface == SMBUS_ANY_ID ||
+		      /* minimum version must be supported */
+		     (((id->interface & 0xf) <= (udid->interface & 0xf)) &&
+		      /* the protocol must match */
+		      ((id->interface & 0xf0) == (udid->interface & 0xf0)))) &&
+		    (id->subvendor == SMBUS_ANY_ID || id->subvendor == udid->subvendor) &&
+		    (id->subdevice == SMBUS_ANY_ID || id->subdevice == udid->subdevice) &&
+		    (id->vendor_specific_id == SMBUS_ANY_VENDOR_SPECIFIC_ID ||
+		     id->vendor_specific_id == udid->vendor_specific_id))
+			return id;
+		id++;
+	}
+
+	return NULL;
+}
+
+static int i2c_smbus_arp_get_udid(struct i2c_adapter *adapter, u8 target,
+				  struct get_udid_data *get_udid)
+{
+	union i2c_smbus_data data;
+	u8 command;
+	int ret;
+
+	if (target)
+		command = (target << 1) | 1;
+	else
+		command = ARP_CMD_GET_UDID;
+
+	ret = i2c_smbus_xfer(adapter, I2C_SMBUS_DEFAULT_ADDR,
+			     I2C_CLIENT_PEC, I2C_SMBUS_READ,
+			     command, I2C_SMBUS_BLOCK_DATA, &data);
+	if (ret)
+		return ret;
+
+	memcpy(get_udid, &data.block[1], sizeof(*get_udid));
+
+	return 0;
+}
+
+static int i2c_smbus_arp_verify_address(struct i2c_adapter *adapter,
+					struct get_udid_data *get_udid)
+{
+	u8 addr_type = FIELD_GET(ARP_CAP_ADDRESS_TYPE_MASK,
+				 get_udid->udid.capabilities);
+	u8 addr = get_udid->target_address;
+
+	if (addr_type == ARP_CAP_ADDRESS_TYPE_FIXED)
+		return 0;
+
+	/* Find a free address if necessary. */
+	if (addr == 0xff || i2c_check_addr_busy(adapter, addr)) {
+		for_each_clear_bit(addr, reserved_addrs, 128)
+			if (!i2c_check_addr_busy(adapter, addr))
+				break;
+		if (addr == 128)
+			return -EBUSY;
+
+		get_udid->target_address = addr;
+	}
+
+	return 0;
+}
+
+static int i2c_smbus_arp_assign_address(struct i2c_adapter *adapter,
+					struct get_udid_data *get_udid)
+{
+	union i2c_smbus_data data;
+	int ret;
+
+	ret = i2c_smbus_arp_verify_address(adapter, get_udid);
+	if (ret)
+		return ret;
+
+	data.block[0] = sizeof(*get_udid);
+	memcpy(&data.block[1], get_udid, data.block[0]);
+
+	ret = i2c_smbus_xfer(adapter, I2C_SMBUS_DEFAULT_ADDR,
+			     I2C_CLIENT_PEC, I2C_SMBUS_WRITE,
+			     ARP_CMD_ASSIGN_ADDRESS, I2C_SMBUS_BLOCK_DATA, &data);
+	if (ret)
+		return -EAGAIN;
+
+	return 0;
+}
+
+static void i2c_smbus_arp_remove_client(void *udid)
+{
+	kfree(udid);
+}
+
+static int i2c_smbus_arp_new_client(struct i2c_adapter *adapter,
+				    struct get_udid_data *data)
+{
+	struct i2c_board_info info = { };
+	struct i2c_client *client;
+	int ret;
+
+	info.udid = kmemdup(&data->udid, sizeof(data->udid), GFP_KERNEL);
+	if (!info.udid)
+		return -ENOMEM;
+
+	info.addr = data->target_address;
+	info.flags = I2C_CLIENT_HOST_NOTIFY;
+
+	if (data->udid.capabilities & ARP_CAP_PEC_SUPPORTED)
+		info.flags |= I2C_CLIENT_PEC;
+
+	sprintf(info.type, "%d:arp-%zu", i2c_adapter_id(adapter),
+		list_count_nodes(&adapter->arp->clients));
+	info.dev_name = info.type;
+
+	client = i2c_new_client_device(adapter, &info);
+	if (IS_ERR(client)) {
+		kfree(info.udid);
+		return PTR_ERR(client);
+	}
+
+	ret = devm_add_action_or_reset(&client->dev, i2c_smbus_arp_remove_client, info.udid);
+	if (ret)
+		return ret;
+
+	list_add_tail(&client->detected, &adapter->arp->clients);
+
+	return 0;
+}
+
+static void i2c_smbus_arp_work(struct work_struct *work)
+{
+	struct arp_work *awork = container_of(work, struct arp_work, work);
+	struct i2c_adapter *adapter = awork->adapter;
+	u8 target = awork->target;
+	struct get_udid_data data;
+	int ret;
+
+	mutex_lock(&adapter->arp->lock);
+
+	do {
+		if (i2c_smbus_arp_get_udid(adapter, target, &data))
+			break;
+
+		ret = i2c_smbus_arp_assign_address(adapter, &data);
+		if (ret == -EAGAIN)
+			continue;
+		if (ret < 0) {
+			dev_warn(&adapter->dev, "out of addresses\n");
+			break;
+		}
+
+		if (i2c_smbus_arp_new_client(adapter, &data))
+			break;
+	} while (!target);
+
+	mutex_unlock(&adapter->arp->lock);
+	kfree(awork);
+}
+
+/**
+ * i2c_smbus_arp_detect - Schedule detection and registration of ARP devices
+ * @adapter: ARP Controller
+ * @target_address: Address for directed ARP commands
+ *
+ * Registers ARP-capable devices attached to @adapter. If @target_address is
+ * supplied, directed Get UDID command will be used. Otherwise, if
+ * @target_address is 0, the general Get UDID command is used until there are no
+ * more responses.
+ *
+ * Returns 0 on success or errno.
+ */
+int i2c_smbus_arp_detect(struct i2c_adapter *adapter, u8 target_address)
+{
+	struct arp_work *awork;
+
+	if (!adapter->arp)
+		return -ENXIO;
+
+	awork = kzalloc(sizeof(*awork), GFP_KERNEL);
+	if (!awork)
+		return -ENOMEM;
+
+	INIT_WORK(&awork->work, i2c_smbus_arp_work);
+	awork->target = target_address;
+	awork->adapter = adapter;
+
+	queue_work(system_long_wq, &awork->work);
+
+	return 0;
+}
+
+/**
+ * i2c_smbus_arp_probe - Declare adapter as ARP controller
+ * @adapter: ARP Controller
+ *
+ * Declare @adapter as the ARP controller with the Prepare to ARP command, and
+ * then detect all available ARP devices with the general Get UDID command.
+ *
+ * Returns 0 on success or errno.
+ */
+int i2c_smbus_arp_probe(struct i2c_adapter *adapter)
+{
+	int ret;
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE | I2C_FUNC_SMBUS_BLOCK_DATA))
+		return 0;
+
+	adapter->arp = devm_kzalloc(&adapter->dev, sizeof(*adapter->arp), GFP_KERNEL);
+	if (!adapter->arp)
+		return -ENOMEM;
+
+	mutex_init(&adapter->arp->lock);
+	INIT_LIST_HEAD(&adapter->arp->clients);
+
+	/* Broadcast "Prepare to ARP" command. */
+	ret = i2c_smbus_xfer(adapter, I2C_SMBUS_DEFAULT_ADDR, I2C_CLIENT_PEC,
+			     I2C_SMBUS_WRITE, ARP_CMD_PREPARE_TO_ARP,
+			     I2C_SMBUS_BYTE, NULL);
+	if (ret)
+		return 0;
+
+	return i2c_smbus_arp_detect(adapter, 0);
+}
+
+/**
+ * i2c_smbus_arp_remove - Unregister all ARP devices
+ * @adapter: ARP Controller
+ *
+ * Unregister all ARP devices attached to @adapter.
+ */
+void i2c_smbus_arp_remove(struct i2c_adapter *adapter)
+{
+	struct i2c_client *client, *next;
+
+	if (!adapter->arp)
+		return;
+
+	mutex_lock(&adapter->arp->lock);
+
+	list_for_each_entry_safe(client, next, &adapter->arp->clients, detected) {
+		list_del(&client->detected);
+		i2c_unregister_device(client);
+	}
+
+	mutex_unlock(&adapter->arp->lock);
+}
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index f0fb0cfd56e0..6e96ac186921 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -120,19 +120,23 @@ EXPORT_SYMBOL_GPL(i2c_match_id);
 const void *i2c_get_match_data(const struct i2c_client *client)
 {
 	struct i2c_driver *driver = to_i2c_driver(client->dev.driver);
+	const struct smbus_device_id *smbus_match;
 	const struct i2c_device_id *match;
 	const void *data;
 
 	data = device_get_match_data(&client->dev);
-	if (!data) {
-		match = i2c_match_id(driver->id_table, client);
-		if (!match)
-			return NULL;
+	if (data)
+		return data;
 
-		data = (const void *)match->driver_data;
-	}
+	smbus_match = i2c_smbus_match_id(client, driver->smbus_id_table);
+	if (smbus_match)
+		return (const void *)smbus_match->driver_data;
+
+	match = i2c_match_id(driver->id_table, client);
+	if (match)
+		return (const void *)match->driver_data;
 
-	return data;
+	return NULL;
 }
 EXPORT_SYMBOL(i2c_get_match_data);
 
@@ -152,6 +156,10 @@ static int i2c_device_match(struct device *dev, const struct device_driver *drv)
 
 	driver = to_i2c_driver(drv);
 
+	/* SMBus Unique Device Identifier match */
+	if (i2c_smbus_match_id(client, driver->smbus_id_table))
+		return 1;
+
 	/* Finally an I2C match */
 	if (i2c_match_id(driver->id_table, client))
 		return 1;
@@ -172,6 +180,13 @@ static int i2c_device_uevent(const struct device *dev, struct kobj_uevent_env *e
 	if (rc != -ENODEV)
 		return rc;
 
+	if (client->udid)
+		return add_uevent_var(env, "MODALIAS=smbus:v%04xd%04xi%04xsv%04xsd%04xvsi%08x",
+				  client->udid->vendor, client->udid->device,
+				  client->udid->interface,
+				  client->udid->subvendor, client->udid->subdevice,
+				  client->udid->vendor_specific_id);
+
 	return add_uevent_var(env, "MODALIAS=%s%s", I2C_MODULE_PREFIX, client->name);
 }
 
@@ -684,6 +699,13 @@ modalias_show(struct device *dev, struct device_attribute *attr, char *buf)
 	if (len != -ENODEV)
 		return len;
 
+	if (client->udid)
+		return sysfs_emit(buf, "smbus:v%04xd%04xi%04xsv%04xsd%04xvsi%08x\n",
+				  client->udid->vendor, client->udid->device,
+				  client->udid->interface,
+				  client->udid->subvendor, client->udid->subdevice,
+				  client->udid->vendor_specific_id);
+
 	return sprintf(buf, "%s%s\n", I2C_MODULE_PREFIX, client->name);
 }
 static DEVICE_ATTR_RO(modalias);
@@ -822,7 +844,7 @@ static int i2c_check_mux_children(struct device *dev, void *addrp)
 	return result;
 }
 
-static int i2c_check_addr_busy(struct i2c_adapter *adapter, int addr)
+int i2c_check_addr_busy(struct i2c_adapter *adapter, int addr)
 {
 	struct i2c_adapter *parent = i2c_parent_is_i2c_adapter(adapter);
 	int result = 0;
@@ -973,6 +995,7 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf
 	client->dev.platform_data = info->platform_data;
 	client->flags = info->flags;
 	client->addr = info->addr;
+	client->udid = info->udid;
 
 	client->init_irq = info->irq;
 	if (!client->init_irq)
@@ -1508,7 +1531,7 @@ int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short addr)
 
 	irq = irq_find_mapping(adap->host_notify_domain, addr);
 	if (irq <= 0)
-		return -ENXIO;
+		return i2c_smbus_arp_detect(adap, addr);
 
 	generic_handle_irq_safe(irq);
 
@@ -1602,6 +1625,11 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 	bus_for_each_drv(&i2c_bus_type, NULL, adap, __process_new_adapter);
 	mutex_unlock(&core_lock);
 
+	/* Detect ARP clients. */
+	res = i2c_smbus_arp_probe(adap);
+	if (res)
+		dev_err(&adap->dev, "ARP device registration failed\n");
+
 	return 0;
 
 out_reg:
@@ -1766,6 +1794,7 @@ void i2c_del_adapter(struct i2c_adapter *adap)
 		return;
 	}
 
+	i2c_smbus_arp_remove(adap);
 	i2c_acpi_remove_space_handler(adap);
 	/* Tell drivers about this removal */
 	mutex_lock(&core_lock);
diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
index 4797ba88331c..c8f7d8e49621 100644
--- a/drivers/i2c/i2c-core.h
+++ b/drivers/i2c/i2c-core.h
@@ -60,6 +60,8 @@ static inline int __i2c_check_suspended(struct i2c_adapter *adap)
 	return 0;
 }
 
+int i2c_check_addr_busy(struct i2c_adapter *adapter, int addr);
+
 #ifdef CONFIG_ACPI
 void i2c_acpi_register_devices(struct i2c_adapter *adap);
 
@@ -106,3 +108,9 @@ static inline int i2c_setup_smbus_alert(struct i2c_adapter *adap)
 	return 0;
 }
 #endif
+
+const struct smbus_device_id *i2c_smbus_match_id(const struct i2c_client *client,
+						 const struct smbus_device_id *id);
+int i2c_smbus_arp_detect(struct i2c_adapter *adapter, u8 target_address);
+int i2c_smbus_arp_probe(struct i2c_adapter *adapter);
+void i2c_smbus_arp_remove(struct i2c_adapter *adapter);
diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
index dc1bd2ab4c13..c08b35d2858a 100644
--- a/include/linux/i2c-smbus.h
+++ b/include/linux/i2c-smbus.h
@@ -51,4 +51,71 @@ static inline void i2c_register_spd_write_disable(struct i2c_adapter *adap) { }
 static inline void i2c_register_spd_write_enable(struct i2c_adapter *adap) { }
 #endif
 
+/**
+ * struct i2c_arp_udid - ARP Unique Device Identifier
+ * @capabilities:
+ * @version:
+ * @vendor:
+ * @device:
+ * @interface:
+ * @subvendor:
+ * @subdevice:
+ * @vendor_specific_id:
+ */
+struct i2c_arp_udid {
+	u8 capabilities;
+#define ARP_CAP_PEC_SUPPORTED				BIT(0)
+#define ARP_CAP_ADDRESS_TYPE_MASK			GENMASK(7, 6)
+#define   ARP_CAP_ADDRESS_TYPE_FIXED			0
+#define   ARP_CAP_ADDRESS_TYPE_DYNAMIC_PERSISTENT	1
+#define   ARP_CAP_ADDRESS_TYPE_DYNAMIC_VOLATILE		2
+#define   ARP_CAP_ADDRESS_TYPE_RANDOM_NUMBER		3
+	u8 version;
+	u16 vendor;
+	u16 device;
+	u16 interface;
+	u16 subvendor;
+	u16 subdevice;
+	u32 vendor_specific_id;
+} __packed;
+
+/**
+ * SMBUS_DEVICE - macro used to describe a specific SMBus device
+ * @v: the 16 bit UDID Vendor ID
+ * @d: the 16 bit UDID Device ID
+ *
+ * This macro is used to create a struct smbus_device_id that matches a
+ * specific device. The interface, subvendor and subdevice fields will be set to
+ * SMBUS_ANY_ID.
+ */
+#define SMBUS_DEVICE(v, d) \
+	.vendor = (v), .device = (d), \
+	.interface = SMBUS_ANY_ID, \
+	.subvendor = SMBUS_ANY_ID, .subdevice = SMBUS_ANY_ID, \
+	.vendor_specific_id = SMBUS_ANY_VENDOR_SPECIFIC_ID,
+
+/**
+ * SMBUS_INTERFACE - macro used to describe a specific SMBus interface
+ * @i: the 16 bit UDID interface
+ *
+ * This macro is used to create a struct smbus_device_id that matches a
+ * specific interface. The vendor, device, subvendor and subdevice fields will be
+ * set to SMBUS_ANY_ID.
+ */
+#define SMBUS_INTERFACE(i) \
+	.vendor = SMBUS_ANY_ID, .device = SMBUS_ANY_ID, \
+	.interface = (i), \
+	.subvendor = SMBUS_ANY_ID, .subdevice = SMBUS_ANY_ID, \
+	.vendor_specific_id = SMBUS_ANY_VENDOR_SPECIFIC_ID,
+
+/* The version field in the interface - these are used as the minimum. */
+#define SMBUS_INTERFACE_SMBUS_V2_0	0x4
+#define SMBUS_INTERFACE_SMBUS_V3_0	0x5
+
+/* The interfaces defined in the SMBus specification. */
+#define SMBUS_INTERFACE_OEM	SMBUS_INTERFACE(BIT(4) | SMBUS_INTERFACE_SMBUS_V2_0)
+#define SMBUS_INTERFACE_ASF	SMBUS_INTERFACE(BIT(5) | SMBUS_INTERFACE_SMBUS_V2_0)
+#define SMBUS_INTERFACE_IPMI	SMBUS_INTERFACE(BIT(6) | SMBUS_INTERFACE_SMBUS_V2_0)
+#define SMBUS_INTERFACE_ZONE	SMBUS_INTERFACE(BIT(7) | SMBUS_INTERFACE_SMBUS_V3_0)
+
 #endif /* _LINUX_I2C_SMBUS_H */
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 20fd41b51d5c..1f0cd285e711 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -34,6 +34,8 @@ struct i2c_adapter;
 struct i2c_client;
 struct i2c_driver;
 struct i2c_device_identity;
+struct i2c_smbus_arp;
+struct i2c_arp_udid;
 union i2c_smbus_data;
 struct i2c_board_info;
 enum i2c_slave_event;
@@ -242,6 +244,7 @@ enum i2c_driver_flags {
  * @command: Callback for bus-wide signaling (optional)
  * @driver: Device driver model driver
  * @id_table: List of I2C devices supported by this driver
+ * @smbus_id_table: SMBus ARP devices supported by this driver
  * @detect: Callback for device detection
  * @address_list: The I2C addresses to probe (for detect)
  * @clients: List of detected clients we created (for i2c-core use only)
@@ -295,6 +298,7 @@ struct i2c_driver {
 
 	struct device_driver driver;
 	const struct i2c_device_id *id_table;
+	const struct smbus_device_id *smbus_id_table;
 
 	/* Device detection callback for automatic device creation */
 	int (*detect)(struct i2c_client *client, struct i2c_board_info *info);
@@ -311,6 +315,7 @@ struct i2c_driver {
  * @addr: Address used on the I2C bus connected to the parent adapter.
  * @name: Indicates the type of the device, usually a chip name that's
  *	generic enough to hide second-sourcing and compatible revisions.
+ * @udid: SMBus ARP Unique Device Identifier
  * @adapter: manages the bus segment hosting this I2C device
  * @dev: Driver model device node for the slave.
  * @init_irq: IRQ that was set at initialization
@@ -343,6 +348,7 @@ struct i2c_client {
 					/* addresses are stored in the	*/
 					/* _LOWER_ 7 bits		*/
 	char name[I2C_NAME_SIZE];
+	struct i2c_arp_udid *udid;	/* Unique Device Identifier	*/
 	struct i2c_adapter *adapter;	/* the adapter we sit on	*/
 	struct device dev;		/* the device structure		*/
 	int init_irq;			/* irq set at initialization	*/
@@ -410,6 +416,7 @@ static inline bool i2c_detect_slave_mode(struct device *dev) { return false; }
  * @resources: resources associated with the device
  * @num_resources: number of resources in the @resources array
  * @irq: stored in i2c_client.irq
+ * @udid: SMBus ARP Unique Device Identifier
  *
  * I2C doesn't actually support hardware probing, although controllers and
  * devices may be able to use I2C_SMBUS_QUICK to tell whether or not there's
@@ -433,6 +440,7 @@ struct i2c_board_info {
 	const struct resource *resources;
 	unsigned int	num_resources;
 	int		irq;
+	struct i2c_arp_udid *udid;
 };
 
 /**
@@ -755,6 +763,8 @@ struct i2c_adapter {
 	struct mutex userspace_clients_lock;
 	struct list_head userspace_clients;
 
+	struct i2c_smbus_arp *arp;
+
 	struct i2c_bus_recovery_info *bus_recovery_info;
 	const struct i2c_adapter_quirks *quirks;
 
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 24eb5a88a5c5..9d75cdb81e25 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -480,6 +480,19 @@ struct i2c_device_id {
 	kernel_ulong_t driver_data;	/* Data private to the driver */
 };
 
+#define SMBUS_ANY_ID			0xffff
+#define SMBUS_ANY_VENDOR_SPECIFIC_ID	0xffffffff
+
+struct smbus_device_id {
+	__u16 vendor;
+	__u16 device;
+	__u16 interface;
+	__u16 subvendor;
+	__u16 subdevice;
+	__u32 vendor_specific_id;
+	kernel_ulong_t driver_data;	/* Data private to the driver */
+};
+
 /* pci_epf */
 
 #define PCI_EPF_NAME_SIZE	20
diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
index b4178c42d08f..cd53628e1470 100644
--- a/scripts/mod/devicetable-offsets.c
+++ b/scripts/mod/devicetable-offsets.c
@@ -147,6 +147,14 @@ int main(void)
 	DEVID(i2c_device_id);
 	DEVID_FIELD(i2c_device_id, name);
 
+	DEVID(smbus_device_id);
+	DEVID_FIELD(smbus_device_id, vendor);
+	DEVID_FIELD(smbus_device_id, device);
+	DEVID_FIELD(smbus_device_id, interface);
+	DEVID_FIELD(smbus_device_id, subvendor);
+	DEVID_FIELD(smbus_device_id, subdevice);
+	DEVID_FIELD(smbus_device_id, vendor_specific_id);
+
 	DEVID(i3c_device_id);
 	DEVID_FIELD(i3c_device_id, match_flags);
 	DEVID_FIELD(i3c_device_id, dcr);
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 4e99393a35f1..66efff6d3623 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -840,6 +840,29 @@ static void do_i2c_entry(struct module *mod, void *symval)
 	module_alias_printf(mod, false, I2C_MODULE_PREFIX "%s", *name);
 }
 
+/* Looks like: smbus:vNdNiNsvNsdNvsiN*/
+static void do_smbus_entry(struct module *mod, void *symval)
+{
+	char alias[256] = {};
+
+	DEF_FIELD(symval, smbus_device_id, vendor);
+	DEF_FIELD(symval, smbus_device_id, device);
+	DEF_FIELD(symval, smbus_device_id, interface);
+	DEF_FIELD(symval, smbus_device_id, subvendor);
+	DEF_FIELD(symval, smbus_device_id, subdevice);
+	DEF_FIELD(symval, smbus_device_id, vendor_specific_id);
+
+	ADD(alias, "v", vendor != SMBUS_ANY_ID, vendor);
+	ADD(alias, "d", device != SMBUS_ANY_ID, device);
+	ADD(alias, "i", interface != SMBUS_ANY_ID, interface);
+	ADD(alias, "sv", subvendor != SMBUS_ANY_ID, subvendor);
+	ADD(alias, "sd", subdevice != SMBUS_ANY_ID, subdevice);
+	ADD(alias, "vsi", vendor_specific_id != SMBUS_ANY_VENDOR_SPECIFIC_ID,
+	    vendor_specific_id);
+
+	module_alias_printf(mod, true, "smbus:%s", alias);
+}
+
 static void do_i3c_entry(struct module *mod, void *symval)
 {
 	char alias[256] = {};
@@ -1439,6 +1462,7 @@ static const struct devtable devtable[] = {
 	{"vmbus", SIZE_hv_vmbus_device_id, do_vmbus_entry},
 	{"rpmsg", SIZE_rpmsg_device_id, do_rpmsg_entry},
 	{"i2c", SIZE_i2c_device_id, do_i2c_entry},
+	{"smbus", SIZE_smbus_device_id, do_smbus_entry},
 	{"i3c", SIZE_i3c_device_id, do_i3c_entry},
 	{"slim", SIZE_slim_device_id, do_slim_entry},
 	{"spi", SIZE_spi_device_id, do_spi_entry},
-- 
2.50.1

[PATCH v2 2/4] i2c: Sysfs attribute files for the Unique Device Identifier fields

[Heikki Krogerus]

In order to utilise the SMBus Address Resolution Protocol's
(ARP) Unique Device Identifier (UDID) also in user space,
the UDID details need to be exposed. With ARP the address is
also dynamically assigned (and may be reset) so it also
needs to be exposed to the user space with its own file.

The UDID details are only visible with ARP devices, but the
address file is made always visible for all I2C client
devices.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 Documentation/ABI/testing/sysfs-bus-i2c |  53 ++++++++++++
 drivers/i2c/i2c-core-base.c             | 107 +++++++++++++++++++++++-
 2 files changed, 159 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c

diff --git a/Documentation/ABI/testing/sysfs-bus-i2c b/Documentation/ABI/testing/sysfs-bus-i2c
new file mode 100644
index 000000000000..b9ebfc2e1b9d
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-i2c
@@ -0,0 +1,53 @@
+What:		/sys/bus/i2c/devices/.../address
+Date:		September 2025
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		This file has the target address the I2C client responds to.
+
+What:		/sys/bus/i2c/devices/.../vendor
+Date:		September 2025
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		This file is only visible for ARP devices. It returns the Vendor
+		ID field from the SMBus ARP Unique Device Identifier. It is the
+		device manufacturer's ID assigned by the SBS Implementers Forum
+		or the PCI SIG.
+
+What:		/sys/bus/i2c/devices/.../device
+Date:		September 2025
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		This file is only visible for ARP devices. It returns the Device
+		ID field from the SMBus ARP Unique Device Identifier. The
+		device ID is assigned by the device manufacturer.
+
+What:		/sys/bus/i2c/devices/.../interface
+Date:		September 2025
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		This file is only visible for ARP devices. It returns the
+		Interface field from the SMBus ARP Unique Device Identifier.
+
+What:		/sys/bus/i2c/devices/.../subsystem_vendor
+Date:		September 2025
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		This file is only visible for ARP devices. It returns the
+		Subsystem Vendor ID field from the SMBus ARP Unique Device
+		Identifier.
+
+What:		/sys/bus/i2c/devices/.../subsystem_device
+Date:		September 2025
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		This file is only visible for ARP devices. It returns the
+		Subsystem Device ID field from the SMBus ARP Unique Device
+		Identifier.
+
+What:		/sys/bus/i2c/devices/.../vendor_specific_id
+Date:		September 2025
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		This file is only visible for ARP devices. It returns the
+		Vendor Specific ID field from the SMBus ARP Unique Device
+		Identifier.
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 6e96ac186921..4e2422cd0448 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -685,6 +685,13 @@ name_show(struct device *dev, struct device_attribute *attr, char *buf)
 }
 static DEVICE_ATTR_RO(name);
 
+static ssize_t
+address_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "0x%02x\n", to_i2c_client(dev)->addr);
+}
+static DEVICE_ATTR_RO(address);
+
 static ssize_t
 modalias_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
@@ -712,11 +719,109 @@ static DEVICE_ATTR_RO(modalias);
 
 static struct attribute *i2c_dev_attrs[] = {
 	&dev_attr_name.attr,
+	&dev_attr_address.attr,
 	/* modalias helps coldplug:  modprobe $(cat .../modalias) */
 	&dev_attr_modalias.attr,
 	NULL
 };
-ATTRIBUTE_GROUPS(i2c_dev);
+
+static const struct attribute_group i2c_dev_group = {
+	.attrs = i2c_dev_attrs,
+};
+
+static ssize_t
+capabilities_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+
+	return sysfs_emit(buf, "0x%02x\n", client->udid->capabilities);
+}
+static DEVICE_ATTR_RO(capabilities);
+
+static ssize_t
+vendor_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+
+	return sysfs_emit(buf, "0x%04x\n", client->udid->vendor);
+}
+static DEVICE_ATTR_RO(vendor);
+
+static ssize_t
+device_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+
+	return sysfs_emit(buf, "0x%04x\n", client->udid->device);
+}
+static DEVICE_ATTR_RO(device);
+
+static ssize_t
+interface_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+
+	return sysfs_emit(buf, "0x%04x\n", client->udid->interface);
+}
+static DEVICE_ATTR_RO(interface);
+
+static ssize_t
+subsystem_vendor_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+
+	return sysfs_emit(buf, "0x%04x\n", client->udid->subvendor);
+}
+static DEVICE_ATTR_RO(subsystem_vendor);
+
+static ssize_t
+subsystem_device_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+
+	return sysfs_emit(buf, "0x%04x\n", client->udid->subdevice);
+}
+static DEVICE_ATTR_RO(subsystem_device);
+
+static ssize_t
+vendor_specific_id_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+
+	return sysfs_emit(buf, "0x%08x\n", client->udid->vendor_specific_id);
+}
+static DEVICE_ATTR_RO(vendor_specific_id);
+
+static struct attribute *udid_attrs[] = {
+	&dev_attr_capabilities.attr,
+	&dev_attr_vendor.attr,
+	&dev_attr_device.attr,
+	&dev_attr_interface.attr,
+	&dev_attr_subsystem_vendor.attr,
+	&dev_attr_subsystem_device.attr,
+	&dev_attr_vendor_specific_id.attr,
+	NULL
+};
+
+static umode_t
+udid_is_visible(struct kobject *kobj, struct attribute *attr, int n)
+{
+	if (to_i2c_client(kobj_to_dev(kobj))->udid)
+		return attr->mode;
+
+	return 0;
+}
+
+static const struct attribute_group udid_group = {
+	.is_visible = udid_is_visible,
+	.attrs = udid_attrs,
+};
+
+static const struct attribute_group *i2c_dev_groups[] = {
+	&i2c_dev_group,
+	&udid_group,
+	NULL
+};
 
 const struct bus_type i2c_bus_type = {
 	.name		= "i2c",
-- 
2.50.1

[PATCH v2 3/4] mctp i2c: Enable SMBus ARP discovery

[Heikki Krogerus]

Since the SMBus Address Resolution Protocol (ARP) is now
supported with all I2C host adapters, every ARP-capable MCTP
device will get automatically enumerated. Those devices just
need to be bind to this driver.

The SMBus ARP-capable MCTP devices are identified by
checking the interface (MCTP SMBus/I2C Transport Binding
Specification section 6.5). The interface must match the ASF
protocol, so all devices that use the ASF protocol as their
interface will be probed by this driver.

Link: https://www.dmtf.org/sites/default/files/standards/documents/DSP0237_1.2.0.pdf
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/net/mctp/mctp-i2c.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/mctp/mctp-i2c.c b/drivers/net/mctp/mctp-i2c.c
index 8043b57bdf25..47d7f6b5212e 100644
--- a/drivers/net/mctp/mctp-i2c.c
+++ b/drivers/net/mctp/mctp-i2c.c
@@ -21,6 +21,7 @@
 #include <linux/netdevice.h>
 #include <linux/i2c.h>
 #include <linux/i2c-mux.h>
+#include <linux/i2c-smbus.h>
 #include <linux/if_arp.h>
 #include <net/mctp.h>
 #include <net/mctpdevice.h>
@@ -1105,6 +1106,12 @@ static const struct i2c_device_id mctp_i2c_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, mctp_i2c_id);
 
+static const struct smbus_device_id mctp_smbus_id[] = {
+	{ SMBUS_INTERFACE_ASF },
+	{}
+};
+MODULE_DEVICE_TABLE(smbus, mctp_smbus_id);
+
 static const struct of_device_id mctp_i2c_of_match[] = {
 	{ .compatible = "mctp-i2c-controller" },
 	{},
@@ -1119,6 +1126,7 @@ static struct i2c_driver mctp_i2c_driver = {
 	.probe = mctp_i2c_probe,
 	.remove = mctp_i2c_remove,
 	.id_table = mctp_i2c_id,
+	.smbus_id_table = mctp_smbus_id,
 };
 
 static __init int mctp_i2c_mod_init(void)
-- 
2.50.1

Re: [PATCH v2 3/4] mctp i2c: Enable SMBus ARP discovery

[Paolo Abeni]

On 1/21/26 10:23 AM, Heikki Krogerus wrote:
> Since the SMBus Address Resolution Protocol (ARP) is now
> supported with all I2C host adapters, every ARP-capable MCTP
> device will get automatically enumerated. Those devices just
> need to be bind to this driver.
> 
> The SMBus ARP-capable MCTP devices are identified by
> checking the interface (MCTP SMBus/I2C Transport Binding
> Specification section 6.5). The interface must match the ASF
> protocol, so all devices that use the ASF protocol as their
> interface will be probed by this driver.
> 
> Link: https://www.dmtf.org/sites/default/files/standards/documents/DSP0237_1.2.0.pdf
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

This patch depends on 1/4 but it looks like it could be a follow-up to
the main series. If so, I think it would be better to re-submit it after
the relevant code landed in net-next to avoid cross sub-tree problems.

Otherwise I think it should go via I2C tree - and we will solve
conflicts as needed later on.

Thanks,

Paolo

Re: [PATCH v2 3/4] mctp i2c: Enable SMBus ARP discovery

[Jakub Kicinski]

On Thu, 22 Jan 2026 15:36:05 +0100 Paolo Abeni wrote:
> On 1/21/26 10:23 AM, Heikki Krogerus wrote:
> > Since the SMBus Address Resolution Protocol (ARP) is now
> > supported with all I2C host adapters, every ARP-capable MCTP
> > device will get automatically enumerated. Those devices just
> > need to be bind to this driver.
> > 
> > The SMBus ARP-capable MCTP devices are identified by
> > checking the interface (MCTP SMBus/I2C Transport Binding
> > Specification section 6.5). The interface must match the ASF
> > protocol, so all devices that use the ASF protocol as their
> > interface will be probed by this driver.
> > 
> > Link: https://www.dmtf.org/sites/default/files/standards/documents/DSP0237_1.2.0.pdf
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>  
> 
> This patch depends on 1/4 but it looks like it could be a follow-up to
> the main series. If so, I think it would be better to re-submit it after
> the relevant code landed in net-next to avoid cross sub-tree problems.
> 
> Otherwise I think it should go via I2C tree - and we will solve
> conflicts as needed later on.

+1 I don't wanna speak for Jeremy and Matt but with their Ack, and
assuming there's no dependency in net-next I think i2c tree may
be the easiest path? There isn't much code in the series under
drivers/net.

Re: [PATCH v2 3/4] mctp i2c: Enable SMBus ARP discovery

[Jeremy Kerr]

Hi Heikki and Jakub,

> +1 I don't wanna speak for Jeremy and Matt but with their Ack, and
> assuming there's no dependency in net-next I think i2c tree may
> be the easiest path?

Yes, given this is more i2c- than net-related, I would be fine for the
changes to go via the i2c tree there. Staging the core via i2c and then
(subsequently) this one via net would also work, but seems to be
overkill for a minor change to the net driver.

I have some overall comments on the i2c side though, I'll follow up with
those separately.

Cheers,


Jeremy

Re: [PATCH v2 3/4] mctp i2c: Enable SMBus ARP discovery

[Heikki Krogerus]

Fri, Jan 23, 2026 at 11:18:42AM +1100, Jeremy Kerr kirjoitti:
> Hi Heikki and Jakub,
> 
> > +1 I don't wanna speak for Jeremy and Matt but with their Ack, and
> > assuming there's no dependency in net-next I think i2c tree may
> > be the easiest path?
> 
> Yes, given this is more i2c- than net-related, I would be fine for the
> changes to go via the i2c tree there. Staging the core via i2c and then
> (subsequently) this one via net would also work, but seems to be
> overkill for a minor change to the net driver.

OK. I'll drop this one from v3 and send it separately later as a
follow-up, once we have the i2c side ready.

> I have some overall comments on the i2c side though, I'll follow up with
> those separately.

thanks,

-- 
heikki

Re: [PATCH v2 3/4] mctp i2c: Enable SMBus ARP discovery

[Jakub Kicinski]

On Fri, 23 Jan 2026 10:46:38 +0200 Heikki Krogerus wrote:
> > > +1 I don't wanna speak for Jeremy and Matt but with their Ack, and
> > > assuming there's no dependency in net-next I think i2c tree may
> > > be the easiest path?  
> > 
> > Yes, given this is more i2c- than net-related, I would be fine for the
> > changes to go via the i2c tree there. Staging the core via i2c and then
> > (subsequently) this one via net would also work, but seems to be
> > overkill for a minor change to the net driver.  
> 
> OK. I'll drop this one from v3 and send it separately later as a
> follow-up, once we have the i2c side ready.

FWIW my reading of Jeremy's reply was that we're okay with these
patches going via i2c, so:

Acked-by: Jakub Kicinski <kuba@kernel.org>

and no need to drop this from the series.

[PATCH v2 4/4] i2c: Add SMBus ARP target mode test driver

[Heikki Krogerus]

Backend that the hosts can enumerate by using the SMBus
Address Resolution Protocol.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/i2c/Kconfig          |   6 ++
 drivers/i2c/Makefile         |   1 +
 drivers/i2c/i2c-target-arp.c | 201 +++++++++++++++++++++++++++++++++++
 3 files changed, 208 insertions(+)
 create mode 100644 drivers/i2c/i2c-target-arp.c

diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
index c232054fddd6..4b97a66fc698 100644
--- a/drivers/i2c/Kconfig
+++ b/drivers/i2c/Kconfig
@@ -134,6 +134,12 @@ config I2C_SLAVE_TESTUNIT
 	  multi-master, SMBus Host Notify, etc. Please read
 	  Documentation/i2c/slave-testunit-backend.rst for further details.
 
+config I2C_TARGET_ARP
+	tristate "SMBus ARP target mode test driver"
+	help
+	  This backend makes Linux respond to SMBus Address Resolution Protocol
+	  commands.
+
 endif
 
 config I2C_DEBUG_CORE
diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
index 4fdb37b94aa4..3ea292dab065 100644
--- a/drivers/i2c/Makefile
+++ b/drivers/i2c/Makefile
@@ -19,5 +19,6 @@ obj-y				+= algos/ busses/ muxes/
 obj-$(CONFIG_I2C_STUB)		+= i2c-stub.o
 obj-$(CONFIG_I2C_SLAVE_EEPROM)	+= i2c-slave-eeprom.o
 obj-$(CONFIG_I2C_SLAVE_TESTUNIT)	+= i2c-slave-testunit.o
+obj-$(CONFIG_I2C_TARGET_ARP)	+= i2c-target-arp.o
 
 ccflags-$(CONFIG_I2C_DEBUG_CORE) := -DDEBUG
diff --git a/drivers/i2c/i2c-target-arp.c b/drivers/i2c/i2c-target-arp.c
new file mode 100644
index 000000000000..458749e4681e
--- /dev/null
+++ b/drivers/i2c/i2c-target-arp.c
@@ -0,0 +1,201 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * SMBus Address Resolution Protocol target mode test driver
+ *
+ * Copyright (C) 2026 Intel Corporation
+ */
+#include <linux/bits.h>
+#include <linux/device/devres.h>
+#include <linux/i2c.h>
+#include <linux/i2c-smbus.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/string.h>
+
+#define ARP_ADDRESS_VALID	BIT(0)
+#define ARP_ADDRESS_RESOLVED	BIT(1)
+
+#define I2C_SMBUS_HOST		0x08
+#define I2C_SMBUS_DEFAULT_ADDR	0x61
+
+#define ARP_CMD_PREPARE_TO_ARP	0x01
+#define ARP_CMD_RESET_DEVICE	0x02
+#define ARP_CMD_GET_UDID	0x03
+#define ARP_CMD_ASSIGN_ADDRESS	0x04
+
+struct get_udid_data {
+	struct i2c_arp_udid	udid;
+	u8			target_addr;
+} __packed;
+
+struct arp_target {
+	struct i2c_client target;
+	struct i2c_client *client;
+
+	int read_count;
+	int write_count;
+	unsigned char buf[24];
+
+	struct get_udid_data data;
+	unsigned char flag;
+};
+
+static u8 i2c_arp_pec(struct arp_target *arp, bool read)
+{
+	u8 addr = I2C_SMBUS_DEFAULT_ADDR << 1;
+	u8 len = sizeof(arp->data);
+	u8 pec = 0;
+
+	pec = i2c_smbus_pec(pec, &addr, 1);
+	pec = i2c_smbus_pec(pec, arp->buf, arp->write_count);
+
+	if (read) {
+		addr |= 1;
+		pec = i2c_smbus_pec(pec, &addr, 1);
+		pec = i2c_smbus_pec(pec, &len, 1);
+		pec = i2c_smbus_pec(pec, (void *)&arp->data, len);
+	}
+
+	return pec;
+}
+
+static int i2c_target_arp_cb(struct i2c_client *target, enum i2c_slave_event event, u8 *val)
+{
+	struct arp_target *arp = container_of(target, struct arp_target, target);
+
+	switch (event) {
+	case I2C_SLAVE_READ_PROCESSED:
+		if (arp->flag)
+			break;
+
+		if (arp->read_count == sizeof(arp->data))
+			*val = i2c_arp_pec(arp, 1);
+		else
+			*val = ((u8 *)&arp->data)[arp->read_count];
+
+		arp->read_count++;
+		break;
+	case I2C_SLAVE_READ_REQUESTED:
+		if (arp->flag)
+			break;
+
+		*val = sizeof(arp->data);
+		break;
+	case I2C_SLAVE_STOP:
+		switch (arp->buf[0]) {
+		case ARP_CMD_PREPARE_TO_ARP:
+			arp->flag = 0;
+			break;
+		case ARP_CMD_ASSIGN_ADDRESS:
+			/* If the UDID matches, this address is for us. */
+			if (!memcmp(&arp->buf[2], &arp->data.udid, sizeof(arp->data.udid)))
+				arp->flag = ARP_ADDRESS_VALID | ARP_ADDRESS_RESOLVED;
+			break;
+		default:
+			break;
+		}
+
+		arp->read_count = 0;
+		arp->write_count = 0;
+		break;
+	case I2C_SLAVE_WRITE_REQUESTED:
+		arp->read_count = 0;
+		arp->write_count = 0;
+		break;
+	case I2C_SLAVE_WRITE_RECEIVED:
+		arp->buf[arp->write_count] = *val;
+		arp->write_count++;
+		break;
+	}
+
+	return 0;
+}
+
+static void i2c_notify_arp_controller(struct i2c_client *client)
+{
+	union i2c_smbus_data data = { };
+	int ret;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
+		return;
+
+	ret = i2c_smbus_xfer(client->adapter, I2C_SMBUS_HOST, client->flags,
+			     I2C_SMBUS_WRITE, client->addr,
+			     I2C_SMBUS_WORD_DATA, &data);
+	if (ret)
+		dev_warn(&client->dev, "Failed to Notify ARP Controller (%d)\n", ret);
+}
+
+static int i2c_target_arp_probe(struct i2c_client *client)
+{
+	struct arp_target *arp;
+	int ret;
+
+	arp = devm_kzalloc(&client->dev, sizeof(*arp), GFP_KERNEL);
+	if (!arp)
+		return -ENOMEM;
+
+	arp->client = client;
+	i2c_set_clientdata(client, arp);
+
+	arp->target = *client;
+	arp->target.addr = I2C_SMBUS_DEFAULT_ADDR;
+	arp->target.flags |= I2C_CLIENT_SLAVE;
+
+	/*
+	 * The vendor and device ID are left undefined. Some hosts may not
+	 * support this. If that's the case, supply values for them.
+	 *
+	 * arp->data.udid.vendor = 0xXXXX;
+	 * arp->data.udid.device = 0xXXXX;
+	 *
+	 * The capabilities are also 0. That makes this a fixed address device
+	 * without support for PEC.
+	 */
+	arp->data.udid.version = BIT(3); /* UDID Version 1 */
+	arp->data.udid.interface = SMBUS_INTERFACE_SMBUS_V2_0;
+	arp->data.target_addr = client->addr;
+
+	/*
+	 * NOTE: This target is only for the ARP. After the address has been
+	 * assigned, another target should be registered for the actual device,
+	 * but this test code does _not_ register it.
+	 */
+	ret = i2c_slave_register(&arp->target, i2c_target_arp_cb);
+	if (ret)
+		return ret;
+
+	i2c_notify_arp_controller(client);
+
+	return 0;
+}
+
+static void i2c_target_arp_remove(struct i2c_client *client)
+{
+	struct arp_target *arp;
+
+	if (IS_ERR_OR_NULL(client))
+		return;
+
+	arp = i2c_get_clientdata(client);
+	i2c_slave_unregister(&arp->target);
+}
+
+static const struct i2c_device_id i2c_target_arp_id[] = {
+	{ "target-arp" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, i2c_target_arp_id);
+
+static struct i2c_driver i2c_target_arp_driver = {
+	.driver = {
+		.name = "i2c-target-arp",
+	},
+	.probe = i2c_target_arp_probe,
+	.remove = i2c_target_arp_remove,
+	.id_table = i2c_target_arp_id,
+};
+module_i2c_driver(i2c_target_arp_driver);
+
+MODULE_DESCRIPTION("SMBus ARP target mode test driver");
+MODULE_LICENSE("GPL");
-- 
2.50.1

Re: [PATCH v2 0/4] i2c: SMBus ARP support

[Jeremy Kerr]

Hi Heikki,

Thanks for submitting these. Supporting SMBus ARP for MCTP has been on
my wishlist for a while, so it's great to have a solid proposal here.

I'm curious about why you're proposing a kernel approach though; the
actual ARP protocol implementation would likely be implementable in
userspace. I think the only kernel facility we would need is a
notification facility for the possible presence of ARP-able devices (ie,
through a Notify ARP Master, or another mechanism described by 5.6.3.9).
I *think* we have existing interfaces for the rest of the ARP process.

It's entirely possible I've missed something there; perhaps it's neater
with the match tables and address allocation being all in-kernel. I'm
keen to hear a bit of the rationale for the in-kernel implementation
overall.

Cheers,


Jeremy

Re: [PATCH v2 0/4] i2c: SMBus ARP support

[Heikki Krogerus]

Hi Jeremy,

Sat, Jan 24, 2026 at 04:32:42PM +1100, Jeremy Kerr wrote:
> Hi Heikki,
> 
> Thanks for submitting these. Supporting SMBus ARP for MCTP has been on
> my wishlist for a while, so it's great to have a solid proposal here.
> 
> I'm curious about why you're proposing a kernel approach though; the
> actual ARP protocol implementation would likely be implementable in
> userspace. I think the only kernel facility we would need is a
> notification facility for the possible presence of ARP-able devices (ie,
> through a Notify ARP Master, or another mechanism described by 5.6.3.9).
> I *think* we have existing interfaces for the rest of the ARP process.
> 
> It's entirely possible I've missed something there; perhaps it's neater
> with the match tables and address allocation being all in-kernel. I'm
> keen to hear a bit of the rationale for the in-kernel implementation
> overall.

Since we use kernel mode device drivers, we need the kernel device
instances (struct device) that bind to them. If you want to deal with
user mode drivers then you can always do that with the i2c-dev
interface, but then you will not be using the kernel drivers such as
the mctp-i2c.c in this case. But just to be clear, this is not only
about MCTP. The ARP-capable i2c-clients can be anything.

So even if you still want to scan the ARP-devices in user space
separately, the kernel must enumerate those devices independently in
any case.

I should also point out that to my surprise the i2c-dev interface
(I2C_CHARDEV) isn't always enabled, even when I2C seems to be
otherwise fully supported in the kernel. We simply can't assume that
it's always available.

thanks,

-- 
heikki

Re: [PATCH v2 0/4] i2c: SMBus ARP support

[Jeremy Kerr]

Hi Heikki,

> Since we use kernel mode device drivers, we need the kernel device
> instances (struct device) that bind to them. If you want to deal with
> user mode drivers then you can always do that with the i2c-dev
> interface, but then you will not be using the kernel drivers such as
> the mctp-i2c.c in this case.

Sure you could - the userspace ARP implementation would be responsible
for binding an existing kernel driver to the newly-allocated dynamic i2c
address - say, through the new_device interface. The choice of driver
would typically depend on the enumerated UDID.

> But just to be clear, this is not only
> about MCTP. The ARP-capable i2c-clients can be anything.

Yes, I'm not just talking about MCTP here either.

> So even if you still want to scan the ARP-devices in user space
> separately, the kernel must enumerate those devices independently in
> any case.
> 
> I should also point out that to my surprise the i2c-dev interface
> (I2C_CHARDEV) isn't always enabled, even when I2C seems to be
> otherwise fully supported in the kernel. We simply can't assume that
> it's always available.

I don't think requiring a specific functionality to be enabled would be
a showstopper for any particular implementation. We need CONFIG_I2C
already, why is CONFIG_I2C_CHARDEV any different?

Cheers,


Jeremy

Re: [PATCH v2 0/4] i2c: SMBus ARP support

[Heikki Krogerus]

Hi,

Tue, Jan 27, 2026 at 08:32:24AM +0800, Jeremy Kerr wrote:
> Hi Heikki,
> 
> > Since we use kernel mode device drivers, we need the kernel device
> > instances (struct device) that bind to them. If you want to deal with
> > user mode drivers then you can always do that with the i2c-dev
> > interface, but then you will not be using the kernel drivers such as
> > the mctp-i2c.c in this case.
> 
> Sure you could - the userspace ARP implementation would be responsible
> for binding an existing kernel driver to the newly-allocated dynamic i2c
> address - say, through the new_device interface. The choice of driver
> would typically depend on the enumerated UDID.

Uh, no. You should only use interfaces like new_device with the
devices that really can't be detected in kernel, which isn't the case
here. There is nothing preventing the kernel from detecting the ARP
devices.

So that really can not be the solution that we'll use with these
devices, but regardless of that, relying on user space in general with
the ARP protocol has considerable challenges that I don't see that
could be solved easily:

- You still need to deliver the UDID to the kernel because of things
  like the PEC flag, and the drivers will also need information from
  it.
- With the static (not hotplugged) devices you need to assign the
  correct ACPI node (or what ever fwnode) to the ARP-device.
- When the device is hotplugged, you would need new ABI, like I think
  you already noticed, but this really does not make any sense. We
  simply don't need it, because the kernel can process this
  information on its own very simply.

On top of those there were concerns, like what if an ARP-device is
needed relatively early on during the system bootup, but I don't
actually know how big issues things like that are.

But in any case, I don't think you would ever be able to make the ARP
work from user space except with the simples cases (possibly not
reliable even with those because of things like the PEC flag) which is
not enough.

So why would you want involve the user space at all since it would
just add complexity and limitations without any benefits? The SMBus
ARP is a standard, and _simple_, method of enumerating devices, so why
in earth would you not just let the kernel always take care of it?

> > But just to be clear, this is not only
> > about MCTP. The ARP-capable i2c-clients can be anything.
> 
> Yes, I'm not just talking about MCTP here either.
> 
> > So even if you still want to scan the ARP-devices in user space
> > separately, the kernel must enumerate those devices independently in
> > any case.
> > 
> > I should also point out that to my surprise the i2c-dev interface
> > (I2C_CHARDEV) isn't always enabled, even when I2C seems to be
> > otherwise fully supported in the kernel. We simply can't assume that
> > it's always available.
> 
> I don't think requiring a specific functionality to be enabled would be
> a showstopper for any particular implementation. We need CONFIG_I2C
> already, why is CONFIG_I2C_CHARDEV any different?

Oh, if I just had the power, I would place this requirement on
everyone and everything. Unfortunately I don't have that power :(

I think this would mean that either we take care of the ARP
enumeration in kernel, like honestly it really has to be done
(regardless of this requirement), or we have to continue maintaining
(and adding :-( ) the code in the drivers and board files that create
the i2c-clients for these devices.

Br,

-- 
heikki

Re: [PATCH v2 0/4] i2c: SMBus ARP support

[Jeremy Kerr]

Hi Heikki,

> Uh, no. You should only use interfaces like new_device with the
> devices that really can't be detected in kernel, which isn't the case
> here.

Who is deciding this "you should only" case? If the facility works, it's
suitable. You raise some good points that may mean it is not a suitable
approach for an ARP implementation, but we should still make sure we're
taking the right approach.

[You seem pretty defensive here? I'm not saying no to the kernel
implementation, just doing our homework before agreeing to it]

> So why would you want involve the user space at all since it would
> just add complexity and limitations without any benefits?

Because we have fewer risks implementing this in userspace.

As an example, you currently seem to have a stack information leak in
the proposed Get UDID implementation, which would be much less of an
issue for the equivalent protocol handling implemented in userspace.

> - You still need to deliver the UDID to the kernel because of things
>   like the PEC flag, and the drivers will also need information from
>   it.

That seems like the main reason for requiring a kernel approach, in that
we need more information than just the assigned address. It's not
possible (at present) to specify the PEC flag through existing
interfaces, right?

For me, this would be the deciding factor to go for a kernel approach,
in that we otherwise cannot properly describe ARPed devices to the i2c
subsystem. We *could* push a new_device with a UDID, but I'm not sure
that's a great idea.

> - With the static (not hotplugged) devices you need to assign the
>   correct ACPI node (or what ever fwnode) to the ARP-device.

Is that possible at present? how are you planning to represent ARPed
devices in the DT - or more importantly, correlate DT (or other fwnode)
nodes to discovered devices?

> - When the device is hotplugged, you would need new ABI, like I think
>   you already noticed, but this really does not make any sense. We
>   simply don't need it, because the kernel can process this
>   information on its own very simply.

Even this "very simple" implementation may have bugs.

Assuming we go with a kernel approach: For the MCTP case, for full ARP
support of MCTP endpoints, we would still need to consume a hotplug
event that indicates that the device is available at its new address
- there is no kernel driver bound for the remote MCTP endpoints. This
event would be consumed by the (existing) MCTP infrastructure in order
to start MCTP enumeration. Is this something you have looked at
already? If so, if you can send an example of an actual event, I will
look at the mctpd part of this.

Cheers,


Jeremy

Re: [PATCH v2 0/4] i2c: SMBus ARP support

[Heikki Krogerus]

Hi Jeremy,

There seems to be another a bit more severe issue with ARP and
i2c-dev. Right now it seems that anything that can access the i2c
character devices can silently (without the kernel having any idea
what's going on) assign a conflicting address to a dynamically
addressed ARP-device. Perhaps more importantly, the user space can
remove access to an ARP-device by silently assigning a new address to
it or simply by resetting its state with Prepare to ARP.

That can happen accidentally, but it can also be done intentionally.

Unless I've missed something, this really is a major threat that we
have to solve. Right now the only idea that I have is that we simply
prevent the i2c-dev from using the SMBus Default Address.

Wed, Jan 28, 2026 at 06:28:24PM +0800, Jeremy Kerr wrote:
> > Uh, no. You should only use interfaces like new_device with the
> > devices that really can't be detected in kernel, which isn't the case
> > here.
> 
> Who is deciding this "you should only" case? If the facility works, it's
> suitable. You raise some good points that may mean it is not a suitable
> approach for an ARP implementation, but we should still make sure we're
> taking the right approach.
> 
> [You seem pretty defensive here? I'm not saying no to the kernel
> implementation, just doing our homework before agreeing to it]

I'm sorry if I sounded arrogant, it was not my intention. We don't
control the user space, so we can not rely on it to enumerate devices
like this. We will not be always even able to wait for user space with
them. The kernel will also still need to be in full control of the
device, also with the ARP protocol, in order to deal with things like
conflicts. So consider for example hotplugged devices that are not
ARP-capable. If the device has a conflicting address with a
dynamically addressed ARP-device, then kernel really has to be able to
assign new address to the ARP-device completely independently.

> > So why would you want involve the user space at all since it would
> > just add complexity and limitations without any benefits?
> 
> Because we have fewer risks implementing this in userspace.
> 
> As an example, you currently seem to have a stack information leak in
> the proposed Get UDID implementation, which would be much less of an
> issue for the equivalent protocol handling implemented in userspace.

If there are bugs in the code then we need to fix them. Can you please
comment to the patch that has the problem?

> > - You still need to deliver the UDID to the kernel because of things
> >   like the PEC flag, and the drivers will also need information from
> >   it.
> 
> That seems like the main reason for requiring a kernel approach, in that
> we need more information than just the assigned address. It's not
> possible (at present) to specify the PEC flag through existing
> interfaces, right?
> 
> For me, this would be the deciding factor to go for a kernel approach,
> in that we otherwise cannot properly describe ARPed devices to the i2c
> subsystem. We *could* push a new_device with a UDID, but I'm not sure
> that's a great idea.
> 
> > - With the static (not hotplugged) devices you need to assign the
> >   correct ACPI node (or what ever fwnode) to the ARP-device.
> 
> Is that possible at present? how are you planning to represent ARPed
> devices in the DT - or more importantly, correlate DT (or other fwnode)
> nodes to discovered devices?

I don't know about DT, but with ACPI the devices are expected to
either be fixed address devices or just use target address that
matches to the address in the I2C Serial Bus Connection Resource
Descriptor. The mapping is not yet done, but the idea is to just
assign the detected UDID to the i2c-client that was already created
from the fwnode.

> > - When the device is hotplugged, you would need new ABI, like I think
> >   you already noticed, but this really does not make any sense. We
> >   simply don't need it, because the kernel can process this
> >   information on its own very simply.
> 
> Even this "very simple" implementation may have bugs.
> 
> Assuming we go with a kernel approach: For the MCTP case, for full ARP
> support of MCTP endpoints, we would still need to consume a hotplug
> event that indicates that the device is available at its new address
> - there is no kernel driver bound for the remote MCTP endpoints. This
> event would be consumed by the (existing) MCTP infrastructure in order
> to start MCTP enumeration. Is this something you have looked at
> already? If so, if you can send an example of an actual event, I will
> look at the mctpd part of this.

We will have the address attribute file that the user space can use.
If the address changes uevent will be send it.

thanks,

-- 
heikki

Re: [PATCH v2 0/4] i2c: SMBus ARP support

[Jeremy Kerr]

Hi Heikki,

> There seems to be another a bit more severe issue with ARP and
> i2c-dev. Right now it seems that anything that can access the i2c
> character devices can silently (without the kernel having any idea
> what's going on) assign a conflicting address to a dynamically
> addressed ARP-device. Perhaps more importantly, the user space can
> remove access to an ARP-device by silently assigning a new address to
> it or simply by resetting its state with Prepare to ARP.
> 
> That can happen accidentally, but it can also be done intentionally.
> 
> Unless I've missed something, this really is a major threat that we
> have to solve. Right now the only idea that I have is that we simply
> prevent the i2c-dev from using the SMBus Default Address.

I don't necessarily see this as an issue; access to the chardev somewhat
implies full access to the i2c bus, and so arbitrary interactions with
devices to alter device states. Including the i2c address as part of the
affected state doesn't seem like a huge difference in access.

There may be a specific case that is interesting though:

 - an ARP-able device is in use by a kernel driver, which excludes
   access through the chardev

 - interactions with that device are possible using ARP commands to
   address 0x61 over the chardev

 - so, the device may be re-addressed, now allowing accesses through
   the chardev

However, I am not sure there are existing cases where access to the
chardev is a distinct privilege domain to not just unbind the driver
anyway.

> I'm sorry if I sounded arrogant, it was not my intention.

No problem, just trying to keep things collaborative!

> We don't control the user space, so we can not rely on it to enumerate
> devices like this. We will not be always even able to wait for user
> space with them.

Could you give some details on your intended use-case? That might help
to understand the constraints you're facing.

> The kernel will also still need to be in full control of the
> device, also with the ARP protocol, in order to deal with things like
> conflicts. So consider for example hotplugged devices that are not
> ARP-capable. If the device has a conflicting address with a
> dynamically addressed ARP-device, then kernel really has to be able to
> assign new address to the ARP-device completely independently.

Yeah, managing the bus addressing would definitely be simpler in-kernel.

So I think there's sufficient justification for your approach here, but
I would have a few requests:

 - that ARP is enabled explicitly. I'd be interested in having a DT
   property on the controller node that allows us to enable ARP on a
   per-bus basis.

   Otherwise, I'm pretty sure we'll break someone's existing platform
   by assuming we can start interactions on the SMBus default address.

   Is there some equivalent facility for ACPI based config?

 - I'd need to ensure that the i2c_client doesn't conflict with the MCTP
   transport's use of the device, post-ARP. I'll get a test setup sorted
   here, but I think that requires some changes to the controller driver
   that I'm using.

> > > So why would you want involve the user space at all since it would
> > > just add complexity and limitations without any benefits?
> > 
> > Because we have fewer risks implementing this in userspace.
> > 
> > As an example, you currently seem to have a stack information leak in
> > the proposed Get UDID implementation, which would be much less of an
> > issue for the equivalent protocol handling implemented in userspace.
> 
> If there are bugs in the code then we need to fix them. Can you please
> comment to the patch that has the problem?

Of course, yes. I'd just like to sort the structural things before
dealing with implementation.

> > Is that possible at present? how are you planning to represent ARPed
> > devices in the DT - or more importantly, correlate DT (or other fwnode)
> > nodes to discovered devices?
> 
> I don't know about DT, but with ACPI the devices are expected to
> either be fixed address devices or just use target address that
> matches to the address in the I2C Serial Bus Connection Resource
> Descriptor. The mapping is not yet done, but the idea is to just
> assign the detected UDID to the i2c-client that was already created
> from the fwnode.

OK, but how do you map the UDID to the resource descriptor? I don't know
much about ACPI, but the descriptor seems to be only keyed on a target
address, which is now dynamic.

(same with DT, devices are keyed by target address)

> > Assuming we go with a kernel approach: For the MCTP case, for full ARP
> > support of MCTP endpoints, we would still need to consume a hotplug
> > event that indicates that the device is available at its new address
> > - there is no kernel driver bound for the remote MCTP endpoints. This
> > event would be consumed by the (existing) MCTP infrastructure in order
> > to start MCTP enumeration. Is this something you have looked at
> > already? If so, if you can send an example of an actual event, I will
> > look at the mctpd part of this.
> 
> We will have the address attribute file that the user space can use.
> If the address changes uevent will be send it.

Sounds good, but for MCTP there is no struct device bound to the remote
i2c device/address. Are you proposing we change that?

Cheers,


Jeremy

Re: [PATCH v2 0/4] i2c: SMBus ARP support

[Heikki Krogerus]

Hi,

> > There seems to be another a bit more severe issue with ARP and
> > i2c-dev. Right now it seems that anything that can access the i2c
> > character devices can silently (without the kernel having any idea
> > what's going on) assign a conflicting address to a dynamically
> > addressed ARP-device. Perhaps more importantly, the user space can
> > remove access to an ARP-device by silently assigning a new address to
> > it or simply by resetting its state with Prepare to ARP.
> > 
> > That can happen accidentally, but it can also be done intentionally.
> > 
> > Unless I've missed something, this really is a major threat that we
> > have to solve. Right now the only idea that I have is that we simply
> > prevent the i2c-dev from using the SMBus Default Address.
> 
> I don't necessarily see this as an issue; access to the chardev somewhat
> implies full access to the i2c bus, and so arbitrary interactions with
> devices to alter device states. Including the i2c address as part of the
> affected state doesn't seem like a huge difference in access.
> 
> There may be a specific case that is interesting though:
> 
>  - an ARP-able device is in use by a kernel driver, which excludes
>    access through the chardev
> 
>  - interactions with that device are possible using ARP commands to
>    address 0x61 over the chardev
> 
>  - so, the device may be re-addressed, now allowing accesses through
>    the chardev
> 
> However, I am not sure there are existing cases where access to the
> chardev is a distinct privilege domain to not just unbind the driver
> anyway.

Thanks for the answer. I don't think this is necessarily a major
problem either, but since it was raiced (internally) I had to make a
note. Let's not worry about this for now.

> I would have a few requests:
> 
>  - that ARP is enabled explicitly. I'd be interested in having a DT
>    property on the controller node that allows us to enable ARP on a
>    per-bus basis.
> 
>    Otherwise, I'm pretty sure we'll break someone's existing platform
>    by assuming we can start interactions on the SMBus default address.
> 
>    Is there some equivalent facility for ACPI based config?

With ACPI systems, we just don't know what the platform is going to be
used for in the end. This is a bit tricky with ACPI.

But I'll make the registration of the feature conditional somehow in
any case.

> > > Is that possible at present? how are you planning to represent ARPed
> > > devices in the DT - or more importantly, correlate DT (or other fwnode)
> > > nodes to discovered devices?
> > 
> > I don't know about DT, but with ACPI the devices are expected to
> > either be fixed address devices or just use target address that
> > matches to the address in the I2C Serial Bus Connection Resource
> > Descriptor. The mapping is not yet done, but the idea is to just
> > assign the detected UDID to the i2c-client that was already created
> > from the fwnode.
> 
> OK, but how do you map the UDID to the resource descriptor? I don't know
> much about ACPI, but the descriptor seems to be only keyed on a target
> address, which is now dynamic.
> 
> (same with DT, devices are keyed by target address)

I don't know how a dynamically addressed device, that does not return
any kind of target address with the UDID, could be mapped to the
fwnode.

> > > Assuming we go with a kernel approach: For the MCTP case, for full ARP
> > > support of MCTP endpoints, we would still need to consume a hotplug
> > > event that indicates that the device is available at its new address
> > > - there is no kernel driver bound for the remote MCTP endpoints. This
> > > event would be consumed by the (existing) MCTP infrastructure in order
> > > to start MCTP enumeration. Is this something you have looked at
> > > already? If so, if you can send an example of an actual event, I will
> > > look at the mctpd part of this.
> > 
> > We will have the address attribute file that the user space can use.
> > If the address changes uevent will be send it.
> 
> Sounds good, but for MCTP there is no struct device bound to the remote
> i2c device/address. Are you proposing we change that?

I'm sorry, I'm probable missing something here. If the address gets
changed, then you have to inform the remote endpoint about the new
address somehow in any case, right? So how did you do that with the
user space approach?

Br,

-- 
heikki