Re: [PATCH net-next 10/10] dpll: document locking rules

[Ivan Vecera <ivecera@redhat.com>]

On 11/10/26 18:53, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] The new Locking section in Documentation/driver-api/dpll.rst
>    covers three cases where a pin callback reaches private data owned by…
> - [Low] The new driver rule in Documentation/driver-api/dpll.rst (lines
>    716-718) says notifications for objects outside the callback's domain…
> 
> --
> 
> dpll: document locking rules
> 
> This commit adds a Locking section to the dpll driver API documentation.
> The section describes the dpll_lock and per-domain lock levels, the domain
> each driver callback runs under, and the rules drivers have to follow. It
> also adds matching notes to the kernel-doc of dpll_pin_register() and
> dpll_pin_on_pin_register().
> 
>> diff --git a/Documentation/driver-api/dpll.rst b/Documentation/driver-api/dpll.rst
>> index 7c117ae37cc1b..40d5037ff3bb0 100644
>> --- a/Documentation/driver-api/dpll.rst
>> +++ b/Documentation/driver-api/dpll.rst
>> @@ -675,6 +675,50 @@ In the error path we have to rewind every allocation in the reverse order:
> 
> [ ... ]
> 
>> +Callbacks of a dpll device run under the domain of the device. Callbacks of a
>> +pin run under the domain of the pin, whichever driver registered them:
>> +
>> +- A driver registering a pin of another driver with its dpll device has these
>> +  callbacks serialized with the driver that created the pin, not with the other
>> +  callbacks of its device, so it has to protect the state they share by itself.
>> +  A callback stuck in such a registration stops the domain of the pin.
>> +- The parent pin of a pin registered with dpll_pin_on_pin_register() and its
>> +  dpll devices can belong to another driver, so the callback must not
>> +  dereference their private data.
>> +- The frequency monitor belongs to the dpll device, so only callbacks of a pin
>> +  of the same domain registered with dpll_pin_register() can include
>> +  ``.measured_freq_get``.
> 
> [Severity: Low]
> Should reference-sync pairs be in this list too?

[...]
> Could the documentation say whether a ref-sync pair may cross domains, and
> whether ref_sync_pin_priv may be dereferenced? The parent pin case in the
> dpll_pin_on_pin_register() bullet already has an explicit rule like this.

Yes. zl3073x and ice pair only their own input pins, so no in-tree pair
crosses domains. v2 will add a rule that both pins of a reference sync pair
have to belong to one domain, as the ref-sync callbacks of a pin get the
private data of the other one.

[...]

> [Severity: Low]
> Can a driver follow the work item suggestion safely without two more rules?

[...]

> Here the writer waits for the callback's read hold. The work's down_read()
> blocks behind the queued writer, and the callback waits for the work.

Right, and if the work notifies an object of the same domain, it also
waits for the domain lock. v2 will extend the rules to waiting for a work
item that makes these calls.

[...]
> The second rule is about lifetime. The text does not say that the work has
> to be cancelled or flushed before the target pin or device is unregistered
> and released with dpll_pin_put() or dpll_device_put(). __dpll_device_put()
> ends in kfree(dpll). A work item that keeps a struct dpll_device or struct
> dpll_pin pointer and runs after the final put would touch freed memory.

This holds for any work item that keeps a pointer to a refcounted object
and is not about locking, so I would leave it out of this section.

Thanks,
Ivan