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

Maximilian Luz
2020-12-08T14:55:05Z

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
>