linux-kernel - Re: [PATCH v3 2/9] media: i2c: ds90ub960: Replace aliased clients list with bitmap

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] [day] [month] [year] [list]

Message-ID: <92f06c2a-84be-4e89-adf5-9fa58d0806e6@ideasonboard.com>
Date: Fri, 29 Nov 2024 15:46:38 +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 v3 2/9] media: i2c: ds90ub960: Replace aliased clients
 list with bitmap

Hi,

On 25/11/2024 10:45, Romain Gantois wrote:
> The ds90ub960 driver currently uses a list of i2c_client structs to keep
> track of used I2C address translator (ATR) alias slots for each RX port.
> 
> Keeping these i2c_client structs in the alias slot list isn't actually
> needed, the driver only needs to know if a specific alias slot is already
> in use or not.
> 
> Convert the aliased_clients list to a bitmap named "alias_use_mask". This
> will allow removing the "client" parameter from the i2c-atr callbacks in a
> future patch.
> 
> Signed-off-by: Romain Gantois <romain.gantois@...tlin.com>
> ---
>   drivers/media/i2c/ds90ub960.c | 23 +++++++++--------------
>   1 file changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
> index ffe5f25f8647624be005da33a6412da2493413b4..f86028894c78187257efc8fd70812387000796f7 100644
> --- a/drivers/media/i2c/ds90ub960.c
> +++ b/drivers/media/i2c/ds90ub960.c
> @@ -468,7 +468,7 @@ struct ub960_rxport {
>   		};
>   	} eq;
>   
> -	const struct i2c_client *aliased_clients[UB960_MAX_PORT_ALIASES];
> +	DECLARE_BITMAP(alias_use_mask, UB960_MAX_PORT_ALIASES);
>   };
>   
>   struct ub960_asd {
> @@ -1032,17 +1032,13 @@ static int ub960_atr_attach_client(struct i2c_atr *atr, u32 chan_id,
>   	struct device *dev = &priv->client->dev;
>   	unsigned int reg_idx;
>   
> -	for (reg_idx = 0; reg_idx < ARRAY_SIZE(rxport->aliased_clients); reg_idx++) {
> -		if (!rxport->aliased_clients[reg_idx])
> -			break;
> -	}
> -
> -	if (reg_idx == ARRAY_SIZE(rxport->aliased_clients)) {
> +	reg_idx = find_first_zero_bit(rxport->alias_use_mask, UB960_MAX_PORT_ALIASES);
> +	if (reg_idx >= UB960_MAX_PORT_ALIASES) {
>   		dev_err(dev, "rx%u: alias pool exhausted\n", rxport->nport);
>   		return -EADDRNOTAVAIL;
>   	}
>   
> -	rxport->aliased_clients[reg_idx] = client;
> +	set_bit(reg_idx, rxport->alias_use_mask);
>   
>   	ub960_rxport_write(priv, chan_id, UB960_RR_SLAVE_ID(reg_idx),
>   			   client->addr << 1);
> @@ -1063,18 +1059,15 @@ static void ub960_atr_detach_client(struct i2c_atr *atr, u32 chan_id,
>   	struct device *dev = &priv->client->dev;
>   	unsigned int reg_idx;
>   
> -	for (reg_idx = 0; reg_idx < ARRAY_SIZE(rxport->aliased_clients); reg_idx++) {
> -		if (rxport->aliased_clients[reg_idx] == client)
> -			break;
> -	}
> +	reg_idx = find_first_zero_bit(rxport->alias_use_mask, UB960_MAX_PORT_ALIASES);

The old code went through the alias table to find the matching client, 
so that it can be removed. The new code... Tries to find the first 
unused entry in the mask, to... free it?

I'm not sure how this is supposed to work, or how the driver even could 
manage with just a bit mask. The driver needs to remove the one that was 
assigned in ub960_atr_attach_addr(), so it somehow has to find the same 
entry using the address or the alias.

  Tomi