New test for patches in openbmc/openbmc

[Thang Nguyen]

Hi Ed,
I have 2 questions on this topics:
1. I have a patch 
meta-ampere/meta-jade/recipes-bsp/u-boot/u-boot-aspeed/0003-aspeed-support-passing-system-reset-status-to-kernel.patch 
which ported from Intel code. It is to add BMC reset cause to boot 
command line (/proc/cmdline) in which I can check for chassis power 
policy which skip when BMC reboots (does not change CPU status). As the 
patch is from Intel, what is the procedure to make it reviewed and 
applied to u-boot?

2. After completing patch removal from meta- folder, can we still add 
patch into meta- folder in the local repo? In the development, we might 
have some fixes which add patches to make features work, before pushing 
to gerrit for review. However, the local repos might be out-of-sync to 
upstream codes for some months so we need to manage them locally in a 
period of time. They will be removed in the next code rebase.


Best Regards,
Thang Q. Nguyen

On 24/09/2021 06:38, Ed Tanous wrote:
> On Wed, Sep 22, 2021 at 2:02 AM Alexander Amelkin <a.amelkin at yadro.com> wrote:
>>
>> Hi Ed!
>>
>> Most patches you listed (at least those for YADRO) are
>> platform specific and no repository will accept them for
>> a general audience.
> 
> Right off the bat, I can see that 5 of the Yardro patches are against
> OpenBMC repositories.  If they're necessary, in line with OpenBMCs
> goals, and maintainers aren't accepting them, why was this not brought
> up sooner?  There are several avenues at your disposal now: discord,
> mailing list, Technical oversight forum, Technical steering committee,
> in that order, where we can discuss these things and come to a
> consensus.  If the answer is "There is no way that you can enable your
> platform to work" then raise it up to the next level, but I suspect
> that wasn't the case.
> 
> The path we're going down would involve every system effectively
> forking the internal codebase, which means that now we have N forks of
> our own codebase for N systems, and it gets to be unmaintainable in a
> hurry if any changes conflict.  As is, a number of our systems that we
> "support" don't build on master, which somewhat proves the point that
> it's unsustainable.
> 
> One minor thing that might prove my point is that I can't see to get
> the nicole platform to build.  I'm not sure if that's related to the
> patch files or not.
> "error: comparison of integer expressions of different signedness:
> 'const unsigned int' and 'const int' [-Werror=sign-compare]"
> 
>>
>> No vendor, I'm confident, is willing to spend endless time
>> persuading maintainers to include vendor-specific or
>> platform-specific patches into their repositories.
> 
> Endless, no, but a number of companies have had success in getting
> their patches and features upstreamed.  I'd like to hear more about
> your experience, maybe with some specific examples (which I can see
> below).
> 
>>
>> For instance,
>> meta-yadro/recipes-phosphor/ipmi/phosphor-ipmi-host/0002-Add-support-for-boot-initiator-mailbox.patch
>> is there because our customers demand this feature and we failed
>> proving to openbmc maintainers that this is a needed feature
>> and not a "security threat" or something. We honestly tried for months.
> 
> I wasn't part of those discussions, so I don't know the full details,
> but I do know that we take security seriously.  Making some
> assumptions for a second, if there's security consequences, and
> security isn't in your list of "must haves" for your platform, maybe
> something similar to the bmcweb-insecure-* compiler options that
> require directly opting into the security consequences might be
> warranted for your systems?  Was any sort of option flag discussed?
> Did the maintainers propose an alternative to you?
> 
>>
>> On the other hand,
>> meta-yadro/meta-nicole/recipes-bsp/u-boot/files/0004-aspeed-add-bmc-position-support.patch
>> is strictly hardware-specific and is not needed as is for other
>> vendors or platforms, and we don't have time to make it a
>> generic solution. If we ever do have that time, we will surely
>> push the developed generic solution to the appropriate
>> repository.
> 
> The problem with this kind of thing is that it encourages companies to
> not reuse these kinds of things, and we end up with duplicated copies
> of patches across the meta layers.  There are ABSOLUTELY other systems
> that use GPIO for determining node position, so it's really tough to
> say that feature is "strictly hardware-specific".
> 
>>
>> What you propose now will force vendors to move farther away
>> from upstream and create their own forks of openbmc where
>> they will not even try to upstream their changes and will just drift
>> farther and farther away.
>>
>> Is that what you really pursue or did I get your idea wrong?
>> So far it looks to me like a destructive decision.
> 
> This is not intended to be a destructive position, but to encourage
> reuse of code, and upstreaming in a way that's sustainable and
> reusable by others.  If the things we build aren't reusable, then to
> some extent, what's the point of upstreaming it?
> 
> Keep in mind, the only new thing here is that CI is now enforcing
> this.  It's been in the meta layer guidelines for some time, and has
> been an unwritten guideline for some time prior to that.  If you
> disagree with the policy, that's fine, let's discuss how to best keep
> the project moving forward, and while I'm open to patch files being
> necessary in some cases, IMO per-machine patch files don't get us the
> maintainability we need in the long run.
> 
> 
>>
>> WBR, Alexander.
>>
>> 22.09.2021 01:35, Ed Tanous пишет:
>>> A few new features have been merged into CI that will now disallow
>>> .patch files within most meta layers.  This is due to a significant
>>> number of them popping up in both reviews and in the repo itself,
>>> despite having documented rules to the contrary.  The hope here is to
>>> better codify our rules, and give very quick response to submitters
>>> about the right procedure so we can encourage getting patches in
>>> faster, and keep machines buildable against master.  As the patches
>>> state, meta-phosphor is still allowed to contain patch files as an
>>> escape hatch, if the community decides it's required.
>>>
>>> The patchsets in question are here:
>>> https://gerrit.openbmc-project.xyz/q/repotest
>>>
>>> And add some ability for us to make more of these expectations for
>>> meta layers codified in the future.
>>>
>>> The script itself is here:
>>> https://github.com/openbmc/openbmc/blob/master/meta-phosphor/scripts/run-repotest.sh
>>> and is runnable on any tree prior to submitting to CI.  We currently
>>> have the following patches in meta layers.
>>>
>>> meta-amd/meta-ethanolx/recipes-x86/chassis/x86-power-control/0001-Amd-power-control-modifications-for-EthanolX.patch
>>> meta-ampere/meta-common/recipes-devtools/mtd/mtd-utils/0001-flashcp-support-offset-option.patch
>>> meta-ampere/meta-jade/recipes-bsp/u-boot/u-boot-aspeed/0001-aspeed-scu-Switch-PWM-pin-to-GPIO-input-mode.patch
>>> meta-ampere/meta-jade/recipes-bsp/u-boot/u-boot-aspeed/0002-aspeed-Disable-internal-PD-resistors-for-GPIOs.patch
>>> meta-ampere/meta-jade/recipes-bsp/u-boot/u-boot-aspeed/0003-aspeed-support-passing-system-reset-status-to-kernel.patch
>>> meta-ampere/meta-jade/recipes-bsp/u-boot/u-boot-aspeed/0004-aspeed-add-gpio-support.patch
>>> meta-ampere/meta-jade/recipes-bsp/u-boot/u-boot-aspeed/0005-aspeed-Enable-SPI-master-mode.patch
>>> meta-ampere/meta-jade/recipes-bsp/u-boot/u-boot-aspeed/0006-aspeed-support-Mt.Jade-platform-init.patch
>>> meta-aspeed/recipes-bsp/u-boot/files/default-gcc.patch
>>> meta-bytedance/meta-g220a/recipes-kernel/linux/linux-aspeed/0001-bytedance-g220a-Enable-ipmb.patch
>>> meta-bytedance/meta-g220a/recipes-kernel/linux/linux-aspeed/0003-misc-aspeed-Add-Aspeed-UART-routing-control-driver.patch
>>> meta-bytedance/meta-g220a/recipes-kernel/linux/linux-aspeed/0004-ARM-dts-aspeed-Add-uart-routing-node.patch
>>> meta-bytedance/meta-g220a/recipes-kernel/linux/linux-aspeed/0005-ARM-dts-aspeed-Enable-g220a-uart-route.patch
>>> meta-bytedance/meta-g220a/recipes-phosphor/ipmi/phosphor-node-manager-proxy/0001-Remove-Total_Power-sensor.patch
>>> meta-facebook/meta-bletchley/recipes-bsp/u-boot/u-boot-aspeed-sdk/0001-u-boot-ast2600-57600-baudrate-for-bletchley.patch
>>> meta-facebook/meta-tiogapass/recipes-bsp/u-boot/u-boot-aspeed/0001-configs-ast-common-use-57600-baud-rate-to-match-Tiog.patch
>>> meta-facebook/meta-yosemitev2/recipes-bsp/u-boot/u-boot-aspeed/0001-board-aspeed-Add-Mux-for-yosemitev2.patch
>>> meta-facebook/meta-yosemitev2/recipes-bsp/u-boot/u-boot-aspeed/0002-spl-host-console-handle.patch
>>> meta-google/dynamic-layers/nuvoton-layer/recipes-bsp/images/npcm7xx-igps/0001-Set-FIU0_DRD_CFG-and-FIU_Clk_divider-for-gbmc-hoth.patch
>>> meta-google/recipes-extended/libconfig/files/0001-conf2struct-Use-the-right-perl.patch
>>> meta-google/recipes-extended/libconfig/files/0001-makefile-Add-missing-LDFLAGS.patch
>>> meta-google/recipes-phosphor/initrdscripts/obmc-phosphor-initfs/rwfs-clean-dev.patch
>>> meta-ingrasys/meta-zaius/recipes-bsp/u-boot/u-boot-aspeed/0001-board-aspeed-Add-reset_phy-for-Zaius.patch
>>> meta-nuvoton/recipes-bsp/images/npcm7xx-igps/0001-Adjust-paths-for-use-with-Bitbake.patch
>>> meta-yadro/meta-nicole/recipes-bsp/u-boot/files/0001-Add-system-reset-status-support.patch
>>> meta-yadro/meta-nicole/recipes-bsp/u-boot/files/0002-config-ast-common-set-fieldmode-to-true.patch
>>> meta-yadro/meta-nicole/recipes-bsp/u-boot/files/0003-aspeed-add-gpio-support.patch
>>> meta-yadro/meta-nicole/recipes-bsp/u-boot/files/0004-aspeed-add-bmc-position-support.patch
>>> meta-yadro/meta-nicole/recipes-kernel/linux/linux-aspeed/0001-Add-NCSI-channel-selector.patch
>>> meta-yadro/meta-nicole/recipes-phosphor/host/op-proc-control/0001-Stop-and-send-SRESET-for-one-thread-only.patch
>>> meta-yadro/recipes-phosphor/dbus/phosphor-dbus-interfaces/0001-Add-boot-initiator-mailbox-interface.patch
>>> meta-yadro/recipes-phosphor/ipmi/phosphor-ipmi-host/0001-Add-support-for-persistent-only-settings.patch
>>> meta-yadro/recipes-phosphor/ipmi/phosphor-ipmi-host/0002-Add-support-for-boot-initiator-mailbox.patch
>>> meta-yadro/recipes-phosphor/ipmi/phosphor-ipmi-host/0003-Fix-version-parsing-update-AUX-revision-info.patch
>>>
>>> If you are a maintainer of these meta layers, please work to get these
>>> patches submitted to the correct repositories using their prefered
>>> review (email for linux/u-boot, gerrit for phosphor repos).
>>>
>>> Thanks,
>>>
>>> -Ed