| Message ID | 20250107090204.6593-5-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | i3c: introduce and use generic parity helper | expand |
On 1/7/25 11:02 AM, Wolfram Sang wrote: > The kernel has now a generic helper for getting parity with easier to > understand semantics. Make use of it. Here, it also fixes a bug because > the correct algorithm is using XOR ('^=') instead of ADD ('+='). > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> > --- > > Change since v3: > > * updated commit message to mention the bugfix > > I intentionally did not add a Fixes tag because this fix depends on > patch 1. The proper fix for backporting would change this to XOR, I'd > think. > Stable rules allow also cherry picking additional patches. To me picking patch 1 and 4 sounds better than an intermediate fix since bug has been here from the beginning. IMHO not so urgent than a regression. Looks like we have been lucky. First dynamic address is 0x9 and previous algorithm gets the same calculated dat_w0 value for at least for the addresses 0x9 and 0xa.
> Stable rules allow also cherry picking additional patches. To me picking > patch 1 and 4 sounds better than an intermediate fix since bug has been here > from the beginning. IMHO not so urgent than a regression. Even then, doesn't that mean that the series needs to be applied upstream first before it can go to stable? How to describe the dependency commit id otherwise? Either Alexandre adds this when applying, or some interested party ;) sends a backport request to stable. Or am I missing something? > Looks like we have been lucky. First dynamic address is 0x9 and previous > algorithm gets the same calculated dat_w0 value for at least for the > addresses 0x9 and 0xa. Makes sense. Still not too many I3C devices out there...
On 1/8/25 2:56 PM, Wolfram Sang wrote: > >> Stable rules allow also cherry picking additional patches. To me picking >> patch 1 and 4 sounds better than an intermediate fix since bug has been here >> from the beginning. IMHO not so urgent than a regression. > > Even then, doesn't that mean that the series needs to be applied > upstream first before it can go to stable? How to describe the > dependency commit id otherwise? Either Alexandre adds this when > applying, or some interested party ;) sends a backport request to > stable. Or am I missing something? > Yes, the latter case was in my mind that whoever runs this driver on earlier stable kernel and has enough devices on the bus where previous code will calculate wrong can request the backport or at least be vocal about it :-) I'd say backporting back to 9ad9a52cce28 ("i3c/master: introduce the mipi-i3c-hci driver") is also needless because driver needs other fixes in order to be really usable.
diff --git a/drivers/i3c/master/mipi-i3c-hci/dat_v1.c b/drivers/i3c/master/mipi-i3c-hci/dat_v1.c index 47b9b4d4ed3f..85c4916972e4 100644 --- a/drivers/i3c/master/mipi-i3c-hci/dat_v1.c +++ b/drivers/i3c/master/mipi-i3c-hci/dat_v1.c @@ -40,15 +40,6 @@ #define dat_w0_write(i, v) writel(v, hci->DAT_regs + (i) * 8) #define dat_w1_write(i, v) writel(v, hci->DAT_regs + (i) * 8 + 4) -static inline bool dynaddr_parity(unsigned int addr) -{ - addr |= 1 << 7; - addr += addr >> 4; - addr += addr >> 2; - addr += addr >> 1; - return (addr & 1); -} - static int hci_dat_v1_init(struct i3c_hci *hci) { unsigned int dat_idx; @@ -123,7 +114,7 @@ static void hci_dat_v1_set_dynamic_addr(struct i3c_hci *hci, dat_w0 = dat_w0_read(dat_idx); dat_w0 &= ~(DAT_0_DYNAMIC_ADDRESS | DAT_0_DYNADDR_PARITY); dat_w0 |= FIELD_PREP(DAT_0_DYNAMIC_ADDRESS, address) | - (dynaddr_parity(address) ? DAT_0_DYNADDR_PARITY : 0); + (parity8(address) ? 0 : DAT_0_DYNADDR_PARITY); dat_w0_write(dat_idx, dat_w0); }
The kernel has now a generic helper for getting parity with easier to understand semantics. Make use of it. Here, it also fixes a bug because the correct algorithm is using XOR ('^=') instead of ADD ('+='). Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> --- Change since v3: * updated commit message to mention the bugfix I intentionally did not add a Fixes tag because this fix depends on patch 1. The proper fix for backporting would change this to XOR, I'd think. drivers/i3c/master/mipi-i3c-hci/dat_v1.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)