Re: [PATCH v2] HID: hid-goodix: Add Goodix HID-over-SPI driver

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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.

...

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

Thanks.

-- 
Dmitry