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

[Charles Wang]

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