Received-SPF: pass (google.com: domain of linux-kernel+bounces-172205-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 7 May 2024 14:09:59 -0700
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Jeff LaBundy <jeff@labundy.com>
Cc: Mark Brown <broonie@kernel.org>, Hans de Goede <hdegoede@redhat.com>,
	Charles Wang <charles.goodix@gmail.com>, hadess@hadess.net,
	Richard Hughes <hughsient@gmail.com>, linux-input@vger.kernel.org,
	linux-kernel@vger.kernel.org, neil.armstrong@linaro.org
Subject: Re: [PATCH] Input: goodix-berlin - Add sysfs interface for reading
 and writing touch IC registers
Message-ID: <ZjqYp1oxPPWcF3jW@google.com>
References: <20240506114752.47204-1-charles.goodix@gmail.com>
 <6362e889-7df2-4c61-8ad5-bfe199e451ec@redhat.com>
 <ZjmOUp725QTHrfcT@google.com>
 <Zjo8eTQQS1LvzFgZ@finisterre.sirena.org.uk>
 <ZjpFVGw6PgjRcZY3@nixie71>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZjpFVGw6PgjRcZY3@nixie71>

On Tue, May 07, 2024 at 10:14:28AM -0500, Jeff LaBundy wrote:
> Hi all,
> 
> On Tue, May 07, 2024 at 11:36:41PM +0900, Mark Brown wrote:
> > On Mon, May 06, 2024 at 07:13:38PM -0700, Dmitry Torokhov wrote:
> > > On Mon, May 06, 2024 at 02:03:13PM +0200, Hans de Goede wrote:
> > 
> > > > If raw register access is seen as a good solution, then I think this
> > > > should use regmap + some generic helpers (to be written) to export
> > > > regmap r/w access to userspace.
> > 
> > > I think the less code we have in kernel the better, especially if in
> > > cases where firmware flashing is not essential for device to work (i.e.
> > > it the controller has a flash memory). That said IIRC Mark felt very
> > > strongly about allowing regmap writes from userspace... but maybe he
> > > softened the stance or we could have this functionality opt-in?
> > 
> > I think unmediated raw register access is a terrible idea, you can't
> > safely write a driver if userspace can just go in and randomly write to
> > registers with no coordination with the running driver and for some
> > devices the kernel needs to ensure that any writes don't damage or
> > destabalise the system.  If a driver provides an interface that looks
> > like raw register accesses that's of course fine (I mean, a lot of
> > firmware formats basically boil down to register write sequences which
> > is clearly fine) but it should be the driver doing that and it should be
> > looking at what's going on and ensure that it's joined up with the needs
> > of the rest of the system.
> 
> I happen to agree here; especially in the case of writing new FW to a
> flash; this is a very hardware-centric and device-specific function,
> which by definition belongs in a kernel driver.
> 
> For example, many devices must be placed in a bootloader mode during
> the FW update, and may clamp or toggle an interrupt pin during this
> mode switch. If user space initiates this sequence while the driver is
> unaware of this process, it may attempt to read status registers from
> an I2C address that is temporarily offline.

And yet we have i2c-dev and hidraw that are often successfully used to
flash the firmware, do diagnostics, etc. without encumbering the kernel.
They are more likely to work on ACPI systems because such systems have
separation between power management and function pieces (whereas on
non-ACPI systems power management is crammed into the same driver and it
is not possible to properly power up device without wiring up the rest
of it). This is something that I feel we will have to fix in the long
term.

> 
> A much more common design pattern is for the driver to expose one W/O
> sysfs attribute for accepting the FW file name, and one R/O attribute
> for displaying the current FW version in flash. A small script runs at
> start-up to check the version against what is stored on "disk", and if
> what is stored in flash is deemed out of date, the script writes to the
> W/O attribute. This is the extent of user space's involvement.

This however means that code that is not used 99.9999 percent of the
time has to stay in the kernel, occupying precious memory. I agree
that if firmware update is very involved and needs precise control of
interrupts coming from the device and has certain timing restriction,
then in-kernel implementation is preferable, but in many instance
userspace updaters work just fine.

Thanks.

-- 
Dmitry