* [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).