| Message ID | 20211222094558.2098791-2-jsd@semihalf.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c-designware: Add support for AMD PSP semaphore | expand |
On 12/22/21 11:45, Jan Dabros wrote: > All accesses to controller's registers should be protected on > probe, disable and xfer paths. This is needed for i2c bus controllers > that are shared with but not controlled by kernel. > > Signed-off-by: Jan Dabros <jsd@semihalf.com> > --- > drivers/i2c/busses/i2c-designware-common.c | 12 ++++++++++++ > drivers/i2c/busses/i2c-designware-master.c | 6 ++++++ > 2 files changed, 18 insertions(+) > Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
On Wed, Dec 22, 2021 at 10:45:57AM +0100, Jan Dabros wrote: > All accesses to controller's registers should be protected on > probe, disable and xfer paths. This is needed for i2c bus controllers > that are shared with but not controlled by kernel. ... > + ret = i2c_dw_acquire_lock(dev); > + if (ret) > + return ret; > + > ret = regmap_read(dev->map, DW_IC_COMP_PARAM_1, ¶m); > if (ret) > return ret; > + i2c_dw_release_lock(dev); Not sure this part is fully correct. Please, fix the leakage. ... > + ret = i2c_dw_acquire_lock(dev); > + if (ret) > + return; > > /* Disable controller */ > __i2c_dw_disable(dev); > @@ -614,6 +624,8 @@ void i2c_dw_disable(struct dw_i2c_dev *dev) > /* Disable all interrupts */ > regmap_write(dev->map, DW_IC_INTR_MASK, 0); > regmap_read(dev->map, DW_IC_CLR_INTR, &dummy); > + i2c_dw_release_lock(dev); Not enough context here, bu I believe there is the same issue(s).
czw., 23 gru 2021 o 16:51 Andy Shevchenko <andriy.shevchenko@linux.intel.com> napisał(a): > > On Wed, Dec 22, 2021 at 10:45:57AM +0100, Jan Dabros wrote: > > All accesses to controller's registers should be protected on > > probe, disable and xfer paths. This is needed for i2c bus controllers > > that are shared with but not controlled by kernel. > > ... > > > + ret = i2c_dw_acquire_lock(dev); > > + if (ret) > > + return ret; > > + > > ret = regmap_read(dev->map, DW_IC_COMP_PARAM_1, ¶m); > > if (ret) > > return ret; > > + i2c_dw_release_lock(dev); > > Not sure this part is fully correct. Please, fix the leakage. Correct, I will move release() right below regmap_read() call, before checking return code. > > ... > > > + ret = i2c_dw_acquire_lock(dev); > > + if (ret) > > + return; > > > > /* Disable controller */ > > __i2c_dw_disable(dev); > > @@ -614,6 +624,8 @@ void i2c_dw_disable(struct dw_i2c_dev *dev) > > /* Disable all interrupts */ > > regmap_write(dev->map, DW_IC_INTR_MASK, 0); > > regmap_read(dev->map, DW_IC_CLR_INTR, &dummy); > > > + i2c_dw_release_lock(dev); > > Not enough context here, bu I believe there is the same issue(s). Since we are ignoring values returned by regmap_write()/read() and also __i2c_dw_disable() returns void I don't see a leakage here. Don't have more function calls here.
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c index bf2a4920638a..3e7ddae446a0 100644 --- a/drivers/i2c/busses/i2c-designware-common.c +++ b/drivers/i2c/busses/i2c-designware-common.c @@ -578,9 +578,14 @@ int i2c_dw_set_fifo_size(struct dw_i2c_dev *dev) * Try to detect the FIFO depth if not set by interface driver, * the depth could be from 2 to 256 from HW spec. */ + ret = i2c_dw_acquire_lock(dev); + if (ret) + return ret; + ret = regmap_read(dev->map, DW_IC_COMP_PARAM_1, ¶m); if (ret) return ret; + i2c_dw_release_lock(dev); tx_fifo_depth = ((param >> 16) & 0xff) + 1; rx_fifo_depth = ((param >> 8) & 0xff) + 1; @@ -607,6 +612,11 @@ u32 i2c_dw_func(struct i2c_adapter *adap) void i2c_dw_disable(struct dw_i2c_dev *dev) { u32 dummy; + int ret; + + ret = i2c_dw_acquire_lock(dev); + if (ret) + return; /* Disable controller */ __i2c_dw_disable(dev); @@ -614,6 +624,8 @@ void i2c_dw_disable(struct dw_i2c_dev *dev) /* Disable all interrupts */ regmap_write(dev->map, DW_IC_INTR_MASK, 0); regmap_read(dev->map, DW_IC_CLR_INTR, &dummy); + + i2c_dw_release_lock(dev); } void i2c_dw_disable_int(struct dw_i2c_dev *dev) diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c index 9b08bb5df38d..3faef28a9ba9 100644 --- a/drivers/i2c/busses/i2c-designware-master.c +++ b/drivers/i2c/busses/i2c-designware-master.c @@ -902,7 +902,13 @@ int i2c_dw_probe_master(struct dw_i2c_dev *dev) irq_flags = IRQF_SHARED | IRQF_COND_SUSPEND; } + ret = i2c_dw_acquire_lock(dev); + if (ret) + return ret; + i2c_dw_disable_int(dev); + i2c_dw_release_lock(dev); + ret = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr, irq_flags, dev_name(dev->dev), dev); if (ret) {
All accesses to controller's registers should be protected on probe, disable and xfer paths. This is needed for i2c bus controllers that are shared with but not controlled by kernel. Signed-off-by: Jan Dabros <jsd@semihalf.com> --- drivers/i2c/busses/i2c-designware-common.c | 12 ++++++++++++ drivers/i2c/busses/i2c-designware-master.c | 6 ++++++ 2 files changed, 18 insertions(+)