linux-kernel - Re: [PATCH RFC 2/4] input: touchscreen: add core support for Goodix Berlin Touchscreen IC

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]

Date:   Mon, 12 Jun 2023 11:59:44 -0500
From:   Jeff LaBundy <jeff@...undy.com>
To:     Neil Armstrong <neil.armstrong@...aro.org>
Cc:     Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Bastien Nocera <hadess@...ess.net>,
        Hans de Goede <hdegoede@...hat.com>,
        Henrik Rydberg <rydberg@...math.org>,
        linux-input@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC 2/4] input: touchscreen: add core support for Goodix
 Berlin Touchscreen IC

Hi Neil,

[...]

> > > + * - PEN Events
> > 
> > What is "PEN"; is it "stylus" as written elsewhere?
> 
> Yes, they use both terms in the vendor driver.

I recommend using a common term throughout; as written, I first thought
"PEN" was an acronym.

[...]

> > > +	misc->fw_attr_len = le16_to_cpu(misc->fw_attr_len);
> > > +	misc->fw_log_len = le16_to_cpu(misc->fw_log_len);
> > > +	misc->stylus_struct_len = le16_to_cpu(misc->stylus_struct_len);
> > > +	misc->mutual_struct_len = le16_to_cpu(misc->mutual_struct_len);
> > > +	misc->self_struct_len = le16_to_cpu(misc->self_struct_len);
> > > +	misc->noise_struct_len = le16_to_cpu(misc->noise_struct_len);
> > > +	misc->touch_data_addr = le32_to_cpu(misc->touch_data_addr);
> > > +	misc->touch_data_head_len = le16_to_cpu(misc->touch_data_head_len);
> > > +	misc->point_struct_len = le16_to_cpu(misc->point_struct_len);
> > > +	misc->mutual_rawdata_addr = le32_to_cpu(misc->mutual_rawdata_addr);
> > > +	misc->mutual_diffdata_addr = le32_to_cpu(misc->mutual_diffdata_addr);
> > > +	misc->mutual_refdata_addr = le32_to_cpu(misc->mutual_refdata_addr);
> > > +	misc->self_rawdata_addr = le32_to_cpu(misc->self_rawdata_addr);
> > > +	misc->self_diffdata_addr = le32_to_cpu(misc->self_diffdata_addr);
> > > +	misc->self_refdata_addr = le32_to_cpu(misc->self_refdata_addr);
> > > +	misc->iq_rawdata_addr = le32_to_cpu(misc->iq_rawdata_addr);
> > > +	misc->iq_refdata_addr = le32_to_cpu(misc->iq_refdata_addr);
> > > +	misc->im_rawdata_addr = le32_to_cpu(misc->im_rawdata_addr);
> > > +	misc->im_readata_len = le16_to_cpu(misc->im_readata_len);
> > > +	misc->noise_rawdata_addr = le32_to_cpu(misc->noise_rawdata_addr);
> > > +	misc->noise_rawdata_len = le16_to_cpu(misc->noise_rawdata_len);
> > > +	misc->stylus_rawdata_addr = le32_to_cpu(misc->stylus_rawdata_addr);
> > > +	misc->stylus_rawdata_len = le16_to_cpu(misc->stylus_rawdata_len);
> > > +	misc->noise_data_addr = le32_to_cpu(misc->noise_data_addr);
> > > +	misc->esd_addr = le32_to_cpu(misc->esd_addr);
> > 
> > What is all of this stuff for? The beginning of the driver explicitly states
> > that ESD recovery is not supported; please consider stripping all of this if
> > its only purpose is to support noise in the logs or sysfs attributes that we
> > cannot reasonably react to.
> 
> Those are all the firmware parameters used by the driver, for now only a little
> are used, we already read the entire struct so stripping isn't an option otherwise
> the values will be shifted.
> 
> We only print those over debugfs se it won't fill any logs

I feel this is a lot of bloat to simply enable pretty printing of register
values. Some of these struct members aren't even used anywhere; others seem
to be related to functions this driver does not support. Do you realistically
expect one to read those values?

For registers that customers would in fact need to read for debugging, can
you not simply rely on regmap's natural debugfs facility? It seems you are
largely duplicating that here.

I understand that some parameters are used elsewhere and it may be efficient
to read the entire memory region as a packed struct, but there is no need to
unalign and store what is effectively padding in many cases.

[...]

Kind regards,
Jeff LaBundy