linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] i2c: brcmstb: Add support for atomic transfers
       [not found] <CGME20231006144123eucas1p111cbbdbd70927ffbd697f7edf6b7ae1c@eucas1p1.samsung.com>
@ 2023-10-06 14:41 ` Marek Szyprowski
  2023-10-09 20:41   ` Florian Fainelli
                     ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Marek Szyprowski @ 2023-10-06 14:41 UTC (permalink / raw)
  To: linux-i2c, linux-arm-kernel, linux-kernel
  Cc: Marek Szyprowski, Kamal Dasu,
	Broadcom internal kernel review list, Andi Shyti,
	Florian Fainelli, Wolfram Sang

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(-)

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,
 };
 
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] i2c: brcmstb: Add support for atomic transfers
  2023-10-06 14:41 ` [PATCH] i2c: brcmstb: Add support for atomic transfers Marek Szyprowski
@ 2023-10-09 20:41   ` Florian Fainelli
  2023-10-11  9:57     ` Marek Szyprowski
  2023-10-11 10:23   ` Gregor Riepl
  2023-10-11 13:40   ` Andi Shyti
  2 siblings, 1 reply; 8+ messages in thread
From: Florian Fainelli @ 2023-10-09 20:41 UTC (permalink / raw)
  To: Marek Szyprowski, linux-i2c, linux-arm-kernel, linux-kernel
  Cc: Kamal Dasu, Broadcom internal kernel review list, Andi Shyti,
	Wolfram Sang


[-- Attachment #1.1: Type: text/plain, Size: 562 bytes --]

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>
-- 
Florian


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] i2c: brcmstb: Add support for atomic transfers
  2023-10-09 20:41   ` Florian Fainelli
@ 2023-10-11  9:57     ` Marek Szyprowski
  2023-10-11 15:43       ` Florian Fainelli
  0 siblings, 1 reply; 8+ messages in thread
From: Marek Szyprowski @ 2023-10-11  9:57 UTC (permalink / raw)
  To: Florian Fainelli, linux-i2c, linux-arm-kernel, linux-kernel
  Cc: Kamal Dasu, Broadcom internal kernel review list, Andi Shyti,
	Wolfram Sang

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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] i2c: brcmstb: Add support for atomic transfers
  2023-10-06 14:41 ` [PATCH] i2c: brcmstb: Add support for atomic transfers Marek Szyprowski
  2023-10-09 20:41   ` Florian Fainelli
@ 2023-10-11 10:23   ` Gregor Riepl
  2023-10-11 11:47     ` Marek Szyprowski
  2023-10-11 13:40   ` Andi Shyti
  2 siblings, 1 reply; 8+ messages in thread
From: Gregor Riepl @ 2023-10-11 10:23 UTC (permalink / raw)
  To: Marek Szyprowski, linux-i2c, linux-arm-kernel, linux-kernel
  Cc: Kamal Dasu, Broadcom internal kernel review list, Andi Shyti,
	Florian Fainelli, Wolfram Sang

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] i2c: brcmstb: Add support for atomic transfers
  2023-10-11 10:23   ` Gregor Riepl
@ 2023-10-11 11:47     ` Marek Szyprowski
  2023-10-11 14:48       ` Wolfram Sang
  0 siblings, 1 reply; 8+ messages in thread
From: Marek Szyprowski @ 2023-10-11 11:47 UTC (permalink / raw)
  To: Gregor Riepl, linux-i2c, linux-arm-kernel, linux-kernel
  Cc: Kamal Dasu, Broadcom internal kernel review list, Andi Shyti,
	Florian Fainelli, Wolfram Sang

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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] i2c: brcmstb: Add support for atomic transfers
  2023-10-06 14:41 ` [PATCH] i2c: brcmstb: Add support for atomic transfers Marek Szyprowski
  2023-10-09 20:41   ` Florian Fainelli
  2023-10-11 10:23   ` Gregor Riepl
@ 2023-10-11 13:40   ` Andi Shyti
  2 siblings, 0 replies; 8+ messages in thread
From: Andi Shyti @ 2023-10-11 13:40 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: linux-i2c, linux-arm-kernel, linux-kernel, Kamal Dasu,
	Broadcom internal kernel review list, Florian Fainelli,
	Wolfram Sang

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] i2c: brcmstb: Add support for atomic transfers
  2023-10-11 11:47     ` Marek Szyprowski
@ 2023-10-11 14:48       ` Wolfram Sang
  0 siblings, 0 replies; 8+ messages in thread
From: Wolfram Sang @ 2023-10-11 14:48 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: Gregor Riepl, linux-i2c, linux-arm-kernel, linux-kernel,
	Kamal Dasu, Broadcom internal kernel review list, Andi Shyti,
	Florian Fainelli


[-- Attachment #1.1: Type: text/plain, Size: 310 bytes --]


> 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.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] i2c: brcmstb: Add support for atomic transfers
  2023-10-11  9:57     ` Marek Szyprowski
@ 2023-10-11 15:43       ` Florian Fainelli
  0 siblings, 0 replies; 8+ messages in thread
From: Florian Fainelli @ 2023-10-11 15:43 UTC (permalink / raw)
  To: Marek Szyprowski, linux-i2c, linux-arm-kernel, linux-kernel
  Cc: Kamal Dasu, Broadcom internal kernel review list, Andi Shyti,
	Wolfram Sang


[-- Attachment #1.1: Type: text/plain, Size: 949 bytes --]



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.
-- 
Florian

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-10-11 15:44 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <CGME20231006144123eucas1p111cbbdbd70927ffbd697f7edf6b7ae1c@eucas1p1.samsung.com>
2023-10-06 14:41 ` [PATCH] i2c: brcmstb: Add support for atomic transfers Marek Szyprowski
2023-10-09 20:41   ` Florian Fainelli
2023-10-11  9:57     ` Marek Szyprowski
2023-10-11 15:43       ` Florian Fainelli
2023-10-11 10:23   ` Gregor Riepl
2023-10-11 11:47     ` Marek Szyprowski
2023-10-11 14:48       ` Wolfram Sang
2023-10-11 13:40   ` Andi Shyti

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).