Re: [PATCH net-next v7 2/3] dpll: zl3073x: add channel ToD, phase step and TIE operations

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 13/08/2026 21:51, Ivan Vecera wrote:
> 
> 
> 13. srpna 2026 22:38:55 SELČ, Vadim Fedorenko <vadim.fedorenko@linux.dev> napsal:
>> On 12/08/2026 12:00, Ivan Vecera wrote:
>>> On 8/12/26 12:04 PM, Vadim Fedorenko wrote:
>>>> On 12/08/2026 07:57, Ivan Vecera wrote:
>>>>> Sashiko findings. Replies inline.
>>>>>
>>>>>   > Could a transient hardware error bring down the system here?
>>>>>   >
>>>>>   > If an I2C/SPI bus glitch causes the device to return 0xFF, the SEM
>>>>>   > bit will be set and the CMD field will hit this default case. On
>>>>>   > systems with panic_on_warn, using WARN_ON for validating external
>>>>>   > hardware states turns recoverable bus errors into fatal kernel panics.
>>>>>
>>>>> The SEM-first check already handles the most common bus glitch (0x00
>>>>> return). For 0xFF: the CMD field is only written by the driver, never
>>>>> by firmware, so an unknown CMD with SEM set indicates either a bus
>>>>> error or firmware misbehavior that warrants attention. The switch
>>>>> structure with WARN_ON in the default case was requested by Vadim
>>>>> in his v4 review. Systems that enable panic_on_warn accept this
>>>>> trade-off.
>>>>>
>>>>>   > Will this sleep-based polling loop destroy the timestamp's precision?
>>>>>   >
>>>>>   > Should the postts be captured immediately after the trigger command
>>>>>   > in zl3073x_chan_tod_ctrl() instead?
>>>>>
>>>>> The hardware latches the ToD value when it processes the command,
>>>>> which completes when the semaphore clears. The post-timestamp must
>>>>> be taken after the semaphore clears to guarantee the window contains
>>>>> the actual latch event. Moving it before the wait would risk the
>>>>> timestamp window not containing the latch moment.
>>>>>
>>>>>   > Could this loop exhaust its retries and return -EBUSY prematurely?
>>>>>   >
>>>>>   > The loop spins without an explicit wait [...] On fast SPI/I2C buses,
>>>>>   > it will execute all 20 reads in a few milliseconds
>>>>>
>>>>> Testing on I2C at both 100 kHz and 400 kHz bus speeds shows that a
>>>>> single iteration of the loop body (two ToD reads, each involving a
>>>>> ready-wait, command write, second ready-wait and data reads) takes
>>>>> approximately 17-19 ms regardless of bus speed. The iteration time
>>>>> is dominated by the device's internal processing, not bus transfer
>>>>> time. With 20 retries the budget is 340-380 ms, well beyond the
>>>>> 20 ms margin window.
>>>>>
>>>>>   > Is it safe to use WARN_ON to validate user-controlled input?
>>>>>   >
>>>>>   > Since delta_ns originates from the clock_adjtime syscall's tx.offset
>>>>>   > (via the adjphase PTP callback) [...]
>>>>>
>>>>> The PTP core already validates the input via getmaxphase, which
>>>>> returns NSEC_PER_SEC - 1, rejecting values with magnitude >=
>>>>> NSEC_PER_SEC before the driver callback is invoked. The WARN_ON is
>>>>> a defensive check for a condition that should never be reached
>>>>> through normal code paths, not user input validation.
>>>>
>>>> AFAIR, the general rule is not to write defensive code in kernel if you know that the core has already validated inputs.
>>>
>>> Yes, but this low-level helper zl3073x_chan_tie_write() is called from
>>> multiple places and current code-paths are OK. But in future, if another
>>> caller will be introduced or existing code will be refactored this WARN
>>> immediately detects potential bug.
>>>
>>> The same is also valid for WARN in zl3073x_chan_tod_ready_wait()... new
>>> TOD command starts to be used but someone forget to update this function
>>> accordingly.
>>
>> That's a little bit weak and goes against "trust internal APIs". It's
>> currently called from adjtime and adjphase callbacks, what do you expect
>> to have in the future?
>>
> 
> No idea
> I will drop it...

ack