linux-kernel - Re: [PATCH v3 4/4] i2c: imx: prevent rescheduling in non dma mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <pva2d5fc76ykjlzyxivrau4qnt6cu6qqlgmuvq3ykzlaqvsqio@xuultvre2f4d>
Date: Tue, 3 Sep 2024 23:00:25 +0200
From: Andi Shyti <andi.shyti@...nel.org>
To: Stefan Eichenberger <eichest@...il.com>
Cc: o.rempel@...gutronix.de, kernel@...gutronix.de, shawnguo@...nel.org, 
	s.hauer@...gutronix.de, festevam@...il.com, francesco.dolcini@...adex.com, 
	Frank.Li@....com, linux-i2c@...r.kernel.org, imx@...ts.linux.dev, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	Stefan Eichenberger <stefan.eichenberger@...adex.com>
Subject: Re: [PATCH v3 4/4] i2c: imx: prevent rescheduling in non dma mode

Hi Stefan,

One final ask...

On Mon, Sep 02, 2024 at 09:42:04AM GMT, Stefan Eichenberger wrote:
> From: Stefan Eichenberger <stefan.eichenberger@...adex.com>
> 
> We are experiencing a problem with the i.MX I2C controller when
> communicating with SMBus devices. We are seeing devices time-out because
> the time between sending/receiving two bytes is too long, and the SMBus
> device returns to the idle state. This happens because the i.MX I2C
> controller sends and receives byte by byte. When a byte is sent or
> received, we get an interrupt and can send or receive the next byte.
> 
> The current implementation sends a byte and then waits for an event
> generated by the interrupt subroutine. After the event is received, the
> next byte is sent and we wait again. This waiting allows the scheduler
> to reschedule other tasks, with the disadvantage that we may not send
> the next byte for a long time because the send task is not immediately
> scheduled. For example, if the rescheduling takes more than 25ms, this
> can cause SMBus devices to timeout and communication to fail.
> 
> This patch changes the behavior so that we do not reschedule the
> send/receive task, but instead send or receive the next byte in the
> interrupt subroutine. This prevents rescheduling and drastically reduces
> the time between sending/receiving bytes. The cost in the interrupt
> subroutine is relatively small, we check what state we are in and then
> send/receive the next byte. Before we had to call wake_up, which is even
> less expensive. However, we also had to do some scheduling, which
> increased the overall cost compared to the new solution. The wake_up
> function to wake up the send/receive task is now only called when an
> error occurs or when the transfer is complete.
> 
> Signed-off-by: Stefan Eichenberger <stefan.eichenberger@...adex.com>
> Acked-by: Oleksij Rempel <o.rempel@...gutronix.de>

The code is fine and looks good to me. The commit log is also
very descriptive. However, there isn’t a single line of comment
in the code.

If someone else encounters this code without understanding your
specific context, they might find it too complex and attempt to
simplify it or revert to the previous implementation.

Please add comments to describe the state machine you
implemented, the reasoning behind it (as you explained in the
commit log), and make it understandable for those who haven’t
reviewed your patches.

Thanks,
Andi