From: Jean Delvare <jdelvare@suse.de>
To: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Andi Shyti <andi.shyti@kernel.org>, linux-i2c@vger.kernel.org
Subject: Re: [PATCH 3/4] i2c: i801: Improve i801_block_transaction_byte_by_byte
Date: Mon, 28 Aug 2023 15:27:47 +0200 [thread overview]
Message-ID: <20230828152747.09444625@endymion.delvare> (raw)
In-Reply-To: <cc1826de-35b1-cd20-900f-3908c7499792@gmail.com>
Hi Heiner,
On Sun, 27 Aug 2023 19:14:38 +0200, Heiner Kallweit wrote:
> On 27.06.2023 15:46, Jean Delvare wrote:
> > Hi Heiner, Andi,
> >
> > On Sat, 04 Mar 2023 22:36:34 +0100, Heiner Kallweit wrote:
> >> Here we don't have to write SMBHSTCNT in each iteration of the loop.
> >> Bit SMBHSTCNT_START is internally cleared immediately, therefore
> >> we don't have to touch the value of SMBHSTCNT until the last byte.
> >>
> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> >> ---
> >> drivers/i2c/busses/i2c-i801.c | 6 +++---
> >> 1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> >> index 7641bd0ac..e1350a8cc 100644
> >> --- a/drivers/i2c/busses/i2c-i801.c
> >> +++ b/drivers/i2c/busses/i2c-i801.c
> >> @@ -677,11 +677,11 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
> >> for (i = 1; i <= len; i++) {
> >> if (i == len && read_write == I2C_SMBUS_READ)
> >> smbcmd |= SMBHSTCNT_LAST_BYTE;
> >> - outb_p(smbcmd, SMBHSTCNT(priv));
> >>
> >> if (i == 1)
> >> - outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
> >> - SMBHSTCNT(priv));
> >> + outb_p(smbcmd | SMBHSTCNT_START, SMBHSTCNT(priv));
> >> + else if (smbcmd & SMBHSTCNT_LAST_BYTE)
> >> + outb_p(smbcmd, SMBHSTCNT(priv));
> >>
> >> status = i801_wait_byte_done(priv);
> >> if (status)
> >
> > I tested this and it works, but I don't understand how.
> >
> > I thought that writing to SMBHSTCNT was what was telling the host
> > controller to proceed with the next byte. If writing to SMBHSTCNT for
> > each byte isn't needed, then what causes the next byte to be processed?
> > Does this happen as soon as SMBHSTSTS_BYTE_DONE is written? If so, then
> > what guarantees that we set SMBHSTCNT_LAST_BYTE *before* the last byte
> > is actually processed?
>
> It's my understanding that writing SMBHSTSTS_BYTE_DONE tells the host to
> continue with the next byte.
That's indeed possible, and quite likely, considering that your patch
works.
> We set SMBHSTCNT_LAST_BYTE whilst the host is receiving the last byte.
> Apparently the host checks for SMBHSTCNT_LAST_BYTE once it received
> a byte, in order to determine whether to ack the byte or not.
> So SMBHSTCNT_LAST_BYTE doesn't have to be set before the host starts
> receiving the last byte.
How is this not racy?
In the interrupt-driven case, at the end of a block read transaction,
we set SMBHSTCNT_LAST_BYTE at the end of i801_isr_byte_done(), then
return to i801_isr() where we write 1 to SMBHSTSTS_BYTE_DONE to clear
it. This lets the controller handle the last byte with the knowledge
that this is the last byte.
However, in the poll-driven case, SMBHSTSTS_BYTE_DONE is being cleared
at the end of the loop in i801_block_transaction_byte_by_byte(), then
at the beginning of the next iteration, we write SMBHSTCNT_LAST_BYTE,
then wait for completion. If the controller is super fast (or, to be
more realistic, the i2c-i801 driver gets preempted between writing
SMBHSTSTS_BYTE_DONE and writing SMBHSTCNT_LAST_BYTE) then the byte may
have been already read and acked, before we have the time to let the
controller know that no ACK should be sent. This looks racy. Am I
missing something?
If nothing else, the fact that the order is different between the
interrupt-driven and poll-driven cases is fishy.
I must add that the problem is not related to your patch, I just
happened to notice it while reviewing your patch.
> For writes SMBHSTCNT_LAST_BYTE isn't used.
Agreed.
--
Jean Delvare
SUSE L3 Support
next prev parent reply other threads:[~2023-08-28 13:28 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-04 21:30 [PATCH 0/4] i2c: i801: next set of improvements Heiner Kallweit
2023-03-04 21:31 ` [PATCH 1/4] i2c: i801: Use i2c_mark_adapter_suspended/resumed Heiner Kallweit
2023-06-14 22:24 ` Andi Shyti
2023-06-15 21:17 ` Heiner Kallweit
2023-06-15 21:45 ` Andi Shyti
2023-06-16 6:00 ` Heiner Kallweit
2023-06-26 17:20 ` Jean Delvare
2023-08-27 16:20 ` Heiner Kallweit
2023-03-04 21:33 ` [PATCH 2/4] i2c: i801: Replace acpi_lock with I2C bus lock Heiner Kallweit
2023-06-14 22:31 ` Andi Shyti
2023-06-15 21:22 ` Heiner Kallweit
2023-06-15 21:49 ` Andi Shyti
2023-06-26 17:59 ` Jean Delvare
2023-08-27 16:21 ` Heiner Kallweit
2023-08-28 7:01 ` Jean Delvare
2023-09-15 14:01 ` Heiner Kallweit
2023-03-04 21:36 ` [PATCH 3/4] i2c: i801: Improve i801_block_transaction_byte_by_byte Heiner Kallweit
2023-06-14 22:32 ` Andi Shyti
2023-06-15 21:48 ` Andi Shyti
2023-06-27 13:46 ` Jean Delvare
2023-08-27 17:14 ` Heiner Kallweit
2023-08-28 13:27 ` Jean Delvare [this message]
2023-08-28 15:10 ` Jean Delvare
2023-08-29 6:08 ` Heiner Kallweit
2023-08-29 6:29 ` Heiner Kallweit
2023-03-04 21:37 ` [PATCH 4/4] i2c: i801: Switch to new macro DEFINE_SIMPLE_DEV_PM_OPS Heiner Kallweit
2023-06-14 22:37 ` Andi Shyti
2023-06-28 7:15 ` Jean Delvare
2023-09-22 9:54 ` Wolfram Sang
2023-09-22 10:20 ` Heiner Kallweit
2023-05-16 20:29 ` [PATCH 0/4] i2c: i801: next set of improvements Heiner Kallweit
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=20230828152747.09444625@endymion.delvare \
--to=jdelvare@suse.de \
--cc=andi.shyti@kernel.org \
--cc=hkallweit1@gmail.com \
--cc=linux-i2c@vger.kernel.org \
/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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).