Re: [PATCH v10 1/8] i2c: add I2C Address Translator (ATR) support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 20/03/2023 10:28, Luca Ceresoli wrote:
Hello Matthias,

thanks for the in-depth review!

On Mon, 20 Mar 2023 07:34:34 +0100
zzam@xxxxxxxxxx wrote:

Some inline comments below.

Regards
Matthias

Am 22.02.23 um 14:29 schrieb Tomi Valkeinen:
From: Luca Ceresoli <luca@xxxxxxxxxxxxxxxx>

An ATR is a device that looks similar to an i2c-mux: it has an I2C
slave "upstream" port and N master "downstream" ports, and forwards
transactions from upstream to the appropriate downstream port. But it
is different in that the forwarded transaction has a different slave
address. The address used on the upstream bus is called the "alias"
and is (potentially) different from the physical slave address of the
downstream chip.

Add a helper file (just like i2c-mux.c for a mux or switch) to allow
implementing ATR features in a device driver. The helper takes care or
adapter creation/destruction and translates addresses at each transaction.

Signed-off-by: Luca Ceresoli <luca@xxxxxxxxxxxxxxxx>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
---
   Documentation/i2c/index.rst         |   1 +
   Documentation/i2c/muxes/i2c-atr.rst |  97 +++++
   MAINTAINERS                         |   8 +
   drivers/i2c/Kconfig                 |   9 +
   drivers/i2c/Makefile                |   1 +
   drivers/i2c/i2c-atr.c               | 548 ++++++++++++++++++++++++++++
   include/linux/i2c-atr.h             | 116 ++++++
   7 files changed, 780 insertions(+)
   create mode 100644 Documentation/i2c/muxes/i2c-atr.rst
   create mode 100644 drivers/i2c/i2c-atr.c
   create mode 100644 include/linux/i2c-atr.h
[...]
diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
new file mode 100644
index 000000000000..5ab890b83670
--- /dev/null
+++ b/drivers/i2c/i2c-atr.c
@@ -0,0 +1,548 @@
[...]
+
+/*
+ * Replace all message addresses with their aliases, saving the original
+ * addresses.
+ *
+ * This function is internal for use in i2c_atr_master_xfer(). It must be
+ * followed by i2c_atr_unmap_msgs() to restore the original addresses.
+ */
+static int i2c_atr_map_msgs(struct i2c_atr_chan *chan, struct i2c_msg *msgs,
+			    int num)
+{
+	struct i2c_atr *atr = chan->atr;
+	static struct i2c_atr_cli2alias_pair *c2a;
+	int i;
+
+	/* Ensure we have enough room to save the original addresses */
+	if (unlikely(chan->orig_addrs_size < num)) {
+		u16 *new_buf;
+
+		/* We don't care about old data, hence no realloc() */
+		new_buf = kmalloc_array(num, sizeof(*new_buf), GFP_KERNEL);
+		if (!new_buf)
+			return -ENOMEM;
+
+		kfree(chan->orig_addrs);
+		chan->orig_addrs = new_buf;
+		chan->orig_addrs_size = num;
+	}
+
+	for (i = 0; i < num; i++) {
+		chan->orig_addrs[i] = msgs[i].addr;
+
+		c2a = i2c_atr_find_mapping_by_addr(&chan->alias_list,
+						   msgs[i].addr);
+		if (!c2a) {
+			dev_err(atr->dev, "client 0x%02x not mapped!\n",
+				msgs[i].addr);
+			return -ENXIO;
I miss the roll-back of previously modified msgs[].addr values.

Indeed you have a point. There is a subtle error in case all of the
following happen in a single i2c_atr_master_xfer() call:

  * there are 2+ messages, having different addresses
  * msg[0] is mapped correctly
  * msg[n] (n > 0) fails mapping

It's very unlikely, but in this case we'd get back to the caller with
an error and modified addresses for the first n messages. Which in turn
is unlikely to create any problems, but it could.

Tomi, do you agree?

This looks like a simple solution:

    if (!c2a) {
+    i2c_atr_unmap_msgs(chan, msgs, i);
      ...
    }

Wouldn't that possibly restore the address from orig_addrs[x] also for messages we haven't handled yet?

I think a simple

while (i--)
	msgs[i].addr = chan->orig_addrs[i];

should do here. It is also, perhaps, a bit more clear this way, as you can see the assignments to msgs[i].addr nearby, and the rollback here with the above code. Instead of seeing a call to an unmap function, having to go and see what exactly it will do.

While there, maybe switching to dev_err_probe would make code cleaner.

The while loop above has to be done after the print, if we use the same i variable in both. dev_err_probe could still be used, but... I don't know if it's worth trying to push it in.

+/*
+ * Restore all message address aliases with the original addresses. This
+ * function is internal for use in i2c_atr_master_xfer().
+ *
+ * @see i2c_atr_map_msgs()
+ */
+static void i2c_atr_unmap_msgs(struct i2c_atr_chan *chan, struct i2c_msg *msgs,
+			       int num)
+{
+	int i;
+
+	for (i = 0; i < num; i++)
+		msgs[i].addr = chan->orig_addrs[i];
Does this code needs null and size checks for orig_addrs/orig_addrs_size
to protect from oopses?
This cannot happen now as i2c_atr_master_xfer returns early when
i2c_atr_map_msgs fails.

The map/unmap functions are really a part of i2c_atr_master_xfer() that
has been extracted for code readability, as the comments say, and I
can't think of a different use for them. So I think this code is OK as
is.

However a small comment might help future readers, especially in case
code will change and these functions gain new use cases.
E.g.

    This function is internal for use in i2c_atr_master_xfer()
+  and for this reason it needs no null and size checks on orig_addr.
    It must be followed by i2c_atr_unmap_msgs() to restore the original addresses.

I can add a comment. as Luca said, it's an internal helper function, I don't think we need to check the parameters there for cases which can't happen.

 Tomi




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux