NACK: [SRU F][PATCH 01/02] UBUNTU: [Packaging] Add support for ODM drivers

Mon Jan 25 08:35:50 UTC 2021

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 at 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 at lists.ubuntu.com
>>> https://lists.ubuntu.com/mailman/listinfo/kernel-team
>>
>> -- 
>> Regards,
>> Marcelo
>>
> 
> 
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20210125/c11a025d/attachment.sig>