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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a129e92b-6f01-4345-979f-e57e1829e506@redhat.com>
Date: Sat, 14 Jun 2025 12:55:44 +0200
From: Ivan Vecera <ivecera@...hat.com>
To: Vadim Fedorenko <vadim.fedorenko@...ux.dev>, netdev@...r.kernel.org
Cc: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>,
 Jiri Pirko <jiri@...nulli.us>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Prathosh Satish <Prathosh.Satish@...rochip.com>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Simon Horman <horms@...nel.org>, Jonathan Corbet <corbet@....net>,
 Jason Gunthorpe <jgg@...pe.ca>, Shannon Nelson <shannon.nelson@....com>,
 Dave Jiang <dave.jiang@...el.com>,
 Jonathan Cameron <Jonathan.Cameron@...wei.com>, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
 Michal Schmidt <mschmidt@...hat.com>, Petr Oros <poros@...hat.com>
Subject: Re: [PATCH net-next v9 06/14] dpll: zl3073x: Fetch invariants during
 probe

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