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

[Ivan Vecera <ivecera@redhat.com>]

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