New test for patches in openbmc/openbmc

[Ed Tanous]

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