From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wolfram Sang Date: Thu, 19 Nov 2015 15:56:40 +0000 Subject: [PATCH v3 00/11] i2c: rcar: tackle race conditions in the driver Message-Id: <1447948611-2615-1-git-send-email-wsa@the-dreams.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: linux-i2c@vger.kernel.org Cc: linux-sh@vger.kernel.org, Magnus Damm , Simon Horman , Laurent Pinchart , Geert Uytterhoeven , Wolfram Sang , Kuninori Morimoto , Yoshihiro Shimoda Hello RCar Fans! So, here is V3 of this series. After a debugging session with Laurent, we finally fixed his issue for good. It was not board dependent as we thought, but toolchain dependent! Hidden by a macro, the driver used a compound assignemt with a function call as the rvalue. After patch 6, this function also changed the flags which were to be changed by the compound assignment. Basically (after macro): priv->flags |= i_change_priv_flags(priv); Which is undefined behaviour, I guess. However, after my refactoring, the called functions always returned 0, so we can simply do: i_change_priv_flags(priv); Nasty one, but finally issue gone, for all toolchains and optimization settings. Furthermore, patch 11 has been added because HW engineers wanted it. The branch can be found here: git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/rcar-i2c-rework-v3 Please test, test, test :) Wolfram Changes since V2: * patch 6/11 was cleaned up to not use the compund assignment * patch 11/11 introduced as requested by HW engineers Changes since V1: * new patch 1/10 to ensure clock is always on * rebased patch 2/10 to the new patch * some patch descriptions slightly reworded Here is the description of the V1 series for those who missed it: Two issues people have seen with the i2c-rcar driver was: a) immediately restarted messages after NACK from client b) duplicated data bytes in messages Some people already worked on those and had a tough time because it was hard to reproduce these issues on non-customer setup. Luckily, I somewhen had a state where the first transfer after boot would always show the above issues on a plain Renesas Lager board. When measuring, I found a third issue thanks to my new tool 'i2ctransfer' (and thanks to projects like sigrok and OpenLogicSniffer, of course. Thank you very much!): c) after read message, no repeated start was sent, but stop + start. Due to some unlucky design choices in the IP core, it has some race windows which can cause problems if interrupts get delayed. Also, for every new message in one transfer, context switches between interrupt and process were needed. So I refactored the driver to setup new messages in interrupt context, too. This avoids the race for b) because we are now setting up the new message before we release the i2c bus clock (before we released the clock and set up the message in process context). c) is also fixed, this was not a race but a bug in the state handling. a) however is not fixed 100% :( We have the race window as small as possible now when utilizing interrupts, so it is an improvement and worked for my test cases well. There were experiments by me and Renesas engineers to use polling to prevent the issue but this caused other side effects, sadly. So, let's improve the situation now and let's see where we get. I did quite some lab testing here and also verified that slave support does not suffer from these changes. However, I'd really appreciate if people could give this real-world-testing which is always different. Please have a look, a test, etc... Thanks, Wolfram Wolfram Sang (11): i2c: rcar: make sure clocks are on when doing clock calculation i2c: rcar: rework hw init i2c: rcar: remove unused IOERROR state i2c: rcar: remove spinlock i2c: rcar: refactor setup of a msg i2c: rcar: init new messages in irq i2c: rcar: don't issue stop when HW does it automatically i2c: rcar: check master irqs before slave irqs i2c: rcar: revoke START request early i2c: rcar: clean up after refactoring i2c: rcar: handle difference in setting up non-first message drivers/i2c/busses/i2c-rcar.c | 249 ++++++++++++++++++------------------------ 1 file changed, 106 insertions(+), 143 deletions(-) -- 2.1.4 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wolfram Sang Subject: [PATCH v3 00/11] i2c: rcar: tackle race conditions in the driver Date: Thu, 19 Nov 2015 16:56:40 +0100 Message-ID: <1447948611-2615-1-git-send-email-wsa@the-dreams.de> Return-path: Sender: linux-sh-owner@vger.kernel.org To: linux-i2c@vger.kernel.org Cc: linux-sh@vger.kernel.org, Magnus Damm , Simon Horman , Laurent Pinchart , Geert Uytterhoeven , Wolfram Sang , Kuninori Morimoto , Yoshihiro Shimoda List-Id: linux-i2c@vger.kernel.org Hello RCar Fans! So, here is V3 of this series. After a debugging session with Laurent, we finally fixed his issue for good. It was not board dependent as we thought, but toolchain dependent! Hidden by a macro, the driver used a compound assignemt with a function call as the rvalue. After patch 6, this function also changed the flags which were to be changed by the compound assignment. Basically (after macro): priv->flags |= i_change_priv_flags(priv); Which is undefined behaviour, I guess. However, after my refactoring, the called functions always returned 0, so we can simply do: i_change_priv_flags(priv); Nasty one, but finally issue gone, for all toolchains and optimization settings. Furthermore, patch 11 has been added because HW engineers wanted it. The branch can be found here: git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/rcar-i2c-rework-v3 Please test, test, test :) Wolfram Changes since V2: * patch 6/11 was cleaned up to not use the compund assignment * patch 11/11 introduced as requested by HW engineers Changes since V1: * new patch 1/10 to ensure clock is always on * rebased patch 2/10 to the new patch * some patch descriptions slightly reworded Here is the description of the V1 series for those who missed it: Two issues people have seen with the i2c-rcar driver was: a) immediately restarted messages after NACK from client b) duplicated data bytes in messages Some people already worked on those and had a tough time because it was hard to reproduce these issues on non-customer setup. Luckily, I somewhen had a state where the first transfer after boot would always show the above issues on a plain Renesas Lager board. When measuring, I found a third issue thanks to my new tool 'i2ctransfer' (and thanks to projects like sigrok and OpenLogicSniffer, of course. Thank you very much!): c) after read message, no repeated start was sent, but stop + start. Due to some unlucky design choices in the IP core, it has some race windows which can cause problems if interrupts get delayed. Also, for every new message in one transfer, context switches between interrupt and process were needed. So I refactored the driver to setup new messages in interrupt context, too. This avoids the race for b) because we are now setting up the new message before we release the i2c bus clock (before we released the clock and set up the message in process context). c) is also fixed, this was not a race but a bug in the state handling. a) however is not fixed 100% :( We have the race window as small as possible now when utilizing interrupts, so it is an improvement and worked for my test cases well. There were experiments by me and Renesas engineers to use polling to prevent the issue but this caused other side effects, sadly. So, let's improve the situation now and let's see where we get. I did quite some lab testing here and also verified that slave support does not suffer from these changes. However, I'd really appreciate if people could give this real-world-testing which is always different. Please have a look, a test, etc... Thanks, Wolfram Wolfram Sang (11): i2c: rcar: make sure clocks are on when doing clock calculation i2c: rcar: rework hw init i2c: rcar: remove unused IOERROR state i2c: rcar: remove spinlock i2c: rcar: refactor setup of a msg i2c: rcar: init new messages in irq i2c: rcar: don't issue stop when HW does it automatically i2c: rcar: check master irqs before slave irqs i2c: rcar: revoke START request early i2c: rcar: clean up after refactoring i2c: rcar: handle difference in setting up non-first message drivers/i2c/busses/i2c-rcar.c | 249 ++++++++++++++++++------------------------ 1 file changed, 106 insertions(+), 143 deletions(-) -- 2.1.4