Skip to content

kernelci.test: match configs by dtb basename #2117

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 18, 2023

Conversation

jluebbe
Copy link
Contributor

@jluebbe jluebbe commented Sep 27, 2023

Since https://git.kernel.org/linus/724ba6751532055db75992fc6ae21c3e322e94a7, the ARM dts files were moved to subdirectories and the configs do not match any more, which results in boards not receiving test jobs for kernels since that commit.

Instead of adding the new directory prefix to every config (which would cause problems with testing stable trees), only compare the basename (and assume that it's unique).

Since https://git.kernel.org/linus/724ba6751532055db75992fc6ae21c3e322e94a7,
the ARM dts files were moved to subdirectories and the configs do not
match any more, which results in boards not receiving test jobs for
kernels since that commit.

Instead of adding the new directory prefix to every config (which would
cause problems with testing stable trees), only compare the basename
(and assume that it's unique).

Signed-off-by: Jan Luebbe <jlu@pengutronix.de>
@jluebbe
Copy link
Contributor Author

jluebbe commented Sep 27, 2023

I'm not sure at all if this actually fixes the issue. :)

@gctucker
Copy link
Contributor

Thanks for looking into this :)

I'm not sure at all if this actually fixes the issue. :)

Yes I think this won't quite work because in some cases, the alternative device types were set up to match another device tree name due to some renames. Maybe the best thing to do here is just to explicitly define the dtb for each device type and remove the default implementation if it can't be automatically deduced in a predictable way. The arm64 logic uses the mach name I believe, if we could do something like this it might work for arm too.

@jluebbe
Copy link
Contributor Author

jluebbe commented Sep 29, 2023

Yes I think this won't quite work because in some cases, the alternative device types were set up to match another device tree name due to some renames.

Do you have an example for that case?

Maybe the best thing to do here is just to explicitly define the dtb for each device type and remove the default implementation if it can't be automatically deduced in a predictable way. The arm64 logic uses the mach name I believe, if we could do something like this it might work for arm too.

I believe the DTB basenames are unique, so I'd prefer to just use those instead of introducing an indirect mapping.

@jluebbe
Copy link
Contributor Author

jluebbe commented Sep 29, 2023

I checked for duplicates:

% find -name '*.dts' -printf '%f\n' | sort | uniq -d | xargs -n1 find -name
./arch/arm/boot/dts/broadcom/bcm2711-rpi-4-b.dts
./arch/arm64/boot/dts/broadcom/bcm2711-rpi-4-b.dts
./arch/arm/boot/dts/broadcom/bcm2711-rpi-400.dts
./arch/arm64/boot/dts/broadcom/bcm2711-rpi-400.dts
./arch/arm/boot/dts/broadcom/bcm2711-rpi-cm4-io.dts
./arch/arm64/boot/dts/broadcom/bcm2711-rpi-cm4-io.dts
./arch/arm/boot/dts/broadcom/bcm2837-rpi-3-a-plus.dts
./arch/arm64/boot/dts/broadcom/bcm2837-rpi-3-a-plus.dts
./arch/arm/boot/dts/broadcom/bcm2837-rpi-3-b-plus.dts
./arch/arm64/boot/dts/broadcom/bcm2837-rpi-3-b-plus.dts
./arch/arm/boot/dts/broadcom/bcm2837-rpi-3-b.dts
./arch/arm64/boot/dts/broadcom/bcm2837-rpi-3-b.dts
./arch/arm/boot/dts/broadcom/bcm2837-rpi-cm3-io3.dts
./arch/arm64/boot/dts/broadcom/bcm2837-rpi-cm3-io3.dts
./arch/arm/boot/dts/broadcom/bcm2837-rpi-zero-2-w.dts
./arch/arm64/boot/dts/broadcom/bcm2837-rpi-zero-2-w.dts

So the only "duplicate" base names are the RPi's. Actually, they are the same DT, as they include the arm variant:

% cat ./arch/arm64/boot/dts/broadcom/bcm*.dts           
// SPDX-License-Identifier: GPL-2.0
#include "arm/broadcom/bcm2711-rpi-4-b.dts"
// SPDX-License-Identifier: GPL-2.0
#include "arm/broadcom/bcm2711-rpi-400.dts"
// SPDX-License-Identifier: GPL-2.0
#include "arm/broadcom/bcm2711-rpi-cm4-io.dts"
// SPDX-License-Identifier: GPL-2.0
#include "arm/broadcom/bcm2837-rpi-3-a-plus.dts"
// SPDX-License-Identifier: GPL-2.0
#include "arm/broadcom/bcm2837-rpi-3-b-plus.dts"
// SPDX-License-Identifier: GPL-2.0
#include "arm/broadcom/bcm2837-rpi-3-b.dts"
// SPDX-License-Identifier: GPL-2.0
#include "arm/broadcom/bcm2837-rpi-cm3-io3.dts"
// SPDX-License-Identifier: GPL-2.0
#include "arm/broadcom/bcm2837-rpi-zero-2-w.dts"

@jluebbe
Copy link
Contributor Author

jluebbe commented Oct 12, 2023

To see the issue, take a look at the test reports for v6.6-rc4 vs stable v5.10.197. The latter has many results for imx and beagles, while the former has none.

@broonie
Copy link
Member

broonie commented Oct 12, 2023

The change looks sensible to me and it looks like the concerns @gctucker raised were answered? I'm not seeing jobs come through for my lab for staging though - jluebbe is in data/staging.ini so I guess there might be some conflict during the merge to produce staging (perhaps just a rebase is needed)? This is a pretty big miss for coverage so it'd be good to get it resolved quickly.

@broonie
Copy link
Member

broonie commented Oct 12, 2023

Even if there's remaining issues due to aliasing set up for renames resulting in some things getting missed we'd still be better off if the simple cases start working again.

@nuclearcat
Copy link
Member

I will take care about it and will check staging tomorrow

@nuclearcat nuclearcat added this pull request to the merge queue Oct 18, 2023
Merged via the queue into kernelci:main with commit 970c198 Oct 18, 2023
@jluebbe jluebbe deleted the dtb-basename-fix branch October 19, 2023 13:19
@jluebbe
Copy link
Contributor Author

jluebbe commented Oct 21, 2023

Is this deployed to the main instance already?

@nuclearcat
Copy link
Member

Not yet, deploy will be completed today (each Monday).

@jluebbe
Copy link
Contributor Author

jluebbe commented Oct 23, 2023

It seems that my change was not enough. We're now receiving jobs for arm32 board again, but they use a wrong URL for the DTB file: https://hekla.openlab.pengutronix.de/scheduler/job/1004156

I think it's not using the paths from meta.get_single_artifact('dtbs', attr='contents') when actually creating the test job. Instead of

http://storage.staging.kernelci.org/kernelci/staging-next/staging-next-20231023.2/arm/multi_v7_defconfig/gcc-10/dtbs/nxp/imx/imx6dl-riotboard.dtb

it's trying

http://storage.staging.kernelci.org/kernelci/staging-next/staging-next-20231023.2/arm/multi_v7_defconfig/gcc-10/dtbs/imx6dl-riotboard.dtb

which is missing nxp/imx/.

@jluebbe
Copy link
Contributor Author

jluebbe commented Oct 25, 2023

@nuclearcat The test jobs are now generated on the main instance as well. And we see the same issue with finding the DTB files: https://hekla.openlab.pengutronix.de/scheduler/job/1004420

I'm not sure where to correct place to fix this is.

@nuclearcat
Copy link
Member

Probably i have to revert this and implement #1941

@jluebbe
Copy link
Contributor Author

jluebbe commented Oct 25, 2023

Probably i have to revert this and implement #1941

Even if multiple DTBs would be allowed per device, it the job would still need to refer to the correct one. As far as I understand it should be the matching entry from meta.get_single_artifact('dtbs', attr='contents'), which includes the subdirectories.

@jluebbe
Copy link
Contributor Author

jluebbe commented Oct 26, 2023

Looking a bit through the source... couldn't it be done by adjusting kernelci.test.get_params() to find the DTB path by basename? It's unique per build.

@jluebbe
Copy link
Contributor Author

jluebbe commented Nov 9, 2023

A follow-up PR is in #2180.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants