linux-kernel - Re: [PATCH v1] i2c: microchip-core: actually use repeated sends

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241001-boring-livestock-0158ccc3fa88@spud>
Date: Tue, 1 Oct 2024 14:02:18 +0100
From: Conor Dooley <conor@...nel.org>
To: Andi Shyti <andi.shyti@...nel.org>
Cc: linux-i2c@...r.kernel.org, Conor Dooley <conor.dooley@...rochip.com>,
	Daire McNamara <daire.mcnamara@...rochip.com>,
	Wolfram Sang <wsa@...nel.org>, linux-riscv@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] i2c: microchip-core: actually use repeated sends

On Tue, Oct 01, 2024 at 02:45:20PM +0200, Andi Shyti wrote:
> Hi Conor,
> 
> On Mon, Sep 30, 2024 at 02:38:27PM GMT, Conor Dooley wrote:
> > From: Conor Dooley <conor.dooley@...rochip.com>
> > 
> > At present, where repeated sends are intended to be used, the
> > i2c-microchip-core driver sends a stop followed by a start. Lots of i2c
> > devices must not malfunction in the face of this behaviour, because the
> > driver has operated like this for years! Try to keep track of whether or
> > not a repeated send is required, and suppress sending a stop in these
> > cases.
> > 
> > Fixes: 64a6f1c4987e ("i2c: add support for microchip fpga i2c controllers")
> 
> I don't think the Fixes tag is needed here if everything worked
> until now, unless you got some other device that requires this
> change and you need to explain it.

I think the fixes tag is accurate, because it only happened to work on
the limited set of devices I and others tried. This patch came about cos
I got reports of it being broken in 6.6

> If this is more an improvement (because it has worked), then we
> shouldn't add the Fixes tag.
> 
> In any case, when patches are going to stable, we need to Cc
> stable too.
> 
> Cc: <stable@...r.kernel.org> # v6.0+
> 
> (This is specified in the
> Documentation/process/stable-kernel-rules.rst and I'm starting to
> enforce it here).

Yah, some maintainers want to add the tags themselves, so got into a
(bad?) habit of leaving them out. I can add it if there's a v2.

> 
> > Signed-off-by: Conor Dooley <conor.dooley@...rochip.com>
> 
> ...
> 
> > +	/*
> > +	 * If there's been an error, the isr needs to return control
> > +	 * to the "main" part of the driver, so as not to keep sending
> > +	 * messages once it completes and clears the SI bit.
> > +	 */
> > +	if (idev->msg_err) {
> > +		complete(&idev->msg_complete);
> > +		return;
> > +	}
> > +
> > +	this_msg = (idev->msg_queue)++;
> 
> do we need parenthesis here?

I suppose not, do you want a v2 if that's the only change?

> 
> ...
> 
> > +	/*
> > +	 * The isr controls the flow of a transfer, this info needs to be saved
> > +	 * to a location that it can access the queue information from.
> > +	 */
> > +	idev->restart_needed = false;
> > +	idev->msg_queue = msgs;
> > +	idev->total_num = num;
> > +	idev->current_num = 0;
> > +
> > +	/*
> > +	 * But the first entry to the isr is triggered by the start in this
> > +	 * function, so the first message needs to be "dequeued".
> > +	 */
> > +	idev->addr = i2c_8bit_addr_from_msg(this_msg);
> > +	idev->msg_len = this_msg->len;
> > +	idev->buf = this_msg->buf;
> > +	idev->msg_err = 0;
> > +
> > +	if (idev->total_num > 1) {
> > +		struct i2c_msg *next_msg = msgs + 1;
> > +
> > +		idev->restart_needed = next_msg->flags & I2C_M_RD;
> > +	}
> > +
> > +	idev->current_num++;
> > +	idev->msg_queue++;
> 
> Can we initialize only once? This part is just adding extra code.

I don't agree that it is extra code, I think it is clearer like this as
I intentionally wrote it this way.

> The rest looks good. I just need to know if Wolfram has some more
> observations here.
> 
> Thanks,
> Andi

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)