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

[Maximilian Luz]

On 11/16/20 6:03 PM, Maximilian Luz wrote:
> On 11/16/20 2:33 PM, Andy Shevchenko wrote:
> > On Sun, Nov 15, 2020 at 9:25 PM Maximilian Luz <luzmaximilian@xxxxxxxxx> wrote:

[...]

> READ_ONCE and WRITE_ONCE are used to ensure proper access to state that
> can be changed outside of the queue/pending locks (or under any one of
> them). In general, I have tried to document all critical access of such
> state with an explanation of why it is safe to do so.

I've looked at this again and noticed that I can guard the packet
timestamp by the pending lock and the packet priority by the queue lock
(after first submission). This makes reasoning about access to them
significantly easier and removes the need for WRITE_ONCE / READ_ONCE.

After that, READ_ONCE is used

- to access the controller state for smoke-testing to (hopefully) detect
  invalid request function usage (note that all other access to this
  state happens under the controller state lock)

- for the "safe counters", where access to the shared value is, after
  initialization, restricted to the increment function

- to update the timeout reaper, where access to the shared value
  (rtx_timeout.expires) is, after initialization, restricted to its
  modification function (ssh_ptl_timeout_reaper_mod() /
  ssh_rtl_timeout_reaper_mod()) and the timer function

- to access the request timestamp, which is, after initialization, only
  set once in the lifetime of a request (all other access is read-only)

- to access the 'ptl' reference of the packet, which, after
  initialization, is only set once, either at packet or request
  submission (all other access is read-only). Note due to this,
  READ_ONCE is only required for functions that can run concurrently
  with ssh_ptl_submit() and ssh_rtl_submit(), i.e. ssh_ptl_cancel() and
  ssh_rtl_cancel().

- to access request state outside of bit-ops when canceling

I'd argue that all of these cases can be checked and verified with a
reasonable amount of effort. Cancellation (last two points) is probably
the most complex one. Unfortunately, I don't see any way to simplify
that part without disallowing cancellation to run concurrently to
submission, which is something I'd like to support as this makes
implementing asynchronous requests in the future easier.

Regards,
Max