| 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
| ||
|
Message-ID: <f63b7744-b86e-4dbe-aa49-10614a025b7e@ideasonboard.com>
Date: Mon, 6 Jan 2025 11:51:20 +0200
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Romain Gantois <romain.gantois@...tlin.com>
Cc: Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
Kory Maincent <kory.maincent@...tlin.com>, linux-i2c@...r.kernel.org,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
linux-media@...r.kernel.org, linux-gpio@...r.kernel.org,
Wolfram Sang <wsa+renesas@...g-engineering.com>,
Luca Ceresoli <luca.ceresoli@...tlin.com>, Andi Shyti
<andi.shyti@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Derek Kiernan <derek.kiernan@....com>,
Dragan Cvetic <dragan.cvetic@....com>, Arnd Bergmann <arnd@...db.de>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Linus Walleij <linus.walleij@...aro.org>, Bartosz Golaszewski <brgl@...ev.pl>
Subject: Re: [PATCH v4 4/9] i2c: use client addresses directly in ATR
interface
Hi,
On 30/12/2024 15:22, Romain Gantois wrote:
> The I2C Address Translator (ATR) module defines mappings from i2c_client
> structs to aliases. However, only the physical address of each i2c_client
> struct is actually relevant to the workings of the ATR module. Moreover,
> some drivers require address translation functionality but do not allocate
> i2c_client structs, accessing the adapter directly instead. The SFP
> subsystem is an example of this.
>
> Replace the "i2c_client" field of the i2c_atr_alias_pair struct with a u16
> "addr" field. Rewrite helper functions and callbacks as needed.
>
> Signed-off-by: Romain Gantois <romain.gantois@...tlin.com>
> ---
> drivers/i2c/i2c-atr.c | 52 ++++++++++++++++---------------------------
> drivers/media/i2c/ds90ub960.c | 24 ++++++++++----------
> include/linux/i2c-atr.h | 20 ++++++++---------
> 3 files changed, 41 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
> index f21475ae592183a45b5e46a20e7a0699fb88132c..894787246846b9965deb03a7ec7eb600b102ddad 100644
> --- a/drivers/i2c/i2c-atr.c
> +++ b/drivers/i2c/i2c-atr.c
> @@ -21,16 +21,16 @@
> #define ATR_MAX_SYMLINK_LEN 11 /* Longest name is 10 chars: "channel-99" */
>
> /**
> - * struct i2c_atr_alias_pair - Holds the alias assigned to a client.
> + * struct i2c_atr_alias_pair - Holds the alias assigned to a client address.
> * @node: List node
> - * @client: Pointer to the client on the child bus
> + * @addr: Address of the client on the child bus.
> * @alias: I2C alias address assigned by the driver.
> * This is the address that will be used to issue I2C transactions
> * on the parent (physical) bus.
> */
> struct i2c_atr_alias_pair {
> struct list_head node;
> - const struct i2c_client *client;
> + u16 addr;
> u16 alias;
> };
>
> @@ -97,27 +97,13 @@ struct i2c_atr {
> struct i2c_adapter *adapter[] __counted_by(max_adapters);
> };
>
> -static struct i2c_atr_alias_pair *
> -i2c_atr_find_mapping_by_client(const struct list_head *list,
> - const struct i2c_client *client)
> -{
> - struct i2c_atr_alias_pair *c2a;
> -
> - list_for_each_entry(c2a, list, node) {
> - if (c2a->client == client)
> - return c2a;
> - }
> -
> - return NULL;
> -}
> -
> static struct i2c_atr_alias_pair *
> i2c_atr_find_mapping_by_addr(const struct list_head *list, u16 phys_addr)
> {
> struct i2c_atr_alias_pair *c2a;
>
> list_for_each_entry(c2a, list, node) {
> - if (c2a->client->addr == phys_addr)
> + if (c2a->addr == phys_addr)
> return c2a;
> }
>
> @@ -313,8 +299,8 @@ static void i2c_atr_release_alias(struct i2c_atr *atr, u16 alias)
> dev_warn(atr->dev, "Unable to find mapped alias\n");
> }
>
> -static int i2c_atr_attach_client(struct i2c_adapter *adapter,
> - const struct i2c_client *client)
> +static int i2c_atr_attach_addr(struct i2c_adapter *adapter,
> + u16 addr)
> {
> struct i2c_atr_chan *chan = adapter->algo_data;
> struct i2c_atr *atr = chan->atr;
> @@ -334,14 +320,14 @@ static int i2c_atr_attach_client(struct i2c_adapter *adapter,
> goto err_release_alias;
> }
>
> - ret = atr->ops->attach_client(atr, chan->chan_id, client, alias);
> + ret = atr->ops->attach_addr(atr, chan->chan_id, addr, alias);
> if (ret)
> goto err_free;
>
> - dev_dbg(atr->dev, "chan%u: client 0x%02x mapped at alias 0x%02x (%s)\n",
> - chan->chan_id, client->addr, alias, client->name);
> + dev_dbg(atr->dev, "chan%u: addr 0x%02x mapped at alias 0x%02x\n",
> + chan->chan_id, addr, alias);
This, and the dev_dbg() below, sound a bit odd to my ear. But I'm not
sure what would be a good print... "added alias 0x12 for address 0x34"?
Other than that, the patch looks fine.
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
Tomi
>
> - c2a->client = client;
> + c2a->addr = addr;
> c2a->alias = alias;
> list_add(&c2a->node, &chan->alias_list);
>
> @@ -355,16 +341,16 @@ static int i2c_atr_attach_client(struct i2c_adapter *adapter,
> return ret;
> }
>
> -static void i2c_atr_detach_client(struct i2c_adapter *adapter,
> - const struct i2c_client *client)
> +static void i2c_atr_detach_addr(struct i2c_adapter *adapter,
> + u16 addr)
> {
> struct i2c_atr_chan *chan = adapter->algo_data;
> struct i2c_atr *atr = chan->atr;
> struct i2c_atr_alias_pair *c2a;
>
> - atr->ops->detach_client(atr, chan->chan_id, client);
> + atr->ops->detach_addr(atr, chan->chan_id, addr);
>
> - c2a = i2c_atr_find_mapping_by_client(&chan->alias_list, client);
> + c2a = i2c_atr_find_mapping_by_addr(&chan->alias_list, addr);
> if (!c2a) {
> /* This should never happen */
> dev_warn(atr->dev, "Unable to find address mapping\n");
> @@ -374,8 +360,8 @@ static void i2c_atr_detach_client(struct i2c_adapter *adapter,
> i2c_atr_release_alias(atr, c2a->alias);
>
> dev_dbg(atr->dev,
> - "chan%u: client 0x%02x unmapped from alias 0x%02x (%s)\n",
> - chan->chan_id, client->addr, c2a->alias, client->name);
> + "chan%u: addr 0x%02x unmapped from alias 0x%02x\n",
> + chan->chan_id, addr, c2a->alias);
>
> list_del(&c2a->node);
> kfree(c2a);
> @@ -405,7 +391,7 @@ static int i2c_atr_bus_notifier_call(struct notifier_block *nb,
>
> switch (event) {
> case BUS_NOTIFY_ADD_DEVICE:
> - ret = i2c_atr_attach_client(client->adapter, client);
> + ret = i2c_atr_attach_addr(client->adapter, client->addr);
> if (ret)
> dev_err(atr->dev,
> "Failed to attach remote client '%s': %d\n",
> @@ -413,7 +399,7 @@ static int i2c_atr_bus_notifier_call(struct notifier_block *nb,
> break;
>
> case BUS_NOTIFY_DEL_DEVICE:
> - i2c_atr_detach_client(client->adapter, client);
> + i2c_atr_detach_addr(client->adapter, client->addr);
> break;
>
> default:
> @@ -506,7 +492,7 @@ struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev,
> if (max_adapters > ATR_MAX_ADAPTERS)
> return ERR_PTR(-EINVAL);
>
> - if (!ops || !ops->attach_client || !ops->detach_client)
> + if (!ops || !ops->attach_addr || !ops->detach_addr)
> return ERR_PTR(-EINVAL);
>
> atr = kzalloc(struct_size(atr, adapter, max_adapters), GFP_KERNEL);
> diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
> index 0510427ac4e9214132bcdf3fa18873ec78c48a5e..52d6879986ed4fcbe8ce3d75e04018b336beabf9 100644
> --- a/drivers/media/i2c/ds90ub960.c
> +++ b/drivers/media/i2c/ds90ub960.c
> @@ -1025,8 +1025,8 @@ static int ub960_ind_update_bits(struct ub960_data *priv, u8 block, u8 reg,
> * I2C-ATR (address translator)
> */
>
> -static int ub960_atr_attach_client(struct i2c_atr *atr, u32 chan_id,
> - const struct i2c_client *client, u16 alias)
> +static int ub960_atr_attach_addr(struct i2c_atr *atr, u32 chan_id,
> + u16 addr, u16 alias)
> {
> struct ub960_data *priv = i2c_atr_get_driver_data(atr);
> struct ub960_rxport *rxport = priv->rxports[chan_id];
> @@ -1047,23 +1047,23 @@ static int ub960_atr_attach_client(struct i2c_atr *atr, u32 chan_id,
> goto out_unlock;
> }
>
> - rxport->aliased_addrs[reg_idx] = client->addr;
> + rxport->aliased_addrs[reg_idx] = addr;
>
> ub960_rxport_write(priv, chan_id, UB960_RR_SLAVE_ID(reg_idx),
> - client->addr << 1);
> + addr << 1);
> ub960_rxport_write(priv, chan_id, UB960_RR_SLAVE_ALIAS(reg_idx),
> alias << 1);
>
> dev_dbg(dev, "rx%u: client 0x%02x assigned alias 0x%02x at slot %u\n",
> - rxport->nport, client->addr, alias, reg_idx);
> + rxport->nport, addr, alias, reg_idx);
>
> out_unlock:
> mutex_unlock(&rxport->aliased_addrs_lock);
> return ret;
> }
>
> -static void ub960_atr_detach_client(struct i2c_atr *atr, u32 chan_id,
> - const struct i2c_client *client)
> +static void ub960_atr_detach_addr(struct i2c_atr *atr, u32 chan_id,
> + u16 addr)
> {
> struct ub960_data *priv = i2c_atr_get_driver_data(atr);
> struct ub960_rxport *rxport = priv->rxports[chan_id];
> @@ -1073,13 +1073,13 @@ static void ub960_atr_detach_client(struct i2c_atr *atr, u32 chan_id,
> mutex_lock(&rxport->aliased_addrs_lock);
>
> for (reg_idx = 0; reg_idx < UB960_MAX_PORT_ALIASES; reg_idx++) {
> - if (rxport->aliased_addrs[reg_idx] == client->addr)
> + if (rxport->aliased_addrs[reg_idx] == addr)
> break;
> }
>
> if (reg_idx == UB960_MAX_PORT_ALIASES) {
> dev_err(dev, "rx%u: client 0x%02x is not mapped!\n",
> - rxport->nport, client->addr);
> + rxport->nport, addr);
> goto out_unlock;
> }
>
> @@ -1088,15 +1088,15 @@ static void ub960_atr_detach_client(struct i2c_atr *atr, u32 chan_id,
> ub960_rxport_write(priv, chan_id, UB960_RR_SLAVE_ALIAS(reg_idx), 0);
>
> dev_dbg(dev, "rx%u: client 0x%02x released at slot %u\n", rxport->nport,
> - client->addr, reg_idx);
> + addr, reg_idx);
>
> out_unlock:
> mutex_unlock(&rxport->aliased_addrs_lock);
> }
>
> static const struct i2c_atr_ops ub960_atr_ops = {
> - .attach_client = ub960_atr_attach_client,
> - .detach_client = ub960_atr_detach_client,
> + .attach_addr = ub960_atr_attach_addr,
> + .detach_addr = ub960_atr_detach_addr,
> };
>
> static int ub960_init_atr(struct ub960_data *priv)
> diff --git a/include/linux/i2c-atr.h b/include/linux/i2c-atr.h
> index 4d5da161c22516b3294e73702ace7a4546cdd588..14c1f9175c0db6a8a9c6ef5d771ae68361132a76 100644
> --- a/include/linux/i2c-atr.h
> +++ b/include/linux/i2c-atr.h
> @@ -20,20 +20,20 @@ struct i2c_atr;
>
> /**
> * struct i2c_atr_ops - Callbacks from ATR to the device driver.
> - * @attach_client: Notify the driver of a new device connected on a child
> - * bus, with the alias assigned to it. The driver must
> - * configure the hardware to use the alias.
> - * @detach_client: Notify the driver of a device getting disconnected. The
> - * driver must configure the hardware to stop using the
> - * alias.
> + * @attach_addr: Notify the driver of a new device connected on a child
> + * bus, with the alias assigned to it. The driver must
> + * configure the hardware to use the alias.
> + * @detach_addr: Notify the driver of a device getting disconnected. The
> + * driver must configure the hardware to stop using the
> + * alias.
> *
> * All these functions return 0 on success, a negative error code otherwise.
> */
> struct i2c_atr_ops {
> - int (*attach_client)(struct i2c_atr *atr, u32 chan_id,
> - const struct i2c_client *client, u16 alias);
> - void (*detach_client)(struct i2c_atr *atr, u32 chan_id,
> - const struct i2c_client *client);
> + int (*attach_addr)(struct i2c_atr *atr, u32 chan_id,
> + u16 addr, u16 alias);
> + void (*detach_addr)(struct i2c_atr *atr, u32 chan_id,
> + u16 addr);
> };
>
> /**
>
Powered by blists - more mailing lists