Message-Id: <D5FAVRTM32ZJ.1HFCHF3L9I68C@kernel.org>
Date: Wed, 06 Nov 2024 20:17:22 +0200
From: "Jarkko Sakkinen" <jarkko@...nel.org>
To: "Rob Herring" <robh@...nel.org>, "Jarkko Sakkinen"
 <jarkko.sakkinen@....fi>
Cc: "Peter Huewe" <peterhuewe@....de>, "Jason Gunthorpe" <jgg@...pe.ca>,
 "Michael Ellerman" <mpe@...erman.id.au>, "Nicholas Piggin"
 <npiggin@...il.com>, "Christophe Leroy" <christophe.leroy@...roup.eu>,
 "Naveen N. Rao" <naveen.n.rao@...ux.ibm.com>,
 <linux-kernel@...r.kernel.org>, <linux-integrity@...r.kernel.org>,
 <linuxppc-dev@...ts.ozlabs.org>
Subject: Re: [PATCH] tpm: atmel: Drop PPC64 specific MMIO setup

On Wed Nov 6, 2024 at 4:50 PM EET, Rob Herring wrote:
> On Thu, Jul 18, 2024 at 11:01 AM Jarkko Sakkinen <jarkko.sakkinen@....fi> wrote:
> >
> > On Thu Jul 18, 2024 at 5:57 PM EEST, Rob Herring wrote:
> > > On Wed, Jul 17, 2024 at 6:14 AM Jarkko Sakkinen <jarkko.sakkinen@....fi> wrote:
> > > >
> > > > On Wed Jul 17, 2024 at 3:08 PM EEST, Jarkko Sakkinen wrote:
> > > > > On Tue Jul 2, 2024 at 7:10 PM EEST, Rob Herring (Arm) wrote:
> > > > > > The PPC64 specific MMIO setup open codes DT address functions rather
> > > > > > than using standard address parsing functions. The open-coded version
> > > > > > fails to handle any address translation and is not endian safe.
> > > > > >
> > > > > > I haven't found any evidence of what platform used this. The only thing
> > > > > > that turned up was a PPC405 platform, but that is 32-bit and PPC405
> > > > > > support is being removed as well. CONFIG_TCG_ATMEL is not enabled for
> > > > > > any powerpc config and never was. The support was added in 2005 and
> > > > > > hasn't been touched since.
> > > > > >
> > > > > > Rather than try to modernize and fix this code, just remove it.
> > > > > >
> > > > > > Signed-off-by: Rob Herring (Arm) <robh@...nel.org>
> > > > > > ---
> > > > > >  drivers/char/tpm/Kconfig     |   2 +-
> > > > > >  drivers/char/tpm/tpm_atmel.c |  64 +++++++++++++++-
> > > > > >  drivers/char/tpm/tpm_atmel.h | 140 -----------------------------------
> > > > > >  3 files changed, 62 insertions(+), 144 deletions(-)
> > > > > >  delete mode 100644 drivers/char/tpm/tpm_atmel.h
> > > > > >
> > > > > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > > > > > index e63a6a17793c..9b655e9fc7ab 100644
> > > > > > --- a/drivers/char/tpm/Kconfig
> > > > > > +++ b/drivers/char/tpm/Kconfig
> > > > > > @@ -162,7 +162,7 @@ config TCG_NSC
> > > > > >
> > > > > >  config TCG_ATMEL
> > > > > >     tristate "Atmel TPM Interface"
> > > > > > -   depends on PPC64 || HAS_IOPORT_MAP
> > > > > > +   depends on HAS_IOPORT_MAP
> > > > > >     depends on HAS_IOPORT
> > > > > >     help
> > > > > >       If you have a TPM security chip from Atmel say Yes and it
> > > > > > diff --git a/drivers/char/tpm/tpm_atmel.c b/drivers/char/tpm/tpm_atmel.c
> > > > > > index 9fb2defa9dc4..622c4abe8cb3 100644
> > > > > > --- a/drivers/char/tpm/tpm_atmel.c
> > > > > > +++ b/drivers/char/tpm/tpm_atmel.c
> > > > > > @@ -15,7 +15,67 @@
> > > > > >   */
> > > > > >
> > > > > >  #include "tpm.h"
> > > > > > -#include "tpm_atmel.h"
> > > > > > +
> > > > > > +struct tpm_atmel_priv {
> > > > > > +   int region_size;
> > > > > > +   int have_region;
> > > > > > +   unsigned long base;
> > > > > > +   void __iomem *iobase;
> > > > > > +};
> > > > > > +
> > > > > > +#define atmel_getb(chip, offset) inb(atmel_get_priv(chip)->base + offset)
> > > > > > +#define atmel_putb(val, chip, offset) \
> > > > > > +   outb(val, atmel_get_priv(chip)->base + offset)
> > > > > > +#define atmel_request_region request_region
> > > > > > +#define atmel_release_region release_region
> > > > > > +/* Atmel definitions */
> > > > > > +enum tpm_atmel_addr {
> > > > > > +   TPM_ATMEL_BASE_ADDR_LO = 0x08,
> > > > > > +   TPM_ATMEL_BASE_ADDR_HI = 0x09
> > > > > > +};
> > > > > > +
> > > > > > +static inline int tpm_read_index(int base, int index)
> > > > > > +{
> > > > > > +   outb(index, base);
> > > > > > +   return inb(base+1) & 0xFF;
> > > > > > +}
> > > > > > +
> > > > > > +/* Verify this is a 1.1 Atmel TPM */
> > > > > > +static int atmel_verify_tpm11(void)
> > > > > > +{
> > > > > > +
> > > > > > +   /* verify that it is an Atmel part */
> > > > > > +   if (tpm_read_index(TPM_ADDR, 4) != 'A' ||
> > > > > > +       tpm_read_index(TPM_ADDR, 5) != 'T' ||
> > > > > > +       tpm_read_index(TPM_ADDR, 6) != 'M' ||
> > > > > > +       tpm_read_index(TPM_ADDR, 7) != 'L')
> > > > > > +           return 1;
> > > > > > +
> > > > > > +   /* query chip for its version number */
> > > > > > +   if (tpm_read_index(TPM_ADDR, 0x00) != 1 ||
> > > > > > +       tpm_read_index(TPM_ADDR, 0x01) != 1)
> > > > > > +           return 1;
> > > > > > +
> > > > > > +   /* This is an atmel supported part */
> > > > > > +   return 0;
> > > > > > +}
> > > > > > +
> > > > > > +/* Determine where to talk to device */
> > > > > > +static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> > > > > > +{
> > > > > > +   int lo, hi;
> > > > > > +
> > > > > > +   if (atmel_verify_tpm11() != 0)
> > > > > > +           return NULL;
> > > > > > +
> > > > > > +   lo = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_LO);
> > > > > > +   hi = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_HI);
> > > > > > +
> > > > > > +   *base = (hi << 8) | lo;
> > > > > > +   *region_size = 2;
> > > > > > +
> > > > > > +   return ioport_map(*base, *region_size);
> > > > > > +}
> > > > > >
> > > > > >  /* write status bits */
> > > > > >  enum tpm_atmel_write_status {
> > > > > > @@ -142,7 +202,6 @@ static void atml_plat_remove(void)
> > > > > >     tpm_chip_unregister(chip);
> > > > > >     if (priv->have_region)
> > > > > >             atmel_release_region(priv->base, priv->region_size);
> > > > > > -   atmel_put_base_addr(priv->iobase);
> > > > > >     platform_device_unregister(pdev);
> > > > > >  }
> > > > > >
> > > > > > @@ -211,7 +270,6 @@ static int __init init_atmel(void)
> > > > > >  err_unreg_dev:
> > > > > >     platform_device_unregister(pdev);
> > > > > >  err_rel_reg:
> > > > > > -   atmel_put_base_addr(iobase);
> > > > > >     if (have_region)
> > > > > >             atmel_release_region(base,
> > > > > >                                  region_size);
> > > > > > diff --git a/drivers/char/tpm/tpm_atmel.h b/drivers/char/tpm/tpm_atmel.h
> > > > > > deleted file mode 100644
> > > > > > index 7ac3f69dcf0f..000000000000
> > > > > > --- a/drivers/char/tpm/tpm_atmel.h
> > > > > > +++ /dev/null
> > > > > > @@ -1,140 +0,0 @@
> > > > > > -/* SPDX-License-Identifier: GPL-2.0-only */
> > > > > > -/*
> > > > > > - * Copyright (C) 2005 IBM Corporation
> > > > > > - *
> > > > > > - * Authors:
> > > > > > - * Kylene Hall <kjhall@...ibm.com>
> > > > > > - *
> > > > > > - * Maintained by: <tpmdd-devel@...ts.sourceforge.net>
> > > > > > - *
> > > > > > - * Device driver for TCG/TCPA TPM (trusted platform module).
> > > > > > - * Specifications at www.trustedcomputinggroup.org
> > > > > > - *
> > > > > > - * These difference are required on power because the device must be
> > > > > > - * discovered through the device tree and iomap must be used to get
> > > > > > - * around the need for holes in the io_page_mask.  This does not happen
> > > > > > - * automatically because the tpm is not a normal pci device and lives
> > > > > > - * under the root node.
> > > > > > - */
> > > > > > -
> > > > > > -struct tpm_atmel_priv {
> > > > > > -   int region_size;
> > > > > > -   int have_region;
> > > > > > -   unsigned long base;
> > > > > > -   void __iomem *iobase;
> > > > > > -};
> > > > > > -
> > > > > > -#ifdef CONFIG_PPC64
> > > > > > -
> > > > > > -#include <linux/of.h>
> > > > > > -
> > > > > > -#define atmel_getb(priv, offset) readb(priv->iobase + offset)
> > > > > > -#define atmel_putb(val, priv, offset) writeb(val, priv->iobase + offset)
> > > > > > -#define atmel_request_region request_mem_region
> > > > > > -#define atmel_release_region release_mem_region
> > > > > > -
> > > > > > -static inline void atmel_put_base_addr(void __iomem *iobase)
> > > > > > -{
> > > > > > -   iounmap(iobase);
> > > > > > -}
> > > > > > -
> > > > > > -static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> > > > > > -{
> > > > > > -   struct device_node *dn;
> > > > > > -   unsigned long address, size;
> > > > > > -   const unsigned int *reg;
> > > > > > -   int reglen;
> > > > > > -   int naddrc;
> > > > > > -   int nsizec;
> > > > > > -
> > > > > > -   dn = of_find_node_by_name(NULL, "tpm");
> > > > > > -
> > > > > > -   if (!dn)
> > > > > > -           return NULL;
> > > > > > -
> > > > > > -   if (!of_device_is_compatible(dn, "AT97SC3201")) {
> > > > > > -           of_node_put(dn);
> > > > > > -           return NULL;
> > > > > > -   }
> > > > > > -
> > > > > > -   reg = of_get_property(dn, "reg", &reglen);
> > > > > > -   naddrc = of_n_addr_cells(dn);
> > > > > > -   nsizec = of_n_size_cells(dn);
> > > > > > -
> > > > > > -   of_node_put(dn);
> > > > > > -
> > > > > > -
> > > > > > -   if (naddrc == 2)
> > > > > > -           address = ((unsigned long) reg[0] << 32) | reg[1];
> > > > > > -   else
> > > > > > -           address = reg[0];
> > > > > > -
> > > > > > -   if (nsizec == 2)
> > > > > > -           size =
> > > > > > -               ((unsigned long) reg[naddrc] << 32) | reg[naddrc + 1];
> > > > > > -   else
> > > > > > -           size = reg[naddrc];
> > > > > > -
> > > > > > -   *base = address;
> > > > > > -   *region_size = size;
> > > > > > -   return ioremap(*base, *region_size);
> > > > > > -}
> > > > > > -#else
> > > > > > -#define atmel_getb(chip, offset) inb(atmel_get_priv(chip)->base + offset)
> > > > > > -#define atmel_putb(val, chip, offset) \
> > > > > > -   outb(val, atmel_get_priv(chip)->base + offset)
> > > > > > -#define atmel_request_region request_region
> > > > > > -#define atmel_release_region release_region
> > > > > > -/* Atmel definitions */
> > > > > > -enum tpm_atmel_addr {
> > > > > > -   TPM_ATMEL_BASE_ADDR_LO = 0x08,
> > > > > > -   TPM_ATMEL_BASE_ADDR_HI = 0x09
> > > > > > -};
> > > > > > -
> > > > > > -static inline int tpm_read_index(int base, int index)
> > > > > > -{
> > > > > > -   outb(index, base);
> > > > > > -   return inb(base+1) & 0xFF;
> > > > > > -}
> > > > > > -
> > > > > > -/* Verify this is a 1.1 Atmel TPM */
> > > > > > -static int atmel_verify_tpm11(void)
> > > > > > -{
> > > > > > -
> > > > > > -   /* verify that it is an Atmel part */
> > > > > > -   if (tpm_read_index(TPM_ADDR, 4) != 'A' ||
> > > > > > -       tpm_read_index(TPM_ADDR, 5) != 'T' ||
> > > > > > -       tpm_read_index(TPM_ADDR, 6) != 'M' ||
> > > > > > -       tpm_read_index(TPM_ADDR, 7) != 'L')
> > > > > > -           return 1;
> > > > > > -
> > > > > > -   /* query chip for its version number */
> > > > > > -   if (tpm_read_index(TPM_ADDR, 0x00) != 1 ||
> > > > > > -       tpm_read_index(TPM_ADDR, 0x01) != 1)
> > > > > > -           return 1;
> > > > > > -
> > > > > > -   /* This is an atmel supported part */
> > > > > > -   return 0;
> > > > > > -}
> > > > > > -
> > > > > > -static inline void atmel_put_base_addr(void __iomem *iobase)
> > > > > > -{
> > > > > > -}
> > > > > > -
> > > > > > -/* Determine where to talk to device */
> > > > > > -static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> > > > > > -{
> > > > > > -   int lo, hi;
> > > > > > -
> > > > > > -   if (atmel_verify_tpm11() != 0)
> > > > > > -           return NULL;
> > > > > > -
> > > > > > -   lo = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_LO);
> > > > > > -   hi = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_HI);
> > > > > > -
> > > > > > -   *base = (hi << 8) | lo;
> > > > > > -   *region_size = 2;
> > > > > > -
> > > > > > -   return ioport_map(*base, *region_size);
> > > > > > -}
> > > > > > -#endif
> > > > >
> > > > > Responding from holidays but:
> > > > >
> > > > > Reviewed-by: Jarkko Sakkinen <jarkko@...nel.org>
> > > > >
> > > > > [still away for couple of weeks]
> > > >
> > > > I got these in with checkpatch.pl --strict:
> > > >
> > > > CHECK: Macro argument 'offset' may be better as '(offset)' to avoid precedence issues
> > > > #59: FILE: drivers/char/tpm/tpm_atmel.c:26:
> > > > +#define atmel_getb(chip, offset) inb(atmel_get_priv(chip)->base + offset)
> > > >
> > > > CHECK: Macro argument 'offset' may be better as '(offset)' to avoid precedence issues
> > > > #60: FILE: drivers/char/tpm/tpm_atmel.c:27:
> > > > +#define atmel_putb(val, chip, offset) \
> > > > +       outb(val, atmel_get_priv(chip)->base + offset)
> > > >
> > > > CHECK: spaces preferred around that '+' (ctx:VxV)
> > > > #73: FILE: drivers/char/tpm/tpm_atmel.c:40:
> > > > +       return inb(base+1) & 0xFF;
> > > >                       ^
> > > >
> > > > CHECK: Blank lines aren't necessary after an open brace '{'
> > > > #79: FILE: drivers/char/tpm/tpm_atmel.c:46:
> > > > +{
> > > > +
> > > >
> > > > Can you address them and I'll tag the next version, once I've
> > > > revisited checkpatch. Otherwise, nothing against the code change.
> > >
> > > Those all existed before because I just moved what was left of the
> > > header contents into the .c file. Fixing them seems like a separate
> > > change to me. I can just leave the header in place and avoid the
> > > warnings if you prefer. Otherwise, those warnings are the least of the
> > > clean-up this driver needs. For starters, I would make those defines
> > > static inlines instead.
> >
> > Ah, ok sorry. It was fairly mechanical check as I'm just quickly
> > going critical stuff from holidays.
> >
> > I'm back after next week, so I'd really like to hold with this
> > up until that and prepare when I'm back a patch set, which includes
> > a supplemental patch fixing those issues (if you don't mind).
>
> Whatever happened to this? Can you please apply my patch if you don't
> have the time for further rework.

Sorry unintentional.

I applied with

-static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
+static void __iomem *atmel_get_base_addr(unsigned long *base, int *region_size)
 
as this gives checkpatch error.

> Rob

BR, Jarkko

Powered by blists - more mailing lists