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: <cnmnr23sif5trz3hiy6swzlyt36dbbemwdyxsqbjmn2nv33mgc@byu4yu4panyi>
Date: Wed, 2 Oct 2024 10:42:20 +0200
From: Andi Shyti <andi.shyti@...nel.org>
To: Conor Dooley <conor@...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

Hi Conor,

On Tue, Oct 01, 2024 at 02:02:18PM GMT, Conor Dooley wrote:
> 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.

I started adding them already from a few releases and this is the
first time I am writing it.

I won't cry if someone doesn't add it :-)

> > > 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?

No need.

> > 
> > ...
> > 
> > > +	/*
> > > +	 * 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.

Yes, I understood the reason. Mine was not a binding comment.

Thanks,
Andi

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