linux-kernel - Re: [PATCH v4 2/3] i2c: tegra: make reset an optional property

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: <aEvMlKIfcccD_s-O@smile.fi.intel.com>
Date: Fri, 13 Jun 2025 10:00:36 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Akhil R <akhilrajeev@...dia.com>
Cc: andy.shevchenko@...il.com, andi.shyti@...nel.org, conor+dt@...nel.org,
	devicetree@...r.kernel.org, digetx@...il.com, jonathanh@...dia.com,
	krzk+dt@...nel.org, ldewangan@...dia.com, linux-i2c@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org,
	p.zabel@...gutronix.de, robh@...nel.org, thierry.reding@...il.com
Subject: Re: [PATCH v4 2/3] i2c: tegra: make reset an optional property

On Fri, Jun 13, 2025 at 11:30:32AM +0530, Akhil R wrote:
> On Thu, 12 Jun 2025 21:43:56 +0300, Andy Shevchenko wrote:

> >> >> >     if (handle)
> >> >> >             err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
> >> >> > -   else
> >> >> > +   else if (i2c_dev->rst)
> >> >> >             err = reset_control_reset(i2c_dev->rst);
> >> >> > +   else
> >> >> > +           err = tegra_i2c_master_reset(i2c_dev);
> >> >>
> >> >> Can you please take a look here? Should the reset happen in ACPI?
> >> >
> >> > This is a good question. Without seeing all the implementations of _RST method
> >> > for the platforms based on this SoC it's hard to say. Ideally the _RST (which
> >> > is called above) must handle it properly, but firmwares have bugs...
> >> >
> >> > TL;DR: I think the approach is correct, and if any bug in ACPI will be found,
> >> > the workaround (quirk) needs to be added here later on.
> >> 
> >> As in Thierry's comment, I was in thought of updating the code as below.
> >> Does it make sense or would it be better keep what it is there now?
> >> 
> >> if (handle && acpi_has_method(handle, "_RST"))
> >> 	err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
> >> else if (i2c_dev->rst)
> >> 	err = reset_control_reset(i2c_dev->rst);
> >> else
> >> 	err = tegra_i2c_master_reset(i2c_dev);
> >
> > This will change current behaviour for the ACPI based platforms that do not
> > have an _RST method. At bare minumum this has to be elaborated in the commit
> > message with an explanation why it's not a probnlem.
> 
> This sequence is hit only at boot and on any error. It should be good to reset
> the controller internally at least for those cases. We are reconfiguring the I2C
> anyway after this and hence should not cause any problem.
> Will add these in the commit message as well.

This is not enough. You should explain the ACPI case. The above is just generic
wording as I read it. It does not explain 1) if there are ACPI firmwares that
have no _RST method for this device; 2) why it's not a problem for them to do
like this and why it was not supported before (with the current code this
platform will return an error on the method evaluation. Moreover the current
code is buggy. The acpi_evaluate_object() returns an ACPI error code and not
Linux one. so, for the such platforms (which I think do not exist, but still)
the err will have positive code which may be interpreted incorrectly.

So, fix the bug first, then rebase your code based on that change and
extend the commit message to really elaborate on all of the aspects.
W/o this done it's no go change.



-- 
With Best Regards,
Andy Shevchenko