Received-SPF: pass (google.com: domain of linux-kernel+bounces-97917-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240305110915.184868-1-jtornosm@redhat.com>
In-Reply-To: <20240305110915.184868-1-jtornosm@redhat.com>
From: Masahiro Yamada <masahiroy@kernel.org>
Date: Sat, 9 Mar 2024 23:24:29 +0900
Message-ID: <CAK7LNAQ6_kr0Q1RB0dELiGUObFJ4HEEu3XTErGf6FaNntKMnTg@mail.gmail.com>
Subject: Re: [PATCH] kbuild: rpm-pkg: add dtb files in kernel rpm
To: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>
Cc: nathan@kernel.org, nicolas@fjasle.eu, linux-kbuild@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Mar 5, 2024 at 8:09=E2=80=AFPM Jose Ignacio Tornos Martinez
<jtornosm@redhat.com> wrote:
>
> Some architectures, like aarch64 ones, need a dtb file to configure the
> hardware. The default dtb file can be preloaded from u-boot, but the fina=
l
> and/or more complete dtb file needs to be able to be loaded later from
> rootfs.
>
> Add the possible dtb files to the kernel rpm and mimic Fedora shipping
> process, storing the dtb files in the module directory. These dtb files
> will be copied to /boot directory by the install scripts, but add fallbac=
k
> just in case, checking if the content in /boot directory is correct.
>
> Mark the files installed to /boot as %ghost to make sure they will be
> removed when the package is uninstalled.
>
> Tested with Fedora Rawhide (x86_64 and aarch64) with dnf and rpm tools.
> In addition, fallback was also tested after modifying the install scripts=
.
>
> Signed-off-by: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>
> ---
>  scripts/package/kernel.spec | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/scripts/package/kernel.spec b/scripts/package/kernel.spec
> index c256b73cca3e..5c4c1ffcba6c 100644
> --- a/scripts/package/kernel.spec
> +++ b/scripts/package/kernel.spec
> @@ -61,6 +61,9 @@ cp $(%{make} %{makeflags} -s image_name) %{buildroot}/l=
ib/modules/%{KERNELRELEAS
>  %{make} %{makeflags} INSTALL_HDR_PATH=3D%{buildroot}/usr headers_install
>  cp System.map %{buildroot}/lib/modules/%{KERNELRELEASE}
>  cp .config %{buildroot}/lib/modules/%{KERNELRELEASE}/config
> +if [ -d "${srctree}/arch/${SRCARCH}/boot/dts" ];then


${srctree} and ${SRCARCH} are not always defined in kernel.spec.


They are not defined if you directly run 'rpmbuild -rb <SRPM>'.


The following will correctly check the presence of
${srctree}/arch/${SRCARCH}/boot/dts.


if %{make} %{makeflags} run-command KBUILD_RUN_COMMAND=3D'test -d
${srctree}/arch/${SRCARCH}/boot/dts'; then


> +       %{make} %{makeflags} INSTALL_DTBS_PATH=3D%{buildroot}/lib/modules=
/%{KERNELRELEASE}/dtb dtbs_install
> +fi
>  ln -fns /usr/src/kernels/%{KERNELRELEASE} %{buildroot}/lib/modules/%{KER=
NELRELEASE}/build
>  %if %{with_devel}
>  %{make} %{makeflags} run-command KBUILD_RUN_COMMAND=3D'${srctree}/script=
s/package/install-extmod-build %{buildroot}/usr/src/kernels/%{KERNELRELEASE=
}'
> @@ -81,6 +84,14 @@ ln -fns /usr/src/kernels/%{KERNELRELEASE} %{buildroot}=
/lib/modules/%{KERNELRELEA
>                 echo "%ghost /boot/${x}-%{KERNELRELEASE}"
>         done
>
> +       if [ -d "%{buildroot}/lib/modules/%{KERNELRELEASE}/dtb" ];then
> +               echo "/lib/modules/%{KERNELRELEASE}/dtb"
> +               for x in $(find "%{buildroot}/lib/modules/%{KERNELRELEASE=
}/dtb" -name "*" \
> +                               -exec realpath --relative-to "%{buildroot=
}/lib/modules/%{KERNELRELEASE}/dtb" {} \;); do
> +                       echo "%ghost /boot/dtb-%{KERNELRELEASE}/${x}"
> +               done


Instead of the for-loop, you can write it in one command:


  find "%{buildroot}/lib/modules/%{KERNELRELEASE}/dtb" -printf
"%%%ghost /boot/dtb-%{KERNELRELEASE}/%%P\n"


Is the escaping % complex?


> +       fi
> +
>         echo "%exclude /lib/modules/%{KERNELRELEASE}/build"
>  } > %{buildroot}/kernel.list
>
> @@ -96,6 +107,12 @@ for file in vmlinuz System.map config; do
>                 cp "/lib/modules/%{KERNELRELEASE}/${file}" "/boot/${file}=
-%{KERNELRELEASE}"
>         fi
>  done
> +if [ -d "/lib/modules/%{KERNELRELEASE}/dtb" ]; then
> +       if ! diff -rq "/lib/modules/%{KERNELRELEASE}/dtb" "/boot/dtb-%{KE=
RNELRELEASE}" &>/dev/null; then


I do not recommend the bash'ism "&>/dev/null".

The scriptlet is executed by /bin/sh, which
may not be bash, although on most RPM-based
distro, it is a symlink to bash.


You can squash the nested if-conditionals:


if [ -d "/lib/modules/%{KERNELRELEASE}/dtb" ] && \
   ! diff -rq "/lib/modules/%{KERNELRELEASE}/dtb"
"/boot/dtb-%{KERNELRELEASE}" >/dev/null 2>&1; then
        ...
fi


> +               rm -rf "/boot/dtb-%{KERNELRELEASE}"
> +               cp -r "/lib/modules/%{KERNELRELEASE}/dtb" "/boot/dtb-%{KE=
RNELRELEASE}"
> +       fi
> +fi
>  if [ ! -e "/lib/modules/%{KERNELRELEASE}/modules.dep" ]; then
>         /usr/sbin/depmod %{KERNELRELEASE}
>  fi
> --
> 2.43.2
>
>


--
Best Regards


Masahiro Yamada