linux-kernel - Re: [PATCH 3/3] wiegand: add Wiegand GPIO bit-banged controller driver

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:   Wed, 4 Jan 2023 18:59:00 +0200
From:   Andy Shevchenko <andriy.shevchenko@...el.com>
To:     Martin Zaťovič <m.zatovic1@...il.com>
Cc:     linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        mani@...nel.org, hemantk@...eaurora.org, quic_jhugo@...cinc.com,
        andersson@...nel.org, Michael.Srba@...nam.cz, arnd@...db.de,
        dipenp@...dia.com, bvanassche@....org, iwona.winiarska@...el.com,
        ogabbay@...nel.org, tzimmermann@...e.de, fmdefrancesco@...il.com,
        jason.m.bills@...ux.intel.com, jae.hyun.yoo@...ux.intel.com,
        gregkh@...uxfoundation.org, krzysztof.kozlowski+dt@...aro.org,
        robh+dt@...nel.org
Subject: Re: [PATCH 3/3] wiegand: add Wiegand GPIO bit-banged controller
 driver

On Wed, Jan 04, 2023 at 02:34:14PM +0100, Martin Zaťovič wrote:
> This controller formats the data to a Wiegand format - appends
> checksum if one of the defined formats is used - and bit-bangs
> the message on devicetree defined GPIO lines.
> 
> Several attributes need to be defined in the devicetree in order
> for this driver to work, namely the data-hi-gpios, data-lo-gpios,
> pulse-len, frame-gap and interval-len. These attributes are
> documented in the devicetree binding documentation file.
> 
> The driver creates a dev file for writing messages on the bus.
> It also creates two sysfs files to control the format and payload
> length of messages. Defined formats are 26, 36 and 37-bit, meaning
> the payloads for these formats are 24, 34 and 35 bit respectively,
> as two bits are allocated for checksum. A custom format is also
> supported and it is set by writing 0 to the format sysfs file.
> Custom format does not calculate and append checksum to messages -
> they are bit-banged as inputted.

Brief look at the code makes me think that this is something from 10 years ago
with slightly removed dust to make it compile. So far I have noticed:
- explicit castings where it's not needed
- bad indentation here and there
- using direct dereference in the cases when we have specific getters available
- NIH this and that

...

> +What:		/sys/devices/platform/.../wiegand-gpio-attributes/format
> +Date:		January 2023
> +Contact:	Martin Zaťovič <m.zatovic1@...il.com>
> +Description:
> +		Read/set Wiegand communication format.
> +		0 - custom format, payload length is specified by
> +		payload_len file
> +		26 - 26-bit format (24 bit payload, 2 bits checksum)
> +		36 - 36-bit format (34 bit payload, 2 bits checksum)
> +		37 - 37-bit format (35 bit payload, 2 bits checksum)
> +
> +What:		/sys/devices/platform/.../wiegand-gpio-attributes/payload_len
> +Date:		January 2023
> +Contact:	Martin Zaťovič <m.zatovic1@...il.com>
> +Description:
> +		Read/set Wiegand communication payload length. File is only
> +		writable if custom format is set.

Why all these attributes? What is special about them and how they are specific
to the hardware in question?

To me it all sounds like layering violation: a GPIO driver that has to be
generic provides a complete custom ABIs which we usually put on the upper
layer (in the kernel as a child driver or in the user space).

-- 
With Best Regards,
Andy Shevchenko