Thread (72 messages) 72 messages, 9 authors, 2021-08-05

Re: [PATCH 08/14] peci: Add device detection

From: Williams, Dan J <hidden>
Date: 2021-07-14 21:05:18
Also in: linux-arm-kernel, linux-aspeed, linux-devicetree, linux-hwmon, lkml, openbmc

On Tue, 2021-07-13 at 00:04 +0200, Iwona Winiarska wrote:
quoted hunk ↗ jump to hunk
Since PECI devices are discoverable, we can dynamically detect devices
that are actually available in the system.

This change complements the earlier implementation by rescanning PECI
bus to detect available devices. For this purpose, it also introduces the
minimal API for PECI requests.

Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
Reviewed-by: Pierre-Louis Bossart <redacted>
---
 drivers/peci/Makefile   |   2 +-
 drivers/peci/core.c     |  13 ++++-
 drivers/peci/device.c   | 111 ++++++++++++++++++++++++++++++++++++++++
 drivers/peci/internal.h |  15 ++++++
 drivers/peci/request.c  |  74 +++++++++++++++++++++++++++
 drivers/peci/sysfs.c    |  34 ++++++++++++
 6 files changed, 246 insertions(+), 3 deletions(-)
 create mode 100644 drivers/peci/device.c
 create mode 100644 drivers/peci/request.c
diff --git a/drivers/peci/Makefile b/drivers/peci/Makefile
index 621a993e306a..917f689e147a 100644
--- a/drivers/peci/Makefile
+++ b/drivers/peci/Makefile
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 
 # Core functionality
-peci-y := core.o sysfs.o
+peci-y := core.o request.o device.o sysfs.o
 obj-$(CONFIG_PECI) += peci.o
 
 # Hardware specific bus drivers
diff --git a/drivers/peci/core.c b/drivers/peci/core.c
index 0ad00110459d..ae7a9572cdf3 100644
--- a/drivers/peci/core.c
+++ b/drivers/peci/core.c
@@ -31,7 +31,15 @@ struct device_type peci_controller_type = {
 
 int peci_controller_scan_devices(struct peci_controller *controller)
 {
-       /* Just a stub, no support for actual devices yet */
+       int ret;
+       u8 addr;
+
+       for (addr = PECI_BASE_ADDR; addr < PECI_BASE_ADDR + PECI_DEVICE_NUM_MAX; addr++) {
+               ret = peci_device_create(controller, addr);
+               if (ret)
+                       return ret;
+       }
+
This seems to be a behavior triggered at peci_controller_add and at the
request of userspace when touching the rescan attribute? A natural way
to handle this would be to have a driver for the peci_controller device
and have that driver issue scan at probe time. Otherwise, how does
userspace know when it is time to rescan the bus?
quoted hunk ↗ jump to hunk
        return 0;
 }
 
@@ -106,7 +114,8 @@ EXPORT_SYMBOL_NS_GPL(peci_controller_add, PECI);
 
 static int _unregister(struct device *dev, void *dummy)
 {
-       /* Just a stub, no support for actual devices yet */
+       peci_device_destroy(to_peci_device(dev));
As mentioned previously, this could be delegated to devm to unregister
when the original driver that added the controller goes through -
remove().
quoted hunk ↗ jump to hunk
+
        return 0;
 }
 
diff --git a/drivers/peci/device.c b/drivers/peci/device.c
new file mode 100644
index 000000000000..1124862211e2
--- /dev/null
+++ b/drivers/peci/device.c
@@ -0,0 +1,111 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// Copyright (c) 2018-2021 Intel Corporation
+
+#include <linux/peci.h>
+#include <linux/slab.h>
+
+#include "internal.h"
+
+static int peci_detect(struct peci_controller *controller, u8 addr)
+{
+       struct peci_request *req;
+       int ret;
+
+       req = peci_request_alloc(NULL, 0, 0);
+       if (!req)
+               return -ENOMEM;
+
+       mutex_lock(&controller->bus_lock);
What is the underlying requirement to prevent 2 simultaneous ->xfer()
invocations?
+       ret = controller->xfer(controller, addr, req);
+       mutex_unlock(&controller->bus_lock);
+
+       peci_request_free(req);
+
+       return ret;
+}
+
+static bool peci_addr_valid(u8 addr)
+{
+       return addr >= PECI_BASE_ADDR && addr < PECI_BASE_ADDR + PECI_DEVICE_NUM_MAX;
+}
+
+static int peci_dev_exists(struct device *dev, void *data)
+{
+       struct peci_device *device = to_peci_device(dev);
+       u8 *addr = data;
+
+       if (device->addr == *addr)
+               return -EBUSY;
+
+       return 0;
+}
+
+int peci_device_create(struct peci_controller *controller, u8 addr)
+{
+       struct peci_device *device;
+       int ret;
+
+       if (WARN_ON(!peci_addr_valid(addr)))
+               return -EINVAL;
+
+       /* Check if we have already detected this device before. */
+       ret = device_for_each_child(&controller->dev, &addr, peci_dev_exists);
+       if (ret)
+               return 0;
+
+       ret = peci_detect(controller, addr);
+       if (ret) {
+               /*
+                * Device not present or host state doesn't allow successful
+                * detection at this time.
+                */
+               if (ret == -EIO || ret == -ETIMEDOUT)
+                       return 0;
+
+               return ret;
+       }
+
+       device = kzalloc(sizeof(*device), GFP_KERNEL);
+       if (!device)
+               return -ENOMEM;
+
+       device->controller = controller;
+       device->addr = addr;
+       device->dev.parent = &device->controller->dev;
+       device->dev.bus = &peci_bus_type;
+       device->dev.type = &peci_device_type;
+
+       ret = dev_set_name(&device->dev, "%d-%02x", controller->id, device->addr);
+       if (ret)
+               goto err_free;
+
+       ret = device_register(&device->dev);
There is a recent movement away from device_register() to an alloc+add
pattern [1]. I.e. have device_initialize() and device_add() steps. With
that you can unify the error exit to be put_device().

[1]: https://lore.kernel.org/r/20210712134233.GA141137@ziepe.ca
quoted hunk ↗ jump to hunk
+       if (ret)
+               goto err_put;
+
+       return 0;
+
+err_put:
+       put_device(&device->dev);
+err_free:
+       kfree(device);
+
+       return ret;
+}
+
+void peci_device_destroy(struct peci_device *device)
+{
+       device_unregister(&device->dev);
+}
+
+static void peci_device_release(struct device *dev)
+{
+       struct peci_device *device = to_peci_device(dev);
+
+       kfree(device);
+}
+
+struct device_type peci_device_type = {
+       .groups         = peci_device_groups,
+       .release        = peci_device_release,
+};
diff --git a/drivers/peci/internal.h b/drivers/peci/internal.h
index 80c61bcdfc6b..6b139adaf6b8 100644
--- a/drivers/peci/internal.h
+++ b/drivers/peci/internal.h
@@ -9,6 +9,21 @@
 
 struct peci_controller;
 struct attribute_group;
+struct peci_device;
+struct peci_request;
+
+/* PECI CPU address range 0x30-0x37 */
+#define PECI_BASE_ADDR         0x30
+#define PECI_DEVICE_NUM_MAX            8
+
+struct peci_request *peci_request_alloc(struct peci_device *device, u8 tx_len, u8 rx_len);
+void peci_request_free(struct peci_request *req);
+
+extern struct device_type peci_device_type;
+extern const struct attribute_group *peci_device_groups[];
+
+int peci_device_create(struct peci_controller *controller, u8 addr);
+void peci_device_destroy(struct peci_device *device);
 
 extern struct bus_type peci_bus_type;
 extern const struct attribute_group *peci_bus_groups[];
diff --git a/drivers/peci/request.c b/drivers/peci/request.c
new file mode 100644
index 000000000000..78cee51dfae1
--- /dev/null
+++ b/drivers/peci/request.c
@@ -0,0 +1,74 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// Copyright (c) 2021 Intel Corporation
+
+#include <linux/export.h>
+#include <linux/peci.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#include "internal.h"
+
+/**
+ * peci_request_alloc() - allocate &struct peci_request with buffers with given lengths
+ * @device: PECI device to which request is going to be sent
+ * @tx_len: requested TX buffer length
+ * @rx_len: requested RX buffer length
+ *
+ * Return: A pointer to a newly allocated &struct peci_request on success or NULL otherwise.
+ */
+struct peci_request *peci_request_alloc(struct peci_device *device, u8 tx_len, u8 rx_len)
+{
How big can these lengths be?
quoted hunk ↗ jump to hunk
+       struct peci_request *req;
+       u8 *tx_buf, *rx_buf;
+
+       req = kzalloc(sizeof(*req), GFP_KERNEL);
+       if (!req)
+               return NULL;
+
+       req->device = device;
+
+       /*
+        * PECI controllers that we are using now don't support DMA, this
+        * should be converted to DMA API once support for controllers that do
+        * allow it is added to avoid an extra copy.
+        */
+       if (tx_len) {
+               tx_buf = kzalloc(tx_len, GFP_KERNEL);
+               if (!tx_buf)
+                       goto err_free_req;
+
+               req->tx.buf = tx_buf;
+               req->tx.len = tx_len;
+       }
+
+       if (rx_len) {
+               rx_buf = kzalloc(rx_len, GFP_KERNEL);
+               if (!rx_buf)
+                       goto err_free_tx;
+
+               req->rx.buf = rx_buf;
+               req->rx.len = rx_len;
+       }
+
+       return req;
+
+err_free_tx:
+       kfree(req->tx.buf);
+err_free_req:
+       kfree(req);
+
+       return NULL;
+}
+EXPORT_SYMBOL_NS_GPL(peci_request_alloc, PECI);
+
+/**
+ * peci_request_free() - free peci_request
+ * @req: the PECI request to be freed
+ */
+void peci_request_free(struct peci_request *req)
+{
+       kfree(req->rx.buf);
+       kfree(req->tx.buf);
+       kfree(req);
+}
+EXPORT_SYMBOL_NS_GPL(peci_request_free, PECI);
diff --git a/drivers/peci/sysfs.c b/drivers/peci/sysfs.c
index 36c5e2a18a92..db9ef05776e3 100644
--- a/drivers/peci/sysfs.c
+++ b/drivers/peci/sysfs.c
@@ -1,6 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0-only
 // Copyright (c) 2021 Intel Corporation
 
+#include <linux/device.h>
+#include <linux/kernel.h>
 #include <linux/peci.h>
 
 #include "internal.h"
@@ -46,3 +48,35 @@ const struct attribute_group *peci_bus_groups[] = {
        &peci_bus_group,
        NULL
 };
+
+static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
+                           const char *buf, size_t count)
+{
+       struct peci_device *device = to_peci_device(dev);
+       bool res;
+       int ret;
+
+       ret = kstrtobool(buf, &res);
+       if (ret)
+               return ret;
+
+       if (res && device_remove_file_self(dev, attr))
+               peci_device_destroy(device);
+
+       return count;
+}
+static DEVICE_ATTR_IGNORE_LOCKDEP(remove, 0200, NULL, remove_store);
Why does userspace need the ability to kick devices off the bus?

Do you have an example userspace tool that is using these sysfs APIs?
+
+static struct attribute *peci_device_attrs[] = {
+       &dev_attr_remove.attr,
+       NULL
+};
+
+static const struct attribute_group peci_device_group = {
+       .attrs = peci_device_attrs,
+};
+
+const struct attribute_group *peci_device_groups[] = {
+       &peci_device_group,
+       NULL
+};
  
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help