On Wed, Sep 23, 2020 at 05:15:08PM +0200, Maximilian Luz wrote:
> The Surface Aggregator EC provides varying functionality, depending on
> the Surface device. To manage this functionality, we use dedicated
> client devices for each subsystem or virtual device of the EC. While
> some of these clients are described as standard devices in ACPI and the
> corresponding client drivers can be implemented as platform drivers in
> the kernel (making use of the controller API already present), many
> devices, especially on newer Surface models, cannot be found there.
>
> To simplify management of these devices, we introduce a new bus and
> client device type for the Surface Aggregator subsystem. The new device
> type takes care of managing the controller reference, essentially
> guaranteeing its validity for as long as the client device exists, thus
> alleviating the need to manually establish device links for that purpose
> in the client driver (as has to be done with the platform devices).
>
> Signed-off-by: Maximilian Luz <luzmaximil...@gmail.com>
Overall, nice work on this patch, the integration to the driver core
looks totally correct. Great job.
A few minor nits below:
> --- /dev/null
> +++ b/drivers/misc/surface_aggregator/bus.c
> @@ -0,0 +1,419 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
No copyright?
> +/**
> + * ssam_device_add() - Add a SSAM client device.
> + * @sdev: The SSAM client device to be added.
> + *
> + * Added client devices must be guaranteed to always have a valid and active
> + * controller. Thus, this function will fail with %-ENXIO if the controller
> of
> + * the device has not been initialized yet, has been suspended, or has been
> + * shut down.
> + *
> + * The caller of this function should ensure that the corresponding call to
> + * ssam_device_remove() is issued before the controller is shut down. If the
> + * added device is a direct child of the controller device (default), it will
> + * be automatically removed when the controller is shut down.
> + *
> + * By default, the controller device will become the parent of the newly
> + * created client device. The parent may be changed before ssam_device_add is
> + * called, but care must be taken that a) the correct suspend/resume ordering
> + * is guaranteed and b) the client device does not oultive the controller,
> + * i.e. that the device is removed before the controller is being shut down.
> + * In case these guarantees have to be manually enforced, please refer to the
> + * ssam_client_link() and ssam_client_bind() functions, which are intended to
> + * set up device-links for this purpose.
> + *
> + * Return: Returns zero on success, a negative error code on failure.
> + */
> +int ssam_device_add(struct ssam_device *sdev)
> +{
> + int status;
> +
> + /*
> + * Ensure that we can only add new devices to a controller if it has
> + * been started and is not going away soon. This works in combination
> + * with ssam_controller_remove_clients to ensure driver presence for the
> + * controller device, i.e. it ensures that the controller (sdev->ctrl)
> + * is always valid and can be used for requests as long as the client
> + * device we add here is registered as child under it. This essentially
> + * guarantees that the client driver can always expect the preconditions
> + * for functions like ssam_request_sync (controller has to be started
> + * and is not suspended) to hold and thus does not have to check for
> + * them.
> + *
> + * Note that for this to work, the controller has to be a parent device.
> + * If it is not a direct parent, care has to be taken that the device is
> + * removed via ssam_device_remove(), as device_unregister does not
> + * remove child devices recursively.
> + */
> + ssam_controller_statelock(sdev->ctrl);
> +
> + if (READ_ONCE(sdev->ctrl->state) != SSAM_CONTROLLER_STARTED) {
You locked the state, why the READ_ONCE()? Is taht needed?
> + ssam_controller_stateunlock(sdev->ctrl);
> + return -ENXIO;
odd error value, why this one?
> + }
> +
> + status = device_add(&sdev->dev);
> +
> + ssam_controller_stateunlock(sdev->ctrl);
> + return status;
> +}
> +EXPORT_SYMBOL_GPL(ssam_device_add);
> --- /dev/null
> +++ b/include/linux/surface_aggregator/device.h
> @@ -0,0 +1,408 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
Copyright?
> +/*
> + * Surface System Aggregator Module (SSAM) bus and client-device subsystem.
> + *
> + * Main interface for the surface-aggregator bus, surface-aggregator client
> + * devices, and respective drivers building on top of the SSAM controller.
> + * Provides support for non-platform/non-ACPI SSAM clients via dedicated
> + * subsystem.
> + */
> +
> +#ifndef _LINUX_SURFACE_AGGREGATOR_DEVICE_H
> +#define _LINUX_SURFACE_AGGREGATOR_DEVICE_H
> +
> +#include <linux/device.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/types.h>
> +
> +#include <linux/surface_aggregator/controller.h>
> +
> +
> +/* -- Surface System Aggregator Module Bus.
> --------------------------------- */
> +
> +/**
> + * enum ssam_device_domain - SAM device domain.
> + * @SSAM_DOMAIN_VIRTUAL: Virtual device.
> + * @SSAM_DOMAIN_SERIALHUB: Physical dovice connected via Surface Serial Hub.
> + */
> +enum ssam_device_domain {
> + SSAM_DOMAIN_VIRTUAL = 0x00,
> + SSAM_DOMAIN_SERIALHUB = 0x01,
> +};
> +
> +/**
> + * enum ssam_virtual_tc - Target categories for the virtual SAM domain.
> + * @SSAM_VIRTUAL_TC_HUB: Device hub category.
> + */
> +enum ssam_virtual_tc {
> + SSAM_VIRTUAL_TC_HUB = 0x00,
> +};
> +
> +/**
> + * struct ssam_device_uid - Unique identifier for SSAM device.
> + * @domain: Domain of the device.
> + * @category: Target category of the device.
> + * @target: Target ID of the device.
> + * @instance: Instance ID of the device.
> + * @function: Sub-function of the device. This field can be used to split a
> + * single SAM device into multiple virtual subdevices to separate
> + * different functionality of that device and allow one driver per
> + * such functionality.
> + */
> +struct ssam_device_uid {
> + u8 domain;
> + u8 category;
> + u8 target;
> + u8 instance;
> + u8 function;
> +};
> +
> +/*
> + * Special values for device matching.
> + */
> +#define SSAM_ANY_TID 0xffff
> +#define SSAM_ANY_IID 0xffff
> +#define SSAM_ANY_FUN 0xffff
These are 16 bits, but the uid values above are 8 bits. How does that
match up?
> +
> +/**
> + * SSAM_DEVICE() - Initialize a &struct ssam_device_id with the given
> + * parameters.
> + * @d: Domain of the device.
> + * @cat: Target category of the device.
> + * @tid: Target ID of the device.
> + * @iid: Instance ID of the device.
> + * @fun: Sub-function of the device.
> + *
> + * Initializes a &struct ssam_device_id with the given parameters. See
> &struct
> + * ssam_device_uid for details regarding the parameters. The special values
> + * %SSAM_ANY_TID, %SSAM_ANY_IID, and %SSAM_ANY_FUN can be used to specify
> that
> + * matching should ignore target ID, instance ID, and/or sub-function,
> + * respectively. This macro initializes the ``match_flags`` field based on
> the
> + * given parameters.
> + */
> +#define SSAM_DEVICE(d, cat, tid, iid, fun)
> \
> + .match_flags = (((tid) != SSAM_ANY_TID) ? SSAM_MATCH_TARGET : 0)
> \
> + | (((iid) != SSAM_ANY_IID) ? SSAM_MATCH_INSTANCE : 0)
> \
> + | (((fun) != SSAM_ANY_FUN) ? SSAM_MATCH_FUNCTION : 0),
> \
> + .domain = d,
> \
> + .category = cat,
> \
> + .target = ((tid) != SSAM_ANY_TID) ? (tid) : 0,
> \
> + .instance = ((iid) != SSAM_ANY_IID) ? (iid) : 0,
> \
> + .function = ((fun) != SSAM_ANY_FUN) ? (fun) : 0
> \
Again, the 16 vs 8 bits here feels odd. No casting???
> +/**
> + * ssam_device_get() - Increment reference count of SSAM client device.
> + * @sdev: The device to increment the reference count of.
> + *
> + * Increments the reference count of the given SSAM client device by
> + * incrementing the reference count of the enclosed &struct device via
> + * get_device().
> + *
> + * See ssam_device_put() for the counter-part of this function.
> + *
> + * Return: Returns the device provided as input.
> + */
> +static inline struct ssam_device *ssam_device_get(struct ssam_device *sdev)
> +{
> + get_device(&sdev->dev);
> + return sdev;
Do you want to check if sdev is NULL or not here before referencing
it?
> +}
> +
> +/**
> + * ssam_device_put() - Decrement reference count of SSAM client device.
> + * @sdev: The device to decrement the reference count of.
> + *
> + * Decrements the reference count of the given SSAM client device by
> + * decrementing the reference count of the enclosed &struct device via
> + * put_device().
> + *
> + * See ssam_device_get() for the counter-part of this function.
> + */
> +static inline void ssam_device_put(struct ssam_device *sdev)
> +{
> + put_device(&sdev->dev);
Same here, do you need to check?
anyway, again, nice work, if only all of my code reviews were this easy
:)
thanks,
greg k-h
