From: "Winiarska, Iwona" <iwona.winiarska@intel.com>
To: "Williams, Dan J" <dan.j.williams@intel.com>
Cc: "jason.m.bills@linux.intel.com" <jason.m.bills@linux.intel.com>,
"linux-hwmon@vger.kernel.org" <linux-hwmon@vger.kernel.org>,
"tglx@linutronix.de" <tglx@linutronix.de>,
"robh+dt@kernel.org" <robh+dt@kernel.org>,
"zweiss@equinix.com" <zweiss@equinix.com>,
"Luck, Tony" <tony.luck@intel.com>,
"Lutomirski, Andy" <luto@kernel.org>,
"olof@lixom.net" <olof@lixom.net>,
"jae.hyun.yoo@linux.intel.com" <jae.hyun.yoo@linux.intel.com>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"openbmc@lists.ozlabs.org" <openbmc@lists.ozlabs.org>,
"corbet@lwn.net" <corbet@lwn.net>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"yazen.ghannam@amd.com" <yazen.ghannam@amd.com>,
"andrew@aj.id.au" <andrew@aj.id.au>,
"pierre-louis.bossart@linux.intel.com"
<pierre-louis.bossart@linux.intel.com>,
"jdelvare@suse.com" <jdelvare@suse.com>,
"x86@kernel.org" <x86@kernel.org>,
"d.mueller@elsoft.ch" <d.mueller@elsoft.ch>,
"rdunlap@infradead.org" <rdunlap@infradead.org>,
"bp@alien8.de" <bp@alien8.de>, "arnd@arndb.de" <arnd@arndb.de>,
"mchehab@kernel.org" <mchehab@kernel.org>,
"joel@jms.id.au" <joel@jms.id.au>,
"andriy.shevchenko@linux.intel.com"
<andriy.shevchenko@linux.intel.com>,
"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
"mingo@redhat.com" <mingo@redhat.com>,
"linux-aspeed@lists.ozlabs.org" <linux-aspeed@lists.ozlabs.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
"linux@roeck-us.net" <linux@roeck-us.net>
Subject: Re: [PATCH v2 06/15] peci: Add core infrastructure
Date: Thu, 26 Aug 2021 22:40:43 +0000 [thread overview]
Message-ID: <4aa3fac69a92b175beb59d2678eb914c35ea98b7.camel@intel.com> (raw)
In-Reply-To: <CAPcyv4giVBjZWjXO2H+vYSR29Vapi6_FvMwO7nvts-JXmCHcOA@mail.gmail.com>
On Wed, 2021-08-25 at 15:58 -0700, Dan Williams wrote:
> On Tue, Aug 3, 2021 at 4:35 AM Iwona Winiarska
> <iwona.winiarska@intel.com> wrote:
> >
> > Intel processors provide access for various services designed to support
> > processor and DRAM thermal management, platform manageability and
> > processor interface tuning and diagnostics.
> > Those services are available via the Platform Environment Control
> > Interface (PECI) that provides a communication channel between the
> > processor and the Baseboard Management Controller (BMC) or other
> > platform management device.
> >
> > This change introduces PECI subsystem by adding the initial core module
> > and API for controller drivers.
> >
> > Co-developed-by: Jason M Bills <jason.m.bills@linux.intel.com>
> > Signed-off-by: Jason M Bills <jason.m.bills@linux.intel.com>
> > Co-developed-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> > Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.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/Kconfig | 3 +
> > drivers/Makefile | 1 +
> > drivers/peci/Kconfig | 15 ++++
> > drivers/peci/Makefile | 5 ++
> > drivers/peci/core.c | 155 ++++++++++++++++++++++++++++++++++++++++
> > drivers/peci/internal.h | 16 +++++
> > include/linux/peci.h | 99 +++++++++++++++++++++++++
> > 8 files changed, 303 insertions(+)
> > create mode 100644 drivers/peci/Kconfig
> > create mode 100644 drivers/peci/Makefile
> > create mode 100644 drivers/peci/core.c
> > create mode 100644 drivers/peci/internal.h
> > create mode 100644 include/linux/peci.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 7cdab7229651..d411974aaa5e 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -14503,6 +14503,15 @@ L: platform-driver-x86@vger.kernel.org
> > S: Maintained
> > F: drivers/platform/x86/peaq-wmi.c
> >
> > +PECI SUBSYSTEM
> > +M: Iwona Winiarska <iwona.winiarska@intel.com>
> > +R: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> > +L: openbmc@lists.ozlabs.org (moderated for non-subscribers)
> > +S: Supported
> > +F: Documentation/devicetree/bindings/peci/
> > +F: drivers/peci/
> > +F: include/linux/peci.h
> > +
> > PENSANDO ETHERNET DRIVERS
> > M: Shannon Nelson <snelson@pensando.io>
> > M: drivers@pensando.io
> > diff --git a/drivers/Kconfig b/drivers/Kconfig
> > index 8bad63417a50..f472b3d972b3 100644
> > --- a/drivers/Kconfig
> > +++ b/drivers/Kconfig
> > @@ -236,4 +236,7 @@ source "drivers/interconnect/Kconfig"
> > source "drivers/counter/Kconfig"
> >
> > source "drivers/most/Kconfig"
> > +
> > +source "drivers/peci/Kconfig"
> > +
> > endmenu
> > diff --git a/drivers/Makefile b/drivers/Makefile
> > index 27c018bdf4de..8d96f0c3dde5 100644
> > --- a/drivers/Makefile
> > +++ b/drivers/Makefile
> > @@ -189,3 +189,4 @@ obj-$(CONFIG_GNSS) += gnss/
> > obj-$(CONFIG_INTERCONNECT) += interconnect/
> > obj-$(CONFIG_COUNTER) += counter/
> > obj-$(CONFIG_MOST) += most/
> > +obj-$(CONFIG_PECI) += peci/
> > diff --git a/drivers/peci/Kconfig b/drivers/peci/Kconfig
> > new file mode 100644
> > index 000000000000..71a4ad81225a
> > --- /dev/null
> > +++ b/drivers/peci/Kconfig
> > @@ -0,0 +1,15 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +
> > +menuconfig PECI
> > + tristate "PECI support"
> > + help
> > + The Platform Environment Control Interface (PECI) is an interface
> > + that provides a communication channel to Intel processors and
> > + chipset components from external monitoring or control devices.
> > +
> > + If you are building a Baseboard Management Controller (BMC) kernel
> > + for Intel platform say Y here and also to the specific driver for
> > + your adapter(s) below. If unsure say N.
> > +
> > + This support is also available as a module. If so, the module
> > + will be called peci.
> > diff --git a/drivers/peci/Makefile b/drivers/peci/Makefile
> > new file mode 100644
> > index 000000000000..e789a354e842
> > --- /dev/null
> > +++ b/drivers/peci/Makefile
> > @@ -0,0 +1,5 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +
> > +# Core functionality
> > +peci-y := core.o
> > +obj-$(CONFIG_PECI) += peci.o
> > diff --git a/drivers/peci/core.c b/drivers/peci/core.c
> > new file mode 100644
> > index 000000000000..7b3938af0396
> > --- /dev/null
> > +++ b/drivers/peci/core.c
> > @@ -0,0 +1,155 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +// Copyright (c) 2018-2021 Intel Corporation
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> This looks like overkill for only one print statement in this module,
> especially when the dev_ print helpers offer more detail.
Ok, I'll remove it.
>
> > +
> > +#include <linux/bug.h>
> > +#include <linux/device.h>
> > +#include <linux/export.h>
> > +#include <linux/idr.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/peci.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/property.h>
> > +#include <linux/slab.h>
> > +
> > +#include "internal.h"
> > +
> > +static DEFINE_IDA(peci_controller_ida);
> > +
> > +static void peci_controller_dev_release(struct device *dev)
> > +{
> > + struct peci_controller *controller = to_peci_controller(dev);
> > +
> > + pm_runtime_disable(&controller->dev);
>
> This seems late to be disabling power management, the device is about
> to be freed. Keep in mind the lifetime of the this object can be
> artificially prolonged. I expect this to be done when the device is
> unregistered from the bus.
Makes sense.
>
> > +
> > + mutex_destroy(&controller->bus_lock);
> > + ida_free(&peci_controller_ida, controller->id);
> > + fwnode_handle_put(controller->dev.fwnode);
>
> Shouldn't the get / put of this handle reference be bound to specific
> accesses not held for the entire lifetime of the object? At a minimum
> it seems to be a reference that can taken at registration and dropped
> at unregistration.
I'll move it to take ref at registration and to drop it at unregistration.
>
> > + kfree(controller);
> > +}
> > +
> > +struct device_type peci_controller_type = {
> > + .release = peci_controller_dev_release,
> > +};
> > +
> > +static struct peci_controller *peci_controller_alloc(struct device *dev,
> > + struct
> > peci_controller_ops *ops)
> > +{
> > + struct fwnode_handle *node = fwnode_handle_get(dev_fwnode(dev));
> > + struct peci_controller *controller;
> > + int ret;
> > +
> > + if (!ops->xfer)
> > + return ERR_PTR(-EINVAL);
> > +
> > + controller = kzalloc(sizeof(*controller), GFP_KERNEL);
> > + if (!controller)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + ret = ida_alloc_max(&peci_controller_ida, U8_MAX, GFP_KERNEL);
> > + if (ret < 0)
> > + goto err;
> > + controller->id = ret;
> > +
> > + controller->ops = ops;
> > +
> > + controller->dev.parent = dev;
> > + controller->dev.bus = &peci_bus_type;
> > + controller->dev.type = &peci_controller_type;
> > + controller->dev.fwnode = node;
> > + controller->dev.of_node = to_of_node(node);
> > +
> > + device_initialize(&controller->dev);
> > +
> > + mutex_init(&controller->bus_lock);
> > +
> > + pm_runtime_no_callbacks(&controller->dev);
> > + pm_suspend_ignore_children(&controller->dev, true);
> > + pm_runtime_enable(&controller->dev);
>
> Per above, are you sure unregistered devices need pm_runtime enabled?
>
> Rest looks ok to me.
Thanks
-Iwona
next prev parent reply other threads:[~2021-08-26 22:40 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-03 11:31 [PATCH v2 00/15] Introduce PECI subsystem Iwona Winiarska
2021-08-03 11:31 ` [PATCH v2 01/15] x86/cpu: Move intel-family to arch-independent headers Iwona Winiarska
2021-10-04 19:03 ` Borislav Petkov
2021-10-11 19:21 ` Winiarska, Iwona
2021-10-11 19:40 ` Dave Hansen
2021-10-11 20:53 ` Winiarska, Iwona
2021-10-11 23:12 ` Dave Hansen
2021-10-11 20:06 ` Borislav Petkov
2021-10-11 20:38 ` Winiarska, Iwona
2021-10-11 21:31 ` Borislav Petkov
2021-10-12 23:15 ` Winiarska, Iwona
2021-10-13 6:42 ` Borislav Petkov
2021-08-03 11:31 ` [PATCH v2 02/15] x86/cpu: Extract cpuid helpers to arch-independent Iwona Winiarska
2021-10-04 19:08 ` Borislav Petkov
2021-10-11 19:32 ` Winiarska, Iwona
2021-08-03 11:31 ` [PATCH v2 03/15] dt-bindings: Add generic bindings for PECI Iwona Winiarska
2021-08-11 18:11 ` Rob Herring
2021-08-03 11:31 ` [PATCH v2 04/15] dt-bindings: Add bindings for peci-aspeed Iwona Winiarska
2021-08-11 18:11 ` Rob Herring
2021-08-03 11:31 ` [PATCH v2 05/15] ARM: dts: aspeed: Add PECI controller nodes Iwona Winiarska
2021-08-03 11:31 ` [PATCH v2 06/15] peci: Add core infrastructure Iwona Winiarska
2021-08-25 22:58 ` Dan Williams
2021-08-26 22:40 ` Winiarska, Iwona [this message]
2021-08-03 11:31 ` [PATCH v2 07/15] peci: Add peci-aspeed controller driver Iwona Winiarska
2021-08-26 1:35 ` Dan Williams
2021-08-26 23:54 ` Winiarska, Iwona
2021-08-27 16:24 ` Dan Williams
2021-08-29 19:42 ` Winiarska, Iwona
2021-08-03 11:31 ` [PATCH v2 08/15] peci: Add device detection Iwona Winiarska
2021-08-27 19:01 ` Dan Williams
2021-11-15 22:18 ` Winiarska, Iwona
2021-08-03 11:31 ` [PATCH v2 09/15] peci: Add sysfs interface for PECI bus Iwona Winiarska
2021-08-27 19:11 ` Dan Williams
2021-11-15 22:19 ` Winiarska, Iwona
2021-08-03 11:31 ` [PATCH v2 10/15] peci: Add support for PECI device drivers Iwona Winiarska
2021-08-27 21:19 ` Dan Williams
2021-11-15 22:20 ` Winiarska, Iwona
2021-08-03 11:31 ` [PATCH v2 11/15] peci: Add peci-cpu driver Iwona Winiarska
2021-08-03 11:31 ` [PATCH v2 12/15] hwmon: peci: Add cputemp driver Iwona Winiarska
2021-08-03 15:24 ` Guenter Roeck
2021-08-04 10:43 ` Winiarska, Iwona
2021-08-03 11:31 ` [PATCH v2 13/15] hwmon: peci: Add dimmtemp driver Iwona Winiarska
2021-08-03 15:39 ` Guenter Roeck
2021-08-04 10:46 ` Winiarska, Iwona
2021-08-04 17:33 ` Guenter Roeck
2021-08-05 21:48 ` Winiarska, Iwona
2021-08-03 11:31 ` [PATCH v2 14/15] docs: hwmon: Document PECI drivers Iwona Winiarska
2021-08-03 11:31 ` [PATCH v2 15/15] docs: Add PECI documentation Iwona Winiarska
2021-08-05 12:17 ` [PATCH v2 00/15] Introduce PECI subsystem Greg Kroah-Hartman
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=4aa3fac69a92b175beb59d2678eb914c35ea98b7.camel@intel.com \
--to=iwona.winiarska@intel.com \
--cc=andrew@aj.id.au \
--cc=andriy.shevchenko@linux.intel.com \
--cc=arnd@arndb.de \
--cc=bp@alien8.de \
--cc=corbet@lwn.net \
--cc=d.mueller@elsoft.ch \
--cc=dan.j.williams@intel.com \
--cc=devicetree@vger.kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=jae.hyun.yoo@linux.intel.com \
--cc=jason.m.bills@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=olof@lixom.net \
--cc=openbmc@lists.ozlabs.org \
--cc=pierre-louis.bossart@linux.intel.com \
--cc=rdunlap@infradead.org \
--cc=robh+dt@kernel.org \
--cc=tglx@linutronix.de \
--cc=tony.luck@intel.com \
--cc=x86@kernel.org \
--cc=yazen.ghannam@amd.com \
--cc=zweiss@equinix.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).