* Re: [PATCH v9 00/13] firmware: qcom: qseecom: convert to using the TZ allocator
From: Maximilian Luz @ 2024-03-28 16:50 UTC (permalink / raw)
To: Bartosz Golaszewski, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Elliot Berman, Krzysztof Kozlowski, Guru Das Srinagesh,
Andrew Halaney, Alex Elder, Srini Kandagatla, Arnd Bergmann
Cc: linux-arm-msm, linux-kernel, linux-arm-kernel, kernel,
Bartosz Golaszewski
In-Reply-To: <20240325100359.17001-1-brgl@bgdev.pl>
On 3/25/24 11:03 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> SCM calls that take memory buffers as arguments require that they be
> page-aligned, physically continuous and non-cachable. The same
> requirements apply to the buffer used to pass additional arguments to SCM
> calls that take more than 4.
>
> To that end drivers typically use dma_alloc_coherent() to allocate memory
> of suitable format which is slow and inefficient space-wise.
>
> SHM Bridge is a safety mechanism that - once enabled - will only allow
> passing buffers to the TrustZone that have been explicitly marked as
> shared. It improves the overall system safety with SCM calls and is
> required by the upcoming scminvoke functionality.
>
> The end goal of this series is to enable SHM bridge support for those
> architectures that support it but to that end we first need to unify the
> way memory for SCM calls is allocated. This in itself is beneficial as
> the current approach of using dma_alloc_coherent() in most places is quite
> slow.
>
> First let's add a new TZ Memory allocator that allows users to create
> dynamic memory pools of format suitable for sharing with the TrustZone.
> Make it ready for implementing multiple build-time modes.
>
> Convert all relevant drivers to using it. Add separate pools for SCM core
> and for qseecom.
>
> Finally add support for SHM bridge and make it the default mode of
> operation with the generic allocator as fallback for the platforms that
> don't support SHM bridge.
>
> Tested on db410c, RB5, sm8550-qrd. Previous iteration tested also on
> sa8775p-ride and lenovo X13s (please do retest on those platforms if you
> can).
Not sure in which version things changed (I haven't really kept up with
that, sorry), but this version (with the generic allocator selected in
the config) fails reading EFI vars on my Surface Pro X (sc8180x):
[ 2.381020] BUG: scheduling while atomic: mount/258/0x00000002
[ 2.383356] Modules linked in:
[ 2.385669] Preemption disabled at:
[ 2.385672] [<ffff800080f7868c>] qcom_tzmem_alloc+0x7c/0x224
[ 2.390325] CPU: 1 PID: 258 Comm: mount Not tainted 6.8.0-1-surface-dev #2
[ 2.392632] Hardware name: Microsoft Corporation Surface Pro X/Surface Pro X, BIOS 7.620.140 08/11/2023
[ 2.394955] Call trace:
[ 2.397269] dump_backtrace+0x94/0x114
[ 2.399583] show_stack+0x18/0x24
[ 2.401883] dump_stack_lvl+0x48/0x60
[ 2.404181] dump_stack+0x18/0x24
[ 2.406476] __schedule_bug+0x84/0xa0
[ 2.408770] __schedule+0x6f4/0x7fc
[ 2.411051] schedule+0x30/0xf0
[ 2.413323] synchronize_rcu_expedited+0x158/0x1ec
[ 2.415594] lru_cache_disable+0x28/0x74
[ 2.417853] __alloc_contig_migrate_range+0x68/0x210
[ 2.420122] alloc_contig_range+0x140/0x280
[ 2.422384] cma_alloc+0x128/0x404
[ 2.424643] cma_alloc_aligned+0x44/0x6c
[ 2.426881] dma_alloc_contiguous+0x30/0x44
[ 2.429111] __dma_direct_alloc_pages.isra.0+0x60/0x20c
[ 2.431343] dma_direct_alloc+0x6c/0x2ec
[ 2.433569] dma_alloc_attrs+0x80/0xf4
[ 2.435786] qcom_tzmem_pool_add_memory+0x8c/0x178
[ 2.438008] qcom_tzmem_alloc+0xe8/0x224
[ 2.440232] qsee_uefi_get_next_variable+0x78/0x2cc
[ 2.442443] qcuefi_get_next_variable+0x50/0x94
[ 2.444643] efivar_get_next_variable+0x20/0x2c
[ 2.446832] efivar_init+0x8c/0x29c
[ 2.449009] efivarfs_fill_super+0xd4/0xec
[ 2.451182] get_tree_single+0x74/0xbc
[ 2.453349] efivarfs_get_tree+0x18/0x24
[ 2.455513] vfs_get_tree+0x28/0xe8
[ 2.457680] vfs_cmd_create+0x5c/0xf4
[ 2.459840] __do_sys_fsconfig+0x458/0x598
[ 2.461993] __arm64_sys_fsconfig+0x24/0x30
[ 2.464143] invoke_syscall+0x48/0x110
[ 2.466281] el0_svc_common.constprop.0+0x40/0xe0
[ 2.468415] do_el0_svc+0x1c/0x28
[ 2.470546] el0_svc+0x34/0xb4
[ 2.472669] el0t_64_sync_handler+0x120/0x12c
[ 2.474793] el0t_64_sync+0x190/0x194
and subsequently
[ 2.477613] DEBUG_LOCKS_WARN_ON(val > preempt_count())
[ 2.477618] WARNING: CPU: 4 PID: 258 at kernel/sched/core.c:5889 preempt_count_sub+0x90/0xd4
[ 2.478682] Modules linked in:
[ 2.479214] CPU: 4 PID: 258 Comm: mount Tainted: G W 6.8.0-1-surface-dev #2
[ 2.479752] Hardware name: Microsoft Corporation Surface Pro X/Surface Pro X, BIOS 7.620.140 08/11/2023
[ 2.480296] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 2.480839] pc : preempt_count_sub+0x90/0xd4
[ 2.481380] lr : preempt_count_sub+0x90/0xd4
[ 2.481917] sp : ffff8000857cbb00
[ 2.482450] x29: ffff8000857cbb00 x28: ffff8000806b759c x27: 8000000000000005
[ 2.482988] x26: 0000000000000000 x25: ffff0000802cbaa0 x24: ffff0000809228b0
[ 2.483525] x23: 0000000000000000 x22: ffff800082b462f0 x21: 0000000000001000
[ 2.484062] x20: ffff80008363d000 x19: ffff000080922880 x18: fffffffffffc9660
[ 2.484600] x17: 0000000000000020 x16: 0000000000000000 x15: 0000000000000038
[ 2.485137] x14: 0000000000000000 x13: ffff800082649258 x12: 00000000000006db
[ 2.485674] x11: 0000000000000249 x10: ffff8000826fc930 x9 : ffff800082649258
[ 2.486207] x8 : 00000000ffffdfff x7 : ffff8000826f9258 x6 : 0000000000000249
[ 2.486738] x5 : 0000000000000000 x4 : 40000000ffffe249 x3 : 0000000000000000
[ 2.487267] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000841fa300
[ 2.487792] Call trace:
[ 2.488311] preempt_count_sub+0x90/0xd4
[ 2.488831] _raw_spin_unlock_irqrestore+0x1c/0x44
[ 2.489352] qcom_tzmem_alloc+0x1cc/0x224
[ 2.489873] qsee_uefi_get_next_variable+0x78/0x2cc
[ 2.490390] qcuefi_get_next_variable+0x50/0x94
[ 2.490907] efivar_get_next_variable+0x20/0x2c
[ 2.491420] efivar_init+0x8c/0x29c
[ 2.491931] efivarfs_fill_super+0xd4/0xec
[ 2.492440] get_tree_single+0x74/0xbc
[ 2.492948] efivarfs_get_tree+0x18/0x24
[ 2.493453] vfs_get_tree+0x28/0xe8
[ 2.493957] vfs_cmd_create+0x5c/0xf4
[ 2.494459] __do_sys_fsconfig+0x458/0x598
[ 2.494963] __arm64_sys_fsconfig+0x24/0x30
[ 2.495468] invoke_syscall+0x48/0x110
[ 2.495972] el0_svc_common.constprop.0+0x40/0xe0
[ 2.496475] do_el0_svc+0x1c/0x28
[ 2.496976] el0_svc+0x34/0xb4
[ 2.497475] el0t_64_sync_handler+0x120/0x12c
[ 2.497975] el0t_64_sync+0x190/0x194
[ 2.498466] ---[ end trace 0000000000000000 ]---
[ 2.507347] qcom_scm firmware:scm: qseecom: scm call failed with error -22
[ 2.507813] efivars: get_next_variable: status=8000000000000007
If I understand correctly, it enters an atomic section in
qcom_tzmem_alloc() and then tries to schedule somewhere down the line.
So this shouldn't be qseecom specific.
I should probably also say that I'm currently testing this on a patched
v6.8 kernel, so there's a chance that it's my fault. However, as far as
I understand, it enters an atomic section in qcom_tzmem_alloc() and then
later tries to expand the pool memory with dma_alloc_coherent(). Which
AFAIK is allowed to sleep with GFP_KERNEL (and I guess that that's the
issue here).
I've also tried the shmem allocator option, but that seems to get stuck
quite early at boot, before I even have usb-serial access to get any
logs. If I can find some more time, I'll try to see if I can get some
useful output for that.
Best regards,
Max
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line
From: Alban Browaeys @ 2024-03-28 17:00 UTC (permalink / raw)
To: Conor Dooley, dev
Cc: Vinod Koul, Kishon Vijay Abraham I, Heiko Stuebner, Chris Ruehl,
Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Christopher Obbard, Doug Anderson, Brian Norris, Jensen Huang,
linux-phy, linux-arm-kernel, linux-rockchip, linux-kernel,
devicetree
In-Reply-To: <20240326-tactical-onlooker-3df8d2352dc2@spud>
Le mardi 26 mars 2024 à 19:46 +0000, Conor Dooley a écrit :
> On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4
> Relay wrote:
> > From: Folker Schwesinger <dev@folker-schwesinger.de>
> > - if (of_property_read_bool(dev->of_node, "rockchip,enable-
> > strobe-pulldown"))
> > - rk_phy->enable_strobe_pulldown =
> > PHYCTRL_REN_STRB_ENABLE;
> > + if (of_property_read_bool(dev->of_node, "rockchip,disable-
> > strobe-pulldown"))
> > + rk_phy->enable_strobe_pulldown =
> > PHYCTRL_REN_STRB_DISABLE;
>
> Unfortunately you cannot do this.
> Previously no property at all meant disabled and a property was
> required
> to enable it. With this change the absence of a property means that
> it
> will be enabled.
> An old devicetree is that wanted this to be disabled would have no
> property and will now end up with it enabled. This is an ABI break
> and is
> clearly not backwards compatible, that's a NAK unless it is
> demonstrable
> that noone actually wants to disable it at all.
But the patch that introduced the new default to disable the pulldown
explicitely introduced a regression for at least 4 boards.
It took time to sort out that the default to disable pulldown was the
culprit but still.
Will we carry this new behavor that breaks the default design for
rk3399 because since the regression was introduced new board definition
might have expceted this new behavior.
Could the best option be to revert to énot set a default enable/disable
pulldown" (as before the commit that introduced the regression) and
allow one to force the pulldown via the enable/disable pulldown
property?
I mean the commit that introduced a default value for the pulldown did
not seem to be about fixing anything. But it broke a lot. ANd it was
really really hard to find the description of this commit to understand
that one had to enable pulldown to restore hs400.
In more than 3 years, only one board maintainer noticed that this
property was required to get back HS400 and thanks to a user telling
me that this board was working I found from this board that this
property was "missing" from most board definitions (while it was not
required before).
I am all for not breaking ABI. But what about not reverting a patch
that already broke ABI because this patch introduced a new ABI that we
don't want to break?
I mean shouldn't a new commit with a new ABI that regressed the kernel
be reverted?
Mind fixing the initial regression 8b5c2b45b8f0 "phy: rockchip: set
pulldown for strobe line in dts" does not necessarily mean changing the
default to the opposite value but could also be reverting to not
setting a default.
Though I don't know if there are pros to setting a default.
> If this patch fixes a problem on a board that you have, I would
> suggest
> that you add the property to enable it, as the binding tells you to.
>
> Thanks,
> Conor.
Regards,
Alban
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH REVIEW] hwrng: add exynos Secure World RNG device driver
From: Krzysztof Kozlowski @ 2024-03-28 17:01 UTC (permalink / raw)
To: Alexey Klimov, olivia, herbert, sehi.kim, linux-samsung-soc,
peter.griffin, Łukasz Stelmach
Cc: alim.akhtar, linux-crypto, linux-arm-kernel, linux-kernel,
kernel-team, andre.draszik, willmcvicker, saravanak, elder,
tudor.ambarus, klimov.linux
In-Reply-To: <6b691a48-ca97-4f23-a09f-69b9254f0c11@linaro.org>
On 28/03/2024 14:36, Krzysztof Kozlowski wrote:
>> +
>> +static UNIVERSAL_DEV_PM_OPS(exyswd_rng_pm_ops, exyswd_rng_suspend,
>> + exyswd_rng_resume, NULL);
>> +
>> +static struct platform_driver exyswd_rng_driver = {
>> + .probe = exyswd_rng_probe,
>> + .remove = exyswd_rng_remove,
>> + .driver = {
>> + .name = DRVNAME,
>> + .owner = THIS_MODULE,
>
> So this was fixed ~8-10 years ago. Yet it re-appears. Please do not use
> downstream code as template.
>
> Take upstream driver and either change it or customize it.
Alex Elder pointed out that some of my comments might not be precise or
not helping enough. Let me clarify then:
Please run all standard, open-source tools when submitting new driver,
which is:
1. Coccinelle, which points to this specific line since 2014,
2. smatch,
3. sparse,
4. checkpatch,
5. If changing bindings: dt_binding_check,
6. If changing DTS or bindings: dtbs_check.
I still did not point to specific error I see, because I would like you
to setup the tools and find it. This way you will have toolset ready for
any other submissions. I hope this will be helpful.
Thank you for your contribution.
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH 2/7] security: Remove the now superfluous sentinel element from ctl_table array
From: Joel Granados via B4 Relay @ 2024-03-28 15:57 UTC (permalink / raw)
To: Andrew Morton, Muchun Song, Miaohe Lin, Naoya Horiguchi,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
David Howells, Jarkko Sakkinen, Kees Cook, Herbert Xu,
David S. Miller, Jens Axboe, Pavel Begunkov, Atish Patra,
Anup Patel, Will Deacon, Mark Rutland, Paul Walmsley,
Palmer Dabbelt, Albert Ou
Cc: Luis Chamberlain, linux-mm, linux-kernel, linux-fsdevel, apparmor,
linux-security-module, keyrings, linux-crypto, io-uring,
linux-riscv, linux-arm-kernel, Joel Granados
In-Reply-To: <20240328-jag-sysctl_remset_misc-v1-0-47c1463b3af2@samsung.com>
From: Joel Granados <j.granados@samsung.com>
This commit comes at the tail end of a greater effort to remove the
empty elements at the end of the ctl_table arrays (sentinels) which will
reduce the overall build time size of the kernel and run time memory
bloat by ~64 bytes per sentinel (further information Link :
https://lore.kernel.org/all/ZO5Yx5JFogGi%2FcBo@bombadil.infradead.org/)
Remove the sentinel from all files under security/ that register a
sysctl table.
Signed-off-by: Joel Granados <j.granados@samsung.com>
---
security/apparmor/lsm.c | 1 -
security/keys/sysctl.c | 1 -
security/loadpin/loadpin.c | 1 -
security/yama/yama_lsm.c | 1 -
4 files changed, 4 deletions(-)
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index cef8c466af80..6239777090c4 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -2064,7 +2064,6 @@ static struct ctl_table apparmor_sysctl_table[] = {
.mode = 0600,
.proc_handler = apparmor_dointvec,
},
- { }
};
static int __init apparmor_init_sysctl(void)
diff --git a/security/keys/sysctl.c b/security/keys/sysctl.c
index b348e1679d5d..91f000eef3ad 100644
--- a/security/keys/sysctl.c
+++ b/security/keys/sysctl.c
@@ -66,7 +66,6 @@ static struct ctl_table key_sysctls[] = {
.extra2 = (void *) SYSCTL_INT_MAX,
},
#endif
- { }
};
static int __init init_security_keys_sysctls(void)
diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
index 8e93cda130f1..93fd4d47b334 100644
--- a/security/loadpin/loadpin.c
+++ b/security/loadpin/loadpin.c
@@ -63,7 +63,6 @@ static struct ctl_table loadpin_sysctl_table[] = {
.extra1 = SYSCTL_ONE,
.extra2 = SYSCTL_ONE,
},
- { }
};
static void set_sysctl(bool is_writable)
diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
index 49dc52b454ef..b6684a074a59 100644
--- a/security/yama/yama_lsm.c
+++ b/security/yama/yama_lsm.c
@@ -463,7 +463,6 @@ static struct ctl_table yama_sysctl_table[] = {
.extra1 = SYSCTL_ZERO,
.extra2 = &max_scope,
},
- { }
};
static void __init yama_init_sysctl(void)
{
--
2.43.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 7/7] drivers: perf: Remove the now superfluous sentinel elements from ctl_table array
From: Joel Granados via B4 Relay @ 2024-03-28 15:57 UTC (permalink / raw)
To: Andrew Morton, Muchun Song, Miaohe Lin, Naoya Horiguchi,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
David Howells, Jarkko Sakkinen, Kees Cook, Herbert Xu,
David S. Miller, Jens Axboe, Pavel Begunkov, Atish Patra,
Anup Patel, Will Deacon, Mark Rutland, Paul Walmsley,
Palmer Dabbelt, Albert Ou
Cc: Luis Chamberlain, linux-mm, linux-kernel, linux-fsdevel, apparmor,
linux-security-module, keyrings, linux-crypto, io-uring,
linux-riscv, linux-arm-kernel, Joel Granados
In-Reply-To: <20240328-jag-sysctl_remset_misc-v1-0-47c1463b3af2@samsung.com>
From: Joel Granados <j.granados@samsung.com>
This commit comes at the tail end of a greater effort to remove the
empty elements at the end of the ctl_table arrays (sentinels) which will
reduce the overall build time size of the kernel and run time memory
bloat by ~64 bytes per sentinel (further information Link :
https://lore.kernel.org/all/ZO5Yx5JFogGi%2FcBo@bombadil.infradead.org/)
Remove sentinel from sbi_pmu_sysctl_table
Signed-off-by: Joel Granados <j.granados@samsung.com>
---
drivers/perf/riscv_pmu_sbi.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
index 8cbe6e5f9c39..5aef5a8737b2 100644
--- a/drivers/perf/riscv_pmu_sbi.c
+++ b/drivers/perf/riscv_pmu_sbi.c
@@ -1043,7 +1043,6 @@ static struct ctl_table sbi_pmu_sysctl_table[] = {
.extra1 = SYSCTL_ZERO,
.extra2 = SYSCTL_TWO,
},
- { }
};
static int pmu_sbi_device_probe(struct platform_device *pdev)
--
2.43.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 6/7] io_uring: Remove the now superfluous sentinel elements from ctl_table array
From: Joel Granados via B4 Relay @ 2024-03-28 15:57 UTC (permalink / raw)
To: Andrew Morton, Muchun Song, Miaohe Lin, Naoya Horiguchi,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
David Howells, Jarkko Sakkinen, Kees Cook, Herbert Xu,
David S. Miller, Jens Axboe, Pavel Begunkov, Atish Patra,
Anup Patel, Will Deacon, Mark Rutland, Paul Walmsley,
Palmer Dabbelt, Albert Ou
Cc: Luis Chamberlain, linux-mm, linux-kernel, linux-fsdevel, apparmor,
linux-security-module, keyrings, linux-crypto, io-uring,
linux-riscv, linux-arm-kernel, Joel Granados
In-Reply-To: <20240328-jag-sysctl_remset_misc-v1-0-47c1463b3af2@samsung.com>
From: Joel Granados <j.granados@samsung.com>
This commit comes at the tail end of a greater effort to remove the
empty elements at the end of the ctl_table arrays (sentinels) which will
reduce the overall build time size of the kernel and run time memory
bloat by ~64 bytes per sentinel (further information Link :
https://lore.kernel.org/all/ZO5Yx5JFogGi%2FcBo@bombadil.infradead.org/)
Remove sentinel element from kernel_io_uring_disabled_table
Signed-off-by: Joel Granados <j.granados@samsung.com>
---
io_uring/io_uring.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 5d4b448fdc50..fe3c93e21e2c 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -169,7 +169,6 @@ static struct ctl_table kernel_io_uring_disabled_table[] = {
.mode = 0644,
.proc_handler = proc_dointvec,
},
- {},
};
#endif
--
2.43.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH REVIEW] hwrng: add exynos Secure World RNG device driver
From: Krzysztof Kozlowski @ 2024-03-28 17:05 UTC (permalink / raw)
To: Alexey Klimov, olivia, herbert, sehi.kim, linux-samsung-soc,
peter.griffin, Łukasz Stelmach
Cc: alim.akhtar, linux-crypto, linux-arm-kernel, linux-kernel,
kernel-team, andre.draszik, willmcvicker, saravanak, elder,
tudor.ambarus, klimov.linux
In-Reply-To: <83607b46-56e4-45eb-ac69-9bc5be5bdee4@linaro.org>
On 28/03/2024 18:01, Krzysztof Kozlowski wrote:
> On 28/03/2024 14:36, Krzysztof Kozlowski wrote:
>>> +
>>> +static UNIVERSAL_DEV_PM_OPS(exyswd_rng_pm_ops, exyswd_rng_suspend,
>>> + exyswd_rng_resume, NULL);
>>> +
>>> +static struct platform_driver exyswd_rng_driver = {
>>> + .probe = exyswd_rng_probe,
>>> + .remove = exyswd_rng_remove,
>>> + .driver = {
>>> + .name = DRVNAME,
>>> + .owner = THIS_MODULE,
>>
>> So this was fixed ~8-10 years ago. Yet it re-appears. Please do not use
>> downstream code as template.
>>
>> Take upstream driver and either change it or customize it.
>
> Alex Elder pointed out that some of my comments might not be precise or
> not helping enough. Let me clarify then:
>
> Please run all standard, open-source tools when submitting new driver,
> which is:
> 1. Coccinelle, which points to this specific line since 2014,
> 2. smatch,
> 3. sparse,
> 4. checkpatch,
> 5. If changing bindings: dt_binding_check,
> 6. If changing DTS or bindings: dtbs_check.
And I forgot:
7. make W=1
Many of these, including W=1 above, can be with target, e.g.
`make W=1 drivers/char/hw_random/`
to reduce the scope of tests/warnings etc.
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 18/23] dt-bindings: media: imx258: Add alternate compatible strings
From: Krzysztof Kozlowski @ 2024-03-28 17:06 UTC (permalink / raw)
To: Luigi311, git, linux-media
Cc: dave.stevenson, jacopo.mondi, mchehab, robh,
krzysztof.kozlowski+dt, conor+dt, shawnguo, s.hauer, kernel,
festevam, sakari.ailus, devicetree, imx, linux-arm-kernel,
linux-kernel
In-Reply-To: <30d886be-cac8-400a-9b2f-dd2ce64b34d8@luigi311.com>
On 28/03/2024 18:05, Luigi311 wrote:
>
> Looks like it no longer complains when i run
> make dt_binding_check DT_SCHEMA_FILES=media/i2c/sony,imx258
>
> with the following
>
> properties:
> compatible:
> enum:
> - sony,imx258
> - sony,imx258-pdaf
>
> If that looks good I can go ahead and include that in the v3
>
Looks good, thanks.
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 18/23] dt-bindings: media: imx258: Add alternate compatible strings
From: Luigi311 @ 2024-03-28 17:05 UTC (permalink / raw)
To: Krzysztof Kozlowski, git, linux-media
Cc: dave.stevenson, jacopo.mondi, mchehab, robh,
krzysztof.kozlowski+dt, conor+dt, shawnguo, s.hauer, kernel,
festevam, sakari.ailus, devicetree, imx, linux-arm-kernel,
linux-kernel
In-Reply-To: <586bdcc9-793d-4cbe-9544-9012a665288e@linaro.org>
On 3/28/24 01:47, Krzysztof Kozlowski wrote:
> On 28/03/2024 00:17, git@luigi311.com wrote:
>> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>
>> There are a number of variants of the imx258 modules that can not
>> be differentiated at runtime, so add compatible strings for them.
>>
>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>> Signed-off-by: Luigi311 <git@luigi311.com>
>> ---
>> .../devicetree/bindings/media/i2c/sony,imx258.yaml | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml
>> index bee61a443b23..c7856de15ba3 100644
>> --- a/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml
>> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml
>> @@ -14,10 +14,14 @@ description: |-
>> type stacked image sensor with a square pixel array of size 4208 x 3120. It
>> is programmable through I2C interface. Image data is sent through MIPI
>> CSI-2.
>> + There are a number of variants of the sensor which cannot be detected at
>> + runtime, so multiple compatible strings are required to differentiate these.
>>
>> properties:
>> compatible:
>> - const: sony,imx258
>> + - enum:
>> + - sony,imx258
>
> Two people working on patch but no one tested it before sending. Do not
> send untested code.
>
> It does not look like you tested the bindings, at least after quick
> look. Please run `make dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> Maybe you need to update your dtschema and yamllint.
Hello, looks like I messed this up during my v2 (sorry missed the v in
my format patch) when I took this off Dave's hands. This is all new to
me so thank you for the command used to check, I was only compiling
the kernel and testing that so I didn't realize this needed separate
testing.
Looks like it no longer complains when i run
make dt_binding_check DT_SCHEMA_FILES=media/i2c/sony,imx258
with the following
properties:
compatible:
enum:
- sony,imx258
- sony,imx258-pdaf
If that looks good I can go ahead and include that in the v3
>
> Best regards,
> Krzysztof
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH RFC 0/3] mm/gup: consistently call it GUP-fast
From: Vineet Gupta @ 2024-03-28 17:46 UTC (permalink / raw)
To: Mike Rapoport, Arnd Bergmann
Cc: Vineet Gupta, David Hildenbrand, peterx, linux-kernel,
Andrew Morton, Jason Gunthorpe, John Hubbard, linux-arm-kernel,
loongarch, linux-mips, linuxppc-dev, linux-s390, linux-sh,
linux-mm, linux-perf-users, linux-fsdevel, x86, Ryan Roberts,
Alexander Viro, Matt Turner, Alexey Brodkin
In-Reply-To: <ZgUZCBNloC-grPWJ@kernel.org>
On 3/28/24 00:15, Mike Rapoport wrote:
> On Thu, Mar 28, 2024 at 07:09:13AM +0100, Arnd Bergmann wrote:
>> On Thu, Mar 28, 2024, at 06:51, Vineet Gupta wrote:
>>> On 3/27/24 09:22, Arnd Bergmann wrote:
>>>> On Wed, Mar 27, 2024, at 16:39, David Hildenbrand wrote:
>>>>> On 27.03.24 16:21, Peter Xu wrote:
>>>>>> On Wed, Mar 27, 2024 at 02:05:35PM +0100, David Hildenbrand wrote:
>>>>>>
>>>>>> I'm not sure what config you tried there; as I am doing some build tests
>>>>>> recently, I found turning off CONFIG_SAMPLES + CONFIG_GCC_PLUGINS could
>>>>>> avoid a lot of issues, I think it's due to libc missing. But maybe not the
>>>>>> case there.
>>>>> CCin Arnd; I use some of his compiler chains, others from Fedora directly. For
>>>>> example for alpha and arc, the Fedora gcc is "13.2.1".
>>>>> But there is other stuff like (arc):
>>>>>
>>>>> ./arch/arc/include/asm/mmu-arcv2.h: In function 'mmu_setup_asid':
>>>>> ./arch/arc/include/asm/mmu-arcv2.h:82:9: error: implicit declaration of
>>>>> function 'write_aux_reg' [-Werro
>>>>> r=implicit-function-declaration]
>>>>> 82 | write_aux_reg(ARC_REG_PID, asid | MMU_ENABLE);
>>>>> | ^~~~~~~~~~~~~
>>>> Seems to be missing an #include of soc/arc/aux.h, but I can't
>>>> tell when this first broke without bisecting.
>>> Weird I don't see this one but I only have gcc 12 handy ATM.
>>>
>>> gcc version 12.2.1 20230306 (ARC HS GNU/Linux glibc toolchain -
>>> build 1360)
>>>
>>> I even tried W=1 (which according to scripts/Makefile.extrawarn) should
>>> include -Werror=implicit-function-declaration but don't see this still.
>>>
>>> Tomorrow I'll try building a gcc 13.2.1 for ARC.
>> David reported them with the toolchains I built at
>> https://mirrors.edge.kernel.org/pub/tools/crosstool/
>> I'm fairly sure the problem is specific to the .config
>> and tree, not the toolchain though.
> This happens with defconfig and both gcc 12.2.0 and gcc 13.2.0 from your
> crosstools. I also see these on the current Linus' tree:
>
> arc/kernel/ptrace.c:342:16: warning: no previous prototype for 'syscall_trace_enter' [-Wmissing-prototypes]
> arch/arc/kernel/kprobes.c:193:15: warning: no previous prototype for 'arc_kprobe_handler' [-Wmissing-prototypes]
Yep these two I could trigger and fix posted [1]
> This fixed the warning about write_aux_reg for me, probably Vineet would
> want this include somewhere else...
>
> diff --git a/arch/arc/include/asm/mmu-arcv2.h b/arch/arc/include/asm/mmu-arcv2.h
> index ed9036d4ede3..0fca342d7b79 100644
> --- a/arch/arc/include/asm/mmu-arcv2.h
> +++ b/arch/arc/include/asm/mmu-arcv2.h
> @@ -69,6 +69,8 @@
>
> #ifndef __ASSEMBLY__
>
> +#include <asm/arcregs.h>
> +
> struct mm_struct;
> extern int pae40_exist_but_not_enab(void);
Thx Mike. Indeed the fix is trivial but on tip of tree I still can't
trigger the warning to even test anything. I'm at following with my
other fixes.
2024-03-27 962490525cff Merge tag 'probes-fixes-v6.9-rc1' of
git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace
I tried defconfig build as well as the exact config from Linaro report
[2], and/or various toolchains: from snps github, Arnd's crosstool
toolchain.
Granted all of these are linux toolchains - I vaguely remember at some
time, baremetal elf32 toolchain behaved differently due to different
defaults etc.
I have a feeling this was something transient which got fixed up due to
order of header includes etc.
Anyone in the followup email David only reported 2 warnings which have
been tended to as mentioned above - will be sent to Linus soon.
[1]
http://lists.infradead.org/pipermail/linux-snps-arc/2024-March/007916.html
[2]
https://storage.tuxsuite.com/public/linaro/lkft/builds/2eA2VSZdDsL0DMBBhjoauN9IVoK/
Thx,
-Vineet
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] Input: stmpe - drop driver owner assignment
From: Dmitry Torokhov @ 2024-03-28 17:54 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Maxime Coquelin, Alexandre Torgue, linux-input, linux-stm32,
linux-arm-kernel, linux-kernel
In-Reply-To: <20240327174655.519503-1-krzysztof.kozlowski@linaro.org>
On Wed, Mar 27, 2024 at 06:46:55PM +0100, Krzysztof Kozlowski wrote:
> Core in platform_driver_register() already sets the .owner, so driver
> does not need to.
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Applied, thank you.
--
Dmitry
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] spi: s3c64xx: Use DMA mode from fifo size
From: Sam Protsenko @ 2024-03-28 17:58 UTC (permalink / raw)
To: Jaewon Kim
Cc: Andi Shyti, Mark Brown, Krzysztof Kozlowski, Alim Akhtar,
linux-spi, linux-samsung-soc, linux-arm-kernel, linux-kernel
In-Reply-To: <20240327033041.83625-1-jaewon02.kim@samsung.com>
On Tue, Mar 26, 2024 at 10:35 PM Jaewon Kim <jaewon02.kim@samsung.com> wrote:
>
> The SPI data size is smaller than FIFO, it operates in PIO mode,
Spelling: "The" -> "If the"
> and if it is larger than FIFO mode, DMA mode is selected.
>
> If the data size is the same as the FIFO size, it operates in PIO mode
> and data is separated into two transfer. In order to prevent,
Nit: "transfer" -> "transfers", "prevent" -> "prevent it"
> DMA mode must be used from the case of FIFO and data size.
>
You probably mean this code (it occurs two times in the driver):
xfer->len = fifo_len - 1;
Can you please elaborate on why it's done this way? Why can't we just
do "xfer->len = fifo_len" and use the whole FIFO for the transfer
instead? I don't understand the necessity to split the transfer into
two chunks if its size is of FIFO length -- wouldn't it fit into FIFO
in that case? (I'm pretty sure this change is correct, just want to
understand how exactly it works).
> Fixes: 1ee806718d5e ("spi: s3c64xx: support interrupt based pio mode")
Just wonder if that fixes some throughput regression, or something
worse (like failed transfers when the transfer size is the same as
FIFO size)?
> Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com>
> ---
> drivers/spi/spi-s3c64xx.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 9fcbe040cb2f..81ed5fddf83e 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -430,7 +430,7 @@ static bool s3c64xx_spi_can_dma(struct spi_controller *host,
> struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host);
>
> if (sdd->rx_dma.ch && sdd->tx_dma.ch)
> - return xfer->len > sdd->fifo_depth;
> + return xfer->len >= sdd->fifo_depth;
>
> return false;
> }
> @@ -826,11 +826,11 @@ static int s3c64xx_spi_transfer_one(struct spi_controller *host,
> return status;
> }
>
> - if (!is_polling(sdd) && (xfer->len > fifo_len) &&
> + if (!is_polling(sdd) && xfer->len >= fifo_len &&
> sdd->rx_dma.ch && sdd->tx_dma.ch) {
> use_dma = 1;
>
Would be nice to remove this empty line, while at it.
> - } else if (xfer->len >= fifo_len) {
> + } else if (xfer->len > fifo_len) {
Below in the same function I can see similar code:
if (target_len >= fifo_len)
xfer->len = fifo_len - 1;
Shouldn't that 'if' condition be fixed too? Or it's ok as it is? (Just
noticed it by searching, not sure myself, hence asking).
> tx_buf = xfer->tx_buf;
> rx_buf = xfer->rx_buf;
> origin_len = xfer->len;
> --
> 2.43.2
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] clk: samsung: exynosautov9: fix wrong pll clock id value
From: Sam Protsenko @ 2024-03-28 18:00 UTC (permalink / raw)
To: Jaewon Kim
Cc: Krzysztof Kozlowski, Sylwester Nawrocki, Chanwoo Choi,
Alim Akhtar, Michael Turquette, Stephen Boyd, linux-samsung-soc,
linux-clk, linux-arm-kernel, linux-kernel
In-Reply-To: <20240328091000.17660-1-jaewon02.kim@samsung.com>
On Thu, Mar 28, 2024 at 4:14 AM Jaewon Kim <jaewon02.kim@samsung.com> wrote:
>
> All PLL id values of CMU_TOP were incorrectly set to FOUT_SHARED0_PLL.
> It modified to the correct PLL clock id value.
>
> Fixes: 6587c62f69dc ("clk: samsung: add top clock support for Exynos Auto v9 SoC")
> Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com>
> ---
Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
> drivers/clk/samsung/clk-exynosautov9.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clk/samsung/clk-exynosautov9.c b/drivers/clk/samsung/clk-exynosautov9.c
> index e9c06eb93e66..f04bacacab2c 100644
> --- a/drivers/clk/samsung/clk-exynosautov9.c
> +++ b/drivers/clk/samsung/clk-exynosautov9.c
> @@ -352,13 +352,13 @@ static const struct samsung_pll_clock top_pll_clks[] __initconst = {
> /* CMU_TOP_PURECLKCOMP */
> PLL(pll_0822x, FOUT_SHARED0_PLL, "fout_shared0_pll", "oscclk",
> PLL_LOCKTIME_PLL_SHARED0, PLL_CON3_PLL_SHARED0, NULL),
> - PLL(pll_0822x, FOUT_SHARED0_PLL, "fout_shared1_pll", "oscclk",
> + PLL(pll_0822x, FOUT_SHARED1_PLL, "fout_shared1_pll", "oscclk",
> PLL_LOCKTIME_PLL_SHARED1, PLL_CON3_PLL_SHARED1, NULL),
> - PLL(pll_0822x, FOUT_SHARED0_PLL, "fout_shared2_pll", "oscclk",
> + PLL(pll_0822x, FOUT_SHARED2_PLL, "fout_shared2_pll", "oscclk",
> PLL_LOCKTIME_PLL_SHARED2, PLL_CON3_PLL_SHARED2, NULL),
> - PLL(pll_0822x, FOUT_SHARED0_PLL, "fout_shared3_pll", "oscclk",
> + PLL(pll_0822x, FOUT_SHARED3_PLL, "fout_shared3_pll", "oscclk",
> PLL_LOCKTIME_PLL_SHARED3, PLL_CON3_PLL_SHARED3, NULL),
> - PLL(pll_0822x, FOUT_SHARED0_PLL, "fout_shared4_pll", "oscclk",
> + PLL(pll_0822x, FOUT_SHARED4_PLL, "fout_shared4_pll", "oscclk",
> PLL_LOCKTIME_PLL_SHARED4, PLL_CON3_PLL_SHARED4, NULL),
> };
>
> --
> 2.43.2
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 08/23] media: i2c: imx258: Add support for 24MHz clock
From: Luigi311 @ 2024-03-28 17:55 UTC (permalink / raw)
To: Sakari Ailus, git
Cc: linux-media, dave.stevenson, jacopo.mondi, mchehab, robh,
krzysztof.kozlowski+dt, conor+dt, shawnguo, s.hauer, kernel,
festevam, devicetree, imx, linux-arm-kernel, linux-kernel
In-Reply-To: <ZgUlzCfUN-PnF8Yy@kekkonen.localdomain>
On 3/28/24 02:09, Sakari Ailus wrote:
> Hi Luigi311,
>
> Thank you for the patchset.
>
> On Wed, Mar 27, 2024 at 05:16:54PM -0600, git@luigi311.com wrote:
>> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>
>> There's no reason why only a clock of 19.2MHz is supported.
>> Indeed this isn't even a frequency listed in the datasheet.
>>
>> Add support for 24MHz as well.
>> The PLL settings result in slightly different link frequencies,
>> so parameterise those.
>>
>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>> Signed-off-by: Luigi311 <git@luigi311.com>
>
> Is Luigi311 your real name? As per
> Documentation/process/submitting-patches.rst, anonymous (or pseudonym I'd
> say as well) contributions are not an option.
Luigi311 is not my real name but it would be a lot easier to find me if
it was. My real name is Luis Garcia which is a super common name so its
actually way easier to find me and all my work using my online name of
Luigi311. I can go ahead and swap over to Luis Garcia if required but a
name like that would provide no value in contacting/finding me since I'm
not famous like all the other Luis Garcia's that appear on google.
>
>> ---
>> drivers/media/i2c/imx258.c | 133 +++++++++++++++++++++++++++++--------
>> 1 file changed, 107 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
>> index 351add1bc5d5..6ee7de079454 100644
>> --- a/drivers/media/i2c/imx258.c
>> +++ b/drivers/media/i2c/imx258.c
>> @@ -76,9 +76,6 @@
>> #define REG_CONFIG_MIRROR_FLIP 0x03
>> #define REG_CONFIG_FLIP_TEST_PATTERN 0x02
>>
>> -/* Input clock frequency in Hz */
>> -#define IMX258_INPUT_CLOCK_FREQ 19200000
>> -
>> struct imx258_reg {
>> u16 address;
>> u8 val;
>> @@ -115,7 +112,9 @@ struct imx258_mode {
>> };
>>
>> /* 4208x3120 needs 1267Mbps/lane, 4 lanes */
>> -static const struct imx258_reg mipi_data_rate_1267mbps[] = {
>> +static const struct imx258_reg mipi_1267mbps_19_2mhz[] = {
>> + { 0x0136, 0x13 },
>> + { 0x0137, 0x33 },
>> { 0x0301, 0x05 },
>> { 0x0303, 0x02 },
>> { 0x0305, 0x03 },
>> @@ -133,7 +132,29 @@ static const struct imx258_reg mipi_data_rate_1267mbps[] = {
>> { 0x0823, 0xCC },
>> };
>>
>> -static const struct imx258_reg mipi_data_rate_640mbps[] = {
>> +static const struct imx258_reg mipi_1272mbps_24mhz[] = {
>> + { 0x0136, 0x18 },
>> + { 0x0137, 0x00 },
>> + { 0x0301, 0x05 },
>> + { 0x0303, 0x02 },
>> + { 0x0305, 0x04 },
>> + { 0x0306, 0x00 },
>> + { 0x0307, 0xD4 },
>> + { 0x0309, 0x0A },
>> + { 0x030B, 0x01 },
>> + { 0x030D, 0x02 },
>> + { 0x030E, 0x00 },
>> + { 0x030F, 0xD8 },
>> + { 0x0310, 0x00 },
>> + { 0x0820, 0x13 },
>> + { 0x0821, 0x4C },
>> + { 0x0822, 0xCC },
>> + { 0x0823, 0xCC },
>> +};
>> +
>> +static const struct imx258_reg mipi_640mbps_19_2mhz[] = {
>> + { 0x0136, 0x13 },
>> + { 0x0137, 0x33 },
>> { 0x0301, 0x05 },
>> { 0x0303, 0x02 },
>> { 0x0305, 0x03 },
>> @@ -151,9 +172,27 @@ static const struct imx258_reg mipi_data_rate_640mbps[] = {
>> { 0x0823, 0x00 },
>> };
>>
>> +static const struct imx258_reg mipi_642mbps_24mhz[] = {
>> + { 0x0136, 0x18 },
>> + { 0x0137, 0x00 },
>> + { 0x0301, 0x05 },
>> + { 0x0303, 0x02 },
>> + { 0x0305, 0x04 },
>> + { 0x0306, 0x00 },
>> + { 0x0307, 0x6B },
>> + { 0x0309, 0x0A },
>> + { 0x030B, 0x01 },
>> + { 0x030D, 0x02 },
>> + { 0x030E, 0x00 },
>> + { 0x030F, 0xD8 },
>> + { 0x0310, 0x00 },
>> + { 0x0820, 0x0A },
>> + { 0x0821, 0x00 },
>> + { 0x0822, 0x00 },
>> + { 0x0823, 0x00 },
>> +};
>> +
>> static const struct imx258_reg mode_common_regs[] = {
>> - { 0x0136, 0x13 },
>> - { 0x0137, 0x33 },
>> { 0x3051, 0x00 },
>> { 0x3052, 0x00 },
>> { 0x4E21, 0x14 },
>> @@ -313,10 +352,6 @@ static const char * const imx258_supply_name[] = {
>>
>> #define IMX258_NUM_SUPPLIES ARRAY_SIZE(imx258_supply_name)
>>
>> -/* Configurations for supported link frequencies */
>> -#define IMX258_LINK_FREQ_634MHZ 633600000ULL
>> -#define IMX258_LINK_FREQ_320MHZ 320000000ULL
>> -
>> enum {
>> IMX258_LINK_FREQ_1267MBPS,
>> IMX258_LINK_FREQ_640MBPS,
>> @@ -335,25 +370,55 @@ static u64 link_freq_to_pixel_rate(u64 f)
>> }
>>
>> /* Menu items for LINK_FREQ V4L2 control */
>> -static const s64 link_freq_menu_items[] = {
>> +/* Configurations for supported link frequencies */
>> +#define IMX258_LINK_FREQ_634MHZ 633600000ULL
>> +#define IMX258_LINK_FREQ_320MHZ 320000000ULL
>> +
>> +static const s64 link_freq_menu_items_19_2[] = {
>> IMX258_LINK_FREQ_634MHZ,
>> IMX258_LINK_FREQ_320MHZ,
>> };
>>
>> +/* Configurations for supported link frequencies */
>> +#define IMX258_LINK_FREQ_636MHZ 636000000ULL
>> +#define IMX258_LINK_FREQ_321MHZ 321000000ULL
>
> These values aren't used outside the array below and the macro names are
> imprecise anyway. Could you put the numerical values to the array instead?
Ok I've removed the defines and just threw the values into the array instead.
>
>> +
>> +static const s64 link_freq_menu_items_24[] = {
>> + IMX258_LINK_FREQ_636MHZ,
>> + IMX258_LINK_FREQ_321MHZ,
>> +};
>> +
>> /* Link frequency configs */
>> -static const struct imx258_link_freq_config link_freq_configs[] = {
>> +static const struct imx258_link_freq_config link_freq_configs_19_2[] = {
>> [IMX258_LINK_FREQ_1267MBPS] = {
>> .pixels_per_line = IMX258_PPL_DEFAULT,
>> .reg_list = {
>> - .num_of_regs = ARRAY_SIZE(mipi_data_rate_1267mbps),
>> - .regs = mipi_data_rate_1267mbps,
>> + .num_of_regs = ARRAY_SIZE(mipi_1267mbps_19_2mhz),
>> + .regs = mipi_1267mbps_19_2mhz,
>> }
>> },
>> [IMX258_LINK_FREQ_640MBPS] = {
>> .pixels_per_line = IMX258_PPL_DEFAULT,
>> .reg_list = {
>> - .num_of_regs = ARRAY_SIZE(mipi_data_rate_640mbps),
>> - .regs = mipi_data_rate_640mbps,
>> + .num_of_regs = ARRAY_SIZE(mipi_640mbps_19_2mhz),
>> + .regs = mipi_640mbps_19_2mhz,
>> + }
>> + },
>> +};
>> +
>> +static const struct imx258_link_freq_config link_freq_configs_24[] = {
>> + [IMX258_LINK_FREQ_1267MBPS] = {
>> + .pixels_per_line = IMX258_PPL_DEFAULT,
>> + .reg_list = {
>> + .num_of_regs = ARRAY_SIZE(mipi_1272mbps_24mhz),
>> + .regs = mipi_1272mbps_24mhz,
>> + }
>> + },
>> + [IMX258_LINK_FREQ_640MBPS] = {
>> + .pixels_per_line = IMX258_PPL_DEFAULT,
>> + .reg_list = {
>> + .num_of_regs = ARRAY_SIZE(mipi_642mbps_24mhz),
>> + .regs = mipi_642mbps_24mhz,
>> }
>> },
>> };
>> @@ -410,6 +475,9 @@ struct imx258 {
>> /* Current mode */
>> const struct imx258_mode *cur_mode;
>>
>> + const struct imx258_link_freq_config *link_freq_configs;
>> + const s64 *link_freq_menu_items;
>> +
>> /*
>> * Mutex for serialized access:
>> * Protect sensor module set pad format and start/stop streaming safely.
>> @@ -713,7 +781,7 @@ static int imx258_set_pad_format(struct v4l2_subdev *sd,
>> imx258->cur_mode = mode;
>> __v4l2_ctrl_s_ctrl(imx258->link_freq, mode->link_freq_index);
>>
>> - link_freq = link_freq_menu_items[mode->link_freq_index];
>> + link_freq = imx258->link_freq_menu_items[mode->link_freq_index];
>> pixel_rate = link_freq_to_pixel_rate(link_freq);
>> __v4l2_ctrl_s_ctrl_int64(imx258->pixel_rate, pixel_rate);
>> /* Update limits and set FPS to default */
>> @@ -727,7 +795,7 @@ static int imx258_set_pad_format(struct v4l2_subdev *sd,
>> vblank_def);
>> __v4l2_ctrl_s_ctrl(imx258->vblank, vblank_def);
>> h_blank =
>> - link_freq_configs[mode->link_freq_index].pixels_per_line
>> + imx258->link_freq_configs[mode->link_freq_index].pixels_per_line
>> - imx258->cur_mode->width;
>> __v4l2_ctrl_modify_range(imx258->hblank, h_blank,
>> h_blank, 1, h_blank);
>> @@ -747,7 +815,7 @@ static int imx258_start_streaming(struct imx258 *imx258)
>>
>> /* Setup PLL */
>> link_freq_index = imx258->cur_mode->link_freq_index;
>> - reg_list = &link_freq_configs[link_freq_index].reg_list;
>> + reg_list = &imx258->link_freq_configs[link_freq_index].reg_list;
>> ret = imx258_write_regs(imx258, reg_list->regs, reg_list->num_of_regs);
>> if (ret) {
>> dev_err(&client->dev, "%s failed to set plls\n", __func__);
>> @@ -946,9 +1014,9 @@ static int imx258_init_controls(struct imx258 *imx258)
>> imx258->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
>> &imx258_ctrl_ops,
>> V4L2_CID_LINK_FREQ,
>> - ARRAY_SIZE(link_freq_menu_items) - 1,
>> + ARRAY_SIZE(link_freq_menu_items_19_2) - 1,
>> 0,
>> - link_freq_menu_items);
>> + imx258->link_freq_menu_items);
>>
>> if (imx258->link_freq)
>> imx258->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>> @@ -964,8 +1032,10 @@ static int imx258_init_controls(struct imx258 *imx258)
>> if (vflip)
>> vflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>>
>> - pixel_rate_max = link_freq_to_pixel_rate(link_freq_menu_items[0]);
>> - pixel_rate_min = link_freq_to_pixel_rate(link_freq_menu_items[1]);
>> + pixel_rate_max =
>> + link_freq_to_pixel_rate(imx258->link_freq_menu_items[0]);
>> + pixel_rate_min =
>> + link_freq_to_pixel_rate(imx258->link_freq_menu_items[1]);
>
> The arrays currently have two entries so this works but it'd nice to have a
> bit more robust way to handle differences between the two arrays. Could you
> maintain e.g. the number of entries in the array in a struct field perhaps?
Would it make more sense to do something like default to index 0 and then use
ARRAY_SIZE to iterate through the array and do a comparison to get the min and
max size so it would always choose the correct value no matter how many entries
there are?
>
>> /* By default, PIXEL_RATE is read only */
>> imx258->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx258_ctrl_ops,
>> V4L2_CID_PIXEL_RATE,
>> @@ -1086,8 +1156,19 @@ static int imx258_probe(struct i2c_client *client)
>> } else {
>> val = clk_get_rate(imx258->clk);
>> }
>> - if (val != IMX258_INPUT_CLOCK_FREQ) {
>> - dev_err(&client->dev, "input clock frequency not supported\n");
>> +
>> + switch (val) {
>> + case 19200000:
>> + imx258->link_freq_configs = link_freq_configs_19_2;
>> + imx258->link_freq_menu_items = link_freq_menu_items_19_2;
>> + break;
>> + case 24000000:
>> + imx258->link_freq_configs = link_freq_configs_24;
>> + imx258->link_freq_menu_items = link_freq_menu_items_24;
>> + break;
>> + default:
>> + dev_err(&client->dev, "input clock frequency of %u not supported\n",
>> + val);
>> return -EINVAL;
>> }
>>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line
From: Conor Dooley @ 2024-03-28 18:01 UTC (permalink / raw)
To: Alban Browaeys
Cc: dev, Vinod Koul, Kishon Vijay Abraham I, Heiko Stuebner,
Chris Ruehl, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Christopher Obbard, Doug Anderson, Brian Norris, Jensen Huang,
linux-phy, linux-arm-kernel, linux-rockchip, linux-kernel,
devicetree
In-Reply-To: <871f0b24a38208d9c5d6abc87d83b067162c103e.camel@gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 3829 bytes --]
On Thu, Mar 28, 2024 at 06:00:03PM +0100, Alban Browaeys wrote:
> Le mardi 26 mars 2024 à 19:46 +0000, Conor Dooley a écrit :
> > On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4
> > Relay wrote:
> > > From: Folker Schwesinger <dev@folker-schwesinger.de>
> > > - if (of_property_read_bool(dev->of_node, "rockchip,enable-
> > > strobe-pulldown"))
> > > - rk_phy->enable_strobe_pulldown =
> > > PHYCTRL_REN_STRB_ENABLE;
> > > + if (of_property_read_bool(dev->of_node, "rockchip,disable-
> > > strobe-pulldown"))
> > > + rk_phy->enable_strobe_pulldown =
> > > PHYCTRL_REN_STRB_DISABLE;
> >
> > Unfortunately you cannot do this.
> > Previously no property at all meant disabled and a property was
> > required
> > to enable it. With this change the absence of a property means that
> > it
> > will be enabled.
> > An old devicetree is that wanted this to be disabled would have no
> > property and will now end up with it enabled. This is an ABI break
> > and is
> > clearly not backwards compatible, that's a NAK unless it is
> > demonstrable
> > that noone actually wants to disable it at all.
>
>
> But the patch that introduced the new default to disable the pulldown
> explicitely introduced a regression for at least 4 boards.
> It took time to sort out that the default to disable pulldown was the
> culprit but still.
> Will we carry this new behavor that breaks the default design for
> rk3399 because since the regression was introduced new board definition
> might have expceted this new behavior.
>
> Could the best option be to revert to énot set a default enable/disable
> pulldown" (as before the commit that introduced the regression) and
> allow one to force the pulldown via the enable/disable pulldown
> property?
> I mean the commit that introduced a default value for the pulldown did
> not seem to be about fixing anything. But it broke a lot. ANd it was
> really really hard to find the description of this commit to understand
> that one had to enable pulldown to restore hs400.
>
> In more than 3 years, only one board maintainer noticed that this
> property was required to get back HS400 and thanks to a user telling
> me that this board was working I found from this board that this
> property was "missing" from most board definitions (while it was not
> required before).
>
>
> I am all for not breaking ABI. But what about not reverting a patch
> that already broke ABI because this patch introduced a new ABI that we
> don't want to break?
> I mean shouldn't a new commit with a new ABI that regressed the kernel
> be reverted?
I think I said it after OP replied to me yesterday, but this is a pretty
shitty situation in that the original default picked for the property
was actually incorrect. Given it's been like this for four years before
anyone noticed, and others probably depend on the current behaviour,
that's hard to justify.
> Mind fixing the initial regression 8b5c2b45b8f0 "phy: rockchip: set
> pulldown for strobe line in dts" does not necessarily mean changing the
> default to the opposite value but could also be reverting to not
> setting a default.
That's also problematic, as the only way to do this is make setting
one of the enabled or disabled properties required, which is also an ABI
break, since you'd then be rejecting probe if one is not present.
> Though I don't know if there are pros to setting a default.
What you probably have to weigh up is the cons of each side. If what you
lose is HS400 mode with what's in the kernel right now but switching to
what's been proposed would entirely break some boards, I know which
I think the lesser of two evils is.
It's probably up to the platform maintainer to weigh in at this point.
Hope that helps?
Conor.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [linux-next:master] BUILD REGRESSION a6bd6c9333397f5a0e2667d4d82fef8c970108f2
From: kernel test robot @ 2024-03-28 18:09 UTC (permalink / raw)
To: Andrew Morton
Cc: Linux Memory Management List, amd-gfx, bpf, dri-devel, lima,
linux-arch, linux-arm-kernel, linux-omap, linux-renesas-soc,
linux-sound, linux-stm32, linux-usb, netdev, nouveau, spice-devel,
virtualization
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
branch HEAD: a6bd6c9333397f5a0e2667d4d82fef8c970108f2 Add linux-next specific files for 20240328
Error/Warning: (recently discovered and may have been fixed)
ERROR: modpost: "memcpy" [crypto/chacha20poly1305.ko] undefined!
ERROR: modpost: "memcpy" [fs/efs/efs.ko] undefined!
ERROR: modpost: "memcpy" [mm/z3fold.ko] undefined!
drivers/gpu/drm/lima/lima_drv.c:387:13: error: cast to smaller integer type 'enum lima_gpu_id' from 'const void *' [-Werror,-Wvoid-pointer-to-enum-cast]
drivers/gpu/drm/panthor/panthor_sched.c:2048:6: error: variable 'csg_mod_mask' set but not used [-Werror,-Wunused-but-set-variable]
drivers/gpu/drm/pl111/pl111_versatile.c:488:24: error: cast to smaller integer type 'enum versatile_clcd' from 'const void *' [-Werror,-Wvoid-pointer-to-enum-cast]
drivers/gpu/drm/qxl/qxl_cmd.c:424:6: error: variable 'count' set but not used [-Werror,-Wunused-but-set-variable]
drivers/gpu/drm/qxl/qxl_ioctl.c:148:14: error: variable 'num_relocs' set but not used [-Werror,-Wunused-but-set-variable]
include/asm-generic/io.h:547:31: error: performing pointer arithmetic on a null pointer has undefined behavior [-Werror,-Wnull-pointer-arithmetic]
mm/mempolicy.c:2223: warning: expecting prototype for alloc_pages_mpol_noprof(). Prototype was for alloc_pages_mpol() instead
mm/mempolicy.c:2298: warning: expecting prototype for vma_alloc_folio_noprof(). Prototype was for vma_alloc_folio() instead
mm/mempolicy.c:2326: warning: expecting prototype for alloc_pages_noprof(). Prototype was for alloc_pages() instead
mm/page_alloc.c:4857: warning: expecting prototype for alloc_pages_exact_noprof(). Prototype was for alloc_pages_exact() instead
mm/page_alloc.c:4882: warning: expecting prototype for alloc_pages_exact_nid_noprof(). Prototype was for alloc_pages_exact_nid() instead
mm/page_alloc.c:6348: warning: expecting prototype for alloc_contig_range_noprof(). Prototype was for alloc_contig_range() instead
mm/page_alloc.c:6535: warning: expecting prototype for alloc_contig_pages_noprof(). Prototype was for alloc_contig_pages() instead
Unverified Error/Warning (likely false positive, please contact us if interested):
{standard input}:2055: Error: unknown pseudo-op: `.cfi_restore_st'
Error/Warning ids grouped by kconfigs:
gcc_recent_errors
|-- alpha-randconfig-r015-20220508
| `-- ERROR:memcpy-mm-z3fold.ko-undefined
|-- alpha-randconfig-r034-20220715
| `-- ERROR:memcpy-fs-efs-efs.ko-undefined
|-- alpha-randconfig-r062-20240328
| `-- ERROR:memcpy-crypto-chacha20poly1305.ko-undefined
|-- parisc-allmodconfig
| `-- drivers-gpu-drm-nouveau-nvif-object.c:error:memcpy-accessing-or-more-bytes-at-offsets-and-overlaps-bytes-at-offset
|-- parisc-allyesconfig
| `-- drivers-gpu-drm-nouveau-nvif-object.c:error:memcpy-accessing-or-more-bytes-at-offsets-and-overlaps-bytes-at-offset
|-- sh-allmodconfig
| `-- drivers-pwm-pwm-stm32.c:error:implicit-declaration-of-function-devm_clk_rate_exclusive_get
|-- sh-allyesconfig
| `-- drivers-pwm-pwm-stm32.c:error:implicit-declaration-of-function-devm_clk_rate_exclusive_get
|-- sh-buildonly-randconfig-r003-20221218
| `-- standard-input:Error:unknown-pseudo-op:cfi_restore_st
|-- sparc-allmodconfig
| |-- mm-mempolicy.c:warning:expecting-prototype-for-alloc_pages_mpol_noprof().-Prototype-was-for-alloc_pages_mpol()-instead
| |-- mm-mempolicy.c:warning:expecting-prototype-for-alloc_pages_noprof().-Prototype-was-for-alloc_pages()-instead
| `-- mm-mempolicy.c:warning:expecting-prototype-for-vma_alloc_folio_noprof().-Prototype-was-for-vma_alloc_folio()-instead
|-- sparc-randconfig-001-20240328
| |-- (.head.text):relocation-truncated-to-fit:R_SPARC_WDISP22-against-init.text
| `-- (.init.text):relocation-truncated-to-fit:R_SPARC_WDISP22-against-symbol-leon_smp_cpu_startup-defined-in-.text-section-in-arch-sparc-kernel-trampoline_32.o
|-- um-randconfig-002-20240328
| `-- sound-soc-codecs-rk3308_codec.c:warning:rk3308_codec_of_match-defined-but-not-used
|-- x86_64-randconfig-073-20240328
| |-- mm-mempolicy.c:warning:expecting-prototype-for-alloc_pages_mpol_noprof().-Prototype-was-for-alloc_pages_mpol()-instead
| |-- mm-mempolicy.c:warning:expecting-prototype-for-alloc_pages_noprof().-Prototype-was-for-alloc_pages()-instead
| `-- mm-mempolicy.c:warning:expecting-prototype-for-vma_alloc_folio_noprof().-Prototype-was-for-vma_alloc_folio()-instead
|-- x86_64-randconfig-121-20240328
| |-- mm-mempolicy.c:warning:expecting-prototype-for-alloc_pages_mpol_noprof().-Prototype-was-for-alloc_pages_mpol()-instead
| |-- mm-mempolicy.c:warning:expecting-prototype-for-alloc_pages_noprof().-Prototype-was-for-alloc_pages()-instead
| `-- mm-mempolicy.c:warning:expecting-prototype-for-vma_alloc_folio_noprof().-Prototype-was-for-vma_alloc_folio()-instead
|-- x86_64-randconfig-123-20240328
| |-- mm-mempolicy.c:warning:expecting-prototype-for-alloc_pages_mpol_noprof().-Prototype-was-for-alloc_pages_mpol()-instead
| |-- mm-mempolicy.c:warning:expecting-prototype-for-alloc_pages_noprof().-Prototype-was-for-alloc_pages()-instead
| `-- mm-mempolicy.c:warning:expecting-prototype-for-vma_alloc_folio_noprof().-Prototype-was-for-vma_alloc_folio()-instead
`-- x86_64-randconfig-r071-20240327
|-- mm-page_alloc.c:warning:expecting-prototype-for-alloc_contig_pages_noprof().-Prototype-was-for-alloc_contig_pages()-instead
|-- mm-page_alloc.c:warning:expecting-prototype-for-alloc_contig_range_noprof().-Prototype-was-for-alloc_contig_range()-instead
|-- mm-page_alloc.c:warning:expecting-prototype-for-alloc_pages_exact_nid_noprof().-Prototype-was-for-alloc_pages_exact_nid()-instead
`-- mm-page_alloc.c:warning:expecting-prototype-for-alloc_pages_exact_noprof().-Prototype-was-for-alloc_pages_exact()-instead
clang_recent_errors
|-- arm-defconfig
| `-- arch-arm-mach-omap2-prm33xx.c:warning:expecting-prototype-for-am33xx_prm_global_warm_sw_reset().-Prototype-was-for-am33xx_prm_global_sw_reset()-instead
|-- arm-randconfig-r051-20240328
| `-- drivers-firmware-arm_scmi-raw_mode.c:WARNING:scmi_dbg_raw_mode_reset_fops:write()-has-stream-semantic-safe-to-change-nonseekable_open-stream_open.
|-- arm64-allmodconfig
| |-- drivers-gpu-drm-amd-amdgpu-amdgpu_drv.c:error:bitwise-operation-between-different-enumeration-types-(-enum-amd_asic_type-and-enum-amd_chip_flags-)-Werror-Wenum-enum-conversion
| |-- drivers-gpu-drm-amd-amdgpu-amdgpu_ras.c:error:arithmetic-between-different-enumeration-types-(-enum-amdgpu_ras_block-and-enum-amdgpu_ras_mca_block-)-Werror-Wenum-enum-conversion
| |-- drivers-gpu-drm-lima-lima_drv.c:error:cast-to-smaller-integer-type-enum-lima_gpu_id-from-const-void-Werror-Wvoid-pointer-to-enum-cast
| |-- drivers-gpu-drm-panthor-panthor_sched.c:error:variable-csg_mod_mask-set-but-not-used-Werror-Wunused-but-set-variable
| |-- drivers-gpu-drm-pl111-pl111_versatile.c:error:cast-to-smaller-integer-type-enum-versatile_clcd-from-const-void-Werror-Wvoid-pointer-to-enum-cast
| |-- drivers-gpu-drm-qxl-qxl_cmd.c:error:variable-count-set-but-not-used-Werror-Wunused-but-set-variable
| |-- drivers-gpu-drm-qxl-qxl_ioctl.c:error:variable-num_relocs-set-but-not-used-Werror-Wunused-but-set-variable
| |-- drivers-gpu-drm-radeon-radeon_drv.c:error:bitwise-operation-between-different-enumeration-types-(-enum-radeon_family-and-enum-radeon_chip_flags-)-Werror-Wenum-enum-conversion
| |-- drivers-gpu-drm-renesas-rcar-du-rcar_cmm.c:error:unused-function-rcar_cmm_read-Werror-Wunused-function
| |-- kernel-bpf-bpf_struct_ops.c:warning:bitwise-operation-between-different-enumeration-types-(-enum-bpf_type_flag-and-enum-bpf_reg_type-)
| |-- mm-mempolicy.c:warning:expecting-prototype-for-alloc_pages_mpol_noprof().-Prototype-was-for-alloc_pages_mpol()-instead
| |-- mm-mempolicy.c:warning:expecting-prototype-for-alloc_pages_noprof().-Prototype-was-for-alloc_pages()-instead
| `-- mm-mempolicy.c:warning:expecting-prototype-for-vma_alloc_folio_noprof().-Prototype-was-for-vma_alloc_folio()-instead
|-- hexagon-allmodconfig
| `-- include-asm-generic-io.h:error:performing-pointer-arithmetic-on-a-null-pointer-has-undefined-behavior-Werror-Wnull-pointer-arithmetic
|-- hexagon-allyesconfig
| `-- include-asm-generic-io.h:error:performing-pointer-arithmetic-on-a-null-pointer-has-undefined-behavior-Werror-Wnull-pointer-arithmetic
|-- i386-randconfig-141-20240328
| `-- drivers-usb-dwc2-hcd_ddma.c-dwc2_cmpl_host_isoc_dma_desc()-warn:variable-dereferenced-before-check-qtd-urb-(see-line-)
|-- powerpc-allyesconfig
| |-- drivers-gpu-drm-amd-amdgpu-amdgpu_drv.c:error:bitwise-operation-between-different-enumeration-types-(-enum-amd_asic_type-and-enum-amd_chip_flags-)-Werror-Wenum-enum-conversion
| |-- drivers-gpu-drm-amd-amdgpu-amdgpu_ras.c:error:arithmetic-between-different-enumeration-types-(-enum-amdgpu_ras_block-and-enum-amdgpu_ras_mca_block-)-Werror-Wenum-enum-conversion
| |-- drivers-gpu-drm-lima-lima_drv.c:error:cast-to-smaller-integer-type-enum-lima_gpu_id-from-const-void-Werror-Wvoid-pointer-to-enum-cast
| |-- drivers-gpu-drm-nouveau-nvkm-subdev-bios-shadowof.c:error:cast-from-void-(-)(const-void-)-to-void-(-)(void-)-converts-to-incompatible-function-type-Werror-Wcast-function-type-strict
| |-- drivers-gpu-drm-panthor-panthor_sched.c:error:variable-csg_mod_mask-set-but-not-used-Werror-Wunused-but-set-variable
| |-- drivers-gpu-drm-pl111-pl111_versatile.c:error:cast-to-smaller-integer-type-enum-versatile_clcd-from-const-void-Werror-Wvoid-pointer-to-enum-cast
| |-- drivers-gpu-drm-qxl-qxl_cmd.c:error:variable-count-set-but-not-used-Werror-Wunused-but-set-variable
| |-- drivers-gpu-drm-qxl-qxl_ioctl.c:error:variable-num_relocs-set-but-not-used-Werror-Wunused-but-set-variable
| |-- drivers-gpu-drm-radeon-radeon_drv.c:error:bitwise-operation-between-different-enumeration-types-(-enum-radeon_family-and-enum-radeon_chip_flags-)-Werror-Wenum-enum-conversion
| `-- kernel-bpf-bpf_struct_ops.c:warning:bitwise-operation-between-different-enumeration-types-(-enum-bpf_type_flag-and-enum-bpf_reg_type-)
|-- powerpc-randconfig-r113-20240328
| `-- drivers-gpu-drm-nouveau-nvkm-subdev-bios-shadowof.c:error:cast-from-void-(-)(const-void-)-to-void-(-)(void-)-converts-to-incompatible-function-type-Werror-Wcast-function-type-strict
|-- riscv-allmodconfig
| |-- drivers-gpu-drm-amd-amdgpu-amdgpu_drv.c:error:bitwise-operation-between-different-enumeration-types-(-enum-amd_asic_type-and-enum-amd_chip_flags-)-Werror-Wenum-enum-conversion
| |-- drivers-gpu-drm-amd-amdgpu-amdgpu_ras.c:error:arithmetic-between-different-enumeration-types-(-enum-amdgpu_ras_block-and-enum-amdgpu_ras_mca_block-)-Werror-Wenum-enum-conversion
| |-- drivers-gpu-drm-lima-lima_drv.c:error:cast-to-smaller-integer-type-enum-lima_gpu_id-from-const-void-Werror-Wvoid-pointer-to-enum-cast
| |-- drivers-gpu-drm-panthor-panthor_sched.c:error:variable-csg_mod_mask-set-but-not-used-Werror-Wunused-but-set-variable
| |-- drivers-gpu-drm-pl111-pl111_versatile.c:error:cast-to-smaller-integer-type-enum-versatile_clcd-from-const-void-Werror-Wvoid-pointer-to-enum-cast
| |-- drivers-gpu-drm-qxl-qxl_cmd.c:error:variable-count-set-but-not-used-Werror-Wunused-but-set-variable
| |-- drivers-gpu-drm-qxl-qxl_ioctl.c:error:variable-num_relocs-set-but-not-used-Werror-Wunused-but-set-variable
| |-- drivers-gpu-drm-radeon-radeon_drv.c:error:bitwise-operation-between-different-enumeration-types-(-enum-radeon_family-and-enum-radeon_chip_flags-)-Werror-Wenum-enum-conversion
| `-- kernel-bpf-bpf_struct_ops.c:warning:bitwise-operation-between-different-enumeration-types-(-enum-bpf_type_flag-and-enum-bpf_reg_type-)
|-- riscv-allyesconfig
| |-- drivers-gpu-drm-amd-amdgpu-amdgpu_drv.c:error:bitwise-operation-between-different-enumeration-types-(-enum-amd_asic_type-and-enum-amd_chip_flags-)-Werror-Wenum-enum-conversion
| |-- drivers-gpu-drm-amd-amdgpu-amdgpu_ras.c:error:arithmetic-between-different-enumeration-types-(-enum-amdgpu_ras_block-and-enum-amdgpu_ras_mca_block-)-Werror-Wenum-enum-conversion
| |-- drivers-gpu-drm-lima-lima_drv.c:error:cast-to-smaller-integer-type-enum-lima_gpu_id-from-const-void-Werror-Wvoid-pointer-to-enum-cast
| |-- drivers-gpu-drm-panthor-panthor_sched.c:error:variable-csg_mod_mask-set-but-not-used-Werror-Wunused-but-set-variable
| |-- drivers-gpu-drm-pl111-pl111_versatile.c:error:cast-to-smaller-integer-type-enum-versatile_clcd-from-const-void-Werror-Wvoid-pointer-to-enum-cast
| |-- drivers-gpu-drm-qxl-qxl_cmd.c:error:variable-count-set-but-not-used-Werror-Wunused-but-set-variable
| |-- drivers-gpu-drm-qxl-qxl_ioctl.c:error:variable-num_relocs-set-but-not-used-Werror-Wunused-but-set-variable
| |-- drivers-gpu-drm-radeon-radeon_drv.c:error:bitwise-operation-between-different-enumeration-types-(-enum-radeon_family-and-enum-radeon_chip_flags-)-Werror-Wenum-enum-conversion
| `-- kernel-bpf-bpf_struct_ops.c:warning:bitwise-operation-between-different-enumeration-types-(-enum-bpf_type_flag-and-enum-bpf_reg_type-)
|-- s390-allmodconfig
| |-- drivers-gpu-drm-amd-amdgpu-amdgpu_drv.c:error:bitwise-operation-between-different-enumeration-types-(-enum-amd_asic_type-and-enum-amd_chip_flags-)-Werror-Wenum-enum-conversion
| |-- drivers-gpu-drm-amd-amdgpu-amdgpu_ras.c:error:arithmetic-between-different-enumeration-types-(-enum-amdgpu_ras_block-and-enum-amdgpu_ras_mca_block-)-Werror-Wenum-enum-conversion
| |-- drivers-gpu-drm-lima-lima_drv.c:error:cast-to-smaller-integer-type-enum-lima_gpu_id-from-const-void-Werror-Wvoid-pointer-to-enum-cast
| |-- drivers-gpu-drm-panthor-panthor_sched.c:error:variable-csg_mod_mask-set-but-not-used-Werror-Wunused-but-set-variable
| |-- drivers-gpu-drm-pl111-pl111_versatile.c:error:cast-to-smaller-integer-type-enum-versatile_clcd-from-const-void-Werror-Wvoid-pointer-to-enum-cast
| |-- drivers-gpu-drm-qxl-qxl_cmd.c:error:variable-count-set-but-not-used-Werror-Wunused-but-set-variable
| |-- drivers-gpu-drm-qxl-qxl_ioctl.c:error:variable-num_relocs-set-but-not-used-Werror-Wunused-but-set-variable
| |-- drivers-gpu-drm-radeon-radeon_drv.c:error:bitwise-operation-between-different-enumeration-types-(-enum-radeon_family-and-enum-radeon_chip_flags-)-Werror-Wenum-enum-conversion
| |-- include-asm-generic-io.h:error:performing-pointer-arithmetic-on-a-null-pointer-has-undefined-behavior-Werror-Wnull-pointer-arithmetic
| `-- kernel-bpf-bpf_struct_ops.c:warning:bitwise-operation-between-different-enumeration-types-(-enum-bpf_type_flag-and-enum-bpf_reg_type-)
|-- s390-defconfig
| `-- kernel-bpf-bpf_struct_ops.c:warning:bitwise-operation-between-different-enumeration-types-(-enum-bpf_type_flag-and-enum-bpf_reg_type-)
|-- x86_64-allmodconfig
| |-- drivers-gpu-drm-lima-lima_drv.c:error:cast-to-smaller-integer-type-enum-lima_gpu_id-from-const-void-Werror-Wvoid-pointer-to-enum-cast
| |-- drivers-gpu-drm-panthor-panthor_sched.c:error:variable-csg_mod_mask-set-but-not-used-Werror-Wunused-but-set-variable
| |-- drivers-gpu-drm-pl111-pl111_versatile.c:error:cast-to-smaller-integer-type-enum-versatile_clcd-from-const-void-Werror-Wvoid-pointer-to-enum-cast
| |-- drivers-gpu-drm-qxl-qxl_cmd.c:error:variable-count-set-but-not-used-Werror-Wunused-but-set-variable
| `-- drivers-gpu-drm-qxl-qxl_ioctl.c:error:variable-num_relocs-set-but-not-used-Werror-Wunused-but-set-variable
|-- x86_64-allyesconfig
| |-- drivers-gpu-drm-lima-lima_drv.c:error:cast-to-smaller-integer-type-enum-lima_gpu_id-from-const-void-Werror-Wvoid-pointer-to-enum-cast
| |-- drivers-gpu-drm-panthor-panthor_sched.c:error:variable-csg_mod_mask-set-but-not-used-Werror-Wunused-but-set-variable
| |-- drivers-gpu-drm-pl111-pl111_versatile.c:error:cast-to-smaller-integer-type-enum-versatile_clcd-from-const-void-Werror-Wvoid-pointer-to-enum-cast
| |-- drivers-gpu-drm-qxl-qxl_cmd.c:error:variable-count-set-but-not-used-Werror-Wunused-but-set-variable
| `-- drivers-gpu-drm-qxl-qxl_ioctl.c:error:variable-num_relocs-set-but-not-used-Werror-Wunused-but-set-variable
`-- x86_64-randconfig-072-20240328
|-- mm-mempolicy.c:warning:expecting-prototype-for-alloc_pages_mpol_noprof().-Prototype-was-for-alloc_pages_mpol()-instead
|-- mm-mempolicy.c:warning:expecting-prototype-for-alloc_pages_noprof().-Prototype-was-for-alloc_pages()-instead
`-- mm-mempolicy.c:warning:expecting-prototype-for-vma_alloc_folio_noprof().-Prototype-was-for-vma_alloc_folio()-instead
elapsed time: 724m
configs tested: 179
configs skipped: 3
tested configs:
alpha allnoconfig gcc
alpha allyesconfig gcc
alpha defconfig gcc
arc allmodconfig gcc
arc allnoconfig gcc
arc allyesconfig gcc
arc defconfig gcc
arc haps_hs_smp_defconfig gcc
arc randconfig-001-20240328 gcc
arc randconfig-002-20240328 gcc
arm allmodconfig gcc
arm allnoconfig clang
arm allyesconfig gcc
arm defconfig clang
arm ixp4xx_defconfig gcc
arm moxart_defconfig gcc
arm multi_v4t_defconfig clang
arm randconfig-001-20240328 gcc
arm randconfig-002-20240328 gcc
arm randconfig-003-20240328 gcc
arm randconfig-004-20240328 gcc
arm s5pv210_defconfig gcc
arm sama5_defconfig gcc
arm64 allmodconfig clang
arm64 allnoconfig gcc
arm64 defconfig gcc
arm64 randconfig-001-20240328 gcc
arm64 randconfig-002-20240328 gcc
arm64 randconfig-003-20240328 gcc
arm64 randconfig-004-20240328 gcc
csky allmodconfig gcc
csky allnoconfig gcc
csky allyesconfig gcc
csky defconfig gcc
csky randconfig-001-20240328 gcc
csky randconfig-002-20240328 gcc
hexagon allmodconfig clang
hexagon allnoconfig clang
hexagon allyesconfig clang
hexagon defconfig clang
hexagon randconfig-001-20240328 clang
hexagon randconfig-002-20240328 clang
i386 allmodconfig gcc
i386 allnoconfig gcc
i386 allyesconfig gcc
i386 buildonly-randconfig-001-20240328 gcc
i386 buildonly-randconfig-002-20240328 gcc
i386 buildonly-randconfig-003-20240328 clang
i386 buildonly-randconfig-004-20240328 gcc
i386 buildonly-randconfig-005-20240328 gcc
i386 buildonly-randconfig-006-20240328 gcc
i386 defconfig clang
i386 randconfig-001-20240328 clang
i386 randconfig-002-20240328 clang
i386 randconfig-003-20240328 clang
i386 randconfig-004-20240328 clang
i386 randconfig-005-20240328 gcc
i386 randconfig-006-20240328 gcc
i386 randconfig-011-20240328 clang
i386 randconfig-012-20240328 clang
i386 randconfig-013-20240328 clang
i386 randconfig-014-20240328 clang
i386 randconfig-015-20240328 clang
i386 randconfig-016-20240328 clang
loongarch allmodconfig gcc
loongarch allnoconfig gcc
loongarch defconfig gcc
loongarch randconfig-001-20240328 gcc
loongarch randconfig-002-20240328 gcc
m68k allmodconfig gcc
m68k allnoconfig gcc
m68k allyesconfig gcc
m68k amcore_defconfig gcc
m68k defconfig gcc
microblaze allmodconfig gcc
microblaze allnoconfig gcc
microblaze allyesconfig gcc
microblaze defconfig gcc
mips allnoconfig gcc
mips allyesconfig gcc
mips ath25_defconfig clang
mips gcw0_defconfig clang
mips ip22_defconfig gcc
mips ip32_defconfig clang
mips loongson1c_defconfig gcc
mips rs90_defconfig gcc
nios2 allmodconfig gcc
nios2 allnoconfig gcc
nios2 allyesconfig gcc
nios2 defconfig gcc
nios2 randconfig-001-20240328 gcc
nios2 randconfig-002-20240328 gcc
openrisc allnoconfig gcc
openrisc allyesconfig gcc
openrisc defconfig gcc
parisc allmodconfig gcc
parisc allnoconfig gcc
parisc allyesconfig gcc
parisc defconfig gcc
parisc randconfig-001-20240328 gcc
parisc randconfig-002-20240328 gcc
parisc64 defconfig gcc
powerpc allmodconfig gcc
powerpc allnoconfig gcc
powerpc allyesconfig clang
powerpc randconfig-001-20240328 clang
powerpc randconfig-002-20240328 clang
powerpc randconfig-003-20240328 clang
powerpc storcenter_defconfig gcc
powerpc64 randconfig-001-20240328 clang
powerpc64 randconfig-002-20240328 gcc
powerpc64 randconfig-003-20240328 gcc
riscv allmodconfig clang
riscv allnoconfig gcc
riscv allyesconfig clang
riscv defconfig clang
riscv randconfig-001-20240328 gcc
riscv randconfig-002-20240328 gcc
s390 allmodconfig clang
s390 allnoconfig clang
s390 allyesconfig gcc
s390 defconfig clang
s390 randconfig-001-20240328 clang
s390 randconfig-002-20240328 clang
sh allmodconfig gcc
sh allnoconfig gcc
sh allyesconfig gcc
sh defconfig gcc
sh dreamcast_defconfig gcc
sh randconfig-001-20240328 gcc
sh randconfig-002-20240328 gcc
sh urquell_defconfig gcc
sparc allmodconfig gcc
sparc allnoconfig gcc
sparc defconfig gcc
sparc64 allmodconfig gcc
sparc64 allyesconfig gcc
sparc64 defconfig gcc
sparc64 randconfig-001-20240328 gcc
sparc64 randconfig-002-20240328 gcc
um allmodconfig clang
um allnoconfig clang
um allyesconfig gcc
um defconfig clang
um i386_defconfig gcc
um randconfig-001-20240328 gcc
um randconfig-002-20240328 gcc
um x86_64_defconfig clang
x86_64 allnoconfig clang
x86_64 allyesconfig clang
x86_64 buildonly-randconfig-001-20240328 gcc
x86_64 buildonly-randconfig-002-20240328 clang
x86_64 buildonly-randconfig-003-20240328 gcc
x86_64 buildonly-randconfig-004-20240328 gcc
x86_64 buildonly-randconfig-005-20240328 gcc
x86_64 buildonly-randconfig-006-20240328 gcc
x86_64 defconfig gcc
x86_64 randconfig-001-20240328 clang
x86_64 randconfig-002-20240328 gcc
x86_64 randconfig-003-20240328 clang
x86_64 randconfig-004-20240328 gcc
x86_64 randconfig-005-20240328 clang
x86_64 randconfig-006-20240328 clang
x86_64 randconfig-011-20240328 clang
x86_64 randconfig-012-20240328 clang
x86_64 randconfig-013-20240328 gcc
x86_64 randconfig-014-20240328 gcc
x86_64 randconfig-015-20240328 clang
x86_64 randconfig-016-20240328 clang
x86_64 randconfig-071-20240328 gcc
x86_64 randconfig-072-20240328 clang
x86_64 randconfig-073-20240328 gcc
x86_64 randconfig-074-20240328 gcc
x86_64 randconfig-075-20240328 gcc
x86_64 randconfig-076-20240328 clang
x86_64 rhel-8.3-rust clang
xtensa allnoconfig gcc
xtensa randconfig-001-20240328 gcc
xtensa randconfig-002-20240328 gcc
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v7 6/6] docs: trusted-encrypted: add DCP as new trust source
From: Jarkko Sakkinen @ 2024-03-28 18:47 UTC (permalink / raw)
To: David Gstir
Cc: Mimi Zohar, James Bottomley, Herbert Xu, David S. Miller,
Shawn Guo, Jonathan Corbet, Sascha Hauer, kernel@pengutronix.de,
Fabio Estevam, NXP Linux Team, Ahmad Fatoum,
sigma star Kernel Team, David Howells, Li Yang, Paul Moore,
James Morris, Serge E. Hallyn, Paul E. McKenney, Randy Dunlap,
Catalin Marinas, Rafael J. Wysocki, Tejun Heo,
Steven Rostedt (Google), linux-doc, linux-kernel@vger.kernel.org,
linux-integrity@vger.kernel.org, keyrings@vger.kernel.org,
linux-crypto@vger.kernel.org, linux-arm-kernel, linuxppc-dev,
linux-security-module@vger.kernel.org, Richard Weinberger,
David Oberhollenzer
In-Reply-To: <A3831544-E47C-4AEB-9963-536F0B1EE8FD@sigma-star.at>
On Thu Mar 28, 2024 at 10:05 AM EET, David Gstir wrote:
> Jarkko,
>
> > On 27.03.2024, at 16:40, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> >
> > On Wed Mar 27, 2024 at 10:24 AM EET, David Gstir wrote:
> >> Update the documentation for trusted and encrypted KEYS with DCP as new
> >> trust source:
> >>
> >> - Describe security properties of DCP trust source
> >> - Describe key usage
> >> - Document blob format
> >>
> >> Co-developed-by: Richard Weinberger <richard@nod.at>
> >> Signed-off-by: Richard Weinberger <richard@nod.at>
> >> Co-developed-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
> >> Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
> >> Signed-off-by: David Gstir <david@sigma-star.at>
> >> ---
> >> .../security/keys/trusted-encrypted.rst | 85 +++++++++++++++++++
> >> 1 file changed, 85 insertions(+)
> >>
> >> diff --git a/Documentation/security/keys/trusted-encrypted.rst b/Documentation/security/keys/trusted-encrypted.rst
> >> index e989b9802f92..81fb3540bb20 100644
> >> --- a/Documentation/security/keys/trusted-encrypted.rst
> >> +++ b/Documentation/security/keys/trusted-encrypted.rst
> >> @@ -42,6 +42,14 @@ safe.
> >> randomly generated and fused into each SoC at manufacturing time.
> >> Otherwise, a common fixed test key is used instead.
> >>
> >> + (4) DCP (Data Co-Processor: crypto accelerator of various i.MX SoCs)
> >> +
> >> + Rooted to a one-time programmable key (OTP) that is generally burnt
> >> + in the on-chip fuses and is accessible to the DCP encryption engine only.
> >> + DCP provides two keys that can be used as root of trust: the OTP key
> >> + and the UNIQUE key. Default is to use the UNIQUE key, but selecting
> >> + the OTP key can be done via a module parameter (dcp_use_otp_key).
> >> +
> >> * Execution isolation
> >>
> >> (1) TPM
> >> @@ -57,6 +65,12 @@ safe.
> >>
> >> Fixed set of operations running in isolated execution environment.
> >>
> >> + (4) DCP
> >> +
> >> + Fixed set of cryptographic operations running in isolated execution
> >> + environment. Only basic blob key encryption is executed there.
> >> + The actual key sealing/unsealing is done on main processor/kernel space.
> >> +
> >> * Optional binding to platform integrity state
> >>
> >> (1) TPM
> >> @@ -79,6 +93,11 @@ safe.
> >> Relies on the High Assurance Boot (HAB) mechanism of NXP SoCs
> >> for platform integrity.
> >>
> >> + (4) DCP
> >> +
> >> + Relies on Secure/Trusted boot process (called HAB by vendor) for
> >> + platform integrity.
> >> +
> >> * Interfaces and APIs
> >>
> >> (1) TPM
> >> @@ -94,6 +113,11 @@ safe.
> >>
> >> Interface is specific to silicon vendor.
> >>
> >> + (4) DCP
> >> +
> >> + Vendor-specific API that is implemented as part of the DCP crypto driver in
> >> + ``drivers/crypto/mxs-dcp.c``.
> >> +
> >> * Threat model
> >>
> >> The strength and appropriateness of a particular trust source for a given
> >> @@ -129,6 +153,13 @@ selected trust source:
> >> CAAM HWRNG, enable CRYPTO_DEV_FSL_CAAM_RNG_API and ensure the device
> >> is probed.
> >>
> >> + * DCP (Data Co-Processor: crypto accelerator of various i.MX SoCs)
> >> +
> >> + The DCP hardware device itself does not provide a dedicated RNG interface,
> >> + so the kernel default RNG is used. SoCs with DCP like the i.MX6ULL do have
> >> + a dedicated hardware RNG that is independent from DCP which can be enabled
> >> + to back the kernel RNG.
> >> +
> >> Users may override this by specifying ``trusted.rng=kernel`` on the kernel
> >> command-line to override the used RNG with the kernel's random number pool.
> >>
> >> @@ -231,6 +262,19 @@ Usage::
> >> CAAM-specific format. The key length for new keys is always in bytes.
> >> Trusted Keys can be 32 - 128 bytes (256 - 1024 bits).
> >>
> >> +Trusted Keys usage: DCP
> >> +-----------------------
> >> +
> >> +Usage::
> >> +
> >> + keyctl add trusted name "new keylen" ring
> >> + keyctl add trusted name "load hex_blob" ring
> >> + keyctl print keyid
> >> +
> >> +"keyctl print" returns an ASCII hex copy of the sealed key, which is in format
> >> +specific to this DCP key-blob implementation. The key length for new keys is
> >> +always in bytes. Trusted Keys can be 32 - 128 bytes (256 - 1024 bits).
> >> +
> >> Encrypted Keys usage
> >> --------------------
> >>
> >> @@ -426,3 +470,44 @@ string length.
> >> privkey is the binary representation of TPM2B_PUBLIC excluding the
> >> initial TPM2B header which can be reconstructed from the ASN.1 octed
> >> string length.
> >> +
> >> +DCP Blob Format
> >> +---------------
> >> +
> >> +The Data Co-Processor (DCP) provides hardware-bound AES keys using its
> >> +AES encryption engine only. It does not provide direct key sealing/unsealing.
> >> +To make DCP hardware encryption keys usable as trust source, we define
> >> +our own custom format that uses a hardware-bound key to secure the sealing
> >> +key stored in the key blob.
> >> +
> >> +Whenever a new trusted key using DCP is generated, we generate a random 128-bit
> >> +blob encryption key (BEK) and 128-bit nonce. The BEK and nonce are used to
> >> +encrypt the trusted key payload using AES-128-GCM.
> >> +
> >> +The BEK itself is encrypted using the hardware-bound key using the DCP's AES
> >> +encryption engine with AES-128-ECB. The encrypted BEK, generated nonce,
> >> +BEK-encrypted payload and authentication tag make up the blob format together
> >> +with a version number, payload length and authentication tag::
> >> +
> >> + /*
> >> + * struct dcp_blob_fmt - DCP BLOB format.
> >> + *
> >> + * @fmt_version: Format version, currently being %1
> >> + * @blob_key: Random AES 128 key which is used to encrypt @payload,
> >> + * @blob_key itself is encrypted with OTP or UNIQUE device key in
> >> + * AES-128-ECB mode by DCP.
> >> + * @nonce: Random nonce used for @payload encryption.
> >> + * @payload_len: Length of the plain text @payload.
> >> + * @payload: The payload itself, encrypted using AES-128-GCM and @blob_key,
> >> + * GCM auth tag of size AES_BLOCK_SIZE is attached at the end of it.
> >> + *
> >> + * The total size of a DCP BLOB is sizeof(struct dcp_blob_fmt) + @payload_len +
> >> + * AES_BLOCK_SIZE.
> >> + */
> >> + struct dcp_blob_fmt {
> >> + __u8 fmt_version;
> >> + __u8 blob_key[AES_KEYSIZE_128];
> >> + __u8 nonce[AES_KEYSIZE_128];
> >> + __le32 payload_len;
> >> + __u8 payload[];
> >> + } __packed;
> >
> > I'm thinking here given that you need to replicate the same thing that
> > is in the source files. E.g. Documentation/gpu/i915.rst.
> >
> > The rationale would so many sources so maybe it would make sense to
> > maintain this in the source code.
> >
> > Also this documents how to generally insert documentation inline:
> > https://docs.kernel.org/doc-guide/kernel-doc.html
> >
> > I.e. I'm feeling that this is good time to improve scalability so that
> > documentation will keep up to date. Also then backend specific patches
> > mostly go to their subdirectories and not to Documentation/ subtree
> > (or that would be more rare case).
> >
> > So a good chance to do more than just a new backend for the benefit
> > of the trusted keys subsystem :-)
> >
> > Also, later on if something is changed e.g. in the above struct you
> > don't have to do matching update to the documentation so it will save
> > time too (over time).
>
> sound good! I’ll maintain the blob format documentation to the source and insert
> a reference in the documentation. Thanks for pointing that out!
>
> Is there anything else I can improve for this patchset? I’d like to include that in v8
> too and make it the last iteration of this patchset.
Yeah, I don't enforce you to convert all the existing work to this
model, but we could use this as a reference for that work.
The patch set is overally in a pretty good shape I think :-)
BR, Jarkko
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v7 6/6] docs: trusted-encrypted: add DCP as new trust source
From: Jarkko Sakkinen @ 2024-03-28 18:50 UTC (permalink / raw)
To: Jarkko Sakkinen, David Gstir
Cc: Mimi Zohar, James Bottomley, Herbert Xu, David S. Miller,
Shawn Guo, Jonathan Corbet, Sascha Hauer, kernel@pengutronix.de,
Fabio Estevam, NXP Linux Team, Ahmad Fatoum,
sigma star Kernel Team, David Howells, Li Yang, Paul Moore,
James Morris, Serge E. Hallyn, Paul E. McKenney, Randy Dunlap,
Catalin Marinas, Rafael J. Wysocki, Tejun Heo,
Steven Rostedt (Google), linux-doc, linux-kernel@vger.kernel.org,
linux-integrity@vger.kernel.org, keyrings@vger.kernel.org,
linux-crypto@vger.kernel.org, linux-arm-kernel, linuxppc-dev,
linux-security-module@vger.kernel.org, Richard Weinberger,
David Oberhollenzer
In-Reply-To: <D05LV2A6J8X7.11895O5TC10TS@kernel.org>
On Thu Mar 28, 2024 at 8:47 PM EET, Jarkko Sakkinen wrote:
> On Thu Mar 28, 2024 at 10:05 AM EET, David Gstir wrote:
> > Jarkko,
> >
> > > On 27.03.2024, at 16:40, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > >
> > > On Wed Mar 27, 2024 at 10:24 AM EET, David Gstir wrote:
> > >> Update the documentation for trusted and encrypted KEYS with DCP as new
> > >> trust source:
> > >>
> > >> - Describe security properties of DCP trust source
> > >> - Describe key usage
> > >> - Document blob format
> > >>
> > >> Co-developed-by: Richard Weinberger <richard@nod.at>
> > >> Signed-off-by: Richard Weinberger <richard@nod.at>
> > >> Co-developed-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
> > >> Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
> > >> Signed-off-by: David Gstir <david@sigma-star.at>
> > >> ---
> > >> .../security/keys/trusted-encrypted.rst | 85 +++++++++++++++++++
> > >> 1 file changed, 85 insertions(+)
> > >>
> > >> diff --git a/Documentation/security/keys/trusted-encrypted.rst b/Documentation/security/keys/trusted-encrypted.rst
> > >> index e989b9802f92..81fb3540bb20 100644
> > >> --- a/Documentation/security/keys/trusted-encrypted.rst
> > >> +++ b/Documentation/security/keys/trusted-encrypted.rst
> > >> @@ -42,6 +42,14 @@ safe.
> > >> randomly generated and fused into each SoC at manufacturing time.
> > >> Otherwise, a common fixed test key is used instead.
> > >>
> > >> + (4) DCP (Data Co-Processor: crypto accelerator of various i.MX SoCs)
> > >> +
> > >> + Rooted to a one-time programmable key (OTP) that is generally burnt
> > >> + in the on-chip fuses and is accessible to the DCP encryption engine only.
> > >> + DCP provides two keys that can be used as root of trust: the OTP key
> > >> + and the UNIQUE key. Default is to use the UNIQUE key, but selecting
> > >> + the OTP key can be done via a module parameter (dcp_use_otp_key).
> > >> +
> > >> * Execution isolation
> > >>
> > >> (1) TPM
> > >> @@ -57,6 +65,12 @@ safe.
> > >>
> > >> Fixed set of operations running in isolated execution environment.
> > >>
> > >> + (4) DCP
> > >> +
> > >> + Fixed set of cryptographic operations running in isolated execution
> > >> + environment. Only basic blob key encryption is executed there.
> > >> + The actual key sealing/unsealing is done on main processor/kernel space.
> > >> +
> > >> * Optional binding to platform integrity state
> > >>
> > >> (1) TPM
> > >> @@ -79,6 +93,11 @@ safe.
> > >> Relies on the High Assurance Boot (HAB) mechanism of NXP SoCs
> > >> for platform integrity.
> > >>
> > >> + (4) DCP
> > >> +
> > >> + Relies on Secure/Trusted boot process (called HAB by vendor) for
> > >> + platform integrity.
> > >> +
> > >> * Interfaces and APIs
> > >>
> > >> (1) TPM
> > >> @@ -94,6 +113,11 @@ safe.
> > >>
> > >> Interface is specific to silicon vendor.
> > >>
> > >> + (4) DCP
> > >> +
> > >> + Vendor-specific API that is implemented as part of the DCP crypto driver in
> > >> + ``drivers/crypto/mxs-dcp.c``.
> > >> +
> > >> * Threat model
> > >>
> > >> The strength and appropriateness of a particular trust source for a given
> > >> @@ -129,6 +153,13 @@ selected trust source:
> > >> CAAM HWRNG, enable CRYPTO_DEV_FSL_CAAM_RNG_API and ensure the device
> > >> is probed.
> > >>
> > >> + * DCP (Data Co-Processor: crypto accelerator of various i.MX SoCs)
> > >> +
> > >> + The DCP hardware device itself does not provide a dedicated RNG interface,
> > >> + so the kernel default RNG is used. SoCs with DCP like the i.MX6ULL do have
> > >> + a dedicated hardware RNG that is independent from DCP which can be enabled
> > >> + to back the kernel RNG.
> > >> +
> > >> Users may override this by specifying ``trusted.rng=kernel`` on the kernel
> > >> command-line to override the used RNG with the kernel's random number pool.
> > >>
> > >> @@ -231,6 +262,19 @@ Usage::
> > >> CAAM-specific format. The key length for new keys is always in bytes.
> > >> Trusted Keys can be 32 - 128 bytes (256 - 1024 bits).
> > >>
> > >> +Trusted Keys usage: DCP
> > >> +-----------------------
> > >> +
> > >> +Usage::
> > >> +
> > >> + keyctl add trusted name "new keylen" ring
> > >> + keyctl add trusted name "load hex_blob" ring
> > >> + keyctl print keyid
> > >> +
> > >> +"keyctl print" returns an ASCII hex copy of the sealed key, which is in format
> > >> +specific to this DCP key-blob implementation. The key length for new keys is
> > >> +always in bytes. Trusted Keys can be 32 - 128 bytes (256 - 1024 bits).
> > >> +
> > >> Encrypted Keys usage
> > >> --------------------
> > >>
> > >> @@ -426,3 +470,44 @@ string length.
> > >> privkey is the binary representation of TPM2B_PUBLIC excluding the
> > >> initial TPM2B header which can be reconstructed from the ASN.1 octed
> > >> string length.
> > >> +
> > >> +DCP Blob Format
> > >> +---------------
> > >> +
> > >> +The Data Co-Processor (DCP) provides hardware-bound AES keys using its
> > >> +AES encryption engine only. It does not provide direct key sealing/unsealing.
> > >> +To make DCP hardware encryption keys usable as trust source, we define
> > >> +our own custom format that uses a hardware-bound key to secure the sealing
> > >> +key stored in the key blob.
> > >> +
> > >> +Whenever a new trusted key using DCP is generated, we generate a random 128-bit
> > >> +blob encryption key (BEK) and 128-bit nonce. The BEK and nonce are used to
> > >> +encrypt the trusted key payload using AES-128-GCM.
> > >> +
> > >> +The BEK itself is encrypted using the hardware-bound key using the DCP's AES
> > >> +encryption engine with AES-128-ECB. The encrypted BEK, generated nonce,
> > >> +BEK-encrypted payload and authentication tag make up the blob format together
> > >> +with a version number, payload length and authentication tag::
> > >> +
> > >> + /*
> > >> + * struct dcp_blob_fmt - DCP BLOB format.
> > >> + *
> > >> + * @fmt_version: Format version, currently being %1
> > >> + * @blob_key: Random AES 128 key which is used to encrypt @payload,
> > >> + * @blob_key itself is encrypted with OTP or UNIQUE device key in
> > >> + * AES-128-ECB mode by DCP.
> > >> + * @nonce: Random nonce used for @payload encryption.
> > >> + * @payload_len: Length of the plain text @payload.
> > >> + * @payload: The payload itself, encrypted using AES-128-GCM and @blob_key,
> > >> + * GCM auth tag of size AES_BLOCK_SIZE is attached at the end of it.
> > >> + *
> > >> + * The total size of a DCP BLOB is sizeof(struct dcp_blob_fmt) + @payload_len +
> > >> + * AES_BLOCK_SIZE.
> > >> + */
> > >> + struct dcp_blob_fmt {
> > >> + __u8 fmt_version;
> > >> + __u8 blob_key[AES_KEYSIZE_128];
> > >> + __u8 nonce[AES_KEYSIZE_128];
> > >> + __le32 payload_len;
> > >> + __u8 payload[];
> > >> + } __packed;
> > >
> > > I'm thinking here given that you need to replicate the same thing that
> > > is in the source files. E.g. Documentation/gpu/i915.rst.
> > >
> > > The rationale would so many sources so maybe it would make sense to
> > > maintain this in the source code.
> > >
> > > Also this documents how to generally insert documentation inline:
> > > https://docs.kernel.org/doc-guide/kernel-doc.html
> > >
> > > I.e. I'm feeling that this is good time to improve scalability so that
> > > documentation will keep up to date. Also then backend specific patches
> > > mostly go to their subdirectories and not to Documentation/ subtree
> > > (or that would be more rare case).
> > >
> > > So a good chance to do more than just a new backend for the benefit
> > > of the trusted keys subsystem :-)
> > >
> > > Also, later on if something is changed e.g. in the above struct you
> > > don't have to do matching update to the documentation so it will save
> > > time too (over time).
> >
> > sound good! I’ll maintain the blob format documentation to the source and insert
> > a reference in the documentation. Thanks for pointing that out!
> >
> > Is there anything else I can improve for this patchset? I’d like to include that in v8
> > too and make it the last iteration of this patchset.
>
> Yeah, I don't enforce you to convert all the existing work to this
> model, but we could use this as a reference for that work.
I mean that way documentation update will become more natural part of
the code update and less frustrating to do which usually encourages
to document stuff better and also later on tweak and improve it.
So yeah I feel that GPU documentation in kernel is doing lot's of things
right, and other subsystems should follow. I.e. it is good reference
model for trusted keys documentation, and said I'm cool with realizing
this for only this new key type :-) Otherwise, finishing this patch set
will torture for you and also for me if it tries to fix everything.
BR, Jarkko
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3 2/2] phy: add driver for MediaTek XFI T-PHY
From: Vinod Koul @ 2024-03-28 18:52 UTC (permalink / raw)
To: Daniel Golle
Cc: Bc-bocun Chen, Steven Liu, John Crispin, Chunfeng Yun,
Kishon Vijay Abraham I, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Matthias Brugger, AngeloGioacchino Del Regno,
Qingfang Deng, SkyLake Huang, Philipp Zabel, linux-arm-kernel,
linux-mediatek, linux-phy, devicetree, linux-kernel, netdev
In-Reply-To: <3bb95f1d795eede63284dbcb224e06ea6886b421.1707530671.git.daniel@makrotopia.org>
On 10-02-24, 02:10, Daniel Golle wrote:
> Add driver for MediaTek's XFI T-PHY which can be found in the MT7988
What does XFI mean?
> SoC. The XFI T-PHY is a 10 Gigabit/s Ethernet SerDes PHY with muxes on
> the internal side to be used with either USXGMII PCS or LynxI PCS,
> depending on the selected PHY interface mode.
>
> The PHY can operates only in PHY_MODE_ETHERNET, the submode is one of
> PHY_INTERFACE_MODE_* corresponding to the supported modes:
>
> * USXGMII \
> * 10GBase-R }- USXGMII PCS - XGDM \
> * 5GBase-R / \
> }- Ethernet MAC
> * 2500Base-X \ /
> * 1000Base-X }- LynxI PCS - GDM /
> * Cisco SGMII (MAC side) /
>
> In order to work-around a performance issue present on the first of
> two XFI T-PHYs present in MT7988, special tuning is applied which can be
> selected by adding the 'mediatek,usxgmii-performance-errata' property to
> the device tree node.
>
> There is no documentation for most registers used for the
> analog/tuning part, however, most of the registers have been partially
> reverse-engineered from MediaTek's SDK implementation (an opaque
> sequence of 32-bit register writes) and descriptions for all relevant
> digital registers and bits such as resets and muxes have been supplied
> by MediaTek.
>
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
> v3: no changes
> v2:
> * use IO helpers from mtk-io.h instead of rolling my own
> * use devm_clk_bulk_get()
> * yse devm_platform_ioremap_resource()
> * unify name and description everywhere
> * invert bool is_xgmii into bool use_lynxi_pcs and add comments
> describing the meaning of each of the stack variables
> * not much we can do about remaining magic values unless MTK provides
> definitions for them
>
>
> MAINTAINERS | 1 +
> drivers/phy/mediatek/Kconfig | 12 +
> drivers/phy/mediatek/Makefile | 1 +
> drivers/phy/mediatek/phy-mtk-xfi-tphy.c | 360 ++++++++++++++++++++++++
> 4 files changed, 374 insertions(+)
> create mode 100644 drivers/phy/mediatek/phy-mtk-xfi-tphy.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4be2fd097f261..616b86e3e62fd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13776,6 +13776,7 @@ L: netdev@vger.kernel.org
> S: Maintained
> F: drivers/net/phy/mediatek-ge-soc.c
> F: drivers/net/phy/mediatek-ge.c
> +F: drivers/phy/mediatek/phy-mtk-xfi-tphy.c
>
> MEDIATEK I2C CONTROLLER DRIVER
> M: Qii Wang <qii.wang@mediatek.com>
> diff --git a/drivers/phy/mediatek/Kconfig b/drivers/phy/mediatek/Kconfig
> index 3849b7c87d287..117d0e84c7360 100644
> --- a/drivers/phy/mediatek/Kconfig
> +++ b/drivers/phy/mediatek/Kconfig
> @@ -13,6 +13,18 @@ config PHY_MTK_PCIE
> callback for PCIe GEN3 port, it supports software efuse
> initialization.
>
> +config PHY_MTK_XFI_TPHY
> + tristate "MediaTek 10GE SerDes XFI T-PHY driver"
> + depends on ARCH_MEDIATEK || COMPILE_TEST
> + depends on OF && OF_ADDRESS
why both, is OF not enough?
> + depends on HAS_IOMEM
> + select GENERIC_PHY
> + help
> + Say 'Y' here to add support for MediaTek XFI T-PHY driver.
> + The driver provides access to the Ethernet SerDes T-PHY supporting
> + 1GE and 2.5GE modes via the LynxI PCS, and 5GE and 10GE modes
> + via the USXGMII PCS found in MediaTek SoCs with 10G Ethernet.
> +
> config PHY_MTK_TPHY
> tristate "MediaTek T-PHY Driver"
> depends on ARCH_MEDIATEK || COMPILE_TEST
> diff --git a/drivers/phy/mediatek/Makefile b/drivers/phy/mediatek/Makefile
> index f6e24a47e0815..1b8088df71e84 100644
> --- a/drivers/phy/mediatek/Makefile
> +++ b/drivers/phy/mediatek/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_PHY_MTK_PCIE) += phy-mtk-pcie.o
> obj-$(CONFIG_PHY_MTK_TPHY) += phy-mtk-tphy.o
> obj-$(CONFIG_PHY_MTK_UFS) += phy-mtk-ufs.o
> obj-$(CONFIG_PHY_MTK_XSPHY) += phy-mtk-xsphy.o
> +obj-$(CONFIG_PHY_MTK_XFI_TPHY) += phy-mtk-xfi-tphy.o
>
> phy-mtk-hdmi-drv-y := phy-mtk-hdmi.o
> phy-mtk-hdmi-drv-y += phy-mtk-hdmi-mt2701.o
> diff --git a/drivers/phy/mediatek/phy-mtk-xfi-tphy.c b/drivers/phy/mediatek/phy-mtk-xfi-tphy.c
> new file mode 100644
> index 0000000000000..551d6cee33f94
> --- /dev/null
> +++ b/drivers/phy/mediatek/phy-mtk-xfi-tphy.c
> @@ -0,0 +1,360 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/* MediaTek 10GE SerDes XFI T-PHY driver
> + *
> + * Copyright (c) 2024 Daniel Golle <daniel@makrotopia.org>
> + * Bc-bocun Chen <bc-bocun.chen@mediatek.com>
> + * based on mtk_usxgmii.c and mtk_sgmii.c found in MediaTek's SDK (GPL-2.0)
> + * Copyright (c) 2022 MediaTek Inc.
> + * Author: Henry Yen <henry.yen@mediatek.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/io.h>
> +#include <linux/clk.h>
> +#include <linux/reset.h>
> +#include <linux/phy.h>
> +#include <linux/phy/phy.h>
> +
> +#include "phy-mtk-io.h"
> +
> +#define MTK_XFI_TPHY_NUM_CLOCKS 2
> +
> +#define REG_DIG_GLB_70 0x0070
> +#define XTP_PCS_RX_EQ_IN_PROGRESS(x) FIELD_PREP(GENMASK(25, 24), (x))
> +#define XTP_PCS_MODE_MASK GENMASK(17, 16)
> +#define XTP_PCS_MODE(x) FIELD_PREP(GENMASK(17, 16), (x))
> +#define XTP_PCS_RST_B BIT(15)
> +#define XTP_FRC_PCS_RST_B BIT(14)
> +#define XTP_PCS_PWD_SYNC_MASK GENMASK(13, 12)
> +#define XTP_PCS_PWD_SYNC(x) FIELD_PREP(XTP_PCS_PWD_SYNC_MASK, (x))
> +#define XTP_PCS_PWD_ASYNC_MASK GENMASK(11, 10)
> +#define XTP_PCS_PWD_ASYNC(x) FIELD_PREP(XTP_PCS_PWD_ASYNC_MASK, (x))
> +#define XTP_FRC_PCS_PWD_ASYNC BIT(8)
> +#define XTP_PCS_UPDT BIT(4)
> +#define XTP_PCS_IN_FR_RG BIT(0)
> +
> +#define REG_DIG_GLB_F4 0x00f4
> +#define XFI_DPHY_PCS_SEL BIT(0)
> +#define XFI_DPHY_PCS_SEL_SGMII FIELD_PREP(XFI_DPHY_PCS_SEL, 1)
> +#define XFI_DPHY_PCS_SEL_USXGMII FIELD_PREP(XFI_DPHY_PCS_SEL, 0)
> +#define XFI_DPHY_AD_SGDT_FRC_EN BIT(5)
> +
> +#define REG_DIG_LN_TRX_40 0x3040
> +#define XTP_LN_FRC_TX_DATA_EN BIT(29)
> +#define XTP_LN_TX_DATA_EN BIT(28)
> +
> +#define REG_DIG_LN_TRX_B0 0x30b0
> +#define XTP_LN_FRC_TX_MACCK_EN BIT(5)
> +#define XTP_LN_TX_MACCK_EN BIT(4)
> +
> +#define REG_ANA_GLB_D0 0x90d0
> +#define XTP_GLB_USXGMII_SEL_MASK GENMASK(3, 1)
> +#define XTP_GLB_USXGMII_SEL(x) FIELD_PREP(GENMASK(3, 1), (x))
> +#define XTP_GLB_USXGMII_EN BIT(0)
> +
> +struct mtk_xfi_tphy {
> + void __iomem *base;
> + struct device *dev;
> + struct reset_control *reset;
> + struct clk_bulk_data clocks[MTK_XFI_TPHY_NUM_CLOCKS];
> + bool da_war;
> +};
> +
> +static void mtk_xfi_tphy_setup(struct mtk_xfi_tphy *xfi_tphy,
> + phy_interface_t interface)
> +{
> + /* Override 10GBase-R tuning value if work-around is selected */
> + bool da_war = (xfi_tphy->da_war && (interface == PHY_INTERFACE_MODE_10GBASER));
why do you need braces around this?
> + /* Bools to make setting up values for specific PHY speeds easier */
> + bool is_2p5g = (interface == PHY_INTERFACE_MODE_2500BASEX);
> + bool is_1g = (interface == PHY_INTERFACE_MODE_1000BASEX ||
> + interface == PHY_INTERFACE_MODE_SGMII);
> + bool is_10g = (interface == PHY_INTERFACE_MODE_10GBASER ||
> + interface == PHY_INTERFACE_MODE_USXGMII);
> + bool is_5g = (interface == PHY_INTERFACE_MODE_5GBASER);
> + /* Bool to configure input mux to either
> + * - USXGMII PCS (64b/66b coding) for 5G/10G
> + * - LynxI PCS (8b/10b coding) for 1G/2.5G
> + */
> + bool use_lynxi_pcs = (is_1g || is_2p5g);
This is quite terrible to read, how about declaring variables first and
then doing the initialization?
> +
> + dev_dbg(xfi_tphy->dev, "setting up for mode %s\n", phy_modes(interface));
> +
> + /* Setup PLL setting */
> + mtk_phy_update_bits(xfi_tphy->base + 0x9024, 0x100000, is_10g ? 0x0 : 0x100000);
> + mtk_phy_update_bits(xfi_tphy->base + 0x2020, 0x202000, is_5g ? 0x202000 : 0x0);
> + mtk_phy_update_bits(xfi_tphy->base + 0x2030, 0x500, is_1g ? 0x0 : 0x500);
> + mtk_phy_update_bits(xfi_tphy->base + 0x2034, 0xa00, is_1g ? 0x0 : 0xa00);
> + mtk_phy_update_bits(xfi_tphy->base + 0x2040, 0x340000, is_1g ? 0x200000 : 0x140000);
magic numbers?
> +
> + /* Setup RXFE BW setting */
> + mtk_phy_update_bits(xfi_tphy->base + 0x50f0, 0xc10, is_1g ? 0x410 : is_5g ? 0x800 : 0x400);
> + mtk_phy_update_bits(xfi_tphy->base + 0x50e0, 0x4000, is_5g ? 0x0 : 0x4000);
> +
> + /* Setup RX CDR setting */
> + mtk_phy_update_bits(xfi_tphy->base + 0x506c, 0x30000, is_5g ? 0x0 : 0x30000);
> + mtk_phy_update_bits(xfi_tphy->base + 0x5070, 0x670000, is_5g ? 0x620000 : 0x50000);
> + mtk_phy_update_bits(xfi_tphy->base + 0x5074, 0x180000, is_5g ? 0x180000 : 0x0);
> + mtk_phy_update_bits(xfi_tphy->base + 0x5078, 0xf000400, is_5g ? 0x8000000 :
> + 0x7000400);
> + mtk_phy_update_bits(xfi_tphy->base + 0x507c, 0x5000500, is_5g ? 0x4000400 :
> + 0x1000100);
> + mtk_phy_update_bits(xfi_tphy->base + 0x5080, 0x1410, is_1g ? 0x400 : is_5g ? 0x1010 : 0x0);
> + mtk_phy_update_bits(xfi_tphy->base + 0x5084, 0x30300, is_1g ? 0x30300 :
> + is_5g ? 0x30100 :
> + 0x100);
> + mtk_phy_update_bits(xfi_tphy->base + 0x5088, 0x60200, is_1g ? 0x20200 :
> + is_5g ? 0x40000 :
> + 0x20000);
> +
> + /* Setting RXFE adaptation range setting */
> + mtk_phy_update_bits(xfi_tphy->base + 0x50e4, 0xc0000, is_5g ? 0x0 : 0xc0000);
> + mtk_phy_update_bits(xfi_tphy->base + 0x50e8, 0x40000, is_5g ? 0x0 : 0x40000);
> + mtk_phy_update_bits(xfi_tphy->base + 0x50ec, 0xa00, is_1g ? 0x200 : 0x800);
> + mtk_phy_update_bits(xfi_tphy->base + 0x50a8, 0xee0000, is_5g ? 0x800000 :
> + 0x6e0000);
> + mtk_phy_update_bits(xfi_tphy->base + 0x6004, 0x190000, is_5g ? 0x0 : 0x190000);
> +
> + if (is_10g)
> + writel(0x01423342, xfi_tphy->base + 0x00f8);
> + else if (is_5g)
> + writel(0x00a132a1, xfi_tphy->base + 0x00f8);
> + else if (is_2p5g)
> + writel(0x009c329c, xfi_tphy->base + 0x00f8);
> + else
> + writel(0x00fa32fa, xfi_tphy->base + 0x00f8);
> +
> + /* Force SGDT_OUT off and select PCS */
> + mtk_phy_update_bits(xfi_tphy->base + REG_DIG_GLB_F4,
> + XFI_DPHY_AD_SGDT_FRC_EN | XFI_DPHY_PCS_SEL,
> + XFI_DPHY_AD_SGDT_FRC_EN |
> + (use_lynxi_pcs ? XFI_DPHY_PCS_SEL_SGMII :
> + XFI_DPHY_PCS_SEL_USXGMII));
> +
> + /* Force GLB_CKDET_OUT */
> + mtk_phy_set_bits(xfi_tphy->base + 0x0030, 0xc00);
> +
> + /* Force AEQ on */
> + writel(XTP_PCS_RX_EQ_IN_PROGRESS(2) | XTP_PCS_PWD_SYNC(2) | XTP_PCS_PWD_ASYNC(2),
> + xfi_tphy->base + REG_DIG_GLB_70);
> +
> + usleep_range(1, 5);
> +
> + /* Setup TX DA default value */
> + mtk_phy_update_bits(xfi_tphy->base + 0x30b0, 0x30, 0x20);
> + writel(0x00008a01, xfi_tphy->base + 0x3028);
> + writel(0x0000a884, xfi_tphy->base + 0x302c);
> + writel(0x00083002, xfi_tphy->base + 0x3024);
> +
> + /* Setup RG default value */
> + if (use_lynxi_pcs) {
> + writel(0x00011110, xfi_tphy->base + 0x3010);
> + writel(0x40704000, xfi_tphy->base + 0x3048);
> + } else {
> + writel(0x00022220, xfi_tphy->base + 0x3010);
> + writel(0x0f020a01, xfi_tphy->base + 0x5064);
> + writel(0x06100600, xfi_tphy->base + 0x50b4);
> + if (interface == PHY_INTERFACE_MODE_USXGMII)
> + writel(0x40704000, xfi_tphy->base + 0x3048);
> + else
> + writel(0x47684100, xfi_tphy->base + 0x3048);
> + }
> +
> + if (is_1g)
> + writel(0x0000c000, xfi_tphy->base + 0x3064);
> +
> + /* Setup RX EQ initial value */
> + mtk_phy_update_bits(xfi_tphy->base + 0x3050, 0xa8000000,
> + (interface != PHY_INTERFACE_MODE_10GBASER) ? 0xa8000000 : 0x0);
> + mtk_phy_update_bits(xfi_tphy->base + 0x3054, 0xaa,
> + (interface != PHY_INTERFACE_MODE_10GBASER) ? 0xaa : 0x0);
> +
> + if (!use_lynxi_pcs)
> + writel(0x00000f00, xfi_tphy->base + 0x306c);
> + else if (is_2p5g)
> + writel(0x22000f00, xfi_tphy->base + 0x306c);
> + else
> + writel(0x20200f00, xfi_tphy->base + 0x306c);
> +
> + mtk_phy_update_bits(xfi_tphy->base + 0xa008, 0x10000, da_war ? 0x10000 : 0x0);
> +
> + mtk_phy_update_bits(xfi_tphy->base + 0xa060, 0x50000, use_lynxi_pcs ? 0x50000 : 0x40000);
> +
> + /* Setup PHYA speed */
> + mtk_phy_update_bits(xfi_tphy->base + REG_ANA_GLB_D0,
> + XTP_GLB_USXGMII_SEL_MASK | XTP_GLB_USXGMII_EN,
> + is_10g ? XTP_GLB_USXGMII_SEL(0) :
> + is_5g ? XTP_GLB_USXGMII_SEL(1) :
> + is_2p5g ? XTP_GLB_USXGMII_SEL(2) :
> + XTP_GLB_USXGMII_SEL(3));
> + mtk_phy_set_bits(xfi_tphy->base + REG_ANA_GLB_D0, XTP_GLB_USXGMII_EN);
> +
> + /* Release reset */
> + mtk_phy_set_bits(xfi_tphy->base + REG_DIG_GLB_70,
> + XTP_PCS_RST_B | XTP_FRC_PCS_RST_B);
> + usleep_range(150, 500);
> +
> + /* Switch to P0 */
> + mtk_phy_update_bits(xfi_tphy->base + REG_DIG_GLB_70,
> + XTP_PCS_IN_FR_RG |
> + XTP_FRC_PCS_PWD_ASYNC |
> + XTP_PCS_PWD_ASYNC_MASK |
> + XTP_PCS_PWD_SYNC_MASK |
> + XTP_PCS_UPDT,
> + XTP_PCS_IN_FR_RG |
> + XTP_FRC_PCS_PWD_ASYNC |
> + XTP_PCS_UPDT);
> + usleep_range(1, 5);
> +
> + mtk_phy_clear_bits(xfi_tphy->base + REG_DIG_GLB_70, XTP_PCS_UPDT);
> + usleep_range(15, 50);
> +
> + if (use_lynxi_pcs) {
> + /* Switch to Gen2 */
> + mtk_phy_update_bits(xfi_tphy->base + REG_DIG_GLB_70,
> + XTP_PCS_MODE_MASK | XTP_PCS_UPDT,
> + XTP_PCS_MODE(1) | XTP_PCS_UPDT);
> + } else {
> + /* Switch to Gen3 */
> + mtk_phy_update_bits(xfi_tphy->base + REG_DIG_GLB_70,
> + XTP_PCS_MODE_MASK | XTP_PCS_UPDT,
> + XTP_PCS_MODE(2) | XTP_PCS_UPDT);
> + }
> + usleep_range(1, 5);
> +
> + mtk_phy_clear_bits(xfi_tphy->base + REG_DIG_GLB_70, XTP_PCS_UPDT);
> +
> + usleep_range(100, 500);
> +
> + /* Enable MAC CK */
> + mtk_phy_set_bits(xfi_tphy->base + REG_DIG_LN_TRX_B0, XTP_LN_TX_MACCK_EN);
> + mtk_phy_clear_bits(xfi_tphy->base + REG_DIG_GLB_F4, XFI_DPHY_AD_SGDT_FRC_EN);
> +
> + /* Enable TX data */
> + mtk_phy_set_bits(xfi_tphy->base + REG_DIG_LN_TRX_40,
> + XTP_LN_FRC_TX_DATA_EN | XTP_LN_TX_DATA_EN);
> + usleep_range(400, 1000);
> +}
> +
> +static int mtk_xfi_tphy_set_mode(struct phy *phy, enum phy_mode mode, int
> + submode)
> +{
> + struct mtk_xfi_tphy *xfi_tphy = phy_get_drvdata(phy);
> +
> + if (mode != PHY_MODE_ETHERNET)
> + return -EINVAL;
> +
> + switch (submode) {
> + case PHY_INTERFACE_MODE_1000BASEX:
> + case PHY_INTERFACE_MODE_2500BASEX:
> + case PHY_INTERFACE_MODE_SGMII:
> + case PHY_INTERFACE_MODE_5GBASER:
> + case PHY_INTERFACE_MODE_10GBASER:
> + case PHY_INTERFACE_MODE_USXGMII:
> + mtk_xfi_tphy_setup(xfi_tphy, submode);
> + return 0;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int mtk_xfi_tphy_reset(struct phy *phy)
> +{
> + struct mtk_xfi_tphy *xfi_tphy = phy_get_drvdata(phy);
> +
> + reset_control_assert(xfi_tphy->reset);
> + usleep_range(100, 500);
> + reset_control_deassert(xfi_tphy->reset);
> + usleep_range(1, 10);
> +
> + return 0;
> +}
> +
> +static int mtk_xfi_tphy_power_on(struct phy *phy)
> +{
> + struct mtk_xfi_tphy *xfi_tphy = phy_get_drvdata(phy);
> +
> + return clk_bulk_prepare_enable(MTK_XFI_TPHY_NUM_CLOCKS, xfi_tphy->clocks);
> +}
> +
> +static int mtk_xfi_tphy_power_off(struct phy *phy)
> +{
> + struct mtk_xfi_tphy *xfi_tphy = phy_get_drvdata(phy);
> +
> + clk_bulk_disable_unprepare(MTK_XFI_TPHY_NUM_CLOCKS, xfi_tphy->clocks);
> +
> + return 0;
> +}
> +
> +static const struct phy_ops mtk_xfi_tphy_ops = {
> + .power_on = mtk_xfi_tphy_power_on,
> + .power_off = mtk_xfi_tphy_power_off,
> + .set_mode = mtk_xfi_tphy_set_mode,
> + .reset = mtk_xfi_tphy_reset,
> + .owner = THIS_MODULE,
> +};
> +
> +static int mtk_xfi_tphy_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct phy_provider *phy_provider;
> + struct mtk_xfi_tphy *xfi_tphy;
> + struct phy *phy;
> + int ret;
> +
> + if (!np)
> + return -ENODEV;
> +
> + xfi_tphy = devm_kzalloc(&pdev->dev, sizeof(*xfi_tphy), GFP_KERNEL);
> + if (!xfi_tphy)
> + return -ENOMEM;
> +
> + xfi_tphy->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(xfi_tphy->base))
> + return PTR_ERR(xfi_tphy->base);
> +
> + xfi_tphy->dev = &pdev->dev;
> + xfi_tphy->clocks[0].id = "topxtal";
> + xfi_tphy->clocks[1].id = "xfipll";
> + ret = devm_clk_bulk_get(&pdev->dev, MTK_XFI_TPHY_NUM_CLOCKS, xfi_tphy->clocks);
> + if (ret)
> + return ret;
> +
> + xfi_tphy->reset = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> + if (IS_ERR(xfi_tphy->reset))
> + return PTR_ERR(xfi_tphy->reset);
> +
> + xfi_tphy->da_war = of_property_read_bool(np, "mediatek,usxgmii-performance-errata");
> +
> + phy = devm_phy_create(&pdev->dev, NULL, &mtk_xfi_tphy_ops);
> + if (IS_ERR(phy))
> + return PTR_ERR(phy);
> +
> + phy_set_drvdata(phy, xfi_tphy);
> + phy_provider = devm_of_phy_provider_register(&pdev->dev, of_phy_simple_xlate);
> +
> + return PTR_ERR_OR_ZERO(phy_provider);
> +}
> +
> +static const struct of_device_id mtk_xfi_tphy_match[] = {
> + { .compatible = "mediatek,mt7988-xfi-tphy", },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mtk_xfi_tphy_match);
> +
> +static struct platform_driver mtk_xfi_tphy_driver = {
> + .probe = mtk_xfi_tphy_probe,
> + .driver = {
> + .name = "mtk-xfi-tphy",
> + .of_match_table = mtk_xfi_tphy_match,
> + },
> +};
> +module_platform_driver(mtk_xfi_tphy_driver);
> +
> +MODULE_DESCRIPTION("MediaTek 10GE SerDes XFI T-PHY driver");
> +MODULE_AUTHOR("Daniel Golle <daniel@makrotopia.org>");
> +MODULE_AUTHOR("Bc-bocun Chen <bc-bocun.chen@mediatek.com>");
> +MODULE_LICENSE("GPL");
> --
> 2.43.0
--
~Vinod
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 18/23] dt-bindings: media: imx258: Add alternate compatible strings
From: Rob Herring @ 2024-03-28 18:55 UTC (permalink / raw)
To: git
Cc: linux-media, dave.stevenson, jacopo.mondi, mchehab,
krzysztof.kozlowski+dt, conor+dt, shawnguo, s.hauer, kernel,
festevam, sakari.ailus, devicetree, imx, linux-arm-kernel,
linux-kernel
In-Reply-To: <20240327231710.53188-19-git@luigi311.com>
On Wed, Mar 27, 2024 at 05:17:04PM -0600, git@luigi311.com wrote:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
>
> There are a number of variants of the imx258 modules that can not
> be differentiated at runtime, so add compatible strings for them.
But you are only adding 1 variant.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Signed-off-by: Luigi311 <git@luigi311.com>
> ---
> .../devicetree/bindings/media/i2c/sony,imx258.yaml | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml
> index bee61a443b23..c7856de15ba3 100644
> --- a/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml
> @@ -14,10 +14,14 @@ description: |-
> type stacked image sensor with a square pixel array of size 4208 x 3120. It
> is programmable through I2C interface. Image data is sent through MIPI
> CSI-2.
> + There are a number of variants of the sensor which cannot be detected at
> + runtime, so multiple compatible strings are required to differentiate these.
That's more reasoning/why for the patch than description of the h/w.
> properties:
> compatible:
> - const: sony,imx258
> + - enum:
> + - sony,imx258
> + - sony,imx258-pdaf
How do I know which one to use? Please define what PDAF means somewhere
as well as perhaps what the original/default variant is or isn't.
>
> assigned-clocks: true
> assigned-clock-parents: true
> --
> 2.42.0
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v9 00/13] firmware: qcom: qseecom: convert to using the TZ allocator
From: Bartosz Golaszewski @ 2024-03-28 18:55 UTC (permalink / raw)
To: Maximilian Luz
Cc: Andy Gross, Bjorn Andersson, Konrad Dybcio, Elliot Berman,
Krzysztof Kozlowski, Guru Das Srinagesh, Andrew Halaney,
Alex Elder, Srini Kandagatla, Arnd Bergmann, linux-arm-msm,
linux-kernel, linux-arm-kernel, kernel, Bartosz Golaszewski
In-Reply-To: <56e1c63a-4c09-4d92-9ef2-aad5390879cc@gmail.com>
On Thu, Mar 28, 2024 at 5:50 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>
> On 3/25/24 11:03 AM, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > SCM calls that take memory buffers as arguments require that they be
> > page-aligned, physically continuous and non-cachable. The same
> > requirements apply to the buffer used to pass additional arguments to SCM
> > calls that take more than 4.
> >
> > To that end drivers typically use dma_alloc_coherent() to allocate memory
> > of suitable format which is slow and inefficient space-wise.
> >
> > SHM Bridge is a safety mechanism that - once enabled - will only allow
> > passing buffers to the TrustZone that have been explicitly marked as
> > shared. It improves the overall system safety with SCM calls and is
> > required by the upcoming scminvoke functionality.
> >
> > The end goal of this series is to enable SHM bridge support for those
> > architectures that support it but to that end we first need to unify the
> > way memory for SCM calls is allocated. This in itself is beneficial as
> > the current approach of using dma_alloc_coherent() in most places is quite
> > slow.
> >
> > First let's add a new TZ Memory allocator that allows users to create
> > dynamic memory pools of format suitable for sharing with the TrustZone.
> > Make it ready for implementing multiple build-time modes.
> >
> > Convert all relevant drivers to using it. Add separate pools for SCM core
> > and for qseecom.
> >
> > Finally add support for SHM bridge and make it the default mode of
> > operation with the generic allocator as fallback for the platforms that
> > don't support SHM bridge.
> >
> > Tested on db410c, RB5, sm8550-qrd. Previous iteration tested also on
> > sa8775p-ride and lenovo X13s (please do retest on those platforms if you
> > can).
>
> Not sure in which version things changed (I haven't really kept up with
> that, sorry), but this version (with the generic allocator selected in
> the config) fails reading EFI vars on my Surface Pro X (sc8180x):
>
> [ 2.381020] BUG: scheduling while atomic: mount/258/0x00000002
> [ 2.383356] Modules linked in:
> [ 2.385669] Preemption disabled at:
> [ 2.385672] [<ffff800080f7868c>] qcom_tzmem_alloc+0x7c/0x224
> [ 2.390325] CPU: 1 PID: 258 Comm: mount Not tainted 6.8.0-1-surface-dev #2
> [ 2.392632] Hardware name: Microsoft Corporation Surface Pro X/Surface Pro X, BIOS 7.620.140 08/11/2023
> [ 2.394955] Call trace:
> [ 2.397269] dump_backtrace+0x94/0x114
> [ 2.399583] show_stack+0x18/0x24
> [ 2.401883] dump_stack_lvl+0x48/0x60
> [ 2.404181] dump_stack+0x18/0x24
> [ 2.406476] __schedule_bug+0x84/0xa0
> [ 2.408770] __schedule+0x6f4/0x7fc
> [ 2.411051] schedule+0x30/0xf0
> [ 2.413323] synchronize_rcu_expedited+0x158/0x1ec
> [ 2.415594] lru_cache_disable+0x28/0x74
> [ 2.417853] __alloc_contig_migrate_range+0x68/0x210
> [ 2.420122] alloc_contig_range+0x140/0x280
> [ 2.422384] cma_alloc+0x128/0x404
> [ 2.424643] cma_alloc_aligned+0x44/0x6c
> [ 2.426881] dma_alloc_contiguous+0x30/0x44
> [ 2.429111] __dma_direct_alloc_pages.isra.0+0x60/0x20c
> [ 2.431343] dma_direct_alloc+0x6c/0x2ec
> [ 2.433569] dma_alloc_attrs+0x80/0xf4
> [ 2.435786] qcom_tzmem_pool_add_memory+0x8c/0x178
> [ 2.438008] qcom_tzmem_alloc+0xe8/0x224
> [ 2.440232] qsee_uefi_get_next_variable+0x78/0x2cc
> [ 2.442443] qcuefi_get_next_variable+0x50/0x94
> [ 2.444643] efivar_get_next_variable+0x20/0x2c
> [ 2.446832] efivar_init+0x8c/0x29c
> [ 2.449009] efivarfs_fill_super+0xd4/0xec
> [ 2.451182] get_tree_single+0x74/0xbc
> [ 2.453349] efivarfs_get_tree+0x18/0x24
> [ 2.455513] vfs_get_tree+0x28/0xe8
> [ 2.457680] vfs_cmd_create+0x5c/0xf4
> [ 2.459840] __do_sys_fsconfig+0x458/0x598
> [ 2.461993] __arm64_sys_fsconfig+0x24/0x30
> [ 2.464143] invoke_syscall+0x48/0x110
> [ 2.466281] el0_svc_common.constprop.0+0x40/0xe0
> [ 2.468415] do_el0_svc+0x1c/0x28
> [ 2.470546] el0_svc+0x34/0xb4
> [ 2.472669] el0t_64_sync_handler+0x120/0x12c
> [ 2.474793] el0t_64_sync+0x190/0x194
>
> and subsequently
>
> [ 2.477613] DEBUG_LOCKS_WARN_ON(val > preempt_count())
> [ 2.477618] WARNING: CPU: 4 PID: 258 at kernel/sched/core.c:5889 preempt_count_sub+0x90/0xd4
> [ 2.478682] Modules linked in:
> [ 2.479214] CPU: 4 PID: 258 Comm: mount Tainted: G W 6.8.0-1-surface-dev #2
> [ 2.479752] Hardware name: Microsoft Corporation Surface Pro X/Surface Pro X, BIOS 7.620.140 08/11/2023
> [ 2.480296] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [ 2.480839] pc : preempt_count_sub+0x90/0xd4
> [ 2.481380] lr : preempt_count_sub+0x90/0xd4
> [ 2.481917] sp : ffff8000857cbb00
> [ 2.482450] x29: ffff8000857cbb00 x28: ffff8000806b759c x27: 8000000000000005
> [ 2.482988] x26: 0000000000000000 x25: ffff0000802cbaa0 x24: ffff0000809228b0
> [ 2.483525] x23: 0000000000000000 x22: ffff800082b462f0 x21: 0000000000001000
> [ 2.484062] x20: ffff80008363d000 x19: ffff000080922880 x18: fffffffffffc9660
> [ 2.484600] x17: 0000000000000020 x16: 0000000000000000 x15: 0000000000000038
> [ 2.485137] x14: 0000000000000000 x13: ffff800082649258 x12: 00000000000006db
> [ 2.485674] x11: 0000000000000249 x10: ffff8000826fc930 x9 : ffff800082649258
> [ 2.486207] x8 : 00000000ffffdfff x7 : ffff8000826f9258 x6 : 0000000000000249
> [ 2.486738] x5 : 0000000000000000 x4 : 40000000ffffe249 x3 : 0000000000000000
> [ 2.487267] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000841fa300
> [ 2.487792] Call trace:
> [ 2.488311] preempt_count_sub+0x90/0xd4
> [ 2.488831] _raw_spin_unlock_irqrestore+0x1c/0x44
> [ 2.489352] qcom_tzmem_alloc+0x1cc/0x224
> [ 2.489873] qsee_uefi_get_next_variable+0x78/0x2cc
> [ 2.490390] qcuefi_get_next_variable+0x50/0x94
> [ 2.490907] efivar_get_next_variable+0x20/0x2c
> [ 2.491420] efivar_init+0x8c/0x29c
> [ 2.491931] efivarfs_fill_super+0xd4/0xec
> [ 2.492440] get_tree_single+0x74/0xbc
> [ 2.492948] efivarfs_get_tree+0x18/0x24
> [ 2.493453] vfs_get_tree+0x28/0xe8
> [ 2.493957] vfs_cmd_create+0x5c/0xf4
> [ 2.494459] __do_sys_fsconfig+0x458/0x598
> [ 2.494963] __arm64_sys_fsconfig+0x24/0x30
> [ 2.495468] invoke_syscall+0x48/0x110
> [ 2.495972] el0_svc_common.constprop.0+0x40/0xe0
> [ 2.496475] do_el0_svc+0x1c/0x28
> [ 2.496976] el0_svc+0x34/0xb4
> [ 2.497475] el0t_64_sync_handler+0x120/0x12c
> [ 2.497975] el0t_64_sync+0x190/0x194
> [ 2.498466] ---[ end trace 0000000000000000 ]---
> [ 2.507347] qcom_scm firmware:scm: qseecom: scm call failed with error -22
> [ 2.507813] efivars: get_next_variable: status=8000000000000007
>
> If I understand correctly, it enters an atomic section in
> qcom_tzmem_alloc() and then tries to schedule somewhere down the line.
> So this shouldn't be qseecom specific.
>
> I should probably also say that I'm currently testing this on a patched
> v6.8 kernel, so there's a chance that it's my fault. However, as far as
> I understand, it enters an atomic section in qcom_tzmem_alloc() and then
> later tries to expand the pool memory with dma_alloc_coherent(). Which
> AFAIK is allowed to sleep with GFP_KERNEL (and I guess that that's the
> issue here).
>
> I've also tried the shmem allocator option, but that seems to get stuck
> quite early at boot, before I even have usb-serial access to get any
> logs. If I can find some more time, I'll try to see if I can get some
> useful output for that.
>
Ah, I think it happens here:
+ guard(spinlock_irqsave)(&pool->lock);
+
+again:
+ vaddr = gen_pool_alloc(pool->genpool, size);
+ if (!vaddr) {
+ if (qcom_tzmem_try_grow_pool(pool, size, gfp))
+ goto again;
We were called with GFP_KERNEL so this is what we pass on to
qcom_tzmem_try_grow_pool() but we're now holding the spinlock. I need
to revisit it. Thanks for the catch!
Bart
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 06/13] mm/gup: Drop folio_fast_pin_allowed() in hugepd processing
From: Andrew Morton @ 2024-03-28 19:01 UTC (permalink / raw)
To: David Hildenbrand
Cc: peterx, linux-mm, linux-kernel, Yang Shi, Kirill A . Shutemov,
Mike Kravetz, John Hubbard, Michael Ellerman, Andrew Jones,
Muchun Song, linux-riscv, linuxppc-dev, Christophe Leroy,
Christoph Hellwig, Lorenzo Stoakes, Matthew Wilcox, Rik van Riel,
linux-arm-kernel, Andrea Arcangeli, Aneesh Kumar K . V,
Vlastimil Babka, James Houghton, Jason Gunthorpe, Mike Rapoport,
Axel Rasmussen
In-Reply-To: <372a9331-6d95-4083-9a8f-a4f714868bea@redhat.com>
On Thu, 28 Mar 2024 11:10:59 +0100 David Hildenbrand <david@redhat.com> wrote:
> @Andrew, you properly adjusted the code to remove the
> gup_fast_folio_allowed() call instead of the folio_fast_pin_allowed()
> call, but
>
> (1) the commit subject
> (2) comment for gup_huge_pd()
>
> Still mention folio_fast_pin_allowed().
>
> The patch "mm/gup: handle hugepd for follow_page()" then moves that
> (outdated) comment.
OK, thanks, I fixed all that up.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [RFC] KVM: arm64: improving IO performance during unmap?
From: Krister Johansen @ 2024-03-28 19:04 UTC (permalink / raw)
To: Marc Zyngier, Oliver Upton
Cc: James Morse, Suzuki K Poulose, Zenghui Yu, Catalin Marinas,
Will Deacon, Ali Saidi, David Reaver, linux-arm-kernel, kvmarm,
linux-kernel
Hi,
Ali and I have been looking into ways to reduce the impact a unmap_stage2_range
operation has on IO performance when a device interrupt shares the cpu where the
unmap operation is occurring.
This came to our attention after porting a container VM / hardware virtualized
containers workload to arm64 from x86_64. On ARM64, the unmap operations took
longer. kvm_tlb_flush_vmid_ipa runs with interrupts disabled. Unmaps that don't
check for reschedule promptly may delay the IO.
One approach that we investigated was to modify the deferred TLBI code to run
even if range based operations were not supported. (Provided FWB is enabled).
If range based operations were supported, the code would use them. However, if
the CPU didn't support FEAT_TLBIRANGE or the unmap was larger than a certain
size, we'd fall back to vmalls12e1is instead. This reduced the performance
impact of the unmap operation to less than 5% impact on IO performance.
However, with Will's recent patches[1] to fix cases where free'd PTEs may still
be referenced, we were concerned this might not be a viable approach.
As a follow-up to this e-mail, I'm sending a patch for a different approach. It
shrinks the stage2_apply_range batch size to the minimum block size instead of
the maximum block size. This eliminates the IO performance regressions, but
increases the overall map / unmap operation times when the CPU is receiving IO
interrupts. I'm unsure if this is the optimal solution, however, since it may
generate extra unmap walks on 1gb hugepages. I'm also unclear if this creates
problems for any of the other users of stage2_apply_range().
I'd love to get some feedback on the best way to proceed here.
Thanks,
-K
[1] https://lore.kernel.org/kvmarm/20240325185158.8565-1-will@kernel.org/
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH] KVM: arm64: Limit stage2_apply_range() batch size to smallest block
From: Krister Johansen @ 2024-03-28 19:05 UTC (permalink / raw)
To: Marc Zyngier, Oliver Upton
Cc: James Morse, Suzuki K Poulose, Zenghui Yu, Catalin Marinas,
Will Deacon, Ali Saidi, David Reaver, linux-arm-kernel, kvmarm,
linux-kernel
In-Reply-To: <cover.1711649501.git.kjlx@templeofstupid.com>
stage2_apply_range() for unmap operations can interfere with the
performance of IO if the device's interrupts share the CPU where the
unmap operation is occurring. commit 5994bc9e05c2 ("KVM: arm64: Limit
stage2_apply_range() batch size to largest block") improved this. Prior
to that commit, workloads that were unfortunate enough to have their IO
interrupts pinned to the same CPU as the unmap operation would observe a
complete stall. With the switch to using the largest block size, it is
possible for IO to make progress, albeit at a reduced speed.
This author tested network and storage where the interrupts were pinned
to the same CPU where an unmap was occurring and found that throughput
was reduced about 4.75-5.8x for networking, and 65.5x-500x for storage.
The use-case where this has been especially painful is with hardware
virtualized containers. Many containers have a short lifetime and may
be run on systems where the host is intentionally oversubscribed. This
limits the options for pinning and prefaulting. Although NIC interrupts
allow their CPU affinity to be altered, some NVMe devices do not permit
it. Some cloud-block storage devices have only a few queues, which
means unlucky placement can have high performance impact.
Further reducing the stage2_apply_range() batch size has substantial
performance improvements for IO that share a CPU performing an unmap
operation. By switching to a 2mb chunk, IO performance regressions were
no longer observed in this author's tests. E.g. it was possible to
obtain the advertised device throughput despite an unmap operation
occurring on the CPU where the interrupt was running. There is a
tradeoff, however. No changes were observed in per-operation timings
when running the kvm_pagetable_test without an interrupt load. However,
with a 64gb VM, 1 vcpu, and 4k pages and a IO load, map times increased
by about 15% and unmap times increased by about 58%. In essence, this
trades slower map/unmap times for improved IO throughput.
This introduces KVM_PGTABLE_MAX_BLOCK_LEVEL, and then uses it to limit
the size of stage2_apply_range() chunks to the smallest size that's
addressable via a block mapping -- 2mb on a 4k granule size.
Cc: <stable@vger.kernel.org> # 5.15.x: 3b5c082bbfa2: KVM: arm64: Work out supported block level at compile time
Cc: <stable@vger.kernel.org> # 5.15.x: 5994bc9e05c2: KVM: arm64: Limit stage2_apply_range() batch size to largest block
Cc: <stable@vger.kernel.org> # 5.15.x
Suggested-by: Ali Saidi <alisaidi@amazon.com>
Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
---
arch/arm64/include/asm/kvm_pgtable.h | 4 ++++
arch/arm64/kvm/mmu.c | 2 +-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
index 19278dfe7978..b0c4651a4d9a 100644
--- a/arch/arm64/include/asm/kvm_pgtable.h
+++ b/arch/arm64/include/asm/kvm_pgtable.h
@@ -19,11 +19,15 @@
* - 4K (level 1): 1GB
* - 16K (level 2): 32MB
* - 64K (level 2): 512MB
+ *
+ * The max block level is the _smallest_ supported block size for KVM.
*/
#ifdef CONFIG_ARM64_4K_PAGES
#define KVM_PGTABLE_MIN_BLOCK_LEVEL 1
+#define KVM_PGTABLE_MAX_BLOCK_LEVEL 2
#else
#define KVM_PGTABLE_MIN_BLOCK_LEVEL 2
+#define KVM_PGTABLE_MAX_BLOCK_LEVEL KVM_PGTABLE_MIN_BLOCK_LEVEL
#endif
#define kvm_lpa2_is_enabled() system_supports_lpa2()
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index dc04bc767865..1e927b306aee 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -41,7 +41,7 @@ static phys_addr_t __stage2_range_addr_end(phys_addr_t addr, phys_addr_t end,
static phys_addr_t stage2_range_addr_end(phys_addr_t addr, phys_addr_t end)
{
- phys_addr_t size = kvm_granule_size(KVM_PGTABLE_MIN_BLOCK_LEVEL);
+ phys_addr_t size = kvm_granule_size(KVM_PGTABLE_MAX_BLOCK_LEVEL);
return __stage2_range_addr_end(addr, end, size);
}
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 1/2] dt-bindings: arm: ti: Add BeagleY-AI
From: Robert Nelson @ 2024-03-28 19:12 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel, devicetree
Cc: Robert Nelson, Rob Herring, Nishanth Menon, Jared McArthur,
Jason Kridner, Deepak Khatri
This board is based on ti,j722s
https://beagley-ai.org/
https://openbeagle.org/beagley-ai/beagley-ai
Signed-off-by: Robert Nelson <robertcnelson@gmail.com>
CC: Rob Herring <robh@kernel.org>
CC: Nishanth Menon <nm@ti.com>
CC: Jared McArthur <j-mcarthur@ti.com>
CC: Jason Kridner <jkridner@beagleboard.org>
CC: Deepak Khatri <lorforlinux@beagleboard.org>
---
Documentation/devicetree/bindings/arm/ti/k3.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/arm/ti/k3.yaml b/Documentation/devicetree/bindings/arm/ti/k3.yaml
index 52b51fd7044e..ca23b7e6a35e 100644
--- a/Documentation/devicetree/bindings/arm/ti/k3.yaml
+++ b/Documentation/devicetree/bindings/arm/ti/k3.yaml
@@ -134,6 +134,7 @@ properties:
- description: K3 J722S SoC and Boards
items:
- enum:
+ - beagle,j722s-beagley-ai
- ti,j722s-evm
- const: ti,j722s
--
2.39.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox