Re: [PATCH net-next v9 06/14] dpll: zl3073x: Fetch invariants during probe

[Ivan Vecera]

On 13. 06. 25 9:13 odp., Vadim Fedorenko wrote:
> > +    synth->enabled = FIELD_GET(ZL_SYNTH_CTRL_EN, synth_ctrl);
> > +    synth->dpll = FIELD_GET(ZL_SYNTH_CTRL_DPLL_SEL, synth_ctrl);
> > +
> > +    dev_dbg(zldev->dev, "SYNTH%u is %s and driven by DPLL%u\n", index,
> > +        synth->enabled ? "enabled" : "disabled", synth->dpll);
> > +
> > +    guard(mutex)(&zldev->multiop_lock);
>
> Not a strong suggestion, but it would be good to follow netdev style
> (same for some previous functions):

Hi Vadim,

I'm using guard() on places (functions) where it is necessary to hold
the lock from that place to the end of the function. Due to this
scoped_guard() does not give any advantage. Using classic mutex_lock()
and mutex_unlock() would only increases the risks of locking-related
bugs. Also manual locking enforces to use mutex_unlock() or goto in
all error paths after taking lock.

> https://docs.kernel.org/process/maintainer-netdev.html#using-device- 
> managed-and-cleanup-h-constructs
>
> "Use of guard() is discouraged within any function longer than 20 lines,
> scoped_guard() is considered more readable. Using normal lock/unlock is 
> still (weakly) preferred."
>
> > +
> > +    /* Read synth configuration */
> > +    rc = zl3073x_mb_op(zldev, ZL_REG_SYNTH_MB_SEM, ZL_SYNTH_MB_SEM_RD,
> > +               ZL_REG_SYNTH_MB_MASK, BIT(index));
> > +    if (rc)
> > +        return rc;
> > +
> > +    /* The output frequency is determined by the following formula:
> > +     * base * multiplier * numerator / denominator
> > +     *
> > +     * Read registers with these values
> > +     */
> > +    rc = zl3073x_read_u16(zldev, ZL_REG_SYNTH_FREQ_BASE, &base);
> > +    if (rc)
> > +        return rc;
> > +
> > +    rc = zl3073x_read_u32(zldev, ZL_REG_SYNTH_FREQ_MULT, &mult);
> > +    if (rc)
> > +        return rc;
> > +
> > +    rc = zl3073x_read_u16(zldev, ZL_REG_SYNTH_FREQ_M, &m);
> > +    if (rc)
> > +        return rc;
> > +
> > +    rc = zl3073x_read_u16(zldev, ZL_REG_SYNTH_FREQ_N, &n);
> > +    if (rc)
> > +        return rc;
> > +

---> You have to keep the lock to here.

> > +    /* Check denominator for zero to avoid div by 0 */
> > +    if (!n) {
> > +        dev_err(zldev->dev,
> > +            "Zero divisor for SYNTH%u retrieved from device\n",
> > +            index);
> > +        return -EINVAL;
> > +    }
> > +
> > +    /* Compute and store synth frequency */
> > +    zldev->synth[index].freq = div_u64(mul_u32_u32(base * m, mult), n);
> > +
> > +    dev_dbg(zldev->dev, "SYNTH%u frequency: %u Hz\n", index,
> > +        zldev->synth[index].freq);
> > +
> > +    return rc;
> > +} 

This kind of function (above) is mailbox-read:
1. Take lock
2. Ask firmware to fill mailbox latch registers
3. Read latch1
4. ...
5. Unlock

But in later commits there are mailbox-write functions that:
1. Take lock
2. Ask firmware to fill mailbox latch registers
3. Write or read-update-write latch registers
4. ...
5. Ask firmware to update HW from the latch registers (commit)
6. Unlock

Step 5 here is usually represented by:

return zl3073x_mb_op(zldev, ZL_REG_*_MB_SEM, ZL_*_MB_SEM_RD,
                     ZL_REG_*_MB_MASK, BIT(index));

and here is an advantage of guard() that unlocks the mutex automatically
after zl3073x_mb_op() and prior returning its return value.

Thanks,
Ivan