Re: [PATCH 1/9] platform/surface: Add Surface Aggregator subsystem
From: Andy Shevchenko
Date: Mon Nov 16 2020 - 08:33:04 EST
On Sun, Nov 15, 2020 at 9:25 PM Maximilian Luz <luzmaximilian@xxxxxxxxx> wrote:
>
> Add Surface System Aggregator Module core and Surface Serial Hub driver,
> required for the embedded controller found on Microsoft Surface devices.
>
> The Surface System Aggregator Module (SSAM, SAM or Surface Aggregator)
> is an embedded controller (EC) found on 4th and later generation
> Microsoft Surface devices, with the exception of the Surface Go series.
> This EC provides various functionality, depending on the device in
> question. This can include battery status and thermal reporting (5th and
> later generations), but also HID keyboard (6th+) and touchpad input
> (7th+) on Surface Laptop and Surface Book 3 series devices.
>
> This patch provides the basic necessities for communication with the SAM
> EC on 5th and later generation devices. On these devices, the EC
> provides an interface that acts as serial device, called the Surface
> Serial Hub (SSH). 4th generation devices, on which the EC interface is
> provided via an HID-over-I2C device, are not supported by this patch.
>
> Specifically, this patch adds a driver for the SSH device (device HID
> MSHW0084 in ACPI), as well as a controller structure and associated API.
> This represents the functional core of the Surface Aggregator kernel
> subsystem, introduced with this patch, and will be expanded upon in
> subsequent commits.
>
> The SSH driver acts as the main attachment point for this subsystem and
> sets-up and manages the controller structure. The controller in turn
> provides a basic communication interface, allowing to send requests from
> host to EC and receiving the corresponding responses, as well as
> managing and receiving events, sent from EC to host. It is structured
> into multiple layers, with the top layer presenting the API used by
> other kernel drivers and the lower layers modeled after the serial
> protocol used for communication.
>
> Said other drivers are then responsible for providing the (Surface model
> specific) functionality accessible through the EC (e.g. battery status
> reporting, thermal information, ...) via said controller structure and
> API, and will be added in future commits.
...
> +menuconfig SURFACE_AGGREGATOR
> + tristate "Microsoft Surface System Aggregator Module Subsystem and Drivers"
> + depends on SERIAL_DEV_BUS
> + depends on ACPI
> + select CRC_CCITT
> + help
> + The Surface System Aggregator Module (Surface SAM or SSAM) is an
> + embedded controller (EC) found on 5th- and later-generation Microsoft
> + Surface devices (i.e. Surface Pro 5, Surface Book 2, Surface Laptop,
> + and newer, with exception of Surface Go series devices).
> +
> + Depending on the device in question, this EC provides varying
> + functionality, including:
> + - EC access from ACPI via Surface ACPI Notify (5th- and 6th-generation)
> + - battery status information (all devices)
> + - thermal sensor access (all devices)
> + - performance mode / cooling mode control (all devices)
> + - clipboard detachment system control (Surface Book 2 and 3)
> + - HID / keyboard input (Surface Laptops, Surface Book 3)
> +
> + This option controls whether the Surface SAM subsystem core will be
> + built. This includes a driver for the Surface Serial Hub (SSH), which
> + is the device responsible for the communication with the EC, and a
> + basic kernel interface exposing the EC functionality to other client
> + drivers, i.e. allowing them to make requests to the EC and receive
> + events from it. Selecting this option alone will not provide any
> + client drivers and therefore no functionality beyond the in-kernel
> + interface. Said functionality is the responsibility of the respective
> + client drivers.
> +
> + Note: While 4th-generation Surface devices also make use of a SAM EC,
> + due to a difference in the communication interface of the controller,
> + only 5th and later generations are currently supported. Specifically,
> + devices using SAM-over-SSH are supported, whereas devices using
> + SAM-over-HID, which is used on the 4th generation, are currently not
> + supported.
>From this help text I didn't get if it will be a module or what if I
chose the above?
...
> +/* -- Safe counters. -------------------------------------------------------- */
Why can't XArray be used here?
...
> +
> +
One blank line is enough
...
> +static bool ssam_event_matches_notifier(
> + const struct ssam_event_notifier *notif,
> + const struct ssam_event *event)
Perhaps
static bool
ssam_event_matches_notifier(const struct ssam_event_notifier *n,
const struct ssam_event *event)
(or even switch to 100 limit, also note notif ->n — no need to repeat same word)
...
> + nb = rcu_dereference_raw(nh->head);
> + while (nb) {
> + nf = container_of(nb, struct ssam_event_notifier, base);
> + next_nb = rcu_dereference_raw(nb->next);
> +
> + if (ssam_event_matches_notifier(nf, event)) {
> + ret = (ret & SSAM_NOTIF_STATE_MASK) | nb->fn(nf, event);
> + if (ret & SSAM_NOTIF_STOP)
> + break;
The returned value is a bitmask?!
What are you returning at the end?
> + }
> +
> + nb = next_nb;
> + }
...
> +static int __ssam_nfblk_insert(struct ssam_nf_head *nh, struct ssam_notifier_block *nb)
> +{
> + struct ssam_notifier_block **link = &nh->head;
> +
> + while ((*link) != NULL) {
> + if (unlikely((*link) == nb)) {
> + WARN(1, "double register detected");
> + return -EINVAL;
> + }
> +
> + if (nb->priority > (*link)->priority)
> + break;
> +
> + link = &((*link)->next);
> + }
> +
> + nb->next = *link;
> + rcu_assign_pointer(*link, nb);
> +
> + return 0;
> +}
If you need RCU (which is also the Q per se), why not use RCU list?
...
> + while ((*link) != NULL) {
Redundant parentheses, redundant ' != NULL' part.
> + if ((*link) == nb)
> + return link;
> +
> + link = &((*link)->next);
> + }
...
> + * struct ssam_nf_refcount_entry - RB-tree entry for referecnce counting event
In above you mistyped 'which' as 'whic' or so, and here reference.
Perhaps go thru spell checker?
...
> + * registered, or ``ERR_PTR(-ENOMEM)`` if the entry could not be allocated.
Better to spell out "error pointer" instead of cryptic ERR_PTR().
...
> + * Executa registered callbacks in order of their priority until either no
> + * callback is left or a callback returned a value with the %SSAM_NOTIF_STOP
returns
> + * bit set. Note that this bit is set automatically when converting non.zero
> + * error values via ssam_notifier_from_errno() to notifier values.
...
> + for (i = i - 1; i >= 0; i--)
while (i--)
> + ssam_nf_head_destroy(&nf->head[i]);
...
> + // limit number of processed events to avoid livelocking
> + for (i = 0; i < 10; i++) {
Magic number! Also, this will be better to read in a form of
unsigned int iterations = 10;
do {
...
} while (--iterations);
> + item = ssam_event_queue_pop(queue);
> + if (item == NULL)
> + return;
> +
> + ssam_nf_call(nf, dev, item->rqid, &item->event);
> + kfree(item);
> + }
...
> +static const guid_t SSAM_SSH_DSM_GUID = GUID_INIT(0xd5e383e1, 0xd892, 0x4a76,
> + 0x89, 0xfc, 0xf6, 0xaa, 0xae, 0x7e, 0xd5, 0xb5);
Can you use usual pattern for these UIDs, like
static const guid_t SSAM_SSH_DSM_GUID =
GUID_INIT(0xd5e383e1, 0xd892, 0x4a76,
0x89, 0xfc, 0xf6, 0xaa, 0xae, 0x7e, 0xd5, 0xb5);
?
Also put a comment how this UID will look like in a string representation.
...
> + if (!acpi_has_method(handle, "_DSM"))
> + return 0;
Hmm... What's the expectation?
> + obj = acpi_evaluate_dsm_typed(handle, &SSAM_SSH_DSM_GUID,
> + SSAM_SSH_DSM_REVISION, 0, NULL,
> + ACPI_TYPE_BUFFER);
> + if (!obj)
> + return -EFAULT;
EFAULT?! Perhaps you can simply return 0 here, no?
> + for (i = 0; i < obj->buffer.length && i < 8; i++)
> + mask |= (((u64)obj->buffer.pointer[i]) << (i * 8));
Don't we have some helpers for this? At least I remember similar code
went to one of PDx86 drivers like intel-vbtn or so.
> + if (mask & 0x01)
BIT(0) ?
> + *funcs = mask;
...
> + caps->ssh_power_profile = (u32)-1;
> + caps->screen_on_sleep_idle_timeout = (u32)-1;
> + caps->screen_off_sleep_idle_timeout = (u32)-1;
> + caps->d3_closes_handle = false;
> + caps->ssh_buffer_size = (u32)-1;
Use proper types and their limits (limits.h missed?).
...
> + // initialize request and packet transport layers
Inconsistent style of comments.
...
> + * In the course of this shutdown procedure, all currently registered
> + * notifiers will be unregistered. It is, however, strongly recommended to not
> + * rely on this behavior, and instead the party registring the notifier should
registering
> + * unregister it before the controller gets shut down, e.g. via the SSAM bus
> + * which guarantees client devices to be removed before a shutdown.
> + * Note that events may still be pending after this call, but due to the
> + * notifiers being unregistered, the will be dropped when the controller is
the?!
> + * subsequently being destroyed via ssam_controller_destroy().
...
> + * Ensures that all resources associated with the controller get freed. This
> + * function should only be called after the controller has been stopped via
> + * ssam_controller_shutdown(). In general, this function should not be called
> + * directly. The only valid place to call this function direclty is during
directly
> + * initialization, before the controller has been fully initialized and passed
> + * to other processes. This function is called automatically when the
> + * reference count of the controller reaches zero.
...
> + * ssam_request_sync_free() - Free a synchronous request.
> + * @rqst: The request to free.
to be freed?
...
> + * Allocates a synchronous request struct on the stack, fully initializes it
> + * using the provided buffer as message data buffer, submits it, and then
> + * waits for its completion before returning its staus. The
status
> + * SSH_COMMAND_MESSAGE_LENGTH() macro can be used to compute the required
> + * message buffer size.
...
> + * This is a wrapper for the raw SAM request to enable an event, thus it does
> + * not handle referecnce counting for enable/disable of events. If an event
> + * has already been enabled, the EC will ignore this request.
Grammar and English language style somehow feels not okay.
...
> + u8 buf[1] = { 0x00 };
Can't be simply buf ?
...
> + * This function will only send the display-off notification command if
> + * display noticications are supported by the EC. Currently all known devices
> + * support these notification.
Spell check!
...
> + * This function will only send the display-on notification command if display
> + * noticications are supported by the EC. Currently all known devices support
> + * these notification.
Ditto.
...
> + ssam_err(ctrl, "unexpected response from D0-exit notification:"
> + " 0x%02x\n", response);
Don't split string literals. Had you run a checkpatch?
Please do and use strict mode.
...
Overall impression of this submission:
- it's quite huge as for a single patch
- it feels like quite an overengineered solution: READ_ONCE, RCU,
list + spin_lock, RB tree of notifiers, my head!
- where is the architectural document of all these?
--
With Best Regards,
Andy Shevchenko