Re: [PATCH 1/9] platform/surface: Add Surface Aggregator subsystem

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Sun, Nov 15, 2020 at 9:25 PM Maximilian Luz <luzmaximilian@gmail.com> 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