| 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: <Zv15UjD9yvJb43MF@lizhi-Precision-Tower-5810>
Date: Wed, 2 Oct 2024 12:48:18 -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 2/3] i3c: master: Extend address status bit to 4 and add
I3C_ADDR_SLOT_EXT_INIT
On Wed, Oct 02, 2024 at 09:49:44AM +0200, Miquel Raynal wrote:
> Hi Frank,
>
> Frank.Li@....com wrote on Tue, 01 Oct 2024 13:08:21 -0400:
>
> > Extend the address status bit to 4 and introduce the I3C_ADDR_SLOT_EXT_INIT
> > macro to indicate that a device prefers a specific address. This is
> > generally set by the 'assigned-address' in the device tree source (dts)
> > file.
> >
> > ┌────┬─────────────┬───┬─────────┬───┐
> > │S/Sr│ 7'h7E RnW=0 │ACK│ ENTDAA │ T ├────┐
> > └────┴─────────────┴───┴─────────┴───┘ │
> > ┌─────────────────────────────────────────┘
> > │ ┌──┬─────────────┬───┬─────────────────┬────────────────┬───┬─────────┐
> > └─►│Sr│7'h7E RnW=1 │ACK│48bit UID BCR DCR│Assign 7bit Addr│PAR│ ACK/NACK│
> > └──┴─────────────┴───┴─────────────────┴────────────────┴───┴─────────┘
> >
> > Some master controllers (such as HCI) need to prepare the entire above
> > transaction before sending it out to the I3C bus. This means that a 7-bit
> > dynamic address needs to be allocated before knowing the target device's
> > UID information.
> >
> > However, some I3C targets want a specific address (called as
> > "init_dyn_addr"), which is typically specified by the DT's assigned-address
> > property. (Lower addresses have higher IBI priority, and the target can
> > adjust this by using the assigned-address property if using DT). The
> > function i3c_master_add_i3c_dev_locked() will switch to this
> > "init_dyn_addr" if it is not in use.
> >
> > Therefore, i3c_bus_get_free_addr() should return a free address that has
> > not been claimed by any target devices as "init_dyn_addr" (indicated by
> > I3C_ADDR_SLOT_EXT_INIT). 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 all of above address are already used, i3c_bus_get_free_addr() return
> > one from the claimed as init_dyn_addr and free address slot. This ensures
> > support devices as much as possible.
> >
> > Signed-off-by: Frank Li <Frank.Li@....com>
> > ---
> > change from v3 to v4
> > - rewrite commit message and comment for i3c_bus_get_free_addr()
> > ---
> > drivers/i3c/master.c | 68 ++++++++++++++++++++++++++++++++++++++++------
> > include/linux/i3c/master.h | 7 +++--
> > 2 files changed, 64 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index dcf8d23c5941a..a56cb281e6b6d 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
> > @@ -345,7 +345,7 @@ const struct bus_type i3c_bus_type = {
> > EXPORT_SYMBOL_GPL(i3c_bus_type);
> >
> > static enum i3c_addr_slot_status
> > -i3c_bus_get_addr_slot_status(struct i3c_bus *bus, u16 addr)
> > +i3c_bus_get_addr_slot_status_ext(struct i3c_bus *bus, u16 addr)
> > {
> > unsigned long status;
> > int bitpos = addr * I3C_ADDR_SLOT_STATUS_BITS;
> > @@ -356,11 +356,17 @@ i3c_bus_get_addr_slot_status(struct i3c_bus *bus, u16 addr)
> > status = bus->addrslots[bitpos / BITS_PER_LONG];
> > status >>= bitpos % BITS_PER_LONG;
> >
> > - return status & I3C_ADDR_SLOT_STATUS_MASK;
> > + return status & I3C_ADDR_SLOT_EXT_STATUS_MASK;
> > }
> >
> > -static void i3c_bus_set_addr_slot_status(struct i3c_bus *bus, u16 addr,
> > - enum i3c_addr_slot_status status)
> > +static enum i3c_addr_slot_status
> > +i3c_bus_get_addr_slot_status(struct i3c_bus *bus, u16 addr)
> > +{
> > + return i3c_bus_get_addr_slot_status_ext(bus, addr) & I3C_ADDR_SLOT_STATUS_MASK;
> > +}
> > +
> > +static void i3c_bus_set_addr_slot_status_mask(struct i3c_bus *bus, u16 addr,
> > + enum i3c_addr_slot_status status, int mask)
> > {
> > int bitpos = addr * I3C_ADDR_SLOT_STATUS_BITS;
> > unsigned long *ptr;
> > @@ -369,11 +375,22 @@ static void i3c_bus_set_addr_slot_status(struct i3c_bus *bus, u16 addr,
> > return;
> >
> > ptr = bus->addrslots + (bitpos / BITS_PER_LONG);
> > - *ptr &= ~((unsigned long)I3C_ADDR_SLOT_STATUS_MASK <<
> > - (bitpos % BITS_PER_LONG));
> > + *ptr &= ~((unsigned long)mask << (bitpos % BITS_PER_LONG));
> > *ptr |= (unsigned long)status << (bitpos % BITS_PER_LONG);
> > }
> >
> > +static void i3c_bus_set_addr_slot_status(struct i3c_bus *bus, u16 addr,
> > + enum i3c_addr_slot_status status)
> > +{
> > + i3c_bus_set_addr_slot_status_mask(bus, addr, status, I3C_ADDR_SLOT_STATUS_MASK);
> > +}
> > +
> > +static void i3c_bus_set_addr_slot_status_ext(struct i3c_bus *bus, u16 addr,
> > + enum i3c_addr_slot_status status)
> > +{
> > + i3c_bus_set_addr_slot_status_mask(bus, addr, status, I3C_ADDR_SLOT_EXT_STATUS_MASK);
> > +}
>
> Can we drop this helper and instead modify the
> i3c_bus_set_addr_slot_status() prototype to get the mask from its
> parameters?
git grep "i3c_bus_set_addr_slot_status(" drivers/i3c/ | wc
There are 18 places need be modified without this helper function.
are you sue what you want?
Maybe drop i3c_bus_set_addr_slot_status_ext() is good idea by direct use
i3c_bus_set_addr_slot_status_mask().
Frank
>
> > +
> > static bool i3c_bus_dev_addr_is_avail(struct i3c_bus *bus, u8 addr)
> > {
> > enum i3c_addr_slot_status status;
> > @@ -383,11 +400,44 @@ static bool i3c_bus_dev_addr_is_avail(struct i3c_bus *bus, u8 addr)
> > return status == I3C_ADDR_SLOT_FREE;
> > }
> >
> > +/*
> > + * ┌────┬─────────────┬───┬─────────┬───┐
> > + * │S/Sr│ 7'h7E RnW=0 │ACK│ ENTDAA │ T ├────┐
> > + * └────┴─────────────┴───┴─────────┴───┘ │
> > + * ┌─────────────────────────────────────────┘
> > + * │ ┌──┬─────────────┬───┬─────────────────┬────────────────┬───┬─────────┐
> > + * └─►│Sr│7'h7E RnW=1 │ACK│48bit UID BCR DCR│Assign 7bit Addr│PAR│ ACK/NACK│
> > + * └──┴─────────────┴───┴─────────────────┴────────────────┴───┴─────────┘
> > + * Some master controllers (such as HCI) need to prepare the entire above transaction before
> > + * sending it out to the I3C bus. This means that a 7-bit dynamic address needs to be allocated
> > + * before knowing the target device's UID information.
> > + *
> > + * However, some I3C targets want a specific address (called as "init_dyn_addr"), which is
>
> may request specific addresses (called "init...
>
> > + * typically specified by the DT's assigned-address property. (Lower addresses have higher IBI
>
> -'s
>
> > + * priority, and the target can adjust this by using the assigned-address property if using DT).
>
> Can we remove the whole "( ... )" sentence, and replace it with:
>
> "... property, lower addresses having higher IBI priority."
>
> > + * The function i3c_master_add_i3c_dev_locked() will switch to this "init_dyn_addr" if it is not
> > + * in use.
>
> if it is available.
>
> > + *
> > + * Therefore, i3c_bus_get_free_addr() should return a free address that has not been claimed by any
>
> preferably return
>
> that is not in the list of desired addresses.
>
> > + * target devices 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 all of above address are already used, i3c_bus_get_free_addr() return one from the claimed as
> > + * init_dyn_addr and free address slot. This ensures support devices as much as possible.
>
> If the previous step fails, fallback returning one of the remaining
> unassigned address, regardless of its state in the desired list.
>
> > + */
>
> Please update your commit message as well with these changes.
>
> > 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_ext(bus, addr);
>
> So here it could look like:
>
> status = ...get_addr_slot_status(bus, addr, <extended>)
>
> > + 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);
> > if (status == I3C_ADDR_SLOT_FREE)
> > @@ -1918,9 +1968,9 @@ 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_ext(&master->bus,
> > + i3cboardinfo->init_dyn_addr,
> > + I3C_ADDR_SLOT_I3C_DEV | I3C_ADDR_SLOT_EXT_INIT);
> >
> > /*
> > * 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..57ad6044ac856 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_INIT: the bitmask represents addresses that are preferred by some devices,
> > + * such as the "assigned-address" property in a device tree source (DTS).
>
> The naming could be improved, because "extended" does not mean much. I
> believe we should express the fact that this is a desired addressed, so
> what about:
>
> I3C_ADDR_SLOT_I3C_ASSIGNED/DESIRED
>
> > * 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_INIT = BIT(2),
> > };
> >
> > -#define I3C_ADDR_SLOT_STATUS_BITS 2
> > +#define I3C_ADDR_SLOT_STATUS_BITS 4
> >
> > /**
> > * struct i3c_bus - I3C bus object
> >
>
>
> Thanks,
> Miquèl
Powered by blists - more mailing lists