Hi
I have attached some thoughts and comments inline.
2020. november 15., vasárnap 20:21 keltezéssel, Maximilian Luz írta:
> [...]
> +/* -- Event notifier/callbacks. --------------------------------------------- */
> +/*
> + * The notifier system is based on linux/notifier.h, specifically the SRCU
> + * implementation. The difference to that is, that some bits of the notifier
> + * call return value can be tracked across multiple calls. This is done so that
> + * handling of events can be tracked and a warning can be issued in case an
> + * event goes unhandled. The idea of that waring is that it should help discover
^
"warning"
> + * and identify new/currently unimplemented features.
> + */
> +
> +
> +/**
> + * ssam_event_matches_notifier() - Test if an event matches a notifier;
^
Shouldn't it be a period?
> + * @notif: The event notifier to test against.
> + * @event: The event to test.
> + *
> + * Return: Returns %true iff the given event matches the given notifier
> + * according to the rules set in the notifier's event mask, %false otherwise.
> + */
> [...]
> +static int __ssam_nfblk_remove(struct ssam_nf_head *nh,
> + struct ssam_notifier_block *nb)
> +{
> + struct ssam_notifier_block **link;
> +
> + link = __ssam_nfblk_find_link(nh, nb);
> + if (!link)
> + return -ENOENT;
I find it odd that here you return ENOENT, but in `__ssam_nfblk_insert()`
EINVAL is returned instead of EEXIST. I believe either both should be EINVAL,
or EEXIST+ENOENT.
> +
> + __ssam_nfblk_erase(link);
I'm wondering if it's necessary to create a new function which contains just
a single line.
> + return 0;
> +}
> [...]
> +/**
> + * ssam_nf_head_destroy() - Deinitialize the given notifier head.
> + * @nh: The notifier head to deinitialize.
> + */
> +static void ssam_nf_head_destroy(struct ssam_nf_head *nh)
> +{
> + cleanup_srcu_struct(&nh->srcu);
> +}
I'm also wondering if there's any reason why these static one-liner functions are
not explicitly marked inline.
> [...]
> +/**
> + * struct ssam_nf_refcount_entry - RB-tree entry for referecnce counting event
> + * activations.
> + * @node: The node of this entry in the rb-tree.
> + * @key: The key of the event.
> + * @refcount: The reference-count of the event.
> + * @flags: The flags used when enabling the event.
> + */
> +struct ssam_nf_refcount_entry {
> + struct rb_node node;
> + struct ssam_nf_refcount_key key;
> + int refcount;
Is there any reason why a signed type is used for reference counting?
> + u8 flags;
> +};
> +
> +
> +/**
> + * ssam_nf_refcount_inc() - Increment reference-/activation-count of the given
> + * event.
> + * @nf: The notifier system reference.
> + * @reg: The registry used to enable/disable the event.
> + * @id: The event ID.
> + *
> + * Increments the reference-/activation-count associated with the specified
> + * event type/ID, allocating a new entry for this event ID if necessary. A
> + * newly allocated entry will have a refcount of one.
Shouldn't it be noted that nf->lock(?) must(?) be held when calling?
> [...]
> +/**
> + * ssam_nf_call() - Call notification callbacks for the provided event.
> + * @nf: The notifier system
> + * @dev: The associated device, only used for logging.
> + * @rqid: The request ID of the event.
> + * @event: The event provided to the callbacks.
> + *
> + * Executa registered callbacks in order of their priority until either no
^
"execute"
> + * callback is left or a callback returned a value with the %SSAM_NOTIF_STOP
> + * bit set. Note that this bit is set automatically when converting non.zero
^
maybe "non-zero"?
> + * error values via ssam_notifier_from_errno() to notifier values.
> + *
> + * Also note that any callback that could handle an event should return a value
> + * with bit %SSAM_NOTIF_HANDLED set, indicating that the event does not go
> + * unhandled/ignored. In case no registered callback could handle an event,
> + * this function will emit a warning.
> + *
> + * In case a callback failed, this function will emit an error message.
> + */
> +static void ssam_nf_call(struct ssam_nf *nf, struct device *dev, u16 rqid,
> + struct ssam_event *event)
> +{
> + struct ssam_nf_head *nf_head;
> + int status, nf_ret;
> +
> + if (!ssh_rqid_is_event(rqid)) {
> + dev_warn(dev, "event: unsupported rqid: 0x%04x\n", rqid);
A small note, "%#04x" would insert the "0x" prefix.
> + return;
> + }
> [...]
> +/**
> + * ssam_event_item_alloc() - Allocate an event item with the given payload size.
> + * @len: The event payload length.
> + * @flags: The flags used for allocation.
> + *
> + * Allocate an event item with the given payload size. Sets the item
> + * operations and payload length values. The item free callback (``ops.free``)
> + * should not be overwritten after this call.
> + *
> + * Return: Returns the newly allocated event item.
> + */
> +static struct ssam_event_item *ssam_event_item_alloc(size_t len, gfp_t flags)
The `flags` argument is seemingly ignored.
> +{
> + struct ssam_event_item *item;
> +
> + item = kzalloc(sizeof(*item) + len, GFP_KERNEL);
I believe `struct_size(item, event.data, len)` could be utilized here.
> + if (!item)
> + return NULL;
> +
> + item->event.length = len;
> + return item;
> +}
> [...]
> +static void ssam_event_queue_work_fn(struct work_struct *work)
> +{
> + struct ssam_event_queue *queue;
> + struct ssam_event_item *item;
> + struct ssam_nf *nf;
> + struct device *dev;
> + int i;
> +
> + queue = container_of(work, struct ssam_event_queue, work);
> + nf = &queue->cplt->event.notif;
> + dev = queue->cplt->dev;
> +
> + // limit number of processed events to avoid livelocking
> + for (i = 0; i < 10; i++) {
> + item = ssam_event_queue_pop(queue);
> + if (item == NULL)
I believe `!item` is preferred.
> + return;
> +
> + ssam_nf_call(nf, dev, item->rqid, &item->event);
> + kfree(item);
> + }
> +
> + if (!ssam_event_queue_is_empty(queue))
> + ssam_cplt_submit(queue->cplt, &queue->work);
> +}
> [...]
> +static void ssam_handle_event(struct ssh_rtl *rtl,
> + const struct ssh_command *cmd,
> + const struct ssam_span *data)
> +{
> + struct ssam_controller *ctrl = to_ssam_controller(rtl, rtl);
> + struct ssam_event_item *item;
> +
> + item = ssam_event_item_alloc(data->len, GFP_KERNEL);
> + if (!item)
> + return;
> +
> + item->rqid = get_unaligned_le16(&cmd->rqid);
> + item->event.target_category = cmd->tc;
> + item->event.target_id = cmd->tid_in;
> + item->event.command_id = cmd->cid;
> + item->event.instance_id = cmd->iid;
> + memcpy(&item->event.data[0], data->ptr, data->len);
> +
> + WARN_ON(ssam_cplt_submit_event(&ctrl->cplt, item));
I believe that if submission fails, `item` is leaked.
> +}
> +
> +static const struct ssh_rtl_ops ssam_rtl_ops = {
> + .handle_event = ssam_handle_event,
> +};
> +
> +
> +static bool ssam_notifier_empty(struct ssam_controller *ctrl);
I think it'd be better to name it `ssam_notifier_is_empty()` to be consistent
with the rest of the patch.
> [...]
> +static int ssam_controller_caps_load_from_acpi(
> + acpi_handle handle, struct ssam_controller_caps *caps)
> +{
> + u32 d3_closes_handle = false;
Assinging a boolean like this to a `u32` looks very odd to me.
> + u64 funcs;
> + int status;
> +
> + // set defaults
> + 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;
> [...]
> +
> +/**
> + * ssam_controller_start() - Start the receiver and transmitter threads of the
> + * controller.
> + * @ctrl: The controller.
> + *
> + * Note: When this function is called, the controller shouldbe properly hooked
^
space
> + * up to the serdev core via &struct serdev_device_ops. Please refert to
^
"refer"
> [...]
> +void ssam_controller_shutdown(struct ssam_controller *ctrl)
> +{
> + enum ssam_controller_state s = ctrl->state;
> + int status;
> +
> + if (s == SSAM_CONTROLLER_UNINITIALIZED || s == SSAM_CONTROLLER_STOPPED)
> + return;
> +
> + // try to flush pending events and requests while everything still works
> + status = ssh_rtl_flush(&ctrl->rtl, msecs_to_jiffies(5000));
Wouldn't it be better to name that 5000?
> + if (status) {
> + ssam_err(ctrl, "failed to flush request transport layer: %d\n",
> + status);
> + }
> [...]
> +
> +/**
> + * ssam_controller_destroy() - Destroy the controller and free its resources.
> + * @ctrl: The controller.
> + *
> + * 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.
> + *
> + * Must be called from an exclusive context with regards to the controller
> + * state.
> + */
> [...]
> +int ssam_request_sync_alloc(size_t payload_len, gfp_t flags,
> + struct ssam_request_sync **rqst,
> + struct ssam_span *buffer)
> +{
> + size_t msglen = SSH_COMMAND_MESSAGE_LENGTH(payload_len);
> +
> + *rqst = kzalloc(sizeof(**rqst) + msglen, flags);
> + if (!*rqst)
> + return -ENOMEM;
> +
> + buffer->ptr = (u8 *)(*rqst + 1);
> + buffer->len = msglen;
> +
> + return 0;
> +}
I think there is a bit of incosistency: sometimes you use ** pointer + return int,
sometimes you return a pointer with potentially embedded errno. I think it would
be better if you stuck with one or the other.
> [...]
> +static int ssam_ssh_event_disable(struct ssam_controller *ctrl,
> + struct ssam_event_registry reg,
> + struct ssam_event_id id, u8 flags)
> +{
> [...]
> + rqst.command_id = reg.cid_disable;
If I see it correctly, this line is the only significant difference between this one
and the previous function. Is there any reason they're not combined?
> [...]
> +/**
> + * ssam_notifier_disable_registered() - Disable events for all registered
> + * notifiers.
> + * @ctrl: The controller for which to disable the notifiers/events.
> + *
> + * Disables events for all currently registered notifiers. In case of an error
> + * (EC command failing), all previously disabled events will be restored and
> + * the error code returned.
> + *
> + * This function is intended to disable all events prior to hibenration entry.
^
"hibernation"
> [...]
> +void ssam_irq_disarm_wakeup(struct ssam_controller *ctrl)
> +{
> + int status;
> +
> + if (ctrl->irq.wakeup_enabled) {
> + status = disable_irq_wake(ctrl->irq.num);
> + if (status)
> + ssam_err(ctrl, "failed to disable wake IRQ: %d\n", status);
> +
> + ctrl->irq.wakeup_enabled = false;
It's set to false even if `disable_irq_wake()` fails?
> + }
> + disable_irq(ctrl->irq.num);
> +}
> [...]
> +static int ssam_try_set_controller(struct ssam_controller *ctrl)
> +{
> + int status = 0;
> +
> + spin_lock(&__ssam_controller_lock);
> + if (!__ssam_controller)
> + __ssam_controller = ctrl;
> + else
> + status = -EBUSY;
I feel like EBUSY might not be the best errno here.
> + spin_unlock(&__ssam_controller_lock);
> +
> + return status;
> +}
> [...]
> +int ssam_client_link(struct ssam_controller *c, struct device *client)
> +{
> + const u32 flags = DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_CONSUMER;
> + struct device_link *link;
> + struct device *ctrldev;
> +
> + ssam_controller_statelock(c);
> +
> + if (c->state != SSAM_CONTROLLER_STARTED) {
> + ssam_controller_stateunlock(c);
> + return -ENXIO;
> + }
> +
> + ctrldev = ssam_controller_device(c);
> + if (!ctrldev) {
> + ssam_controller_stateunlock(c);
> + return -ENXIO;
> + }
> +
I'm not sure if ENXIO is the best errno in the last two returns;
> + link = device_link_add(client, ctrldev, flags);
> + if (!link) {
> + ssam_controller_stateunlock(c);
> + return -ENOMEM;
> + }
> +
> + /*
> + * Return -ENXIO if supplier driver is on its way to be removed. In this
> + * case, the controller won't be around for much longer and the device
> + * link is not going to save us any more, as unbinding is already in
> + * progress.
> + */
> + if (READ_ONCE(link->status) == DL_STATE_SUPPLIER_UNBIND) {
> + ssam_controller_stateunlock(c);
> + return -ENXIO;
> + }
> +
> + ssam_controller_stateunlock(c);
> + return 0;
> +}
> [...]
> +int ssam_client_bind(struct device *client, struct ssam_controller **ctrl)
> +{
> + struct ssam_controller *c;
> + int status;
> +
> + c = ssam_get_controller();
> + if (!c)
> + return -ENXIO;
To me, ENODEV seems like a better fit here.
> +
> + status = ssam_client_link(c, client);
> [...]
> +static ssize_t firmware_version_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct ssam_controller *ctrl = dev_get_drvdata(dev);
> + u32 version, a, b, c;
> + int status;
> +
> + status = ssam_get_firmware_version(ctrl, &version);
> + if (status < 0)
> + return status;
> +
> + a = (version >> 24) & 0xff;
> + b = ((version >> 8) & 0xffff);
> + c = version & 0xff;
> +
> + return snprintf(buf, PAGE_SIZE - 1, "%u.%u.%u\n", a, b, c);
`snprintf()` takes care of the null byte, so simply `PAGE_SIZE` would've been
sufficient. But that doesn't matter much since you should use `sysfs_emit()`.
> +}
> +static DEVICE_ATTR_RO(firmware_version);
> +
> +static struct attribute *ssam_sam_attrs[] = {
> + &dev_attr_firmware_version.attr,
> + NULL,
^
I believe it is preferred to omit the comma after the terminating entry.
> [...]
> +/**
> + * msgb_push_cmd() - Push a SSH command frame with payload to the buffer.
> + * @msgb: The message buffer.
> + * @seq: The sequence ID (SEQ) of the frame/packet.
> + * @rqid: The request ID (RQID) of the request contained in the frame.
> + * @rqst: The request to wrap in the frame.
> + */
> +static inline void msgb_push_cmd(struct msgbuf *msgb, u8 seq, u16 rqid,
> + const struct ssam_request *rqst)
> +{
> + struct ssh_command *cmd;
> + const u8 *cmd_begin;
> + const u8 type = SSH_FRAME_TYPE_DATA_SEQ;
> +
> + // SYN
> + msgb_push_syn(msgb);
> +
> + // command frame + crc
> + msgb_push_frame(msgb, type, sizeof(*cmd) + rqst->length, seq);
> +
> + // frame payload: command struct + payload
> + if (WARN_ON(msgb->ptr + sizeof(*cmd) > msgb->end))
> + return;
> +
> + cmd_begin = msgb->ptr;
> + cmd = (struct ssh_command *)msgb->ptr;
I believe this violates strict aliasing.
> [...]
> + * Note that the packet completion callback is, in case of success and for a
> + * sequenced packet, guaranteed to run on the receiver thread, thus providing
> + * a way to reliably identify responses to the packet. The packet completion
> + * callback is only run once and it does not indicate that the packet has
> + * fully left the system (for this, one should rely on the release method,
> + * triggered when the reference count of the packet reaches zero). In case of
> + * re-submission (and with somewhat unlikely timing), it may be possible that
> + * the packet is being re-transmitted while the completion callback runs.
> + * Completion will occur both on success and internal error, as well as when
> + * the packet is canceled.
If I understand it correctly, it is possible that submission of a packet fails
for the first time, but it's scheduled for resubmission, and this retransmission
happens at the same time when the complete() callback is called. If that's the
case, then the callback is called with an error condition, no? Thus it is possible
that a packet is successfully submitted (for the second, third, etc. time), but the
complete() callback receives notification about failure? Or am I missing something?
> [...]
> +int ssh_ptl_rx_rcvbuf(struct ssh_ptl *ptl, const u8 *buf, size_t n)
> +{
> + int used;
> +
> + if (test_bit(SSH_PTL_SF_SHUTDOWN_BIT, &ptl->state))
> + return -ESHUTDOWN;
> +
> + used = kfifo_in(&ptl->rx.fifo, buf, n);
Isn't it possible that `n` is greater than the free space in the fifo? What
happens then?
> + if (used)
> + ssh_ptl_rx_wakeup(ptl);
> +
> + return used;
> +}
> [...]
> +void ssh_ptl_shutdown(struct ssh_ptl *ptl)
> +{
> [...]
> + * Note 2: We can re-use queue_node (or pending_node) if we mark the
> + * packet as locked an then remove it from the queue (or pending set
> + * respecitvely). Marking the packet as locked avoids re-queueing
> + * (which should already be prevented by having stopped the treads...)
> + * and not setting QUEUED_BIT (or PENDING_BIT) prevents removal from a
> + * new list via other threads (e.g. canellation).
^
"cancellation"
> + *
> + * Note 3: There may be overlap between complete_p and complete_q.
> + * This is handled via test_and_set_bit() on the "completed" flag
> + * (also handles cancellation).
> + */
> [...]
> +#define __ssam_prcond(func, p, fmt, ...) \
> + do { \
> + if ((p)) \
I believe `if (p)` is sufficient.
> + func((p), fmt, ##__VA_ARGS__); \
> + } while (0)
> [...]
> +int sshp_parse_frame(const struct device *dev, const struct ssam_span *source,
> + struct ssh_frame **frame, struct ssam_span *payload,
> + size_t maxlen)
> +{
> [...]
> + // ensure packet does not exceed maximum length
> + sp.len = get_unaligned_le16(&((struct ssh_frame *)sf.ptr)->len);
> + if (unlikely(sp.len + SSH_MESSAGE_LENGTH(0) > maxlen)) {
> + dev_warn(dev, "rx: parser: frame too large: %u bytes\n",
I believe `%hu` would be more appropriate.
> + ((struct ssh_frame *)sf.ptr)->len);
Why isn't `get_unaligned_le16()` used here? (Or simply even `sp.len`.)
> + return -EMSGSIZE;
> + }
> [...]
> + *frame = (struct ssh_frame *)sf.ptr;
This also violates strict aliasing.
> + *payload = sp;
> +
> + dev_dbg(dev, "rx: parser: valid frame found (type: 0x%02x, len: %u)\n",
> + (*frame)->type, (*frame)->len);
> +
> + return 0;
> +}
> [...]
> +int sshp_parse_command(const struct device *dev, const struct ssam_span *source,
> + struct ssh_command **command,
> + struct ssam_span *command_data)
> +{
> + // check for minimum length
> + if (unlikely(source->len < sizeof(struct ssh_command))) {
> + *command = NULL;
> + command_data->ptr = NULL;
> + command_data->len = 0;
> +
> + dev_err(dev, "rx: parser: command payload is too short\n");
> + return -ENOMSG;
> + }
> +
> + *command = (struct ssh_command *)source->ptr;
I'm quite sure this also violates strict aliasing.
> + command_data->ptr = source->ptr + sizeof(struct ssh_command);
> + command_data->len = source->len - sizeof(struct ssh_command);
> +
> + dev_dbg(dev, "rx: parser: valid command found (tc: 0x%02x, cid: 0x%02x)\n",
> + (*command)->tc, (*command)->cid);
> +
> + return 0;
> +}
> [...]
> +#define SSH_MSGOFFSET_FRAME(field) \
> + (sizeof(u16) + offsetof(struct ssh_frame, field))
> +
> +/**
> + * SSH_MSGOFFSET_COMMAND() - Compute offset in SSH message to specified field
> + * in command.
> + * @field: The field for which the offset should be computed.
> + *
> + * Return: Returns the offset of the specified &struct ssh_command field in
> + * the raw SSH message data.
> + */
> +#define SSH_MSGOFFSET_COMMAND(field) \
> + (2ull * sizeof(u16) + sizeof(struct ssh_frame) \
> + + offsetof(struct ssh_command, field))
I believe it should be noted (here and for `SSH_MSGOFFSET_FRAME()`) why the
`sizeof(u16)`s are necessary.
> +
> +/*
> + * SSH_MSG_SYN - SSH message synchronization (SYN) bytes as u16.
> + */
> +#define SSH_MSG_SYN ((u16)0x55aa)
> [...]
> +enum ssam_ssh_tc {
> + /* Known SSH/EC target categories. */
> + // category 0x00 is invalid for EC use
> + SSAM_SSH_TC_SAM = 0x01, // generic system functionality, real-time clock
> + SSAM_SSH_TC_BAT = 0x02, // battery/power subsystem
> + SSAM_SSH_TC_TMP = 0x03, // thermal subsystem
> + SSAM_SSH_TC_PMC = 0x04,
> + SSAM_SSH_TC_FAN = 0x05,
> + SSAM_SSH_TC_PoM = 0x06,
> + SSAM_SSH_TC_DBG = 0x07,
> + SSAM_SSH_TC_KBD = 0x08, // legacy keyboard (Laptop 1/2)
> + SSAM_SSH_TC_FWU = 0x09,
> + SSAM_SSH_TC_UNI = 0x0a,
> + SSAM_SSH_TC_LPC = 0x0b,
> + SSAM_SSH_TC_TCL = 0x0c,
> + SSAM_SSH_TC_SFL = 0x0d,
> + SSAM_SSH_TC_KIP = 0x0e,
> + SSAM_SSH_TC_EXT = 0x0f,
> + SSAM_SSH_TC_BLD = 0x10,
> + SSAM_SSH_TC_BAS = 0x11, // detachment system (Surface Book 2/3)
> + SSAM_SSH_TC_SEN = 0x12,
> + SSAM_SSH_TC_SRQ = 0x13,
> + SSAM_SSH_TC_MCU = 0x14,
> + SSAM_SSH_TC_HID = 0x15, // generic HID input subsystem
> + SSAM_SSH_TC_TCH = 0x16,
> + SSAM_SSH_TC_BKL = 0x17,
> + SSAM_SSH_TC_TAM = 0x18,
> + SSAM_SSH_TC_ACC = 0x19,
> + SSAM_SSH_TC_UFI = 0x1a,
> + SSAM_SSH_TC_USC = 0x1b,
> + SSAM_SSH_TC_PEN = 0x1c,
> + SSAM_SSH_TC_VID = 0x1d,
> + SSAM_SSH_TC_AUD = 0x1e,
> + SSAM_SSH_TC_SMC = 0x1f,
> + SSAM_SSH_TC_KPD = 0x20,
> + SSAM_SSH_TC_REG = 0x21,
> +};
Is it known what these abbreviations stand for? Maybe I missed them?
> [...]
> +/**
> + * struct ssh_request_ops - Callback operations for a SSH request.
> + * @release: Function called when the request's reference count reaches zero.
> + * This callback must be relied upon to ensure that the request has
> + * left the transport systems (both, packet an request systems).
> + * @complete: Function called when the request is completed, either with
> + * success or failure. The command data for the request response
> + * is provided via the &struct ssh_command parameter (``cmd``),
> + * the command payload of the request response via the &struct
> + * ssh_span parameter (``data``).
> + *
> + * If the request does not have any response or has not been
> + * completed with success, both ``cmd`` and ``data`` parameters will
> + * be NULL. If the request response does not have any command
> + * payload, the ``data`` span will be an empty (zero-length) span.
> + *
> + * In case of failure, the reason for the failure is indicated by
> + * the value of the provided status code argument (``status``). This
> + * value will be zero in case of success.
I believe it should be noted if the `status` argument is a regular errno,
or something different.
> + *
> + * Note that a call to this callback does not guarantee that the
> + * request is not in use by the transport systems any more.
> + */
> +struct ssh_request_ops {
> + void (*release)(struct ssh_request *rqst);
> + void (*complete)(struct ssh_request *rqst,
> + const struct ssh_command *cmd,
> + const struct ssam_span *data, int status);
> +};
> [...]
I have to agree, this is quite a sizable patch, although I think it's well-commented,
which helps reading the code by a lot, however, in some places I feel like it's a bit
over-engineered (or maybe I just cannot fully appreciate the subject at hand at the moment),
nonetheless, I applaud your efforts, I can only imagine the hours that must have gone into it.
Regards,
Barnabás Pőcze
