diff mbox series

i2c: brcmstb: Add support for atomic transfers

Message ID 20231006144117.4079796-1-m.szyprowski@samsung.com
State New
Headers
Series i2c: brcmstb: Add support for atomic transfers |

Commit Message

Marek Szyprowski Oct. 6, 2023, 2:41 p.m. UTC 
  Add support for atomic transfers using polling mode with interrupts
intentionally disabled to get rid of the warning introduced by commit
63b96983a5dd ("i2c: core: introduce callbacks for atomic transfers")
during system reboot and power-off.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/i2c/busses/i2c-brcmstb.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

Comments

Florian Fainelli Oct. 9, 2023, 8:41 p.m. UTC | #1 
On 10/6/23 07:41, Marek Szyprowski wrote:
> Add support for atomic transfers using polling mode with interrupts
> intentionally disabled to get rid of the warning introduced by commit
> 63b96983a5dd ("i2c: core: introduce callbacks for atomic transfers")
> during system reboot and power-off.

Is there an existing system that you have access to which needs atomic 
transfer support, or is this a forward looking change?

> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Marek Szyprowski Oct. 11, 2023, 9:57 a.m. UTC | #2 
On 09.10.2023 22:41, Florian Fainelli wrote:
> On 10/6/23 07:41, Marek Szyprowski wrote:
>> Add support for atomic transfers using polling mode with interrupts
>> intentionally disabled to get rid of the warning introduced by commit
>> 63b96983a5dd ("i2c: core: introduce callbacks for atomic transfers")
>> during system reboot and power-off.
>
> Is there an existing system that you have access to which needs atomic 
> transfer support, or is this a forward looking change?

Frankly speaking I've observed the mentioned warning during system 
reboot on RaspberryPi4 with linux-next kernel compiled from 
multi_v7_defconfig. It looks that this driver is used by VC4 DRM for 
DDC. This issue doesn't look critical, but the fix seems to be trivial, 
thus my patch.


Best regards
Gregor Riepl Oct. 11, 2023, 10:23 a.m. UTC | #3 
Hi,

I admit that I don't understand the I²C subsystem very well, but doesn't 
this introduce a potential race condition?

 > ...
 > @@ -240,7 +241,7 @@ static int brcmstb_i2c_wait_for_completion(struct 
brcmstb_i2c_dev *dev)
 > ...
> -	if (dev->irq >= 0) {
> +	if (dev->irq >= 0 && !dev->atomic) {
 > ...
 > @@ -287,7 +288,7 @@ static int brcmstb_send_i2c_cmd(struct 
brcmstb_i2c_dev *dev,
 > ...
> -	if (dev->irq >= 0)
> +	if (dev->irq >= 0 && !dev->atomic)
 > ...
 > +static int brcmstb_i2c_xfer_atomic(struct i2c_adapter *adapter,
 > +				   struct i2c_msg msgs[], int num)
 > ...
> +	dev->atomic = true;
> +	ret = brcmstb_i2c_xfer(adapter, msgs, num);
> +	dev->atomic = false;
> ...

What happens when one of the if() branches is taken in one thread while 
another thread is just executing the assignment of the atomic flag? My 
expectation would be that the first tread still sees the old flag value 
and happily executes the branch, while brcmstb_i2c_xfer_atomic() sets 
the flag just after and initiates a transfer.

I'd expect that access to the flag must be atomic as well, so maybe 
something like 
https://www.kernel.org/doc/html/latest/core-api/wrappers/atomic_t.html 
is needed, or some other synchronization mechanism.

Or is it guaranteed that brcmstb_i2c_wait_for_completion() and 
brcmstb_send_i2c_cmd() can only be called from the same thread as 
brcmstb_i2c_xfer_atomic() ?

Regards,
Gregor
Marek Szyprowski Oct. 11, 2023, 11:47 a.m. UTC | #4 
On 11.10.2023 12:23, Gregor Riepl wrote:
> I admit that I don't understand the I²C subsystem very well, but 
> doesn't this introduce a potential race condition?
>
> > ...
> > @@ -240,7 +241,7 @@ static int 
> brcmstb_i2c_wait_for_completion(struct brcmstb_i2c_dev *dev)
> > ...
>> -    if (dev->irq >= 0) {
>> +    if (dev->irq >= 0 && !dev->atomic) {
> > ...
> > @@ -287,7 +288,7 @@ static int brcmstb_send_i2c_cmd(struct 
> brcmstb_i2c_dev *dev,
> > ...
>> -    if (dev->irq >= 0)
>> +    if (dev->irq >= 0 && !dev->atomic)
> > ...
> > +static int brcmstb_i2c_xfer_atomic(struct i2c_adapter *adapter,
> > +                   struct i2c_msg msgs[], int num)
> > ...
>> +    dev->atomic = true;
>> +    ret = brcmstb_i2c_xfer(adapter, msgs, num);
>> +    dev->atomic = false;
>> ...
>
> What happens when one of the if() branches is taken in one thread 
> while another thread is just executing the assignment of the atomic 
> flag? My expectation would be that the first tread still sees the old 
> flag value and happily executes the branch, while 
> brcmstb_i2c_xfer_atomic() sets the flag just after and initiates a 
> transfer.
>
> I'd expect that access to the flag must be atomic as well, so maybe 
> something like 
> https://www.kernel.org/doc/html/latest/core-api/wrappers/atomic_t.html 
> is needed, or some other synchronization mechanism.
>
> Or is it guaranteed that brcmstb_i2c_wait_for_completion() and 
> brcmstb_send_i2c_cmd() can only be called from the same thread as 
> brcmstb_i2c_xfer_atomic() ?

Atomic i2c transfers are some kind of a special case.

I guess that i2c core takes care of NOT multiplexing atomic and standard 
i2c transfers. No special locking/protection is needed in the bus 
drivers. This is at least what I see from commits like 08960b022fb6 
("i2c: tegra-bpmp: convert to use new atomic callbacks") or 3d11a12ece85 
("i2c: ocores: enable atomic xfers").

Best regards
Andi Shyti Oct. 11, 2023, 1:40 p.m. UTC | #5 
Hi Marek,

On Fri, Oct 06, 2023 at 04:41:17PM +0200, Marek Szyprowski wrote:
> Add support for atomic transfers using polling mode with interrupts
> intentionally disabled to get rid of the warning introduced by commit
> 63b96983a5dd ("i2c: core: introduce callbacks for atomic transfers")
> during system reboot and power-off.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

Reviewed-by: Andi Shyti <andi.shyti@kernel.org> 

Andi
Wolfram Sang Oct. 11, 2023, 2:48 p.m. UTC | #6 
> I guess that i2c core takes care of NOT multiplexing atomic and standard 
> i2c transfers.

Atomic transfers are only used iff the system is in a certain state,
check i2c_in_atomic_xfer_mode(). Then and only then, transfers are
atomic. All of them. Neither bus drivers nor clients can choose them.
Florian Fainelli Oct. 11, 2023, 3:43 p.m. UTC | #7 
On 10/11/2023 2:57 AM, Marek Szyprowski wrote:
> On 09.10.2023 22:41, Florian Fainelli wrote:
>> On 10/6/23 07:41, Marek Szyprowski wrote:
>>> Add support for atomic transfers using polling mode with interrupts
>>> intentionally disabled to get rid of the warning introduced by commit
>>> 63b96983a5dd ("i2c: core: introduce callbacks for atomic transfers")
>>> during system reboot and power-off.
>>
>> Is there an existing system that you have access to which needs atomic
>> transfer support, or is this a forward looking change?
> 
> Frankly speaking I've observed the mentioned warning during system
> reboot on RaspberryPi4 with linux-next kernel compiled from
> multi_v7_defconfig. It looks that this driver is used by VC4 DRM for
> DDC. This issue doesn't look critical, but the fix seems to be trivial,
> thus my patch.

Makes sense, thanks for confirming this fixes an actual issue you have 
seen and providing a fix for it.
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-brcmstb.c b/drivers/i2c/busses/i2c-brcmstb.c
index acee76732544..38f276c99193 100644
--- a/drivers/i2c/busses/i2c-brcmstb.c
+++ b/drivers/i2c/busses/i2c-brcmstb.c
@@ -160,6 +160,7 @@  struct brcmstb_i2c_dev {
 	struct completion done;
 	u32 clk_freq_hz;
 	int data_regsz;
+	bool atomic;
 };
 
 /* register accessors for both be and le cpu arch */
@@ -240,7 +241,7 @@  static int brcmstb_i2c_wait_for_completion(struct brcmstb_i2c_dev *dev)
 	int ret = 0;
 	unsigned long timeout = msecs_to_jiffies(I2C_TIMEOUT);
 
-	if (dev->irq >= 0) {
+	if (dev->irq >= 0 && !dev->atomic) {
 		if (!wait_for_completion_timeout(&dev->done, timeout))
 			ret = -ETIMEDOUT;
 	} else {
@@ -287,7 +288,7 @@  static int brcmstb_send_i2c_cmd(struct brcmstb_i2c_dev *dev,
 		return rc;
 
 	/* only if we are in interrupt mode */
-	if (dev->irq >= 0)
+	if (dev->irq >= 0 && !dev->atomic)
 		reinit_completion(&dev->done);
 
 	/* enable BSC CTL interrupt line */
@@ -520,6 +521,23 @@  static int brcmstb_i2c_xfer(struct i2c_adapter *adapter,
 
 }
 
+static int brcmstb_i2c_xfer_atomic(struct i2c_adapter *adapter,
+				   struct i2c_msg msgs[], int num)
+{
+	struct brcmstb_i2c_dev *dev = i2c_get_adapdata(adapter);
+	int ret;
+
+	if (dev->irq >= 0)
+		disable_irq(dev->irq);
+	dev->atomic = true;
+	ret = brcmstb_i2c_xfer(adapter, msgs, num);
+	dev->atomic = false;
+	if (dev->irq >= 0)
+		enable_irq(dev->irq);
+
+	return ret;
+}
+
 static u32 brcmstb_i2c_functionality(struct i2c_adapter *adap)
 {
 	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_10BIT_ADDR
@@ -528,6 +546,7 @@  static u32 brcmstb_i2c_functionality(struct i2c_adapter *adap)
 
 static const struct i2c_algorithm brcmstb_i2c_algo = {
 	.master_xfer = brcmstb_i2c_xfer,
+	.master_xfer_atomic = brcmstb_i2c_xfer_atomic,
 	.functionality = brcmstb_i2c_functionality,
 };