linux-kernel - Re: [PATCH v14 1/7] tpm: Make read{16, 32}() and write32() in tpm_tis_phy

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]

Message-ID: <dd7fbcaafad3259a307f1a2d8a72127e7bcc0630.camel@kernel.org>
Date:   Tue, 14 Sep 2021 00:06:07 +0300
From:   Jarkko Sakkinen <jarkko@...nel.org>
To:     amirmizi6@...il.com, Eyal.Cohen@...oton.com,
        oshrialkoby85@...il.com, alexander.steffen@...ineon.com,
        robh+dt@...nel.org, mark.rutland@....com, peterhuewe@....de,
        jgg@...pe.ca, arnd@...db.de, gregkh@...uxfoundation.org,
        benoit.houyere@...com, eajames@...ux.ibm.com, joel@....id.au
Cc:     devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-integrity@...r.kernel.org, oshri.alkoby@...oton.com,
        tmaimon77@...il.com, gcwilson@...ibm.com, kgoldman@...ibm.com,
        Dan.Morav@...oton.com, oren.tanami@...oton.com,
        shmulik.hager@...oton.com, amir.mizinski@...oton.com
Subject: Re: [PATCH v14 1/7] tpm: Make read{16, 32}() and write32() in
 tpm_tis_phy_ops optional

On Mon, 2021-09-13 at 17:43 +0300, amirmizi6@...il.com wrote:
> From: Amir Mizinski <amirmizi6@...il.com>
> 
> Only tpm_tis can use memory-mapped I/O, which is truly mapped into the
> kernel's memory space. Therefore, using ioread16/ioread32/iowrite32 turns
> into a straightforward pointer dereference.
> Some drivers, such as tpm_tis_spi, require more complicated operations to
> read more than one byte at a time and, as a result, will revert to
> read_bytes/write_bytes.
> Therefore, re-implement tpm_tis_{read, write}_{16, 32}, so that they check
> if implementations for {read, write}_{16, 32} in tpm_tis_phys_ops  exist;
> if they do not exist, then revert to {read, write}_bytes().
>  static int tpm_tis_spi_probe(struct spi_device *dev)

This is otherwise good but lacks explanation why this makes sense, other than
somewhat obvious benefit of having less calbacks, which BTW should be stated
explicitly in the commit message too.

You must answer what is your by doing this change, e.g. what you are unable
to do, if the callbacks are removed (just an example).

I get the change, but motivation to do it needs to be there.

/Jarkko