| 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
| ||
|
Message-ID: <6facfb1d-eafa-432c-9896-321ea9cd9a88@redhat.com>
Date: Thu, 10 Apr 2025 14:01:11 +0200
From: Ivan Vecera <ivecera@...hat.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, Andrew Lunn <andrew@...n.ch>
Cc: netdev@...r.kernel.org, Michal Schmidt <mschmidt@...hat.com>,
Vadim Fedorenko <vadim.fedorenko@...ux.dev>,
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>,
Lee Jones <lee@...nel.org>, Kees Cook <kees@...nel.org>,
Andy Shevchenko <andy@...nel.org>, Andrew Morton
<akpm@...ux-foundation.org>, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH 05/28] mfd: zl3073x: Add components versions register defs
On 10. 04. 25 12:42 odp., Krzysztof Kozlowski wrote:
> On 10/04/2025 12:23, Ivan Vecera wrote:
>>
>>
>> On 10. 04. 25 9:11 dop., Krzysztof Kozlowski wrote:
>>> On 09/04/2025 08:44, Ivan Vecera wrote:
>>>> On 07. 04. 25 11:09 odp., Andrew Lunn wrote:
>>>>> On Mon, Apr 07, 2025 at 07:28:32PM +0200, Ivan Vecera wrote:
>>>>>> Add register definitions for components versions and report them
>>>>>> during probe.
>>>>>>
>>>>>> Reviewed-by: Michal Schmidt <mschmidt@...hat.com>
>>>>>> Signed-off-by: Ivan Vecera <ivecera@...hat.com>
>>>>>> ---
>>>>>> drivers/mfd/zl3073x-core.c | 35 +++++++++++++++++++++++++++++++++++
>>>>>> 1 file changed, 35 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/mfd/zl3073x-core.c b/drivers/mfd/zl3073x-core.c
>>>>>> index 39d4c8608a740..b3091b00cffa8 100644
>>>>>> --- a/drivers/mfd/zl3073x-core.c
>>>>>> +++ b/drivers/mfd/zl3073x-core.c
>>>>>> @@ -1,10 +1,19 @@
>>>>>> // SPDX-License-Identifier: GPL-2.0-only
>>>>>>
>>>>>> +#include <linux/bitfield.h>
>>>>>> #include <linux/module.h>
>>>>>> #include <linux/unaligned.h>
>>>>>> #include <net/devlink.h>
>>>>>> #include "zl3073x.h"
>>>>>>
>>>>>> +/*
>>>>>> + * Register Map Page 0, General
>>>>>> + */
>>>>>> +ZL3073X_REG16_DEF(id, 0x0001);
>>>>>> +ZL3073X_REG16_DEF(revision, 0x0003);
>>>>>> +ZL3073X_REG16_DEF(fw_ver, 0x0005);
>>>>>> +ZL3073X_REG32_DEF(custom_config_ver, 0x0007);
>>>>>> +
>>>>>> /*
>>>>>> * Regmap ranges
>>>>>> */
>>>>>> @@ -159,10 +168,36 @@ EXPORT_SYMBOL_NS_GPL(zl3073x_dev_alloc, "ZL3073X");
>>>>>>
>>>>>> int zl3073x_dev_init(struct zl3073x_dev *zldev)
>>>>>> {
>>>>>> + u16 id, revision, fw_ver;
>>>>>> struct devlink *devlink;
>>>>>> + u32 cfg_ver;
>>>>>> + int rc;
>>>>>>
>>>>>> devm_mutex_init(zldev->dev, &zldev->lock);
>>>>>>
>>>>>> + scoped_guard(zl3073x, zldev) {
>>>>>
>>>>> Why the scoped_guard? The locking scheme you have seems very opaque.
>>>>
>>>> We are read the HW registers in this block and the access is protected
>>>> by this device lock. Regmap locking will be disabled in v2 as this is
>>>
>>> Reading ID must be protected by mutex? Why and how?
>>
>> Yes, the ID is read from the hardware register and HW access functions
>> are protected by zl3073x_dev->lock. The access is not protected by
>
> Please do not keep repeating the same. You again describe the code. We
> ask why do you implement that way?
>
>> regmap locking schema. Set of registers are indirect and are accessed by
>> mailboxes where multiple register accesses need to be done atomically.
>
> regmap handles that, but anyway, how multiple register access to ID
> registers happen? From what module? Which code does it? So they write
> here something in the middle and reading would be unsynced?
OK, I'm going to try to explain in detail...
The device have 16 register pages where each of them has 128 registers
and register 0x7f on each page is a page selector.
Pages 0..9 contain direct registers that can be arbitrary read or
written in any order. For these registers implicit regmap locking is
sufficient.
Pages 10..16 contain indirect registers and these pages are called
mailboxes. Each mailbox cover specific part of hardware (synth, DPLL
channel, input ref, output...) and each of them contain mailbox_mask
register and mailbox_sem register. The rest of registers in the
particular page (mailbox) are latch registers.
Read operation (described in patch 8 in this v1 series):
E.g. driver needs to read frequency of input pin 4:
1) it set value of mailbox_mask (in input mailbox/page) to 4
2) it set mailbox_sem register (--"--) to read operation
3) it polls mailbox_sem to be cleared (firmware fills latch registers)
4) it reads frequency latch register (--"--) filled by FW
Write is similar but opposite:
1) it writes frequency to freq latch register (in input mb)
2) it set value of mailbox_mask
3) it set mailbox_sem to write operation
4) it polls mailbox_sem to be cleared (write was finished)
Steps 1-4 for both cases have to be done atomically - other reader
cannot modify mailbox_sem prior step 4 is finished and other writer
cannot touch latch registers prior step 4 is finished.
The module dpll_zl3073x (later in this series) and ptp_zl3073x (will be
posted later) use this intensively from multiple contexts (DPLL core
callbacks and monitoring threads).
So I have decided to use the custom locking scheme for accessing
registers instead of regmap locking that cannot guarantee this atomicity.
Would it be better to leave implicit regmap locking scheme for direct
registers and to have extra locking for mailboxes? If so, single mutex
for all mailboxes or separate mutex for each mailbox type?
Thanks,
Ivan
Powered by blists - more mailing lists