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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <70323408-b823-1f1a-0202-434e6243b2af@gentoo.org>
Date:   Mon, 20 Mar 2023 07:34:34 +0100
From:   zzam@...too.org
To:     Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
        linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-i2c@...r.kernel.org,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Wolfram Sang <wsa@...nel.org>,
        Luca Ceresoli <luca.ceresoli@...tlin.com>,
        Andy Shevchenko <andriy.shevchenko@...el.com>,
        Matti Vaittinen <Matti.Vaittinen@...rohmeurope.com>,
        Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        Peter Rosin <peda@...ntia.se>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Michael Tretter <m.tretter@...gutronix.de>,
        Hans Verkuil <hverkuil@...all.nl>,
        Mike Pagano <mpagano@...too.org>,
        Krzysztof Hałasa <khalasa@...p.pl>,
        Marek Vasut <marex@...x.de>,
        Satish Nagireddy <satish.nagireddy@...cruise.com>,
        Luca Ceresoli <luca@...aceresoli.net>
Subject: Re: [PATCH v10 1/8] i2c: add I2C Address Translator (ATR) support

Some inline comments below.

Regards
Matthias

Am 22.02.23 um 14:29 schrieb Tomi Valkeinen:
> From: Luca Ceresoli <luca@...aceresoli.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@...aceresoli.net>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@...asonboard.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.

> +		}
> +
> +		msgs[i].addr = c2a->alias;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * 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.

> +}
> +
> +static int i2c_atr_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> +			       int num)
> +{
> +	struct i2c_atr_chan *chan = adap->algo_data;
> +	struct i2c_atr *atr = chan->atr;
> +	struct i2c_adapter *parent = atr->parent;
> +	int ret;
> +
> +	/* Translate addresses */
> +	mutex_lock(&chan->orig_addrs_lock);
> +
> +	ret = i2c_atr_map_msgs(chan, msgs, num);
> +	if (ret < 0)
> +		goto err_unlock;
> +
> +	/* Perform the transfer */
> +	ret = i2c_transfer(parent, msgs, num);
> +
> +	/* Restore addresses */
> +	i2c_atr_unmap_msgs(chan, msgs, num);
> +
> +err_unlock:
> +	mutex_unlock(&chan->orig_addrs_lock);
> +
> +	return ret;
> +}
> +
[...]