Received-SPF: pass (google.com: domain of linux-kernel+bounces-201970-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Wed, 5 Jun 2024 16:16:41 +0800
From: Charles Wang <charles.goodix@gmail.com>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: jikos@kernel.org, bentiss@kernel.org, hbarnor@chromium.org,
	dianders@chromium.org, linux-input@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] HID: hid-goodix: Add Goodix HID-over-SPI driver
Message-ID: <ZmAe6fNolzw244ft@ux-UP-WHL01>
References: <20240527042945.57054-1-charles.goodix@gmail.com>
 <Zl5kz_S4FJaGzfiW@google.com>
 <Zl75T3ioNqsofyQd@ux-UP-WHL01>
 <Zl9eUVpojZI2Z3ur@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Zl9eUVpojZI2Z3ur@google.com>

On Tue, Jun 04, 2024 at 11:34:57AM -0700, Dmitry Torokhov wrote:
> On Tue, Jun 04, 2024 at 07:23:59PM +0800, Charles Wang wrote:
> > Hi Dmitry,
> > 
> > On Mon, Jun 03, 2024 at 05:50:23PM -0700, Dmitry Torokhov wrote:
> > > Hi Charles,
> > > 
> ...
> > > > +};
> > > > +
> > > > +static int goodix_spi_read(struct goodix_ts_data *ts, u32 addr,
> > > > +			   u8 *data, unsigned int len)
> > > > +{
> > > > +	struct spi_device *spi = to_spi_device(&ts->spi->dev);
> > > > +	struct spi_transfer xfers;
> > > > +	struct spi_message spi_msg;
> > > > +	u8 *buf;
> > > > +	int error;
> > > > +
> > > > +	buf = kzalloc(GOODIX_SPI_READ_PREFIX_LEN + len, GFP_KERNEL);
> > > > +	if (!buf)
> > > > +		return -ENOMEM;
> > > 
> > > Can you try using ts->xfer_buf without making allocations and copies?
> > > Maybe have goodix_spi_read() have data as u8 **data, and do
> > > 
> > > 	*data = buf + GOODIX_SPI_READ_PREFIX_LEN;
> > > 	return 0;
> > > 
> > > at the end. I.e. callers do not supply buffer but rather are given one.
> > > Of course you need to make sure there are no concurrent calls to
> > > goodix_spi_read(), but I do not think you have them anyways.
> > >
> > 
> > Unfortunately, there are concurrent calls to goodix_spi_read(). The functions
> > goodix_hid_get_raw_report() and goodix_hid_irq() may execute concurrently.
> > 
> > Anyways, I will try to optimize this part and reduce the malloc operations
> > where possible.
> 
> I think you will need to serialize this anyway, as (AFAICS) you write to
> report address, and then perform the read. There is nothing in the upper
> layers that says that several report requests can not be sent at once,
> and I think the device may also raise interrupt at the same time.
> Without serializing/locking you may mix up the data.
> 

Thank you for pointing out this issue. You are correct that
serialization is required to ensure the integrity of report requests.

> 
> > > > +
> > > > +/* Empty callbacks with success return code */
> > > 
> > > Hmm, I see you are using falling edge interrupt. Don't you have concern
> > > of having it "stuck" here? I do not think all these should be stubs...
> > >
> > Thank you for pointing this out. The trigger method shouldn't be fixed
> > within the driver. As for "stuck", I believe this issue does not exit.
> > The firmware won't wait for the host's response.
> 
> It is not the touch controller that will get stuck. The host interrupt
> controller will not repeat signalling the interrupt that is configured
> as edge and it was asserted earlier.
> 
> Or are you saying that the touch controller will de-assert and re-assert
> the interrupt line if it is not serviced within given time?
> 

Yes, to ensure that critical events are not missed by the host, such as the
touch up event, the firmware is designed to re-assert the interrupt line if
it is not serviced within a given time.

Charles

Thanks