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

Open Source and information security mailing list archives

Message-ID: <ZwAHziElHUR7ZdKS@lizhi-Precision-Tower-5810>
Date: Fri, 4 Oct 2024 11:20:46 -0400
From: Frank Li <Frank.li@....com>
To: Miquel Raynal <miquel.raynal@...tlin.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

On Fri, Oct 04, 2024 at 09:29:14AM +0200, Miquel Raynal wrote:
> 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.

Make sense.

>
> >  		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?

The major purpose of set_addr_slot_status_mask() is that reduce code
change. There are already address alloc/free by using I3C_ADDR_SLOT_FREE.

we don't want i3c_bus_set_addr_slot_status() touch bit
I3C_ADDR_SLOT_EXT_DESIRED since it was init at scan dts.

There are 18 place, using i3c_bus_set_addr_slot_status(), but we only need
touch I3C_ADDR_SLOT_EXT_DESIRED bit when scan dts.

Frank

>
> >  		/*
> >  		 * 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
>
> --
> linux-i3c mailing list
> linux-i3c@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-i3c

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