[PATCH v2] i2c: bcm2835: Clear current buffer pointers and counts after a transfer

[PATCH v2] i2c: bcm2835: Clear current buffer pointers and counts after a transfer

[Paul Kocialkowski]

The driver's interrupt handler checks whether a message is currently
being handled with the curr_msg pointer. When it is NULL, the interrupt
is considered to be unexpected. Similarly, the i2c_start_transfer
routine checks for the remaining number of messages to handle in
num_msgs.

However, these values are never cleared and always keep the message and
number relevant to the latest transfer (which might be done already and
the underlying message memory might have been freed).

When an unexpected interrupt hits with the DONE bit set, the isr will
then try to access the flags field of the curr_msg structure, leading
to a fatal page fault.

The msg_buf and msg_buf_remaining fields are also never cleared at the
end of the transfer, which can lead to similar pitfalls.

Fix these issues by introducing a cleanup function and always calling
it after a transfer is finished.

Fixes: e2474541032d ("i2c: bcm2835: Fix hang for writing messages larger than 16 bytes")
Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---

Changes since v1:
* Added cleanup of msg_buf and msg_buf_remaining;
* Moved cleanups to a dedicated function;
* Performed cleanup on timeout as well;
* Added Fixes tag.

 drivers/i2c/busses/i2c-bcm2835.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index 44deae78913e..4d19254f78c8 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -191,6 +191,15 @@ static void bcm2835_i2c_start_transfer(struct bcm2835_i2c_dev *i2c_dev)
 	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, c);
 }
 
+static void bcm2835_i2c_finish_transfer(struct bcm2835_i2c_dev *i2c_dev)
+{
+	i2c_dev->curr_msg = NULL;
+	i2c_dev->num_msgs = 0;
+
+	i2c_dev->msg_buf = NULL;
+	i2c_dev->msg_buf_remaining = 0;
+}
+
 /*
  * Note about I2C_C_CLEAR on error:
  * The I2C_C_CLEAR on errors will take some time to resolve -- if you were in
@@ -291,6 +300,9 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
 
 	time_left = wait_for_completion_timeout(&i2c_dev->completion,
 						adap->timeout);
+
+	bcm2835_i2c_finish_transfer(i2c_dev);
+
 	if (!time_left) {
 		bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C,
 				   BCM2835_I2C_C_CLEAR);
-- 
2.20.1

[PATCH v2] i2c: bcm2835: Clear current buffer pointers and counts after a transfer

[Paul Kocialkowski]

The driver's interrupt handler checks whether a message is currently
being handled with the curr_msg pointer. When it is NULL, the interrupt
is considered to be unexpected. Similarly, the i2c_start_transfer
routine checks for the remaining number of messages to handle in
num_msgs.

However, these values are never cleared and always keep the message and
number relevant to the latest transfer (which might be done already and
the underlying message memory might have been freed).

When an unexpected interrupt hits with the DONE bit set, the isr will
then try to access the flags field of the curr_msg structure, leading
to a fatal page fault.

The msg_buf and msg_buf_remaining fields are also never cleared at the
end of the transfer, which can lead to similar pitfalls.

Fix these issues by introducing a cleanup function and always calling
it after a transfer is finished.

Fixes: e2474541032d ("i2c: bcm2835: Fix hang for writing messages larger than 16 bytes")
Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---

Changes since v1:
* Added cleanup of msg_buf and msg_buf_remaining;
* Moved cleanups to a dedicated function;
* Performed cleanup on timeout as well;
* Added Fixes tag.

 drivers/i2c/busses/i2c-bcm2835.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index 44deae78913e..4d19254f78c8 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -191,6 +191,15 @@ static void bcm2835_i2c_start_transfer(struct bcm2835_i2c_dev *i2c_dev)
 	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, c);
 }
 
+static void bcm2835_i2c_finish_transfer(struct bcm2835_i2c_dev *i2c_dev)
+{
+	i2c_dev->curr_msg = NULL;
+	i2c_dev->num_msgs = 0;
+
+	i2c_dev->msg_buf = NULL;
+	i2c_dev->msg_buf_remaining = 0;
+}
+
 /*
  * Note about I2C_C_CLEAR on error:
  * The I2C_C_CLEAR on errors will take some time to resolve -- if you were in
@@ -291,6 +300,9 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
 
 	time_left = wait_for_completion_timeout(&i2c_dev->completion,
 						adap->timeout);
+
+	bcm2835_i2c_finish_transfer(i2c_dev);
+
 	if (!time_left) {
 		bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C,
 				   BCM2835_I2C_C_CLEAR);
-- 
2.20.1


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

Re: [PATCH v2] i2c: bcm2835: Clear current buffer pointers and counts after a transfer

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1219 bytes --]

On Thu, Dec 27, 2018 at 04:42:25PM +0100, Paul Kocialkowski wrote:
> The driver's interrupt handler checks whether a message is currently
> being handled with the curr_msg pointer. When it is NULL, the interrupt
> is considered to be unexpected. Similarly, the i2c_start_transfer
> routine checks for the remaining number of messages to handle in
> num_msgs.
> 
> However, these values are never cleared and always keep the message and
> number relevant to the latest transfer (which might be done already and
> the underlying message memory might have been freed).
> 
> When an unexpected interrupt hits with the DONE bit set, the isr will
> then try to access the flags field of the curr_msg structure, leading
> to a fatal page fault.
> 
> The msg_buf and msg_buf_remaining fields are also never cleared at the
> end of the transfer, which can lead to similar pitfalls.
> 
> Fix these issues by introducing a cleanup function and always calling
> it after a transfer is finished.
> 
> Fixes: e2474541032d ("i2c: bcm2835: Fix hang for writing messages larger than 16 bytes")
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Stefan, Florian, any comment about this patch?


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

Re: [PATCH v2] i2c: bcm2835: Clear current buffer pointers and counts after a transfer

[Wolfram Sang]

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

On Thu, Dec 27, 2018 at 04:42:25PM +0100, Paul Kocialkowski wrote:
> The driver's interrupt handler checks whether a message is currently
> being handled with the curr_msg pointer. When it is NULL, the interrupt
> is considered to be unexpected. Similarly, the i2c_start_transfer
> routine checks for the remaining number of messages to handle in
> num_msgs.
> 
> However, these values are never cleared and always keep the message and
> number relevant to the latest transfer (which might be done already and
> the underlying message memory might have been freed).
> 
> When an unexpected interrupt hits with the DONE bit set, the isr will
> then try to access the flags field of the curr_msg structure, leading
> to a fatal page fault.
> 
> The msg_buf and msg_buf_remaining fields are also never cleared at the
> end of the transfer, which can lead to similar pitfalls.
> 
> Fix these issues by introducing a cleanup function and always calling
> it after a transfer is finished.
> 
> Fixes: e2474541032d ("i2c: bcm2835: Fix hang for writing messages larger than 16 bytes")
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Stefan, Florian, any comment about this patch?


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

Re: [PATCH v2] i2c: bcm2835: Clear current buffer pointers and counts after a transfer

[Stefan Wahren]

Am 05.02.19 um 13:09 schrieb Wolfram Sang:
> On Thu, Dec 27, 2018 at 04:42:25PM +0100, Paul Kocialkowski wrote:
>> The driver's interrupt handler checks whether a message is currently
>> being handled with the curr_msg pointer. When it is NULL, the interrupt
>> is considered to be unexpected. Similarly, the i2c_start_transfer
>> routine checks for the remaining number of messages to handle in
>> num_msgs.
>>
>> However, these values are never cleared and always keep the message and
>> number relevant to the latest transfer (which might be done already and
>> the underlying message memory might have been freed).
>>
>> When an unexpected interrupt hits with the DONE bit set, the isr will
>> then try to access the flags field of the curr_msg structure, leading
>> to a fatal page fault.
>>
>> The msg_buf and msg_buf_remaining fields are also never cleared at the
>> end of the transfer, which can lead to similar pitfalls.
>>
>> Fix these issues by introducing a cleanup function and always calling
>> it after a transfer is finished.
>>
>> Fixes: e2474541032d ("i2c: bcm2835: Fix hang for writing messages larger than 16 bytes")
>> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> Stefan, Florian, any comment about this patch?

Acked-by: Stefan Wahren <stefan.wahren@i2se.com>

Thanks

Re: [PATCH v2] i2c: bcm2835: Clear current buffer pointers and counts after a transfer

[Stefan Wahren]

Am 05.02.19 um 13:09 schrieb Wolfram Sang:
> On Thu, Dec 27, 2018 at 04:42:25PM +0100, Paul Kocialkowski wrote:
>> The driver's interrupt handler checks whether a message is currently
>> being handled with the curr_msg pointer. When it is NULL, the interrupt
>> is considered to be unexpected. Similarly, the i2c_start_transfer
>> routine checks for the remaining number of messages to handle in
>> num_msgs.
>>
>> However, these values are never cleared and always keep the message and
>> number relevant to the latest transfer (which might be done already and
>> the underlying message memory might have been freed).
>>
>> When an unexpected interrupt hits with the DONE bit set, the isr will
>> then try to access the flags field of the curr_msg structure, leading
>> to a fatal page fault.
>>
>> The msg_buf and msg_buf_remaining fields are also never cleared at the
>> end of the transfer, which can lead to similar pitfalls.
>>
>> Fix these issues by introducing a cleanup function and always calling
>> it after a transfer is finished.
>>
>> Fixes: e2474541032d ("i2c: bcm2835: Fix hang for writing messages larger than 16 bytes")
>> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> Stefan, Florian, any comment about this patch?

Acked-by: Stefan Wahren <stefan.wahren@i2se.com>

Thanks


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

Re: [PATCH v2] i2c: bcm2835: Clear current buffer pointers and counts after a transfer

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1204 bytes --]

On Thu, Dec 27, 2018 at 04:42:25PM +0100, Paul Kocialkowski wrote:
> The driver's interrupt handler checks whether a message is currently
> being handled with the curr_msg pointer. When it is NULL, the interrupt
> is considered to be unexpected. Similarly, the i2c_start_transfer
> routine checks for the remaining number of messages to handle in
> num_msgs.
> 
> However, these values are never cleared and always keep the message and
> number relevant to the latest transfer (which might be done already and
> the underlying message memory might have been freed).
> 
> When an unexpected interrupt hits with the DONE bit set, the isr will
> then try to access the flags field of the curr_msg structure, leading
> to a fatal page fault.
> 
> The msg_buf and msg_buf_remaining fields are also never cleared at the
> end of the transfer, which can lead to similar pitfalls.
> 
> Fix these issues by introducing a cleanup function and always calling
> it after a transfer is finished.
> 
> Fixes: e2474541032d ("i2c: bcm2835: Fix hang for writing messages larger than 16 bytes")
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Applied to for-current, thanks!


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

Re: [PATCH v2] i2c: bcm2835: Clear current buffer pointers and counts after a transfer

[Wolfram Sang]

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

On Thu, Dec 27, 2018 at 04:42:25PM +0100, Paul Kocialkowski wrote:
> The driver's interrupt handler checks whether a message is currently
> being handled with the curr_msg pointer. When it is NULL, the interrupt
> is considered to be unexpected. Similarly, the i2c_start_transfer
> routine checks for the remaining number of messages to handle in
> num_msgs.
> 
> However, these values are never cleared and always keep the message and
> number relevant to the latest transfer (which might be done already and
> the underlying message memory might have been freed).
> 
> When an unexpected interrupt hits with the DONE bit set, the isr will
> then try to access the flags field of the curr_msg structure, leading
> to a fatal page fault.
> 
> The msg_buf and msg_buf_remaining fields are also never cleared at the
> end of the transfer, which can lead to similar pitfalls.
> 
> Fix these issues by introducing a cleanup function and always calling
> it after a transfer is finished.
> 
> Fixes: e2474541032d ("i2c: bcm2835: Fix hang for writing messages larger than 16 bytes")
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Applied to for-current, thanks!


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