| Message ID | 20220303083141.8742-9-warp5tw@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | i2c: npcm: Bug fixes timeout, spurious interrupts | expand |
On Thu, Mar 03, 2022 at 04:31:38PM +0800, Tyrone Ting wrote: > From: Tyrone Ting <kfting@nuvoton.com> > > Use ioread8 instead of ioread32 to access the SMBnCTL3 register since > the register is only 8-bit wide. > Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver") No, this is bad commit message, since you have bitwise masks and there is nothing to fix from functional point of view. So, why is this a fix? > Signed-off-by: Tyrone Ting <kfting@nuvoton.com> > Signed-off-by: Tali Perry <tali.perry1@gmail.com> This is wrong SoB chain. ... > - return !!(I2CCTL3_SCL_LVL & ioread32(bus->reg + NPCM_I2CCTL3)); > + return !!(I2CCTL3_SCL_LVL & ioread8(bus->reg + NPCM_I2CCTL3)); ... > - return !!(I2CCTL3_SDA_LVL & ioread32(bus->reg + NPCM_I2CCTL3)); > + return !!(I2CCTL3_SDA_LVL & ioread8(bus->reg + NPCM_I2CCTL3));
> On Thu, Mar 03, 2022 at 04:31:38PM +0800, Tyrone Ting wrote: > > From: Tyrone Ting <kfting@nuvoton.com> > > > > Use ioread8 instead of ioread32 to access the SMBnCTL3 register since > > the register is only 8-bit wide. > > > Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver") > > No, this is bad commit message, since you have bitwise masks and there is > nothing to fix from functional point of view. So, why is this a fix? > The next gen of this device is a 64 bit cpu. The module is and was 8 bit. The ioread32 that seemed to work smoothly on a 32 bit machine was causing a panic on a 64 bit machine. since the module is 8 bit we changed to ioread8. This is working both for the 32 and 64 CPUs with no issue. > > Signed-off-by: Tyrone Ting <kfting@nuvoton.com> > > Signed-off-by: Tali Perry <tali.perry1@gmail.com> > > This is wrong SoB chain. > > ... > > > - return !!(I2CCTL3_SCL_LVL & ioread32(bus->reg + NPCM_I2CCTL3)); > > + return !!(I2CCTL3_SCL_LVL & ioread8(bus->reg + NPCM_I2CCTL3)); > > ... > > > - return !!(I2CCTL3_SDA_LVL & ioread32(bus->reg + NPCM_I2CCTL3)); > > + return !!(I2CCTL3_SDA_LVL & ioread8(bus->reg + NPCM_I2CCTL3)); > > -- > With Best Regards, > Andy Shevchenko Thanks Andy, BR, Tali Perry
On Thu, Mar 03, 2022 at 02:54:27PM +0200, Tali Perry wrote: > > On Thu, Mar 03, 2022 at 04:31:38PM +0800, Tyrone Ting wrote: > > > From: Tyrone Ting <kfting@nuvoton.com> > > > > > > Use ioread8 instead of ioread32 to access the SMBnCTL3 register since > > > the register is only 8-bit wide. > > > > > Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver") > > > > No, this is bad commit message, since you have bitwise masks and there is > > nothing to fix from functional point of view. So, why is this a fix? > > > > The next gen of this device is a 64 bit cpu. > The module is and was 8 bit. > > The ioread32 that seemed to work smoothly on a 32 bit machine > was causing a panic on a 64 bit machine. > since the module is 8 bit we changed to ioread8. > This is working both for the 32 and 64 CPUs with no issue. Then the commit message is completely wrong here. And provide necessary (no need to have noisy commit messages) bits of the oops to show what's going on
Hello, On Thu, Mar 03, 2022 at 04:15:18PM +0200, Andy Shevchenko wrote: > On Thu, Mar 03, 2022 at 02:54:27PM +0200, Tali Perry wrote: > > > On Thu, Mar 03, 2022 at 04:31:38PM +0800, Tyrone Ting wrote: > > > > From: Tyrone Ting <kfting@nuvoton.com> > > > > > > > > Use ioread8 instead of ioread32 to access the SMBnCTL3 register since > > > > the register is only 8-bit wide. > > > > > > > Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver") > > > > > > No, this is bad commit message, since you have bitwise masks and there is > > > nothing to fix from functional point of view. So, why is this a fix? > > > > > > > The next gen of this device is a 64 bit cpu. > > The module is and was 8 bit. > > > > The ioread32 that seemed to work smoothly on a 32 bit machine > > was causing a panic on a 64 bit machine. > > since the module is 8 bit we changed to ioread8. > > This is working both for the 32 and 64 CPUs with no issue. > > Then the commit message is completely wrong here. I disagree: The commit message is perhaps incomplete, but not wrong. The SMBnCTL3 register was specified as 8 bits wide in the datasheets of multiple chip generations, as far as I can tell, but the driver wrongly made a 32-bit access, which just happened not to blow up. So, indeed, "since the register is only 8-bit wide" seems to be a correct claim. > And provide necessary (no need to have noisy commit messages) > bits of the oops to show what's going on I guess it's blowing up now because SMBnCTL3 isn't 32-bit aligned (being at offset 0x0e in the controller). Jonathan
On Fri, Mar 4, 2022 at 10:42 PM Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote: > > Hello, > > On Thu, Mar 03, 2022 at 04:15:18PM +0200, Andy Shevchenko wrote: > > On Thu, Mar 03, 2022 at 02:54:27PM +0200, Tali Perry wrote: > > > > On Thu, Mar 03, 2022 at 04:31:38PM +0800, Tyrone Ting wrote: > > > > > From: Tyrone Ting <kfting@nuvoton.com> > > > > > > > > > > Use ioread8 instead of ioread32 to access the SMBnCTL3 register since > > > > > the register is only 8-bit wide. > > > > > > > > > Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver") > > > > > > > > No, this is bad commit message, since you have bitwise masks and there is > > > > nothing to fix from functional point of view. So, why is this a fix? > > > > > > > > > > The next gen of this device is a 64 bit cpu. > > > The module is and was 8 bit. > > > > > > The ioread32 that seemed to work smoothly on a 32 bit machine > > > was causing a panic on a 64 bit machine. > > > since the module is 8 bit we changed to ioread8. > > > This is working both for the 32 and 64 CPUs with no issue. > > > > Then the commit message is completely wrong here. > > I disagree: The commit message is perhaps incomplete, but not wrong. > The SMBnCTL3 register was specified as 8 bits wide in the datasheets of > multiple chip generations, as far as I can tell, but the driver wrongly > made a 32-bit access, which just happened not to blow up. > > So, indeed, "since the register is only 8-bit wide" seems to be a > correct claim. > > > And provide necessary (no need to have noisy commit messages) > > bits of the oops to show what's going on > > I guess it's blowing up now because SMBnCTL3 isn't 32-bit aligned > (being at offset 0x0e in the controller). > Hi Andy, After this clarification can you please acknowledge this specific patch? If you think there is a better way to describe this, can you propose one? > > Jonathan
On Tue, Mar 22, 2022 at 07:18:34PM +0200, Avi Fishman wrote: > On Fri, Mar 4, 2022 at 10:42 PM Jonathan Neuschäfer > <j.neuschaefer@gmx.net> wrote: > > > > Hello, > > > > On Thu, Mar 03, 2022 at 04:15:18PM +0200, Andy Shevchenko wrote: > > > On Thu, Mar 03, 2022 at 02:54:27PM +0200, Tali Perry wrote: > > > > > On Thu, Mar 03, 2022 at 04:31:38PM +0800, Tyrone Ting wrote: > > > > > > From: Tyrone Ting <kfting@nuvoton.com> > > > > > > > > > > > > Use ioread8 instead of ioread32 to access the SMBnCTL3 register since > > > > > > the register is only 8-bit wide. > > > > > > > > > > > Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver") > > > > > > > > > > No, this is bad commit message, since you have bitwise masks and there is > > > > > nothing to fix from functional point of view. So, why is this a fix? > > > > > > > > > > > > > The next gen of this device is a 64 bit cpu. > > > > The module is and was 8 bit. > > > > > > > > The ioread32 that seemed to work smoothly on a 32 bit machine > > > > was causing a panic on a 64 bit machine. > > > > since the module is 8 bit we changed to ioread8. > > > > This is working both for the 32 and 64 CPUs with no issue. > > > > > > Then the commit message is completely wrong here. > > > > I disagree: The commit message is perhaps incomplete, but not wrong. > > The SMBnCTL3 register was specified as 8 bits wide in the datasheets of > > multiple chip generations, as far as I can tell, but the driver wrongly > > made a 32-bit access, which just happened not to blow up. > > > > So, indeed, "since the register is only 8-bit wide" seems to be a > > correct claim. > > > > > And provide necessary (no need to have noisy commit messages) > > > bits of the oops to show what's going on > > > > I guess it's blowing up now because SMBnCTL3 isn't 32-bit aligned > > (being at offset 0x0e in the controller). > > > > Hi Andy, > After this clarification can you please acknowledge this specific patch? > If you think there is a better way to describe this, can you propose one? To be honest, I think it's probably best to include all the necessary explanations in the next version of this patch, i.e.: - That the register was always defined as 8-bit in the datasheets, and so the 32-bit access was always incorrect, but simply didn't cause a visible error - How the 32-bit access caused an error now, perhaps with a trimmed Oops log as Andy suggested Jonathan
diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c index 237da0ef32ca..66532c680338 100644 --- a/drivers/i2c/busses/i2c-npcm7xx.c +++ b/drivers/i2c/busses/i2c-npcm7xx.c @@ -360,14 +360,14 @@ static int npcm_i2c_get_SCL(struct i2c_adapter *_adap) { struct npcm_i2c *bus = container_of(_adap, struct npcm_i2c, adap); - return !!(I2CCTL3_SCL_LVL & ioread32(bus->reg + NPCM_I2CCTL3)); + return !!(I2CCTL3_SCL_LVL & ioread8(bus->reg + NPCM_I2CCTL3)); } static int npcm_i2c_get_SDA(struct i2c_adapter *_adap) { struct npcm_i2c *bus = container_of(_adap, struct npcm_i2c, adap); - return !!(I2CCTL3_SDA_LVL & ioread32(bus->reg + NPCM_I2CCTL3)); + return !!(I2CCTL3_SDA_LVL & ioread8(bus->reg + NPCM_I2CCTL3)); } static inline u16 npcm_i2c_get_index(struct npcm_i2c *bus)