On 12/8/20 3:43 PM, Hans de Goede wrote:
Hi,
On 12/8/20 3:37 PM, Maximilian Luz wrote:
<snip>
+
+ obj = acpi_evaluate_dsm_typed(handle, &SSAM_SSH_DSM_GUID,
+ SSAM_SSH_DSM_REVISION, func, NULL,
+ ACPI_TYPE_INTEGER);
+ if (!obj)
+ return -EIO;
+
+ val = obj->integer.value;
+ ACPI_FREE(obj);
+
+ if (val > U32_MAX)
+ return -ERANGE;
+
+ *ret = val;
+ return 0;
+}
[...]
+/**
+ * ssam_controller_start() - Start the receiver and transmitter threads of the
+ * controller.
+ * @ctrl: The controller.
+ *
+ * Note: When this function is called, the controller should be properly
+ * hooked up to the serdev core via &struct serdev_device_ops. Please refer
+ * to ssam_controller_init() for more details on controller initialization.
+ *
+ * This function must be called from an exclusive context with regards to the
+ * state, if necessary, by locking the controller via ssam_controller_lock().
Again you are being a bit hand-wavy (I assume you know what I mean by that)
wrt the locking requirements. If possible I would prefer clearly spelled out
locking requirements in the form of "this and that lock must be held when
calling this function". Preferably backed-up by lockdep_assert-s asserting
these conditions.
The reason for this is that this function specifically is currently only
called during initialization, when the controller has not been published
yet, i.e. when we have an exclusive reference to the controller.
I'll change this to fully enforce locking (with lockdep_assert).
And maybe if you are a bit stricter with always holding the lock when
calling this, you can also drop the WRITE_ONCE and the comment about it
(in all places where you do this).
The WRITE_ONCE is only there to ensure that the basic test in
ssam_request_sync_submit() can be done. I always try to be explicit
about access that can happen without the respective locks being held.
Yes I saw the matching READ_ONCE later on (as the comment indicated
I would), which made it more obvious to me why the WRITE_ONCE is here,'
so maybe I should have gone back and updated this comment.
No worries, always good to have another look at these kinds of things.
Anyways, keeping the WRITE_ONCE + READ_ONCE for this is fine.
Unfortunately it's not feasible to hold the reader lock in
ssam_request_sync_submit() due to reentrancy. Specifically, as the lock,
if at all (i.e. if this is not a client driver bound to the controller),
must be held not only during submission but until the request has been
completed. Note that if we would hold the lock during submission, this
is just a smoke-test.
Ack.
<more snip>
Regards,
Hans
