NACK: pull request for raspi2 eoan kernel (set 2 falvours for armhf and change flavour name to raspi4 for arm64)

Fri Nov 22 11:58:43 UTC 2019

On 2019/11/22 下午5:07, Juerg Haefliger wrote:
> The email subject is invalid. It should be something in the form of:
>
> [SRU][Eoan/raspi2][PULL] <a sensible subject>
OK, got it.
>
>
>> BugLink: https://bugs.launchpad.net/bugs/1853446
>>
>> [Impact]
>> It is risky to use single armhf kernel to support rpi2/3/4, since
>> upstream has a specific config for rpi2/3 and another config for rpi4,
>> the major difference are LPAE, HIGHMEM and VMSPLIT_3G, we already
>> met a bug that is intrduced by HIGHMEM and VMSPLIT_3G on rpi2/3,
>> so add two falvours for rpi2/3 and rpi4.
>> flavour 1: raspi2 (for rpi2/3, disable LPAE, HIGHMEM and VMSPLIT_3G)
>> flavour 2: raspi4 (for rpi4, enable LPAE, HIGHMEM and VMSPLIT_3G)
>>
>> For arm64 kernel, we change the flavour name to raspi4, it supports
>> rpi2/3/4.
>>
>> [Fix]
>> Change the related configuration files in the debian.raspi2.
>>
>>
>> [Test Case]
>> After apply the patches, and I built two flavours of armhf and
>> arm64 kernels, Then check the .config, they all meet the expectation.
>> And boot those kernels on the rpi2/3/4 boards. worked well.
> How did you build the kernels? Did you build source packages and upload them to
> a test builder PPA?

No, I didn't build the kernel with PPA, just built the kernel on my 
computer:

export $(dpkg-architecture -aarmhf); export 
CROSS_COMPILE=arm-linux-gnueabihf-
export CC=arm-linux-gnueabihf-gcc
fakeroot debian/rules clean
fakeroot debian/rules binary-raspi2 or binary-raspi4 or binary

export $(dpkg-architecture -aarm64); export CROSS_COMPILE=aarch64-linux-gnu-
export CC=aarch64-linux-gnu-gcc
fakeroot debian/rules clean
fakeroot debian/rules binary-raspi4

>
>> [Regression Risk]
>> Low, just change the flavour name for arm64, and for armhf, the
>> flavour raspi4 is basically same as bofre (raspi2), the new flavour
>> raspi2 just disable LPAE/HIGHMEM/VMSPLIT_3G, it is same as the config
>> for rpi2/3 in the upstream.
>>
>> The following changes since commit c1a0780b4dd5a9bfdf828d024194156d0ea45774:
>>
>>     UBUNTU: [Packaging] raspi2: Fix snapcraft.yaml (2019-11-13 10:37:29
>> +0100)
>>
>> are available in the Git repository at:
>>
>> https://github.com/jason77-wang/eoan-rpi4.git:Ubuntu-raspi2-lp1853446
> IMO this is not a valid git repo+refspec. It should probably be:
> https://github.com/jason77-wang/eoan-rpi4 Ubuntu-raspi2-lp1853446
> but I assume you created this PR using 'git request-pull' so it must be
> correct and I simply don't understand how to consume it :-)
>
Oh, I didn't notice that before,  I just copy the output from 'git 
request-pull'.

>> for you to fetch changes up to b557f61befe948366fde6f25befd34fdb6af0d89:
>>
>>     UBUNTU: SAUCE: abi: change the abi name to raspi4 for arm64 kernel
>> (2019-11-21 20:59:39 +0800)
>>
>> ----------------------------------------------------------------
>> Hui Wang (4):
>>         UBUNTU: SAUCE: add one more flavour to armhf kernel
>>         UBUNTU: SAUCE: ABI: add abi for the flavour raspi4 on armhf kernel
>>         UBUNTU: SAUCE: change the flavour name to raspi4 on arm64 kernel
>>         UBUNTU: SAUCE: abi: change the abi name to raspi4 for arm64 kernel
> These changes are not SAUCE patches. A SAUCE patch is a change of the kernel
> code itself that is not in upstream. But you're modifying Ubuntu-specific files
> under debian.raspi2. So the convention for the commit subject is:
>
> For config changes (under debian.raspi2/config/)
> UBUNTU: [Config] raspi2: ....
>
> And for packaging changes (like adding a new flavor):
> UBUNTU: [Packaging] raspi2: ....
>
> Also, your commit messages are missing correct BugLink tags. You have:
> https://bugs.launchpad.net/bugs/1853446
> instead of:
> BugLink: https://bugs.launchpad.net/bugs/1853446
Got it. Will address them in the V2.
>
> Regarding that new flavor name 'raspi4': Is that really the name we want? Can't
> we finally create a more generic flavor like 'raspi', so that in two years from
> now people are not confused when they need to use a 'raspi4' kernel on their Pi
> 5 or 6 or something?

OK, will think about it.

Thanks for your review.

Hui.

>
> ...Juerg
>
>
>>    debian.raspi2/abi/5.3.0-1011.12/arm64/{raspi2 =>
>> raspi4}                     |     0
>>    debian.raspi2/abi/5.3.0-1011.12/arm64/{raspi2.compiler =>
>> raspi4.compiler}   |     0
>>    debian.raspi2/abi/5.3.0-1011.12/arm64/{raspi2.modules =>
>> raspi4.modules}     |     0
>>    debian.raspi2/abi/5.3.0-1011.12/arm64/{raspi2.retpoline =>
>> raspi4.retpoline} |     0
>>    debian.raspi2/abi/5.3.0-1011.12/armhf/raspi4 | 19968
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>    debian.raspi2/abi/5.3.0-1011.12/armhf/raspi4.compiler |     1 +
>>    debian.raspi2/abi/5.3.0-1011.12/armhf/raspi4.modules |  4292
>> +++++++++++++++++++++++++
>>    debian.raspi2/abi/5.3.0-1011.12/armhf/raspi4.retpoline |     1 +
>>    debian.raspi2/config/arm64/config.flavour.raspi2 |     3 -
>>    debian.raspi2/config/arm64/config.flavour.raspi4 |     3 +
>>    debian.raspi2/config/armhf/config.common.armhf |     2 -
>>    debian.raspi2/config/armhf/config.flavour.raspi2 |     7 +
>>    debian.raspi2/config/armhf/config.flavour.raspi4 |    10 +
>>    debian.raspi2/config/config.common.ubuntu |     9 +-
>>    debian.raspi2/control.d/vars.raspi4 |     6 +
>>    debian.raspi2/etc/getabis |     4 +-
>>    debian.raspi2/rules.d/arm64.mk |     2 +-
>>    debian.raspi2/rules.d/armhf.mk |     2 +-
>>    18 files changed, 24296 insertions(+), 14 deletions(-)
>>    rename debian.raspi2/abi/5.3.0-1011.12/arm64/{raspi2 => raspi4} (100%)
>>    rename debian.raspi2/abi/5.3.0-1011.12/arm64/{raspi2.compiler =>
>> raspi4.compiler} (100%)
>>    rename debian.raspi2/abi/5.3.0-1011.12/arm64/{raspi2.modules =>
>> raspi4.modules} (100%)
>>    rename debian.raspi2/abi/5.3.0-1011.12/arm64/{raspi2.retpoline =>
>> raspi4.retpoline} (100%)
>>    create mode 100644 debian.raspi2/abi/5.3.0-1011.12/armhf/raspi4
>>    create mode 100644 debian.raspi2/abi/5.3.0-1011.12/armhf/raspi4.compiler
>>    create mode 100644 debian.raspi2/abi/5.3.0-1011.12/armhf/raspi4.modules
>>    create mode 100644 debian.raspi2/abi/5.3.0-1011.12/armhf/raspi4.retpoline
>>    delete mode 100644 debian.raspi2/config/arm64/config.flavour.raspi2
>>    create mode 100644 debian.raspi2/config/arm64/config.flavour.raspi4
>>    create mode 100644 debian.raspi2/config/armhf/config.flavour.raspi4
>>    create mode 100644 debian.raspi2/control.d/vars.raspi4
>>