| Message ID | 20210122162808.75289-2-stefan.bader@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,F,01/02] UBUNTU: [Packaging] Add support for ODM drivers | expand |
Can you give us an explanation regarding the purpose of this and what ODM stands for? I have other comments bellow: On Fri, Jan 22, 2021 at 05:28:07PM +0100, Stefan Bader wrote: > BugLink: https://bugs.launchpad.net/bugs/1912789 > > We want to be able to selectively turn on ODM driver support for those > kernels/arches we have to but otherwise not inherit this to other > derivatives. This is done by a new config option which we will have to > depend on in the new drivers config options. Support is toggled by > changing a makefile rule variable. > The logic is a bit reversed. The common config has it enabled but it > will be turn off for builds unless one asks for it to be on. Example > of that will be in a follow-up patch. > > Signed-off-by: Stefan Bader <stefan.bader@canonical.com> > --- > debian.master/config/config.common.ubuntu | 1 + > debian/rules.d/0-common-vars.mk | 4 ++++ > debian/rules.d/1-maintainer.mk | 1 + > debian/rules.d/2-binary-arch.mk | 3 +++ > ubuntu/Kconfig | 6 ++++++ > 5 files changed, 15 insertions(+) > > diff --git a/debian.master/config/config.common.ubuntu b/debian.master/config/config.common.ubuntu > index 75cc55c2d321..00fae58b1ade 100644 > --- a/debian.master/config/config.common.ubuntu > +++ b/debian.master/config/config.common.ubuntu > @@ -10362,6 +10362,7 @@ CONFIG_UBIFS_FS_ZLIB=y > CONFIG_UBIFS_FS_ZSTD=y > # CONFIG_UBSAN is not set > CONFIG_UBSAN_ALIGNMENT=y > +CONFIG_UBUNTU_ODM_DRIVERS=y > CONFIG_UCB1400_CORE=m > CONFIG_UCLAMP_BUCKETS_COUNT=5 > CONFIG_UCLAMP_TASK=y > diff --git a/debian/rules.d/0-common-vars.mk b/debian/rules.d/0-common-vars.mk > index 5394b7c96f82..b71546ab03a0 100644 > --- a/debian/rules.d/0-common-vars.mk > +++ b/debian/rules.d/0-common-vars.mk > @@ -187,6 +187,10 @@ do_common_headers_indep=true > # add a 'full source' mode > do_full_source=false > > +# Add an option to enable special drivers which should only be build when > +# explicitly enabled. > +do_odm_drivers=false > + > # build tools > ifneq ($(wildcard $(CURDIR)/tools),) > ifeq ($(do_tools),) > diff --git a/debian/rules.d/1-maintainer.mk b/debian/rules.d/1-maintainer.mk > index 956829027e0f..fabdf0888fce 100644 > --- a/debian/rules.d/1-maintainer.mk > +++ b/debian/rules.d/1-maintainer.mk > @@ -86,6 +86,7 @@ printenv: > @echo "do_flavour_header_package = $(do_flavour_header_package)" > @echo "do_common_headers_indep = $(do_common_headers_indep)" > @echo "do_full_source = $(do_full_source)" > + @echo "do_odm_drivers = $(do_odm_drivers)" > @echo "do_tools = $(do_tools)" > @echo "do_any_tools = $(do_any_tools)" > @echo "do_linux_tools = $(do_linux_tools)" > diff --git a/debian/rules.d/2-binary-arch.mk b/debian/rules.d/2-binary-arch.mk > index 703fd2ade338..6311c9a74e05 100644 > --- a/debian/rules.d/2-binary-arch.mk > +++ b/debian/rules.d/2-binary-arch.mk > @@ -31,6 +31,9 @@ $(stampdir)/stamp-prepare-tree-%: $(commonconfdir)/config.common.$(family) $(arc > [ "$(do_full_source)" != 'true' ] && true || \ > rsync -a --exclude debian --exclude debian.master --exclude $(DEBIAN) * $(builddir)/build-$* > cat $(wordlist 1,3,$^) | sed -e 's/.*CONFIG_VERSION_SIGNATURE.*/CONFIG_VERSION_SIGNATURE="Ubuntu $(release)-$(revision)-$* $(raw_kernelversion)"/' > $(builddir)/build-$*/.config > + [ "$(do_odm_drivers)" = 'true' ] && true || \ > + sed -ie 's/.*CONFIG_UBUNTU_ODM_DRIVERS.*/# CONFIG_UBUNTU_ODM_DRIVERS is not set/' \ > + $(builddir)/build-$*/.config So if I understood that correctly, derivatives will have to set the config option to "y" *and* do_odm_drivers=true if they want to opt in. Is that really necessary? I'm probably missing something, but why is do_odm_drivers even necessary? > find $(builddir)/build-$* -name "*.ko" | xargs rm -f > $(build_cd) $(kmake) $(build_O) -j1 syncconfig prepare scripts > touch $@ > diff --git a/ubuntu/Kconfig b/ubuntu/Kconfig > index 8cea998f29a3..d969907b0bfd 100644 > --- a/ubuntu/Kconfig > +++ b/ubuntu/Kconfig > @@ -1,5 +1,11 @@ > menu "Ubuntu Supplied Third-Party Device Drivers" > > + > +config UBUNTU_ODM_DRIVERS > +bool "Enable support for ODM drivers" That will cause all derivatives to be prompted about this config. Is that what you are planning? > +help > + Turn on support for Ubuntu ODM supplied drivers A better description here would be welcomed. What's ODM? > + > # > # NOTE: to allow drivers to be added and removed without causing merge > # collisions you should add new entries in the middle of the six lines > -- > 2.25.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
Hi, Stefan. I'm attaching one alternative that should work in a similar way using do_odm_drivers=true without forcing every derivative to include the new config option. On Fri, Jan 22, 2021 at 02:34:40PM -0300, Marcelo Henrique Cerri wrote: > Can you give us an explanation regarding the purpose of this and what > ODM stands for? > > I have other comments bellow: > > On Fri, Jan 22, 2021 at 05:28:07PM +0100, Stefan Bader wrote: > > BugLink: https://bugs.launchpad.net/bugs/1912789 > > > > We want to be able to selectively turn on ODM driver support for those > > kernels/arches we have to but otherwise not inherit this to other > > derivatives. This is done by a new config option which we will have to > > depend on in the new drivers config options. Support is toggled by > > changing a makefile rule variable. > > The logic is a bit reversed. The common config has it enabled but it > > will be turn off for builds unless one asks for it to be on. Example > > of that will be in a follow-up patch. > > > > Signed-off-by: Stefan Bader <stefan.bader@canonical.com> > > --- > > debian.master/config/config.common.ubuntu | 1 + > > debian/rules.d/0-common-vars.mk | 4 ++++ > > debian/rules.d/1-maintainer.mk | 1 + > > debian/rules.d/2-binary-arch.mk | 3 +++ > > ubuntu/Kconfig | 6 ++++++ > > 5 files changed, 15 insertions(+) > > > > diff --git a/debian.master/config/config.common.ubuntu b/debian.master/config/config.common.ubuntu > > index 75cc55c2d321..00fae58b1ade 100644 > > --- a/debian.master/config/config.common.ubuntu > > +++ b/debian.master/config/config.common.ubuntu > > @@ -10362,6 +10362,7 @@ CONFIG_UBIFS_FS_ZLIB=y > > CONFIG_UBIFS_FS_ZSTD=y > > # CONFIG_UBSAN is not set > > CONFIG_UBSAN_ALIGNMENT=y > > +CONFIG_UBUNTU_ODM_DRIVERS=y > > CONFIG_UCB1400_CORE=m > > CONFIG_UCLAMP_BUCKETS_COUNT=5 > > CONFIG_UCLAMP_TASK=y > > diff --git a/debian/rules.d/0-common-vars.mk b/debian/rules.d/0-common-vars.mk > > index 5394b7c96f82..b71546ab03a0 100644 > > --- a/debian/rules.d/0-common-vars.mk > > +++ b/debian/rules.d/0-common-vars.mk > > @@ -187,6 +187,10 @@ do_common_headers_indep=true > > # add a 'full source' mode > > do_full_source=false > > > > +# Add an option to enable special drivers which should only be build when > > +# explicitly enabled. > > +do_odm_drivers=false > > + > > # build tools > > ifneq ($(wildcard $(CURDIR)/tools),) > > ifeq ($(do_tools),) > > diff --git a/debian/rules.d/1-maintainer.mk b/debian/rules.d/1-maintainer.mk > > index 956829027e0f..fabdf0888fce 100644 > > --- a/debian/rules.d/1-maintainer.mk > > +++ b/debian/rules.d/1-maintainer.mk > > @@ -86,6 +86,7 @@ printenv: > > @echo "do_flavour_header_package = $(do_flavour_header_package)" > > @echo "do_common_headers_indep = $(do_common_headers_indep)" > > @echo "do_full_source = $(do_full_source)" > > + @echo "do_odm_drivers = $(do_odm_drivers)" > > @echo "do_tools = $(do_tools)" > > @echo "do_any_tools = $(do_any_tools)" > > @echo "do_linux_tools = $(do_linux_tools)" > > diff --git a/debian/rules.d/2-binary-arch.mk b/debian/rules.d/2-binary-arch.mk > > index 703fd2ade338..6311c9a74e05 100644 > > --- a/debian/rules.d/2-binary-arch.mk > > +++ b/debian/rules.d/2-binary-arch.mk > > @@ -31,6 +31,9 @@ $(stampdir)/stamp-prepare-tree-%: $(commonconfdir)/config.common.$(family) $(arc > > [ "$(do_full_source)" != 'true' ] && true || \ > > rsync -a --exclude debian --exclude debian.master --exclude $(DEBIAN) * $(builddir)/build-$* > > cat $(wordlist 1,3,$^) | sed -e 's/.*CONFIG_VERSION_SIGNATURE.*/CONFIG_VERSION_SIGNATURE="Ubuntu $(release)-$(revision)-$* $(raw_kernelversion)"/' > $(builddir)/build-$*/.config > > + [ "$(do_odm_drivers)" = 'true' ] && true || \ > > + sed -ie 's/.*CONFIG_UBUNTU_ODM_DRIVERS.*/# CONFIG_UBUNTU_ODM_DRIVERS is not set/' \ > > + $(builddir)/build-$*/.config > > > So if I understood that correctly, derivatives will have to set the > config option to "y" *and* do_odm_drivers=true if they want to opt > in. Is that really necessary? > > I'm probably missing something, but why is do_odm_drivers even > necessary? > > > > find $(builddir)/build-$* -name "*.ko" | xargs rm -f > > $(build_cd) $(kmake) $(build_O) -j1 syncconfig prepare scripts > > touch $@ > > diff --git a/ubuntu/Kconfig b/ubuntu/Kconfig > > index 8cea998f29a3..d969907b0bfd 100644 > > --- a/ubuntu/Kconfig > > +++ b/ubuntu/Kconfig > > @@ -1,5 +1,11 @@ > > menu "Ubuntu Supplied Third-Party Device Drivers" > > > > + > > +config UBUNTU_ODM_DRIVERS > > +bool "Enable support for ODM drivers" > > > That will cause all derivatives to be prompted about this config. Is > that what you are planning? > > > > +help > > + Turn on support for Ubuntu ODM supplied drivers > > > A better description here would be welcomed. What's ODM? > > > > + > > # > > # NOTE: to allow drivers to be added and removed without causing merge > > # collisions you should add new entries in the middle of the six lines > > -- > > 2.25.1 > > > > > > -- > > kernel-team mailing list > > kernel-team@lists.ubuntu.com > > https://lists.ubuntu.com/mailman/listinfo/kernel-team > > -- > Regards, > Marcelo >
On 22.01.21 22:22, Marcelo Henrique Cerri wrote: > Hi, Stefan. > > I'm attaching one alternative that should work in a similar way using > do_odm_drivers=true without forcing every derivative to include the > new config option. While I am not really sure why you so concerned about picking up new config options (you have that all often from upstream changes), I can look into doing something like this. But I would rather export that cryptic code into a script file that has a somewhat more meaningful name. -Stefan > > On Fri, Jan 22, 2021 at 02:34:40PM -0300, Marcelo Henrique Cerri wrote: >> Can you give us an explanation regarding the purpose of this and what >> ODM stands for? >> >> I have other comments bellow: >> >> On Fri, Jan 22, 2021 at 05:28:07PM +0100, Stefan Bader wrote: >>> BugLink: https://bugs.launchpad.net/bugs/1912789 >>> >>> We want to be able to selectively turn on ODM driver support for those >>> kernels/arches we have to but otherwise not inherit this to other >>> derivatives. This is done by a new config option which we will have to >>> depend on in the new drivers config options. Support is toggled by >>> changing a makefile rule variable. >>> The logic is a bit reversed. The common config has it enabled but it >>> will be turn off for builds unless one asks for it to be on. Example >>> of that will be in a follow-up patch. >>> >>> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> >>> --- >>> debian.master/config/config.common.ubuntu | 1 + >>> debian/rules.d/0-common-vars.mk | 4 ++++ >>> debian/rules.d/1-maintainer.mk | 1 + >>> debian/rules.d/2-binary-arch.mk | 3 +++ >>> ubuntu/Kconfig | 6 ++++++ >>> 5 files changed, 15 insertions(+) >>> >>> diff --git a/debian.master/config/config.common.ubuntu b/debian.master/config/config.common.ubuntu >>> index 75cc55c2d321..00fae58b1ade 100644 >>> --- a/debian.master/config/config.common.ubuntu >>> +++ b/debian.master/config/config.common.ubuntu >>> @@ -10362,6 +10362,7 @@ CONFIG_UBIFS_FS_ZLIB=y >>> CONFIG_UBIFS_FS_ZSTD=y >>> # CONFIG_UBSAN is not set >>> CONFIG_UBSAN_ALIGNMENT=y >>> +CONFIG_UBUNTU_ODM_DRIVERS=y >>> CONFIG_UCB1400_CORE=m >>> CONFIG_UCLAMP_BUCKETS_COUNT=5 >>> CONFIG_UCLAMP_TASK=y >>> diff --git a/debian/rules.d/0-common-vars.mk b/debian/rules.d/0-common-vars.mk >>> index 5394b7c96f82..b71546ab03a0 100644 >>> --- a/debian/rules.d/0-common-vars.mk >>> +++ b/debian/rules.d/0-common-vars.mk >>> @@ -187,6 +187,10 @@ do_common_headers_indep=true >>> # add a 'full source' mode >>> do_full_source=false >>> >>> +# Add an option to enable special drivers which should only be build when >>> +# explicitly enabled. >>> +do_odm_drivers=false >>> + >>> # build tools >>> ifneq ($(wildcard $(CURDIR)/tools),) >>> ifeq ($(do_tools),) >>> diff --git a/debian/rules.d/1-maintainer.mk b/debian/rules.d/1-maintainer.mk >>> index 956829027e0f..fabdf0888fce 100644 >>> --- a/debian/rules.d/1-maintainer.mk >>> +++ b/debian/rules.d/1-maintainer.mk >>> @@ -86,6 +86,7 @@ printenv: >>> @echo "do_flavour_header_package = $(do_flavour_header_package)" >>> @echo "do_common_headers_indep = $(do_common_headers_indep)" >>> @echo "do_full_source = $(do_full_source)" >>> + @echo "do_odm_drivers = $(do_odm_drivers)" >>> @echo "do_tools = $(do_tools)" >>> @echo "do_any_tools = $(do_any_tools)" >>> @echo "do_linux_tools = $(do_linux_tools)" >>> diff --git a/debian/rules.d/2-binary-arch.mk b/debian/rules.d/2-binary-arch.mk >>> index 703fd2ade338..6311c9a74e05 100644 >>> --- a/debian/rules.d/2-binary-arch.mk >>> +++ b/debian/rules.d/2-binary-arch.mk >>> @@ -31,6 +31,9 @@ $(stampdir)/stamp-prepare-tree-%: $(commonconfdir)/config.common.$(family) $(arc >>> [ "$(do_full_source)" != 'true' ] && true || \ >>> rsync -a --exclude debian --exclude debian.master --exclude $(DEBIAN) * $(builddir)/build-$* >>> cat $(wordlist 1,3,$^) | sed -e 's/.*CONFIG_VERSION_SIGNATURE.*/CONFIG_VERSION_SIGNATURE="Ubuntu $(release)-$(revision)-$* $(raw_kernelversion)"/' > $(builddir)/build-$*/.config >>> + [ "$(do_odm_drivers)" = 'true' ] && true || \ >>> + sed -ie 's/.*CONFIG_UBUNTU_ODM_DRIVERS.*/# CONFIG_UBUNTU_ODM_DRIVERS is not set/' \ >>> + $(builddir)/build-$*/.config >> >> >> So if I understood that correctly, derivatives will have to set the >> config option to "y" *and* do_odm_drivers=true if they want to opt >> in. Is that really necessary? >> >> I'm probably missing something, but why is do_odm_drivers even >> necessary? >> >> >>> find $(builddir)/build-$* -name "*.ko" | xargs rm -f >>> $(build_cd) $(kmake) $(build_O) -j1 syncconfig prepare scripts >>> touch $@ >>> diff --git a/ubuntu/Kconfig b/ubuntu/Kconfig >>> index 8cea998f29a3..d969907b0bfd 100644 >>> --- a/ubuntu/Kconfig >>> +++ b/ubuntu/Kconfig >>> @@ -1,5 +1,11 @@ >>> menu "Ubuntu Supplied Third-Party Device Drivers" >>> >>> + >>> +config UBUNTU_ODM_DRIVERS >>> +bool "Enable support for ODM drivers" >> >> >> That will cause all derivatives to be prompted about this config. Is >> that what you are planning? >> >> >>> +help >>> + Turn on support for Ubuntu ODM supplied drivers >> >> >> A better description here would be welcomed. What's ODM? >> >> >>> + >>> # >>> # NOTE: to allow drivers to be added and removed without causing merge >>> # collisions you should add new entries in the middle of the six lines >>> -- >>> 2.25.1 >>> >>> >>> -- >>> kernel-team mailing list >>> kernel-team@lists.ubuntu.com >>> https://lists.ubuntu.com/mailman/listinfo/kernel-team >> >> -- >> Regards, >> Marcelo >> > > >
diff --git a/debian.master/config/config.common.ubuntu b/debian.master/config/config.common.ubuntu index 75cc55c2d321..00fae58b1ade 100644 --- a/debian.master/config/config.common.ubuntu +++ b/debian.master/config/config.common.ubuntu @@ -10362,6 +10362,7 @@ CONFIG_UBIFS_FS_ZLIB=y CONFIG_UBIFS_FS_ZSTD=y # CONFIG_UBSAN is not set CONFIG_UBSAN_ALIGNMENT=y +CONFIG_UBUNTU_ODM_DRIVERS=y CONFIG_UCB1400_CORE=m CONFIG_UCLAMP_BUCKETS_COUNT=5 CONFIG_UCLAMP_TASK=y diff --git a/debian/rules.d/0-common-vars.mk b/debian/rules.d/0-common-vars.mk index 5394b7c96f82..b71546ab03a0 100644 --- a/debian/rules.d/0-common-vars.mk +++ b/debian/rules.d/0-common-vars.mk @@ -187,6 +187,10 @@ do_common_headers_indep=true # add a 'full source' mode do_full_source=false +# Add an option to enable special drivers which should only be build when +# explicitly enabled. +do_odm_drivers=false + # build tools ifneq ($(wildcard $(CURDIR)/tools),) ifeq ($(do_tools),) diff --git a/debian/rules.d/1-maintainer.mk b/debian/rules.d/1-maintainer.mk index 956829027e0f..fabdf0888fce 100644 --- a/debian/rules.d/1-maintainer.mk +++ b/debian/rules.d/1-maintainer.mk @@ -86,6 +86,7 @@ printenv: @echo "do_flavour_header_package = $(do_flavour_header_package)" @echo "do_common_headers_indep = $(do_common_headers_indep)" @echo "do_full_source = $(do_full_source)" + @echo "do_odm_drivers = $(do_odm_drivers)" @echo "do_tools = $(do_tools)" @echo "do_any_tools = $(do_any_tools)" @echo "do_linux_tools = $(do_linux_tools)" diff --git a/debian/rules.d/2-binary-arch.mk b/debian/rules.d/2-binary-arch.mk index 703fd2ade338..6311c9a74e05 100644 --- a/debian/rules.d/2-binary-arch.mk +++ b/debian/rules.d/2-binary-arch.mk @@ -31,6 +31,9 @@ $(stampdir)/stamp-prepare-tree-%: $(commonconfdir)/config.common.$(family) $(arc [ "$(do_full_source)" != 'true' ] && true || \ rsync -a --exclude debian --exclude debian.master --exclude $(DEBIAN) * $(builddir)/build-$* cat $(wordlist 1,3,$^) | sed -e 's/.*CONFIG_VERSION_SIGNATURE.*/CONFIG_VERSION_SIGNATURE="Ubuntu $(release)-$(revision)-$* $(raw_kernelversion)"/' > $(builddir)/build-$*/.config + [ "$(do_odm_drivers)" = 'true' ] && true || \ + sed -ie 's/.*CONFIG_UBUNTU_ODM_DRIVERS.*/# CONFIG_UBUNTU_ODM_DRIVERS is not set/' \ + $(builddir)/build-$*/.config find $(builddir)/build-$* -name "*.ko" | xargs rm -f $(build_cd) $(kmake) $(build_O) -j1 syncconfig prepare scripts touch $@ diff --git a/ubuntu/Kconfig b/ubuntu/Kconfig index 8cea998f29a3..d969907b0bfd 100644 --- a/ubuntu/Kconfig +++ b/ubuntu/Kconfig @@ -1,5 +1,11 @@ menu "Ubuntu Supplied Third-Party Device Drivers" + +config UBUNTU_ODM_DRIVERS +bool "Enable support for ODM drivers" +help + Turn on support for Ubuntu ODM supplied drivers + # # NOTE: to allow drivers to be added and removed without causing merge # collisions you should add new entries in the middle of the six lines
BugLink: https://bugs.launchpad.net/bugs/1912789 We want to be able to selectively turn on ODM driver support for those kernels/arches we have to but otherwise not inherit this to other derivatives. This is done by a new config option which we will have to depend on in the new drivers config options. Support is toggled by changing a makefile rule variable. The logic is a bit reversed. The common config has it enabled but it will be turn off for builds unless one asks for it to be on. Example of that will be in a follow-up patch. Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- debian.master/config/config.common.ubuntu | 1 + debian/rules.d/0-common-vars.mk | 4 ++++ debian/rules.d/1-maintainer.mk | 1 + debian/rules.d/2-binary-arch.mk | 3 +++ ubuntu/Kconfig | 6 ++++++ 5 files changed, 15 insertions(+)