LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Williams, Dan J" <dan.j.williams@intel.com>
To: "Winiarska, Iwona" <iwona.winiarska@intel.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"openbmc@lists.ozlabs.org" <openbmc@lists.ozlabs.org>
Cc: "corbet@lwn.net" <corbet@lwn.net>,
	"jae.hyun.yoo@linux.intel.com" <jae.hyun.yoo@linux.intel.com>,
	"linux-hwmon@vger.kernel.org" <linux-hwmon@vger.kernel.org>,
	"andrew@aj.id.au" <andrew@aj.id.au>,
	"Luck, Tony" <tony.luck@intel.com>,
	"Lutomirski, Andy" <luto@kernel.org>,
	"andriy.shevchenko@linux.intel.com" 
	<andriy.shevchenko@linux.intel.com>,
	"mchehab@kernel.org" <mchehab@kernel.org>,
	"jdelvare@suse.com" <jdelvare@suse.com>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"linux-aspeed@lists.ozlabs.org" <linux-aspeed@lists.ozlabs.org>,
	"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
	"linux@roeck-us.net" <linux@roeck-us.net>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"yazen.ghannam@amd.com" <yazen.ghannam@amd.com>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"joel@jms.id.au" <joel@jms.id.au>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"pierre-louis.bossart@linux.intel.com" 
	<pierre-louis.bossart@linux.intel.com>,
	"x86@kernel.org" <x86@kernel.org>, "bp@alien8.de" <bp@alien8.de>
Subject: Re: [PATCH 07/14] peci: Add peci-aspeed controller driver
Date: Wed, 14 Jul 2021 17:39:44 +0000	[thread overview]
Message-ID: <5a567358002b06e9246e83bfa6dd60a0b119a763.camel@intel.com> (raw)
In-Reply-To: <20210712220447.957418-8-iwona.winiarska@intel.com>

On Tue, 2021-07-13 at 00:04 +0200, Iwona Winiarska wrote:
> From: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> 
> ASPEED AST24xx/AST25xx/AST26xx SoCs supports the PECI electrical
> interface (a.k.a PECI wire).
> 
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> Co-developed-by: Iwona Winiarska <iwona.winiarska@intel.com>
> Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  MAINTAINERS                           |   9 +
>  drivers/peci/Kconfig                  |   6 +
>  drivers/peci/Makefile                 |   3 +
>  drivers/peci/controller/Kconfig       |  12 +
>  drivers/peci/controller/Makefile      |   3 +
>  drivers/peci/controller/peci-aspeed.c | 501 ++++++++++++++++++++++++++
>  6 files changed, 534 insertions(+)
>  create mode 100644 drivers/peci/controller/Kconfig
>  create mode 100644 drivers/peci/controller/Makefile
>  create mode 100644 drivers/peci/controller/peci-aspeed.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 47411e2b6336..4ba874afa2fa 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2865,6 +2865,15 @@ S:       Maintained
>  F:     Documentation/hwmon/asc7621.rst
>  F:     drivers/hwmon/asc7621.c
>  
> +ASPEED PECI CONTROLLER
> +M:     Iwona Winiarska <iwona.winiarska@intel.com>
> +M:     Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> +L:     linux-aspeed@lists.ozlabs.org (moderated for non-subscribers)
> +L:     openbmc@lists.ozlabs.org (moderated for non-subscribers)
> +S:     Supported
> +F:     Documentation/devicetree/bindings/peci/peci-aspeed.yaml
> +F:     drivers/peci/controller/peci-aspeed.c
> +
>  ASPEED PINCTRL DRIVERS
>  M:     Andrew Jeffery <andrew@aj.id.au>
>  L:     linux-aspeed@lists.ozlabs.org (moderated for non-subscribers)
> diff --git a/drivers/peci/Kconfig b/drivers/peci/Kconfig
> index 601cc3c3c852..0d0ee8009713 100644
> --- a/drivers/peci/Kconfig
> +++ b/drivers/peci/Kconfig
> @@ -12,3 +12,9 @@ menuconfig PECI
>  
>           This support is also available as a module. If so, the module
>           will be called peci.
> +
> +if PECI
> +
> +source "drivers/peci/controller/Kconfig"
> +
> +endif # PECI
> diff --git a/drivers/peci/Makefile b/drivers/peci/Makefile
> index 2bb2f51bcda7..621a993e306a 100644
> --- a/drivers/peci/Makefile
> +++ b/drivers/peci/Makefile
> @@ -3,3 +3,6 @@
>  # Core functionality
>  peci-y := core.o sysfs.o
>  obj-$(CONFIG_PECI) += peci.o
> +
> +# Hardware specific bus drivers
> +obj-y += controller/
> diff --git a/drivers/peci/controller/Kconfig b/drivers/peci/controller/Kconfig
> new file mode 100644
> index 000000000000..8ddbe494677f
> --- /dev/null
> +++ b/drivers/peci/controller/Kconfig
> @@ -0,0 +1,12 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +config PECI_ASPEED
> +       tristate "ASPEED PECI support"
> +       depends on ARCH_ASPEED || COMPILE_TEST
> +       depends on OF
> +       depends on HAS_IOMEM
> +       help
> +         Enable this driver if you want to support ASPEED PECI controller.

Perhaps a note about how one might make this determination, or maybe a
general recommendation that if they are building for deployment on an
OpenBMC system say Y else say N?

> +
> +         This driver can be also build as a module. If so, the module
> +         will be called peci-aspeed.
> diff --git a/drivers/peci/controller/Makefile b/drivers/peci/controller/Makefile
> new file mode 100644
> index 000000000000..022c28ef1bf0
> --- /dev/null
> +++ b/drivers/peci/controller/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +obj-$(CONFIG_PECI_ASPEED)      += peci-aspeed.o
> diff --git a/drivers/peci/controller/peci-aspeed.c b/drivers/peci/controller/peci-aspeed.c
> new file mode 100644
> index 000000000000..888b46383ea4
> --- /dev/null
> +++ b/drivers/peci/controller/peci-aspeed.c
> @@ -0,0 +1,501 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +// Copyright (C) 2012-2017 ASPEED Technology Inc.
> +// Copyright (c) 2018-2021 Intel Corporation
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/jiffies.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/peci.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +
> +#include <asm/unaligned.h>
> +
> +/* ASPEED PECI Registers */
> +/* Control Register */
> +#define ASPEED_PECI_CTRL                       0x00
> +#define   ASPEED_PECI_CTRL_SAMPLING_MASK       GENMASK(19, 16)
> +#define   ASPEED_PECI_CTRL_READ_MODE_MASK      GENMASK(13, 12)
> +#define   ASPEED_PECI_CTRL_READ_MODE_COUNT     BIT(12)
> +#define   ASPEED_PECI_CTRL_READ_MODE_DBG       BIT(13)
> +#define   ASPEED_PECI_CTRL_CLK_SOURCE_MASK     BIT(11)
> +#define   ASPEED_PECI_CTRL_CLK_DIV_MASK                GENMASK(10, 8)
> +#define   ASPEED_PECI_CTRL_INVERT_OUT          BIT(7)
> +#define   ASPEED_PECI_CTRL_INVERT_IN           BIT(6)
> +#define   ASPEED_PECI_CTRL_BUS_CONTENT_EN      BIT(5)
> +#define   ASPEED_PECI_CTRL_PECI_EN             BIT(4)
> +#define   ASPEED_PECI_CTRL_PECI_CLK_EN         BIT(0)
> +
> +/* Timing Negotiation Register */
> +#define ASPEED_PECI_TIMING_NEGOTIATION         0x04
> +#define   ASPEED_PECI_TIMING_MESSAGE_MASK      GENMASK(15, 8)
> +#define   ASPEED_PECI_TIMING_ADDRESS_MASK      GENMASK(7, 0)
> +
> +/* Command Register */
> +#define ASPEED_PECI_CMD                                0x08
> +#define   ASPEED_PECI_CMD_PIN_MON              BIT(31)
> +#define   ASPEED_PECI_CMD_STS_MASK             GENMASK(27, 24)
> +#define     ASPEED_PECI_CMD_STS_ADDR_T_NEGO    0x3
> +#define   ASPEED_PECI_CMD_IDLE_MASK            \
> +         (ASPEED_PECI_CMD_STS_MASK | ASPEED_PECI_CMD_PIN_MON)
> +#define   ASPEED_PECI_CMD_FIRE                 BIT(0)
> +
> +/* Read/Write Length Register */
> +#define ASPEED_PECI_RW_LENGTH                  0x0c
> +#define   ASPEED_PECI_AW_FCS_EN                        BIT(31)
> +#define   ASPEED_PECI_READ_LEN_MASK            GENMASK(23, 16)
> +#define   ASPEED_PECI_WRITE_LEN_MASK           GENMASK(15, 8)
> +#define   ASPEED_PECI_TAGET_ADDR_MASK          GENMASK(7, 0)
> +
> +/* Expected FCS Data Register */
> +#define ASPEED_PECI_EXP_FCS                    0x10
> +#define   ASPEED_PECI_EXP_READ_FCS_MASK                GENMASK(23, 16)
> +#define   ASPEED_PECI_EXP_AW_FCS_AUTO_MASK     GENMASK(15, 8)
> +#define   ASPEED_PECI_EXP_WRITE_FCS_MASK       GENMASK(7, 0)
> +
> +/* Captured FCS Data Register */
> +#define ASPEED_PECI_CAP_FCS                    0x14
> +#define   ASPEED_PECI_CAP_READ_FCS_MASK                GENMASK(23, 16)
> +#define   ASPEED_PECI_CAP_WRITE_FCS_MASK       GENMASK(7, 0)
> +
> +/* Interrupt Register */
> +#define ASPEED_PECI_INT_CTRL                   0x18
> +#define   ASPEED_PECI_TIMING_NEGO_SEL_MASK     GENMASK(31, 30)
> +#define     ASPEED_PECI_1ST_BIT_OF_ADDR_NEGO   0
> +#define     ASPEED_PECI_2ND_BIT_OF_ADDR_NEGO   1
> +#define     ASPEED_PECI_MESSAGE_NEGO           2
> +#define   ASPEED_PECI_INT_MASK                 GENMASK(4, 0)
> +#define   ASPEED_PECI_INT_BUS_TIMEOUT          BIT(4)
> +#define   ASPEED_PECI_INT_BUS_CONNECT          BIT(3)
> +#define   ASPEED_PECI_INT_W_FCS_BAD            BIT(2)
> +#define   ASPEED_PECI_INT_W_FCS_ABORT          BIT(1)
> +#define   ASPEED_PECI_INT_CMD_DONE             BIT(0)
> +
> +/* Interrupt Status Register */
> +#define ASPEED_PECI_INT_STS                    0x1c
> +#define   ASPEED_PECI_INT_TIMING_RESULT_MASK   GENMASK(29, 16)
> +         /* bits[4..0]: Same bit fields in the 'Interrupt Register' */
> +
> +/* Rx/Tx Data Buffer Registers */
> +#define ASPEED_PECI_W_DATA0                    0x20
> +#define ASPEED_PECI_W_DATA1                    0x24
> +#define ASPEED_PECI_W_DATA2                    0x28
> +#define ASPEED_PECI_W_DATA3                    0x2c
> +#define ASPEED_PECI_R_DATA0                    0x30
> +#define ASPEED_PECI_R_DATA1                    0x34
> +#define ASPEED_PECI_R_DATA2                    0x38
> +#define ASPEED_PECI_R_DATA3                    0x3c
> +#define ASPEED_PECI_W_DATA4                    0x40
> +#define ASPEED_PECI_W_DATA5                    0x44
> +#define ASPEED_PECI_W_DATA6                    0x48
> +#define ASPEED_PECI_W_DATA7                    0x4c
> +#define ASPEED_PECI_R_DATA4                    0x50
> +#define ASPEED_PECI_R_DATA5                    0x54
> +#define ASPEED_PECI_R_DATA6                    0x58
> +#define ASPEED_PECI_R_DATA7                    0x5c
> +#define   ASPEED_PECI_DATA_BUF_SIZE_MAX                32
> +
> +/* Timing Negotiation */
> +#define ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT  8
> +#define ASPEED_PECI_RD_SAMPLING_POINT_MAX      (BIT(4) - 1)
> +#define ASPEED_PECI_CLK_DIV_DEFAULT            0
> +#define ASPEED_PECI_CLK_DIV_MAX                        (BIT(3) - 1)
> +#define ASPEED_PECI_MSG_TIMING_DEFAULT         1
> +#define ASPEED_PECI_MSG_TIMING_MAX             (BIT(8) - 1)
> +#define ASPEED_PECI_ADDR_TIMING_DEFAULT                1
> +#define ASPEED_PECI_ADDR_TIMING_MAX            (BIT(8) - 1)
> +
> +/* Timeout */
> +#define ASPEED_PECI_IDLE_CHECK_TIMEOUT_US      (50 * USEC_PER_MSEC)
> +#define ASPEED_PECI_IDLE_CHECK_INTERVAL_US     (10 * USEC_PER_MSEC)
> +#define ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT     (1000)
> +#define ASPEED_PECI_CMD_TIMEOUT_MS_MAX         (1000)
> +
> +struct aspeed_peci {
> +       struct peci_controller controller;

Uh oh... this looks like a driver private data structure, and I know
there's a 'struct device' allocated in @controller. /me goes to check
->probe()...

> +       struct device *dev;
> +       void __iomem *base;
> +       struct clk *clk;
> +       struct reset_control *rst;
> +       int irq;
> +       spinlock_t lock; /* to sync completion status handling */
> +       struct completion xfer_complete;
> +       u32 status;
> +       u32 cmd_timeout_ms;
> +       u32 msg_timing;
> +       u32 addr_timing;
> +       u32 rd_sampling_point;
> +       u32 clk_div;
> +};
> +
> +static inline struct aspeed_peci *to_aspeed_peci(struct peci_controller *a)
> +{
> +       return container_of(a, struct aspeed_peci, controller);
> +}
> +
> +static void aspeed_peci_init_regs(struct aspeed_peci *priv)
> +{
> +       u32 val;
> +
> +       val = FIELD_PREP(ASPEED_PECI_CTRL_CLK_DIV_MASK, ASPEED_PECI_CLK_DIV_DEFAULT);
> +       val |= ASPEED_PECI_CTRL_PECI_CLK_EN;
> +       writel(val, priv->base + ASPEED_PECI_CTRL);
> +       /*
> +        * Timing negotiation period setting.
> +        * The unit of the programmed value is 4 times of PECI clock period.
> +        */
> +       val = FIELD_PREP(ASPEED_PECI_TIMING_MESSAGE_MASK, priv->msg_timing);
> +       val |= FIELD_PREP(ASPEED_PECI_TIMING_ADDRESS_MASK, priv->addr_timing);
> +       writel(val, priv->base + ASPEED_PECI_TIMING_NEGOTIATION);
> +
> +       /* Clear interrupts */
> +       val = readl(priv->base + ASPEED_PECI_INT_STS) | ASPEED_PECI_INT_MASK;
> +       writel(val, priv->base + ASPEED_PECI_INT_STS);
> +
> +       /* Set timing negotiation mode and enable interrupts */
> +       val = FIELD_PREP(ASPEED_PECI_TIMING_NEGO_SEL_MASK, ASPEED_PECI_1ST_BIT_OF_ADDR_NEGO);
> +       val |= ASPEED_PECI_INT_MASK;
> +       writel(val, priv->base + ASPEED_PECI_INT_CTRL);
> +
> +       val = FIELD_PREP(ASPEED_PECI_CTRL_SAMPLING_MASK, priv->rd_sampling_point);
> +       val |= FIELD_PREP(ASPEED_PECI_CTRL_CLK_DIV_MASK, priv->clk_div);
> +       val |= ASPEED_PECI_CTRL_PECI_EN;
> +       val |= ASPEED_PECI_CTRL_PECI_CLK_EN;
> +       writel(val, priv->base + ASPEED_PECI_CTRL);

Do these MMIO access follow a standard? I.e. is there any possibility
to have a common / generic MMIO xfer function, but just pass in a
different base address discovered by the PECI device rather than a
fully custom xfer function per controller?

> +}
> +
> +static inline int aspeed_peci_check_idle(struct aspeed_peci *priv)
> +{
> +       u32 cmd_sts = readl(priv->base + ASPEED_PECI_CMD);
> +
> +       if (FIELD_GET(ASPEED_PECI_CMD_STS_MASK, cmd_sts) == ASPEED_PECI_CMD_STS_ADDR_T_NEGO)
> +               aspeed_peci_init_regs(priv);
> +
> +       return readl_poll_timeout(priv->base + ASPEED_PECI_CMD,
> +                                 cmd_sts,
> +                                 !(cmd_sts & ASPEED_PECI_CMD_IDLE_MASK),
> +                                 ASPEED_PECI_IDLE_CHECK_INTERVAL_US,
> +                                 ASPEED_PECI_IDLE_CHECK_TIMEOUT_US);
> +}
> +
> +static int aspeed_peci_xfer(struct peci_controller *controller,
> +                           u8 addr, struct peci_request *req)
> +{
> +       struct aspeed_peci *priv = to_aspeed_peci(controller);
> +       unsigned long flags, timeout = msecs_to_jiffies(priv->cmd_timeout_ms);
> +       u32 peci_head;
> +       int ret;
> +
> +       if (req->tx.len > ASPEED_PECI_DATA_BUF_SIZE_MAX ||
> +           req->rx.len > ASPEED_PECI_DATA_BUF_SIZE_MAX)
> +               return -EINVAL;
> +
> +       /* Check command sts and bus idle state */
> +       ret = aspeed_peci_check_idle(priv);
> +       if (ret)
> +               return ret; /* -ETIMEDOUT */
> +
> +       spin_lock_irqsave(&priv->lock, flags);
> +       reinit_completion(&priv->xfer_complete);
> +
> +       peci_head = FIELD_PREP(ASPEED_PECI_TAGET_ADDR_MASK, addr) |
> +                   FIELD_PREP(ASPEED_PECI_WRITE_LEN_MASK, req->tx.len) |
> +                   FIELD_PREP(ASPEED_PECI_READ_LEN_MASK, req->rx.len);
> +
> +       writel(peci_head, priv->base + ASPEED_PECI_RW_LENGTH);
> +
> +       memcpy_toio(priv->base + ASPEED_PECI_W_DATA0, req->tx.buf,
> +                   req->tx.len > 16 ? 16 : req->tx.len);
> +       if (req->tx.len > 16)
> +               memcpy_toio(priv->base + ASPEED_PECI_W_DATA4, req->tx.buf + 16,
> +                           req->tx.len - 16);
> +
> +       dev_dbg(priv->dev, "HEAD : 0x%08x\n", peci_head);
> +       print_hex_dump_bytes("TX : ", DUMP_PREFIX_NONE, req->tx.buf, req->tx.len);
> +
> +       priv->status = 0;
> +       writel(ASPEED_PECI_CMD_FIRE, priv->base + ASPEED_PECI_CMD);
> +       spin_unlock_irqrestore(&priv->lock, flags);
> +
> +       ret = wait_for_completion_interruptible_timeout(&priv->xfer_complete, timeout);
> +       if (ret < 0)
> +               return ret;
> +
> +       if (ret == 0) {
> +               dev_dbg(priv->dev, "Timeout waiting for a response!\n");
> +               return -ETIMEDOUT;
> +       }
> +
> +       spin_lock_irqsave(&priv->lock, flags);
> +
> +       writel(0, priv->base + ASPEED_PECI_CMD);
> +
> +       if (priv->status != ASPEED_PECI_INT_CMD_DONE) {
> +               spin_unlock_irqrestore(&priv->lock, flags);
> +               dev_dbg(priv->dev, "No valid response!\n");
> +               return -EIO;
> +       }
> +
> +       spin_unlock_irqrestore(&priv->lock, flags);
> +
> +       memcpy_fromio(req->rx.buf, priv->base + ASPEED_PECI_R_DATA0,
> +                     req->rx.len > 16 ? 16 : req->rx.len);
> +       if (req->rx.len > 16)
> +               memcpy_fromio(req->rx.buf + 16, priv->base + ASPEED_PECI_R_DATA4,
> +                             req->rx.len - 16);
> +
> +       print_hex_dump_bytes("RX : ", DUMP_PREFIX_NONE, req->rx.buf, req->rx.len);

If dynamic debug is not enabled this will be an unconditional
printk(KERN_DEBUG.

I'm ok with dev_dbg() in slow paths, but in fast paths you should look
to tracing, or putting potentially heavyweight debug behind a
CONFIG_X_DEBUG option. I have seen Greg is even less of a fan of
dev_dbg().

> +
> +       return 0;
> +}
> +
> +static irqreturn_t aspeed_peci_irq_handler(int irq, void *arg)
> +{
> +       struct aspeed_peci *priv = arg;
> +       u32 status;
> +
> +       spin_lock(&priv->lock);
> +       status = readl(priv->base + ASPEED_PECI_INT_STS);
> +       writel(status, priv->base + ASPEED_PECI_INT_STS);
> +       priv->status |= (status & ASPEED_PECI_INT_MASK);
> +
> +       /*
> +        * In most cases, interrupt bits will be set one by one but also note
> +        * that multiple interrupt bits could be set at the same time.
> +        */
> +       if (status & ASPEED_PECI_INT_BUS_TIMEOUT)
> +               dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_BUS_TIMEOUT\n");
> +
> +       if (status & ASPEED_PECI_INT_BUS_CONNECT)
> +               dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_BUS_CONNECT\n");
> +
> +       if (status & ASPEED_PECI_INT_W_FCS_BAD)
> +               dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_W_FCS_BAD\n");
> +
> +       if (status & ASPEED_PECI_INT_W_FCS_ABORT)
> +               dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_W_FCS_ABORT\n");

What's the utility of these debug statements? If they are for
development than maybe they are ok, if they are for debug in the field
I would make them counters and export them via debugfs, or sysfs if
you expect to always be able to debug these events in case a kernel in
the field has dev_dbg and debugfs disabled.

> +
> +       /*
> +        * All commands should be ended up with a ASPEED_PECI_INT_CMD_DONE bit
> +        * set even in an error case.
> +        */
> +       if (status & ASPEED_PECI_INT_CMD_DONE)
> +               complete(&priv->xfer_complete);
> +
> +       spin_unlock(&priv->lock);
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static void __sanitize_clock_divider(struct aspeed_peci *priv)
> +{
> +       u32 clk_div;
> +       int ret;
> +
> +       ret = device_property_read_u32(priv->dev, "clock-divider", &clk_div);
> +       if (ret) {
> +               clk_div = ASPEED_PECI_CLK_DIV_DEFAULT;
> +       } else if (clk_div > ASPEED_PECI_CLK_DIV_MAX) {
> +               dev_warn(priv->dev, "Invalid clock-divider: %u, Using default: %u\n",
> +                        clk_div, ASPEED_PECI_CLK_DIV_DEFAULT);
> +
> +               clk_div = ASPEED_PECI_CLK_DIV_DEFAULT;
> +       }
> +
> +       priv->clk_div = clk_div;
> +}
> +
> +static void __sanitize_msg_timing(struct aspeed_peci *priv)
> +{
> +       u32 msg_timing;
> +       int ret;
> +
> +       ret = device_property_read_u32(priv->dev, "msg-timing", &msg_timing);
> +       if (ret) {
> +               msg_timing = ASPEED_PECI_MSG_TIMING_DEFAULT;
> +       } else if (msg_timing > ASPEED_PECI_MSG_TIMING_MAX) {
> +               dev_warn(priv->dev, "Invalid msg-timing : %u, Use default : %u\n",
> +                        msg_timing, ASPEED_PECI_MSG_TIMING_DEFAULT);
> +
> +               msg_timing = ASPEED_PECI_MSG_TIMING_DEFAULT;
> +       }
> +
> +       priv->msg_timing = msg_timing;
> +}
> +
> +static void __sanitize_addr_timing(struct aspeed_peci *priv)
> +{
> +       u32 addr_timing;
> +       int ret;
> +
> +       ret = device_property_read_u32(priv->dev, "addr-timing", &addr_timing);
> +       if (ret) {
> +               addr_timing = ASPEED_PECI_ADDR_TIMING_DEFAULT;
> +       } else if (addr_timing > ASPEED_PECI_ADDR_TIMING_MAX) {
> +               dev_warn(priv->dev, "Invalid addr-timing : %u, Use default : %u\n",
> +                        addr_timing, ASPEED_PECI_ADDR_TIMING_DEFAULT);
> +
> +               addr_timing = ASPEED_PECI_ADDR_TIMING_DEFAULT;
> +       }
> +
> +       priv->addr_timing = addr_timing;
> +}
> +
> +static void __sanitize_rd_sampling_point(struct aspeed_peci *priv)
> +{
> +       u32 rd_sampling_point;
> +       int ret;
> +
> +       ret = device_property_read_u32(priv->dev, "rd-sampling-point", &rd_sampling_point);
> +       if (ret) {
> +               rd_sampling_point = ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT;
> +       } else if (rd_sampling_point > ASPEED_PECI_RD_SAMPLING_POINT_MAX) {
> +               dev_warn(priv->dev, "Invalid rd-sampling-point: %u, Use default : %u\n",
> +                        rd_sampling_point, ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT);
> +
> +               rd_sampling_point = ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT;
> +       }
> +
> +       priv->rd_sampling_point = rd_sampling_point;
> +}
> +
> +static void __sanitize_cmd_timeout(struct aspeed_peci *priv)
> +{
> +       u32 timeout;
> +       int ret;
> +
> +       ret = device_property_read_u32(priv->dev, "cmd-timeout-ms", &timeout);
> +       if (ret) {
> +               timeout = ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT;
> +       } else if (timeout > ASPEED_PECI_CMD_TIMEOUT_MS_MAX || timeout == 0) {
> +               dev_warn(priv->dev, "Invalid cmd-timeout-ms: %u, Use default: %u\n",
> +                        timeout, ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT);
> +

For all of the same pattern like this above I would say "falling back
to: %u" otherwise "Use default" sounds like an action the platform
owner is expected to take.

Also, if the driver is correcting the issue does the log need to be
spammed with a warning? Is this 'info' or 'debug'?

> +               timeout = ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT;
> +       }
> +
> +       priv->cmd_timeout_ms = timeout;
> +}
> +
> +static void aspeed_peci_device_property_sanitize(struct aspeed_peci *priv)
> +{
> +       __sanitize_clock_divider(priv);
> +       __sanitize_msg_timing(priv);
> +       __sanitize_addr_timing(priv);
> +       __sanitize_rd_sampling_point(priv);
> +       __sanitize_cmd_timeout(priv);
> +}
> +
> +static void aspeed_peci_disable_clk(void *data)
> +{
> +       clk_disable_unprepare(data);
> +}
> +
> +static int aspeed_peci_init_ctrl(struct aspeed_peci *priv)
> +{
> +       int ret;
> +
> +       priv->clk = devm_clk_get(priv->dev, NULL);
> +       if (IS_ERR(priv->clk))
> +               return dev_err_probe(priv->dev, PTR_ERR(priv->clk), "Failed to get clk source\n");
> +
> +       ret = clk_prepare_enable(priv->clk);
> +       if (ret) {
> +               dev_err(priv->dev, "Failed to enable clock\n");
> +               return ret;
> +       }
> +
> +       ret = devm_add_action_or_reset(priv->dev, aspeed_peci_disable_clk, priv->clk);
> +       if (ret)
> +               return ret;
> +
> +       aspeed_peci_device_property_sanitize(priv);
> +
> +       aspeed_peci_init_regs(priv);
> +
> +       return 0;
> +}
> +
> +static int aspeed_peci_probe(struct platform_device *pdev)
> +{
> +       struct aspeed_peci *priv;
> +       int ret;
> +
> +       priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);

..."uh oh" from above confirmed. devm allocation lifetime and 'struct
device' lifetime are not compatible.

You can trigger use after free bugs by turning on
CONFIG_DEBUG_KOBJECT_RELEASE.

devm can be used to automatically unregister the peci_controller
device. The flow would be something like:

priv = devm_kzalloc(..., sizeof(*priv), ...);
controller = peci_controller_alloc(...);
if (IS_ERR(controller))
    return PTR_ERR(controller);
rc = devm_peci_controller_add(...)
if (rc)
    return rc;

This arranges for the peci_controller_alloc() to be undone by
put_device() in all cases. Internal to peci_controller_alloc() is
typical goto unwind allocation error handling.

> +       if (!priv)
> +               return -ENOMEM;
> +
> +       priv->dev = &pdev->dev;
> +       dev_set_drvdata(priv->dev, priv);
> +
> +       priv->base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(priv->base))
> +               return PTR_ERR(priv->base);
> +
> +       priv->irq = platform_get_irq(pdev, 0);
> +       if (!priv->irq)
> +               return priv->irq;
> +
> +       ret = devm_request_irq(&pdev->dev, priv->irq, aspeed_peci_irq_handler,
> +                              0, "peci-aspeed-irq", priv);
> +       if (ret)
> +               return ret;
> +
> +       init_completion(&priv->xfer_complete);
> +       spin_lock_init(&priv->lock);
> +
> +       priv->controller.xfer = aspeed_peci_xfer;
> +
> +       priv->rst = devm_reset_control_get(&pdev->dev, NULL);
> +       if (IS_ERR(priv->rst)) {
> +               dev_err(&pdev->dev, "Missing or invalid reset controller entry\n");
> +               return PTR_ERR(priv->rst);
> +       }
> +       reset_control_deassert(priv->rst);
> +
> +       ret = aspeed_peci_init_ctrl(priv);
> +       if (ret)
> +               return ret;
> +
> +       return peci_controller_add(&priv->controller, priv->dev);
> +}
> +
> +static int aspeed_peci_remove(struct platform_device *pdev)
> +{
> +       struct aspeed_peci *priv = dev_get_drvdata(&pdev->dev);
> +
> +       peci_controller_remove(&priv->controller);
> +       reset_control_assert(priv->rst);
> +

It's odd to have devm in the probe path and still publish a remove
handler, i.e. why not handle controller removal and reset via devm?
The example above with devm_peci_controller_add() already assumes
peci_controller_remove is triggered by devm, reset assert can be
managed the same way.

> +       return 0;
> +}
> +
> +static const struct of_device_id aspeed_peci_of_table[] = {
> +       { .compatible = "aspeed,ast2400-peci", },
> +       { .compatible = "aspeed,ast2500-peci", },
> +       { .compatible = "aspeed,ast2600-peci", },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, aspeed_peci_of_table);
> +
> +static struct platform_driver aspeed_peci_driver = {
> +       .probe  = aspeed_peci_probe,
> +       .remove = aspeed_peci_remove,
> +       .driver = {
> +               .name           = "peci-aspeed",
> +               .of_match_table = aspeed_peci_of_table,
> +       },
> +};
> +module_platform_driver(aspeed_peci_driver);
> +
> +MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>");
> +MODULE_AUTHOR("Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>");

Same comments about MODULE_AUTHOR from patch 6, i.e. make sure this is
not duplicating what MAINTAINERS and git log handle.

I'll pause here until you've had a chance to consider fixes to the
devm vs 'struct device' lifetime issue.

> +MODULE_DESCRIPTION("ASPEED PECI driver");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(PECI);


  parent reply	other threads:[~2021-07-14 17:39 UTC|newest]

Thread overview: 72+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-12 22:04 [PATCH 00/14] Introduce PECI subsystem Iwona Winiarska
2021-07-12 22:04 ` [PATCH 01/14] x86/cpu: Move intel-family to arch-independent headers Iwona Winiarska
2021-07-14 16:54   ` Williams, Dan J
2021-07-15 16:47     ` Winiarska, Iwona
2021-07-15 18:13       ` Dan Williams
2021-07-15 18:29         ` Luck, Tony
2021-07-12 22:04 ` [PATCH 02/14] x86/cpu: Extract cpuid helpers to arch-independent Iwona Winiarska
2021-07-14 16:58   ` Williams, Dan J
2021-07-15 16:51     ` Winiarska, Iwona
2021-07-15 16:58       ` Winiarska, Iwona
2021-07-12 22:04 ` [PATCH 03/14] dt-bindings: Add generic bindings for PECI Iwona Winiarska
2021-07-12 22:04 ` [PATCH 04/14] dt-bindings: Add bindings for peci-aspeed Iwona Winiarska
2021-07-15 16:28   ` Rob Herring
2021-07-16 21:22     ` Winiarska, Iwona
2021-07-12 22:04 ` [PATCH 05/14] ARM: dts: aspeed: Add PECI controller nodes Iwona Winiarska
2021-07-12 22:04 ` [PATCH 06/14] peci: Add core infrastructure Iwona Winiarska
2021-07-14 17:19   ` Williams, Dan J
2021-07-16 21:08     ` Winiarska, Iwona
2021-07-16 21:50       ` Dan Williams
2021-07-17  6:12         ` gregkh
2021-07-17 20:54           ` Dan Williams
2021-07-12 22:04 ` [PATCH 07/14] peci: Add peci-aspeed controller driver Iwona Winiarska
2021-07-13  5:02   ` Randy Dunlap
2021-07-15 16:42     ` Winiarska, Iwona
2021-07-14 17:39   ` Williams, Dan J [this message]
2021-07-16 21:17     ` Winiarska, Iwona
2021-07-27  8:49   ` Zev Weiss
2021-07-29 14:03     ` Winiarska, Iwona
2021-07-29 18:15       ` Zev Weiss
2021-07-12 22:04 ` [PATCH 08/14] peci: Add device detection Iwona Winiarska
2021-07-14 21:05   ` Williams, Dan J
2021-07-16 21:20     ` Winiarska, Iwona
2021-07-27 17:49   ` Zev Weiss
2021-07-29 18:55     ` Winiarska, Iwona
2021-07-29 20:50       ` Zev Weiss
2021-07-30 20:10         ` Winiarska, Iwona
2021-07-12 22:04 ` [PATCH 09/14] peci: Add support for PECI device drivers Iwona Winiarska
2021-07-27 20:10   ` Zev Weiss
2021-07-27 21:23     ` Guenter Roeck
2021-07-29 21:17     ` Winiarska, Iwona
2021-07-29 23:22       ` Zev Weiss
2021-07-30 20:13         ` Winiarska, Iwona
2021-07-12 22:04 ` [PATCH 10/14] peci: Add peci-cpu driver Iwona Winiarska
2021-07-27 11:16   ` David Müller (ELSOFT AG)
2021-07-30 20:14     ` Winiarska, Iwona
2021-07-27 21:33   ` Zev Weiss
2021-07-30 21:21     ` Winiarska, Iwona
2021-07-12 22:04 ` [PATCH 11/14] hwmon: peci: Add cputemp driver Iwona Winiarska
2021-07-15 17:45   ` Guenter Roeck
2021-07-19 20:12     ` Winiarska, Iwona
2021-07-19 20:35       ` Guenter Roeck
2021-07-27  7:06   ` Zev Weiss
2021-07-30 21:51     ` Winiarska, Iwona
2021-07-30 22:04       ` Guenter Roeck
2021-07-12 22:04 ` [PATCH 12/14] hwmon: peci: Add dimmtemp driver Iwona Winiarska
2021-07-15 17:56   ` Guenter Roeck
2021-07-19 20:31     ` Winiarska, Iwona
2021-07-19 20:36       ` Guenter Roeck
2021-07-26 22:08   ` Zev Weiss
2021-07-30 22:48     ` Winiarska, Iwona
2021-07-12 22:04 ` [PATCH 13/14] docs: hwmon: Document PECI drivers Iwona Winiarska
2021-07-27 22:58   ` Zev Weiss
2021-07-28  0:49     ` Guenter Roeck
2021-08-02 11:39       ` Winiarska, Iwona
2021-08-02 11:37     ` Winiarska, Iwona
2021-08-04 17:52       ` Zev Weiss
2021-08-04 18:05         ` Guenter Roeck
2021-08-05 21:42           ` Winiarska, Iwona
2021-07-12 22:04 ` [PATCH 14/14] docs: Add PECI documentation Iwona Winiarska
2021-07-14 16:51 ` [PATCH 00/14] Introduce PECI subsystem Williams, Dan J
2021-07-15 17:33   ` Winiarska, Iwona
2021-07-15 19:34     ` Dan Williams

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5a567358002b06e9246e83bfa6dd60a0b119a763.camel@intel.com \
    --to=dan.j.williams@intel.com \
    --cc=andrew@aj.id.au \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=bp@alien8.de \
    --cc=corbet@lwn.net \
    --cc=devicetree@vger.kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=iwona.winiarska@intel.com \
    --cc=jae.hyun.yoo@linux.intel.com \
    --cc=jdelvare@suse.com \
    --cc=joel@jms.id.au \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-aspeed@lists.ozlabs.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-hwmon@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=luto@kernel.org \
    --cc=mchehab@kernel.org \
    --cc=mingo@redhat.com \
    --cc=openbmc@lists.ozlabs.org \
    --cc=pierre-louis.bossart@linux.intel.com \
    --cc=robh+dt@kernel.org \
    --cc=tglx@linutronix.de \
    --cc=tony.luck@intel.com \
    --cc=x86@kernel.org \
    --cc=yazen.ghannam@amd.com \
    --subject='Re: [PATCH 07/14] peci: Add peci-aspeed controller driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).