linux-kernel - Re: [PATCH v2 4/4] Input: Add TouchNetix aXiom I2C Touchscreen support

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: <20250530094039.n5236kxskha4vrhd@pengutronix.de>
Date: Fri, 30 May 2025 11:40:39 +0200
From: Marco Felsch <m.felsch@...gutronix.de>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Luis Chamberlain <mcgrof@...nel.org>,
	Russ Weight <russ.weight@...ux.dev>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Kamel Bouhara <kamel.bouhara@...tlin.com>,
	Marco Felsch <kernel@...gutronix.de>,
	Henrik Rydberg <rydberg@...math.org>,
	Danilo Krummrich <dakr@...hat.com>, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org, linux-input@...r.kernel.org
Subject: Re: [PATCH v2 4/4] Input: Add TouchNetix aXiom I2C Touchscreen
 support

On 25-05-29, Greg Kroah-Hartman wrote:
> On Thu, May 29, 2025 at 12:08:45AM +0200, Marco Felsch wrote:
> > +	if (!entry->info)
> > +		WARN(1, "Unsupported usage u%x used, driver bug!", i);
> 
> You just crashed the system and caused all data to be lost if this is
> ever hit :(
> 
> As you did detect this, please handle the error and recover.  It's a bit
> rude for a single i2c driver to take down a whole system, right?

Good point.

> > +#define AXIOM_SIMPLE_FW_DEVICE_ATTR(attr)					\
> > +	static ssize_t								\
> > +	fw_ ## attr ## _show(struct device *dev,				\
> > +			     struct device_attribute *_attr, char *buf)		\
> > +	{									\
> > +		struct i2c_client *i2c = to_i2c_client(dev);			\
> > +		struct axiom_data *ts = i2c_get_clientdata(i2c);		\
> > +										\
> > +		return sprintf(buf, "%u\n", ts->fw_##attr);			\
> 
> sysfs_emit() please for all sysfs show functions.

Sure.

> > +	axiom_u42_get_touchslots(ts);
> > +	if (!ts->num_slots && update_in_process) {
> > +		input_free_device(input);
> > +		/*
> > +		 * Skip input device registration but don't throw an error to
> > +		 * not abort the update since some FW updates require a
> > +		 * following CFG update to re-initialize the touchslot handling.
> > +		 */
> > +		if (update_in_process) {
> > +			dev_info(dev, "No touchslots found after FW or CFG update, skip registering input device\n");
> 
> Why is this info?  What can a user do with this?  Shouldn't this be a
> dev_warn() call at the least?

Please see below.

> > +			return 0;
> 
> You return success, but the device is NOT set up properly, how is that
> going to work?

As explained in the comment. If a firmware update changes the register
layout you may end up in such a situation. A subsequent CFG update is
required to provide a correct FW+CFG match.

We don't throw an error because the FW update itself was successful but
a CFG update is required, therefore I went with the dev_info() but I can
change this to dev_warn().

We don't know which combination requires a subsequent CFG update, e.g.
there is a FW version 4.8.9 which comes in a 2D and a 3D flavour. Tests
showed that updating from a 4.8.9 2D FW to a 4.8.9 3D FW don't require
updating the CFG. Also minor FW updates may not require to update the
CFG.

Regards,
  Marco