Re: [PATCH net v3 4/4] dpll: zl3073x: notify sibling pin when shared output config changes

[Ivan Vecera <ivecera@redhat.com>]

On 10/8/26 17:31, 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 commit message describes the old teardown incorrectly.
> - [Low] The documentation for zl3073x_dpll_output_pin_sibling_get() does
>    not match its code, and the commit message's safety claim is only…
> 
> --
> 
> dpll: zl3073x: notify sibling pin when shared output config changes
> 
> The P and N pins of a zl3073x output share the divisor, clock type,
> esync and phase compensation registers. This patch adds a sibling
> lookup helper and sends a change notification to the sibling pin from
> frequency_set(), esync_set() and phase_adjust_set(). It also changes
> the order of list_add()/list_del() on zldpll->pins relative to DPLL
> pin registration, and does both under zldpll->lock.
> 
>> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
>> index 65107b4cc4f82..9c678acc3e777 100644
>> --- a/drivers/dpll/zl3073x/dpll.c
>> +++ b/drivers/dpll/zl3073x/dpll.c
> 
> [ ... ]
> 
>> @@ -132,11 +134,39 @@ zl3073x_dpll_pin_get_by_ref(struct zl3073x_dpll *zldpll, u8 ref_id)
>>   	return NULL;
>>   }
>>   
>> +/**
>> + * zl3073x_dpll_output_pin_sibling_get - get the other pin of an output pair
>> + * @pin: output pin whose sibling is sought
>> + *
>> + * Output pin ids are allocated in P/N pairs (P even, N odd) that share a
>> + * single HW output. Looks up the other pin of the pair, if it is
>> + * registered as a dpll_pin on this DPLL.
>> + *
>> + * Return: pointer to sibling pin, or NULL if it is not registered
>> + */
>> +static struct zl3073x_dpll_pin *
>> +zl3073x_dpll_output_pin_sibling_get(struct zl3073x_dpll_pin *pin)
>> +{
>> +	struct zl3073x_dpll_pin *sibling;
>> +
>> +	lockdep_assert_held(&pin->dpll->lock);
>> +
>> +	list_for_each_entry(sibling, &pin->dpll->pins, list) {
>> +		if (!zl3073x_dpll_is_input_pin(sibling) &&
>> +		    sibling->id == (pin->id ^ 1))
>> +			return sibling;
>> +	}
>> +
>> +	return NULL;
>> +}
> 
> [Severity: Low]
> Does this kernel-doc match what the helper does? It says NULL is
> returned when the sibling is not registered. The loop only checks list
> membership, direction and id.
> 
> This patch makes zl3073x_dpll_pin_register() and
> zl3073x_dpll_nco_pin_register() add the pin to zldpll->pins before
> dpll_pin_register(). It also makes zl3073x_dpll_pin_unregister()
> remove the pin only after dpll_pin_unregister(). During those windows
> the helper can return a pin that is not registered.
> 
> esync_set(), frequency_set() and phase_adjust_set() then pass that pin
> directly to __dpll_pin_change_ntf(), whose kernel-doc says:
> 
>   * @pin: registered pin pointer
> 
> The window looks reachable. zl3073x_dpll_register() registers the DPLL
> device before its pins, so a PIN_SET on the P pin can run while the N
> pin's dpll_pin_register() waits on dpll_lock. On teardown, the N pin is
> unregistered while the P pin is still available.
> 
> The commit message also says:
> 
>    A pin that is transiently on the list while
>    not registered is harmless: __dpll_pin_change_ntf() is a no-op for a
>    pin that is not available
> 
> Is that accurate? Only dpll_pin_event_send() checks
> dpll_pin_available(). dpll_pin_notify() runs first and calls the
> in-kernel notifier chain without any check:
> 
> __dpll_pin_change_ntf()
>      dpll_pin_notify(pin, pin->clock_id, DPLL_PIN_CHANGED);
>          call_dpll_notifiers(action, &info);
>      return dpll_pin_event_send(DPLL_CMD_PIN_CHANGE_NTF, pin);
> 
> So an in-kernel DPLL_PIN_CHANGED event can be sent for a pin that is
> not registered. Nothing in the tree seems to be affected today: the
> only in-tree notifier, ice_dpll_pin_notify(), ignores actions other
> than CREATED and DELETED.
> 
> Should the kernel-doc and the commit message be corrected? The other
> option is to check that the sibling is actually registered before it
> is notified.

Will correct.
> [ ... ]
> 
>> @@ -1784,6 +1874,13 @@ zl3073x_dpll_pin_unregister(struct zl3073x_dpll_pin *pin)
>>   	/* Unregister the pin */
>>   	dpll_pin_unregister(zldpll->dpll_dev, pin->dpll_pin, ops, pin);
>>   
>> +	/* Remove the pin from the list only after it has been unregistered so
>> +	 * that a still-registered pin is always findable as a sibling.
>> +	 */
>> +	mutex_lock(&zldpll->lock);
>> +	list_del(&pin->list);
>> +	mutex_unlock(&zldpll->lock);
>> +
>>   	dpll_pin_put(pin->dpll_pin, &pin->tracker);
>>   	pin->dpll_pin = NULL;
>>   
>> @@ -1803,9 +1900,11 @@ zl3073x_dpll_pins_unregister(struct zl3073x_dpll *zldpll)
>>   {
>>   	struct zl3073x_dpll_pin *pin, *next;
>>   
>> +	/* Unregister each pin before removing it from the list so that a
>> +	 * still-registered pin is always findable as a sibling.
>> +	 */
>>   	list_for_each_entry_safe(pin, next, &zldpll->pins, list) {
>>   		zl3073x_dpll_pin_unregister(pin);
>> -		list_del(&pin->list);
>>   		zl3073x_dpll_pin_free(pin);
>>   	}
>>   }
> 
> [Severity: Low]
> The commit message says:
> 
>    and teardown detached the whole list before unregistering any
>    pin, so a sibling that is registered - and thus reachable by a PIN_SET
>    on the other pin - could be missing from the list and never notified.
> 
> Does this describe the old teardown correctly? Before this patch,
> zl3073x_dpll_pins_unregister() already unregistered each pin before
> removing it from the list:
> 
> 	list_for_each_entry_safe(pin, next, &zldpll->pins, list) {
> 		zl3073x_dpll_pin_unregister(pin);
> 		list_del(&pin->list);
> 		zl3073x_dpll_pin_free(pin);
> 	}
> 
> drivers/dpll/zl3073x/ has no list_splice() or similar call on this
> list. A pin that was still registered therefore stayed on zldpll->pins
> during teardown.
> 
> The list and the registration state only got out of step at
> registration time, because dpll_pin_register() ran before list_add().
> On the teardown side, this hunk mainly moves list_del() under
> zldpll->lock, which the new lockdep-asserted lookup needs.
> 
> Could the commit message describe what the teardown change actually
> does?
Yes, will rewrite it in v4.

Thanks,
Ivan