[<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
Powered by blists - more mailing lists