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. 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
