Messages in this thread | | | Date | Mon, 20 Mar 2023 14:12:32 +0200 | Subject | Re: [PATCH v10 1/8] i2c: add I2C Address Translator (ATR) support | From | Tomi Valkeinen <> |
| |
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@gentoo.org wrote: > >> Some inline comments below. >> >> Regards >> Matthias >> >> Am 22.02.23 um 14:29 schrieb Tomi Valkeinen: >>> From: Luca Ceresoli <luca@lucaceresoli.net> >>> >>> 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@lucaceresoli.net> >>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >>> --- >>> 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
| |