| Message ID | 20220816025111.3702045-1-wolfgang9277@126.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | i2c: designware: fix direct modify risk in xfer | expand |
Hi On 8/16/22 05:51, wolfgang9277@126.com wrote: > From: wolfgang huang <huangjinhui@kylinos.com> > > bind two or more slave devices to master device. What does this sentence mean? > while master device is process reading, dev->msgs > used as a buffer to process the read data, > while the other device uses i2c_dw_xfer to read/write, > this will directly change dev->msgs, causing the > first device reading process to crash or other confusion. > > so we should to check the device status before modifying > dev->msgs and others. > > [ 1244.815334] i2c_dw_isr+0x2c8/0x5e0 > [ 1244.819238] __handle_irq_event_percpu+0x5c/0x168 > [ 1244.824350] handle_irq_event_percpu+0x1c/0x58 > [ 1244.829201] handle_irq_event+0x40/0xa0 > [ 1244.833449] handle_fasteoi_irq+0xcc/0x1b0 > [ 1244.837956] generic_handle_irq+0x24/0x38 > [ 1244.842376] __handle_domain_irq+0x5c/0xb8 > [ 1244.846883] gic_handle_irq+0x94/0x1c8 > [ 1244.851043] el1_irq+0xb8/0x140 > [ 1244.854599] arch_cpu_idle+0x10/0x18 > [ 1244.858588] do_idle+0x19c/0x260 > [ 1244.862231] cpu_startup_entry+0x20/0x28 > I would like to understand this more. Like is the bus not busy wait move enough or if it's just masking the issue away. Are you able to share what kind of setup you have and do you know how to trigger this or does it occur randomly during run? > Signed-off-by: wolfgang huang <huangjinhui@kylinos.com> > --- > drivers/i2c/busses/i2c-designware-master.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c > index 44a94b225ed8..07f7d5e2d12d 100644 > --- a/drivers/i2c/busses/i2c-designware-master.c > +++ b/drivers/i2c/busses/i2c-designware-master.c > @@ -557,6 +557,14 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) > > pm_runtime_get_sync(dev->dev); > > + ret = i2c_dw_acquire_lock(dev); > + if (ret) > + goto done_nolock; > + > + ret = i2c_dw_wait_bus_not_busy(dev); > + if (ret) > + goto done; > + > /* > * Initiate I2C message transfer when AMD NAVI GPU card is enabled, > * As it is polling based transfer mechanism, which does not support Here a few lines below the lock is never released in case of MODEL_AMD_NAVI_GPU since code goes to "done_nolock" after returning from amd_i2c_dw_xfer_quirk(). Jarkko
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c index 44a94b225ed8..07f7d5e2d12d 100644 --- a/drivers/i2c/busses/i2c-designware-master.c +++ b/drivers/i2c/busses/i2c-designware-master.c @@ -557,6 +557,14 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) pm_runtime_get_sync(dev->dev); + ret = i2c_dw_acquire_lock(dev); + if (ret) + goto done_nolock; + + ret = i2c_dw_wait_bus_not_busy(dev); + if (ret) + goto done; + /* * Initiate I2C message transfer when AMD NAVI GPU card is enabled, * As it is polling based transfer mechanism, which does not support @@ -578,14 +586,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) dev->abort_source = 0; dev->rx_outstanding = 0; - ret = i2c_dw_acquire_lock(dev); - if (ret) - goto done_nolock; - - ret = i2c_dw_wait_bus_not_busy(dev); - if (ret < 0) - goto done; - /* Start the transfers */ i2c_dw_xfer_init(dev);