From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> To: Wolfram Sang <wsa@the-dreams.de> Cc: linux-i2c@vger.kernel.org, linux-sh@vger.kernel.org, Magnus Damm <magnus.damm@gmail.com>, Simon Horman <horms@verge.net.au>, Geert Uytterhoeven <geert@linux-m68k.org>, Kuninori Morimoto <kuninori.morimoto.gx@gmail.com>, Yoshihiro Kaneko <ykaneko0929@gmail.com>, Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> Subject: Re: [PATCH 0/9] i2c: rcar: tackle race conditions in the driver Date: Thu, 03 Sep 2015 20:35:01 +0000 [thread overview] Message-ID: <5464456.UnsMOS3MTx@avalon> (raw) In-Reply-To: <1441311613-2681-1-git-send-email-wsa@the-dreams.de> Hi Wolfram, On Thursday 03 September 2015 22:20:04 Wolfram Sang wrote: > Hello RCar Fans! > > 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). Could this fix the HDMI EDID read issue on Koelsch ? > 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. Does that mean that, due to hardware design, it's impossible to use I2C interrupts in a race-free way ? It would be interesting to document why in a commit log message, or possibly in the code itself. > 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 (9): > 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 > > drivers/i2c/busses/i2c-rcar.c | 193 +++++++++++++++------------------------ > 1 file changed, 71 insertions(+), 122 deletions(-) -- Regards, Laurent Pinchart
WARNING: multiple messages have this Message-ID (diff)
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> To: Wolfram Sang <wsa@the-dreams.de> Cc: linux-i2c@vger.kernel.org, linux-sh@vger.kernel.org, Magnus Damm <magnus.damm@gmail.com>, Simon Horman <horms@verge.net.au>, Geert Uytterhoeven <geert@linux-m68k.org>, Kuninori Morimoto <kuninori.morimoto.gx@gmail.com>, Yoshihiro Kaneko <ykaneko0929@gmail.com>, Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> Subject: Re: [PATCH 0/9] i2c: rcar: tackle race conditions in the driver Date: Thu, 03 Sep 2015 23:35:01 +0300 [thread overview] Message-ID: <5464456.UnsMOS3MTx@avalon> (raw) In-Reply-To: <1441311613-2681-1-git-send-email-wsa@the-dreams.de> Hi Wolfram, On Thursday 03 September 2015 22:20:04 Wolfram Sang wrote: > Hello RCar Fans! > > 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). Could this fix the HDMI EDID read issue on Koelsch ? > 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. Does that mean that, due to hardware design, it's impossible to use I2C interrupts in a race-free way ? It would be interesting to document why in a commit log message, or possibly in the code itself. > 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 (9): > 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 > > drivers/i2c/busses/i2c-rcar.c | 193 +++++++++++++++------------------------ > 1 file changed, 71 insertions(+), 122 deletions(-) -- Regards, Laurent Pinchart
next prev parent reply other threads:[~2015-09-03 20:35 UTC|newest] Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-09-03 20:20 [PATCH 0/9] i2c: rcar: tackle race conditions in the driver Wolfram Sang 2015-09-03 20:20 ` Wolfram Sang 2015-09-03 20:20 ` [PATCH 1/9] i2c: rcar: rework hw init Wolfram Sang 2015-09-03 20:20 ` Wolfram Sang 2015-09-03 20:20 ` [PATCH 2/9] i2c: rcar: remove unused IOERROR state Wolfram Sang 2015-09-03 20:20 ` Wolfram Sang 2015-09-03 20:20 ` [PATCH 6/9] i2c: rcar: don't issue stop when HW does it automatically Wolfram Sang 2015-09-03 20:20 ` Wolfram Sang [not found] ` <1441311613-2681-1-git-send-email-wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> 2015-09-03 20:20 ` [PATCH 3/9] i2c: rcar: remove spinlock Wolfram Sang 2015-09-03 20:20 ` Wolfram Sang 2015-09-03 20:20 ` [PATCH 4/9] i2c: rcar: refactor setup of a msg Wolfram Sang 2015-09-03 20:20 ` Wolfram Sang 2015-09-03 20:20 ` [PATCH 5/9] i2c: rcar: init new messages in irq Wolfram Sang 2015-09-03 20:20 ` Wolfram Sang 2015-10-21 23:10 ` Laurent Pinchart 2015-10-21 23:10 ` Laurent Pinchart 2015-10-22 11:05 ` Wolfram Sang 2015-10-22 11:05 ` Wolfram Sang 2015-10-22 11:12 ` Laurent Pinchart 2015-10-22 11:12 ` Laurent Pinchart 2015-10-23 8:06 ` Wolfram Sang 2015-10-23 8:06 ` Wolfram Sang 2015-10-23 8:57 ` Laurent Pinchart 2015-10-23 9:45 ` Wolfram Sang 2015-10-23 9:45 ` Wolfram Sang 2015-10-23 10:28 ` Laurent Pinchart 2015-10-23 10:28 ` Laurent Pinchart 2015-10-23 12:14 ` Wolfram Sang 2015-10-23 12:14 ` Wolfram Sang 2015-10-23 13:14 ` Laurent Pinchart 2015-10-23 13:14 ` Laurent Pinchart 2015-10-25 15:53 ` Wolfram Sang 2015-10-25 15:53 ` Wolfram Sang 2015-10-29 19:23 ` Wolfram Sang 2015-10-29 19:23 ` Wolfram Sang 2015-10-23 10:04 ` Geert Uytterhoeven 2015-10-23 10:04 ` Geert Uytterhoeven 2015-10-23 10:07 ` Wolfram Sang 2015-10-23 10:07 ` Wolfram Sang 2015-09-03 20:20 ` [PATCH 7/9] i2c: rcar: check master irqs before slave irqs Wolfram Sang 2015-09-03 20:20 ` Wolfram Sang 2015-09-03 20:20 ` [PATCH 8/9] i2c: rcar: revoke START request early Wolfram Sang 2015-09-03 20:20 ` Wolfram Sang 2015-09-03 20:20 ` [PATCH 9/9] i2c: rcar: clean up after refactoring Wolfram Sang 2015-09-03 20:20 ` Wolfram Sang 2015-09-03 20:35 ` Laurent Pinchart [this message] 2015-09-03 20:35 ` [PATCH 0/9] i2c: rcar: tackle race conditions in the driver Laurent Pinchart 2015-09-03 20:40 ` Wolfram Sang 2015-09-03 20:40 ` Wolfram Sang 2015-09-03 20:47 ` Laurent Pinchart 2015-09-03 20:47 ` Laurent Pinchart 2015-09-03 20:55 ` Wolfram Sang 2015-09-03 20:55 ` Wolfram Sang 2015-09-04 4:33 ` Magnus Damm 2015-09-04 4:33 ` Magnus Damm 2015-09-05 7:31 ` Wolfram Sang 2015-09-05 7:31 ` Wolfram Sang 2015-09-07 16:04 ` Magnus Damm 2015-09-07 16:04 ` Magnus Damm [not found] ` <CANqRtoRs=f=07B=HSLCVg5G4rnhxj6Heod+spYwxHiKFLZqFWg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2015-09-08 10:53 ` Wolfram Sang 2015-09-08 10:53 ` Wolfram Sang 2015-09-09 5:08 ` Magnus Damm 2015-09-09 5:08 ` Magnus Damm 2015-09-09 8:54 ` Wolfram Sang 2015-09-09 8:54 ` Wolfram Sang 2015-10-09 21:34 ` Wolfram Sang 2015-10-09 21:34 ` Wolfram Sang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=5464456.UnsMOS3MTx@avalon \ --to=laurent.pinchart@ideasonboard.com \ --cc=geert@linux-m68k.org \ --cc=horms@verge.net.au \ --cc=kuninori.morimoto.gx@gmail.com \ --cc=linux-i2c@vger.kernel.org \ --cc=linux-sh@vger.kernel.org \ --cc=magnus.damm@gmail.com \ --cc=sergei.shtylyov@cogentembedded.com \ --cc=wsa@the-dreams.de \ --cc=ykaneko0929@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.