From mboxrd@z Thu Jan 1 00:00:00 1970 From: Williams, Dan J Date: Wed, 14 Jul 2021 21:05:12 +0000 Subject: [PATCH 08/14] peci: Add device detection In-Reply-To: <20210712220447.957418-9-iwona.winiarska@intel.com> References: <20210712220447.957418-1-iwona.winiarska@intel.com> <20210712220447.957418-9-iwona.winiarska@intel.com> Message-ID: List-Id: To: linux-aspeed@lists.ozlabs.org MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit On Tue, 2021-07-13 at 00:04 +0200, Iwona Winiarska wrote: > 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 > Reviewed-by: Pierre-Louis Bossart > --- > ?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? > ????????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(). > + > ????????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 > +#include > + > +#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 at ziepe.ca > +???????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 > +#include > +#include > +#include > + > +#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? > +???????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 > +#include > ?#include > ? > ?#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 > +};