Re: [net-next,5/6] dpll: zl3073x: add reference priority to zl3073x_chan

[Ivan Vecera <ivecera@redhat.com>]

On 3/14/26 8:53 PM, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email says "Is this legit? Quite hard to follow the logic of this change :/"
> 
> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
> ---
> dpll: zl3073x: add reference priority to zl3073x_chan
> 
> This commit caches the ZL_REG_DPLL_REF_PRIO registers in the zl3073x_chan
> cfg group, adding a ref_prio[] array and inline helpers for accessing
> priority values. It extends state_fetch and state_set with DPLL mailbox
> operations to read and write these priority registers.
> 
>> diff --git a/drivers/dpll/zl3073x/chan.c b/drivers/dpll/zl3073x/chan.c
>> index 10189c8a5ded..74d00cee624e 100644
>> --- a/drivers/dpll/zl3073x/chan.c
>> +++ b/drivers/dpll/zl3073x/chan.c
> 
> [ ... ]
> 
>> @@ -85,7 +102,9 @@ const struct zl3073x_chan *zl3073x_chan_state_get(struct zl3073x_dev *zldev,
>>    * @chan: desired channel state
>>    *
>>    * Skips the HW write if the configuration is unchanged, and otherwise
>> - * writes the mode_refsel register to hardware.
>> + * writes only the changed registers to hardware. The mode_refsel register
>> + * is written directly, while the reference priority registers are written
>> + * via the DPLL mailbox interface.
>>    *
>>    * Return: 0 on success, <0 on HW error
>>    */
>> @@ -93,14 +112,48 @@ int zl3073x_chan_state_set(struct zl3073x_dev *zldev, u8 index,
>>   			   const struct zl3073x_chan *chan)
>>   {
>>   	struct zl3073x_chan *dchan = &zldev->chan[index];
>> -	int rc;
>> +	int rc, i;
>>
>>   	/* Skip HW write if configuration hasn't changed */
>>   	if (!memcmp(&dchan->cfg, &chan->cfg, sizeof(chan->cfg)))
>>   		return 0;
>>
>> -	rc = zl3073x_write_u8(zldev, ZL_REG_DPLL_MODE_REFSEL(index),
>> -			      chan->mode_refsel);
>> +	/* Direct register write for mode_refsel */
>> +	if (dchan->mode_refsel != chan->mode_refsel) {
>> +		rc = zl3073x_write_u8(zldev, ZL_REG_DPLL_MODE_REFSEL(index),
>> +				      chan->mode_refsel);
>> +		if (rc)
>> +			return rc;
>> +	}
>> +
>> +	/* Mailbox write for ref_prio if changed */
>> +	if (!memcmp(dchan->ref_prio, chan->ref_prio, sizeof(chan->ref_prio))) {
>> +		dchan->cfg = chan->cfg;
>> +		return 0;
>> +	}
>> +
>> +	guard(mutex)(&zldev->multiop_lock);
>> +
>> +	/* Read DPLL configuration into mailbox */
>> +	rc = zl3073x_mb_op(zldev, ZL_REG_DPLL_MB_SEM, ZL_DPLL_MB_SEM_RD,
>> +			   ZL_REG_DPLL_MB_MASK, BIT(index));
>> +	if (rc)
>> +		return rc;
>> +
>> +	/* Update changed ref_prio registers */
>> +	for (i = 0; i < ARRAY_SIZE(chan->ref_prio); i++) {
>> +		if (dchan->ref_prio[i] != chan->ref_prio[i]) {
>> +			rc = zl3073x_write_u8(zldev,
>> +					      ZL_REG_DPLL_REF_PRIO(i),
>> +					      chan->ref_prio[i]);
>> +			if (rc)
>> +				return rc;
>> +		}
>> +	}
>> +
>> +	/* Commit DPLL configuration */
>> +	rc = zl3073x_mb_op(zldev, ZL_REG_DPLL_MB_SEM, ZL_DPLL_MB_SEM_WR,
>> +			   ZL_REG_DPLL_MB_MASK, BIT(index));
>>   	if (rc)
>>   		return rc;
>>
> 
> Could this function leave the cache and hardware out of sync?
> 
> When both mode_refsel and ref_prio have changed, the function writes
> mode_refsel to hardware first. If the subsequent mailbox operation fails
> (during the mailbox read, a ref_prio write, or the mailbox commit), the
> function returns an error without executing 'dchan->cfg = chan->cfg'.

This potential situation is very unusual (mode_refsel updated and
mailbox access erroneous) but yes, after successful write of mode_refsel
dchan->mon_refsel should be updated to avoid this issue.

Will fix in v2.

Ivan