openbmc.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
From: Ed Tanous <edtanous@google.com>
To: Alexander Amelkin <a.amelkin@yadro.com>
Cc: OpenBMC Maillist <openbmc@lists.ozlabs.org>
Subject: Re: New test for patches in openbmc/openbmc
Date: Thu, 23 Sep 2021 16:38:50 -0700	[thread overview]
Message-ID: <CAH2-KxDT9DTXp99d+eV-zNinLWhw9ihaHVLNQnVsQtB7X2ZkHQ@mail.gmail.com> (raw)
In-Reply-To: <67dbec1b-8598-8814-e85e-848b2eb123cf@yadro.com>

On Wed, Sep 22, 2021 at 2:02 AM Alexander Amelkin <a.amelkin@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

  parent reply	other threads:[~2021-09-23 23:39 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-21 22:35 New test for patches in openbmc/openbmc Ed Tanous
2021-09-22  9:02 ` Alexander Amelkin
2021-09-22 13:15   ` Verdun, Jean-Marie
2021-09-22 23:35   ` Oskar Senft
2021-09-23  2:49     ` John Broadbent
2021-09-23 17:09       ` Benjamin Fair
2021-09-23 23:57       ` Ed Tanous
2021-09-23 23:38   ` Ed Tanous [this message]
2021-09-24 10:28     ` Thang Nguyen
2021-09-24 11:06       ` [External] " Lei Yu
2021-09-27 16:33         ` Ed Tanous
2021-09-28  8:36           ` Lei Yu
2021-10-08  8:32             ` Lei YU
2021-10-08 17:35               ` Ed Tanous
2021-10-09  2:18                 ` Lei Yu
2021-10-11 17:47                   ` Ed Tanous
2021-09-27 16:39       ` Ed Tanous

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAH2-KxDT9DTXp99d+eV-zNinLWhw9ihaHVLNQnVsQtB7X2ZkHQ@mail.gmail.com \
    --to=edtanous@google.com \
    --cc=a.amelkin@yadro.com \
    --cc=openbmc@lists.ozlabs.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).