linux-kernel - Re: [PATCH v6 2/3] i3c: master: Extend address status bit to 4 and add I3C_ADDR_SLOT_EXT

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: <20241004092914.42701e56@xps-13>
Date: Fri, 4 Oct 2024 09:29:14 +0200
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: Frank Li <Frank.Li@....com>
Cc: Alexandre Belloni <alexandre.belloni@...tlin.com>,
 linux-i3c@...ts.infradead.org, linux-kernel@...r.kernel.org, arnd@...db.de,
 bbrezillon@...nel.org, boris.brezillon@...labora.com,
 conor.culhane@...vaco.com, gregkh@...uxfoundation.org, imx@...ts.linux.dev,
 pthombar@...ence.com, ravindra.yashvant.shinde@....com
Subject: Re: [PATCH v6 2/3] i3c: master: Extend address status bit to 4 and
 add I3C_ADDR_SLOT_EXT_DESIRED

Hi Frank,

> + * However, some I3C targets may request specific addresses (called as "init_dyn_addr"), which is
> + * typically specified by the DT-'s assigned-address property. Lower addresses having higher IBI
> + * priority. If it is available, i3c_bus_get_free_addr() preferably return a free address that is
> + * not in the list of desired addresses (called as "init_dyn_addr"). This allows the device with
> + * the "init_dyn_addr" to switch to its "init_dyn_addr" when it hot-joins the I3C bus. Otherwise,
> + * if the "init_dyn_addr" is already in use by another I3C device, the target device will not be
> + * able to switch to its desired address.
> + *
> + * If the previous step fails, fallback returning one of the remaining unassigned address,
> + * regardless of its state in the desired list.
> + */
>  static int i3c_bus_get_free_addr(struct i3c_bus *bus, u8 start_addr)
>  {
>  	enum i3c_addr_slot_status status;
>  	u8 addr;
>  
> +	for (addr = start_addr; addr < I3C_MAX_ADDR; addr++) {
> +		status = i3c_bus_get_addr_slot_status_mask(bus, addr,
> +							   I3C_ADDR_SLOT_EXT_STATUS_MASK);
> +		if (status == I3C_ADDR_SLOT_FREE)
> +			return addr;
> +	}
> +
>  	for (addr = start_addr; addr < I3C_MAX_ADDR; addr++) {
>  		status = i3c_bus_get_addr_slot_status(bus, addr);

Maybe here we could also use the same status_mask() with the other mask,
just to clarify the difference.

>  		if (status == I3C_ADDR_SLOT_FREE)
> @@ -1918,9 +1959,10 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
>  			goto err_rstdaa;
>  		}
>  
> -		i3c_bus_set_addr_slot_status(&master->bus,
> -					     i3cboardinfo->init_dyn_addr,
> -					     I3C_ADDR_SLOT_I3C_DEV);
> +		i3c_bus_set_addr_slot_status_mask(&master->bus,
> +						 i3cboardinfo->init_dyn_addr,
> +						 I3C_ADDR_SLOT_I3C_DEV | I3C_ADDR_SLOT_EXT_DESIRED,
> +						 I3C_ADDR_SLOT_EXT_STATUS_MASK);

However I'm not sure I understand the use of the
set_addr_slot_status_mask() function. Can't we just use the normal
function and just extend the mask in the fist place?

>  		/*
>  		 * Only try to create/attach devices that have a static
> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> index 2100547b2d8d2..6e5328c6c6afd 100644
> --- a/include/linux/i3c/master.h
> +++ b/include/linux/i3c/master.h
> @@ -298,7 +298,8 @@ enum i3c_open_drain_speed {
>   * @I3C_ADDR_SLOT_I2C_DEV: address is assigned to an I2C device
>   * @I3C_ADDR_SLOT_I3C_DEV: address is assigned to an I3C device
>   * @I3C_ADDR_SLOT_STATUS_MASK: address slot mask
> - *
> + * @I3C_ADDR_SLOT_EXT_DESIRED: the bitmask represents addresses that are preferred by some devices,
> + *			       such as the "assigned-address" property in a device tree source.
>   * On an I3C bus, addresses are assigned dynamically, and we need to know which
>   * addresses are free to use and which ones are already assigned.
>   *
> @@ -311,9 +312,11 @@ enum i3c_addr_slot_status {
>  	I3C_ADDR_SLOT_I2C_DEV,
>  	I3C_ADDR_SLOT_I3C_DEV,
>  	I3C_ADDR_SLOT_STATUS_MASK = 3,
> +	I3C_ADDR_SLOT_EXT_STATUS_MASK = 7,
> +	I3C_ADDR_SLOT_EXT_DESIRED = BIT(2),
>  };
>  
> -#define I3C_ADDR_SLOT_STATUS_BITS 2
> +#define I3C_ADDR_SLOT_STATUS_BITS 4
>  
>  /**
>   * struct i3c_bus - I3C bus object
> 

Otherwise lgtm.

Thanks,
Miquèl