New test for patches in openbmc/openbmc

New test for patches in openbmc/openbmc

[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

Re: New test for patches in openbmc/openbmc

[Alexander Amelkin]

Hi Ed!

Most patches you listed (at least those for YADRO) are
platform specific and no repository will accept them for
a general audience.

No vendor, I'm confident, is willing to spend endless time
persuading maintainers to include vendor-specific or
platform-specific patches into their repositories.

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.

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.

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.

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

Re: New test for patches in openbmc/openbmc

[Verdun, Jean-Marie]

[-- Attachment #1: Type: text/plain, Size: 8108 bytes --]

Hi Alexander,

We face the same challenges at HPE and spent some time discussing with code maintainers. I must admit that i had the same position than the one you are taking. But while processing some of our patches, upstreaming them can help enhancing the code and maintainability, by introducing new abstraction layers which are providing interfaces to vendor specific requirements. It is not always the case, but a good example could be found within the iKVM code, which is tightly coupled to Aspeed currently.

Our situation is even worse as we do have our own BMC asic, we do not use Aspeed. So how to integrate a bunch of hardware specific code into something which is specific to somebody else. We need to re-architecture part of the code. It is a pain, but part of the game. It would have been better if the code had been thought from the beginning to be more modular or with wider open mind. But in many cases, learning by mistake is also a good way to build something great. It was about the same regarding the x86-power management where we have GPIOs which are not compatible with upstream. So we had to propose enhancement and modifications.

With that said, it is going to be a huge effort for hardware vendors, but I think we need to do it even if that is going to be boring and complicated.

What worries me is the time required to do that, and avoid breaking the dynamic. I do not recommend forking, this is a massive pain to maintain.

Jean-Marie

________________________________
From: openbmc <openbmc-bounces+jean-marie.verdun=hpe.com@lists.ozlabs.org> on behalf of Alexander Amelkin <a.amelkin@yadro.com>
Sent: Wednesday, September 22, 2021 5:02 AM
To: Ed Tanous <edtanous@google.com>; OpenBMC Maillist <openbmc@lists.ozlabs.org>
Subject: Re: New test for patches in openbmc/openbmc

Hi Ed!

Most patches you listed (at least those for YADRO) are
platform specific and no repository will accept them for
a general audience.

No vendor, I'm confident, is willing to spend endless time
persuading maintainers to include vendor-specific or
platform-specific patches into their repositories.

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.

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.

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.

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

[-- Attachment #2: Type: text/html, Size: 10873 bytes --]

Re: New test for patches in openbmc/openbmc

[Oskar Senft]

Hi Alexander

While I can understand your position, I think there's a bigger picture
to consider. In my understanding Open Source works by individual /
independent contributors providing their use cases, knowledge and
experience by means of designs and source code to the world. Since
there are many individuals trying to do different things and some
people (maintainers) being the gatekeepers for what can be submitted,
it of course often gets to a point where not everyone agrees.

Trust me, I've been there. I had many occasions where I needed a new
feature or a fix to satisfy project requirements and timelines and was
not able to upstream it in the given time. I sometimes gave up, often
found a different, "better" solution and many times worked with the
community to find a solution that would be accepted upstream.

While I agree that deadlines and requirements do not always allow to
go the "everything upstream immediately" route, my experience has
shown me that forks or patches are ultimately costing more than using
clean upstream code, in particular if a device is to be supported for
years through new versions of the upstream code.

As an example, we've been using an i2c sensor chip that needs to be
configured at runtime. Upstream support for that was (still is)
missing. The patch to do that specifically for us was 1 line -
literally. However, it's incredibly difficult to discover and
understand this one line years later. Together with hwmon maintainers
I've spent the last 2 weeks designing and implementing various
versions of a generic solution that we hope can be used for other
hwmon drivers. I understand that I'm in a fortunate position so I can
spend that time. But I still need to justify to my manager and myself
why it's worth it, which I believe I can.

In my experience, having patches checked in is just that - a temporary
patch - not a solution. From Oxford's dictionary: "to patch: treat
someone's injuries or repair the damage to something, especially
hastily" (I know there's also a definition of the noun in the realm of
computing).

So while I agree that not allowing patches is actually making things
harder for some in the short term, I truly believe that it's going to
make things better for everyone in the long term.

Oskar.

On Wed, Sep 22, 2021 at 5:03 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.
>
> No vendor, I'm confident, is willing to spend endless time
> persuading maintainers to include vendor-specific or
> platform-specific patches into their repositories.
>
> 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.
>
> 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.
>
> 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.
>
> 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

Re: New test for patches in openbmc/openbmc

[John Broadbent]

[-- Attachment #1: Type: text/plain, Size: 10178 bytes --]

I am concerned this change will encourage both patches in private layers,
and forks of the entire project.

Oskar is right, patches should be temporary fixes, but I have worked
around, and some organizations never clean up their "temporary fixes".
Their engineers move from one fire to the next. I suppose, I would
prefer to see .patch files in openbmc meta layers rather than have the same
.patch file pushed to a private layer, or worse a fork of openbmc.

Where can I get some more context on why .patch files are disallowed from
open bmc meta layers?

I genuinely appreciate all their effort and hard work
the maintainer put in. They have always guided the community in the
right direction, but some more context for this decision might be helpful
for new people, such as myself.

Thank you
John Broadbent

On Wed, Sep 22, 2021 at 4:36 PM Oskar Senft <osk@google.com> wrote:

> Hi Alexander
>
> While I can understand your position, I think there's a bigger picture
> to consider. In my understanding Open Source works by individual /
> independent contributors providing their use cases, knowledge and
> experience by means of designs and source code to the world. Since
> there are many individuals trying to do different things and some
> people (maintainers) being the gatekeepers for what can be submitted,
> it of course often gets to a point where not everyone agrees.
>
> Trust me, I've been there. I had many occasions where I needed a new
> feature or a fix to satisfy project requirements and timelines and was
> not able to upstream it in the given time. I sometimes gave up, often
> found a different, "better" solution and many times worked with the
> community to find a solution that would be accepted upstream.
>
> While I agree that deadlines and requirements do not always allow to
> go the "everything upstream immediately" route, my experience has
> shown me that forks or patches are ultimately costing more than using
> clean upstream code, in particular if a device is to be supported for
> years through new versions of the upstream code.
>
> As an example, we've been using an i2c sensor chip that needs to be
> configured at runtime. Upstream support for that was (still is)
> missing. The patch to do that specifically for us was 1 line -
> literally. However, it's incredibly difficult to discover and
> understand this one line years later. Together with hwmon maintainers
> I've spent the last 2 weeks designing and implementing various
> versions of a generic solution that we hope can be used for other
> hwmon drivers. I understand that I'm in a fortunate position so I can
> spend that time. But I still need to justify to my manager and myself
> why it's worth it, which I believe I can.
>
> In my experience, having patches checked in is just that - a temporary
> patch - not a solution. From Oxford's dictionary: "to patch: treat
> someone's injuries or repair the damage to something, especially
> hastily" (I know there's also a definition of the noun in the realm of
> computing).
>
> So while I agree that not allowing patches is actually making things
> harder for some in the short term, I truly believe that it's going to
> make things better for everyone in the long term.
>
> Oskar.
>
> On Wed, Sep 22, 2021 at 5:03 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.
> >
> > No vendor, I'm confident, is willing to spend endless time
> > persuading maintainers to include vendor-specific or
> > platform-specific patches into their repositories.
> >
> > 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.
> >
> > 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.
> >
> > 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.
> >
> > 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
>

[-- Attachment #2: Type: text/html, Size: 11629 bytes --]

Re: New test for patches in openbmc/openbmc

[Benjamin Fair]

[-- Attachment #1: Type: text/plain, Size: 10907 bytes --]

This policy isn't new, just this automatic enforcement. It was reviewed in
February: https://gerrit.openbmc-project.xyz/c/openbmc/docs/+/40294

There's also a detailed explanation of the rationale for each guideline in
that document.

I'd like to point out that some of the patches listed in the original email
are against non-OpenBMC projects, which is allowed in some situations by my
understanding of the guidelines.

On Wed, 22 Sept 2021 at 19:50, John Broadbent <jebr@google.com> wrote:

> I am concerned this change will encourage both patches in private layers,
> and forks of the entire project.
>
> Oskar is right, patches should be temporary fixes, but I have worked
> around, and some organizations never clean up their "temporary fixes".
> Their engineers move from one fire to the next. I suppose, I would
> prefer to see .patch files in openbmc meta layers rather than have the same
> .patch file pushed to a private layer, or worse a fork of openbmc.
>
> Where can I get some more context on why .patch files are disallowed from
> open bmc meta layers?
>
> I genuinely appreciate all their effort and hard work
> the maintainer put in. They have always guided the community in the
> right direction, but some more context for this decision might be helpful
> for new people, such as myself.
>
> Thank you
> John Broadbent
>
> On Wed, Sep 22, 2021 at 4:36 PM Oskar Senft <osk@google.com> wrote:
>
>> Hi Alexander
>>
>> While I can understand your position, I think there's a bigger picture
>> to consider. In my understanding Open Source works by individual /
>> independent contributors providing their use cases, knowledge and
>> experience by means of designs and source code to the world. Since
>> there are many individuals trying to do different things and some
>> people (maintainers) being the gatekeepers for what can be submitted,
>> it of course often gets to a point where not everyone agrees.
>>
>> Trust me, I've been there. I had many occasions where I needed a new
>> feature or a fix to satisfy project requirements and timelines and was
>> not able to upstream it in the given time. I sometimes gave up, often
>> found a different, "better" solution and many times worked with the
>> community to find a solution that would be accepted upstream.
>>
>> While I agree that deadlines and requirements do not always allow to
>> go the "everything upstream immediately" route, my experience has
>> shown me that forks or patches are ultimately costing more than using
>> clean upstream code, in particular if a device is to be supported for
>> years through new versions of the upstream code.
>>
>> As an example, we've been using an i2c sensor chip that needs to be
>> configured at runtime. Upstream support for that was (still is)
>> missing. The patch to do that specifically for us was 1 line -
>> literally. However, it's incredibly difficult to discover and
>> understand this one line years later. Together with hwmon maintainers
>> I've spent the last 2 weeks designing and implementing various
>> versions of a generic solution that we hope can be used for other
>> hwmon drivers. I understand that I'm in a fortunate position so I can
>> spend that time. But I still need to justify to my manager and myself
>> why it's worth it, which I believe I can.
>>
>> In my experience, having patches checked in is just that - a temporary
>> patch - not a solution. From Oxford's dictionary: "to patch: treat
>> someone's injuries or repair the damage to something, especially
>> hastily" (I know there's also a definition of the noun in the realm of
>> computing).
>>
>> So while I agree that not allowing patches is actually making things
>> harder for some in the short term, I truly believe that it's going to
>> make things better for everyone in the long term.
>>
>> Oskar.
>>
>> On Wed, Sep 22, 2021 at 5:03 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.
>> >
>> > No vendor, I'm confident, is willing to spend endless time
>> > persuading maintainers to include vendor-specific or
>> > platform-specific patches into their repositories.
>> >
>> > 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.
>> >
>> > 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.
>> >
>> > 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.
>> >
>> > 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
>>
>

[-- Attachment #2: Type: text/html, Size: 12546 bytes --]

Re: New test for patches in openbmc/openbmc

[Ed Tanous]

On Wed, Sep 22, 2021 at 7:49 PM John Broadbent <jebr@google.com> wrote:
>
> I am concerned this change will encourage both patches in private layers, and forks of the entire project.
>
> Oskar is right, patches should be temporary fixes, but I have worked around, and some organizations never clean up their "temporary fixes". Their engineers move from one fire to the next. I suppose, I would prefer to see .patch files in openbmc meta layers rather than have the same .patch file pushed to a private layer, or worse a fork of openbmc.

This is some of the conflict though, why should OpenBMC have to
maintain other companies' temporary fixes that they didn't find
important enough to clean up and make usable to others?  If the goal
is just to host it, there's a million places to host forks for free
these days.  I like to think getting patches upstreamed implies some
level of quality, some level of acceptance, and most importantly, the
idea that I can go and make any modifications to the code I need to
make to support my own OpenBMC.  Patches generally don't allow for
that.

>
> Where can I get some more context on why .patch files are disallowed from open bmc meta layers?

#1 and #2 here:
https://github.com/openbmc/docs/blob/master/meta-layer-guidelines.md

>
> I genuinely appreciate all their effort and hard work the maintainer put in. They have always guided the community in the right direction, but some more context for this decision might be helpful for new people, such as myself.

There's no "decision" being made here, this is just making CI enforce
the things that are already written down.  If we wanted to change the
guidelines, we could, but that would be a different discussion,
ideally in a gerrit docs review proposing the change.  The initial
goal of this CI change was to reduce the turnaround time to getting
new platforms supported on OpenBMC.  The more things like that we can
codify in CI, the faster we can get patches merged to master.  With
that said, I like that it's causing some excellent discussions to
manifest.


Ps, Try to avoid top posting, it's less than desired on this list :)

>
> Thank you
> John Broadbent
>
> On Wed, Sep 22, 2021 at 4:36 PM Oskar Senft <osk@google.com> wrote:
>>
>> Hi Alexander
>>
>> While I can understand your position, I think there's a bigger picture
>> to consider. In my understanding Open Source works by individual /
>> independent contributors providing their use cases, knowledge and
>> experience by means of designs and source code to the world. Since
>> there are many individuals trying to do different things and some
>> people (maintainers) being the gatekeepers for what can be submitted,
>> it of course often gets to a point where not everyone agrees.
>>
>> Trust me, I've been there. I had many occasions where I needed a new
>> feature or a fix to satisfy project requirements and timelines and was
>> not able to upstream it in the given time. I sometimes gave up, often
>> found a different, "better" solution and many times worked with the
>> community to find a solution that would be accepted upstream.
>>
>> While I agree that deadlines and requirements do not always allow to
>> go the "everything upstream immediately" route, my experience has
>> shown me that forks or patches are ultimately costing more than using
>> clean upstream code, in particular if a device is to be supported for
>> years through new versions of the upstream code.
>>
>> As an example, we've been using an i2c sensor chip that needs to be
>> configured at runtime. Upstream support for that was (still is)
>> missing. The patch to do that specifically for us was 1 line -
>> literally. However, it's incredibly difficult to discover and
>> understand this one line years later. Together with hwmon maintainers
>> I've spent the last 2 weeks designing and implementing various
>> versions of a generic solution that we hope can be used for other
>> hwmon drivers. I understand that I'm in a fortunate position so I can
>> spend that time. But I still need to justify to my manager and myself
>> why it's worth it, which I believe I can.
>>
>> In my experience, having patches checked in is just that - a temporary
>> patch - not a solution. From Oxford's dictionary: "to patch: treat
>> someone's injuries or repair the damage to something, especially
>> hastily" (I know there's also a definition of the noun in the realm of
>> computing).
>>
>> So while I agree that not allowing patches is actually making things
>> harder for some in the short term, I truly believe that it's going to
>> make things better for everyone in the long term.
>>
>> Oskar.
>>
>> On Wed, Sep 22, 2021 at 5:03 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.
>> >
>> > No vendor, I'm confident, is willing to spend endless time
>> > persuading maintainers to include vendor-specific or
>> > platform-specific patches into their repositories.
>> >
>> > 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.
>> >
>> > 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.
>> >
>> > 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.
>> >
>> > 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

Re: New test for patches in openbmc/openbmc

[Ed Tanous]

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

Re: 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@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

Re: [External] Re: New test for patches in openbmc/openbmc

[Lei Yu]

On Fri, Sep 24, 2021 at 6:29 PM Thang Nguyen
<thang@amperemail.onmicrosoft.com> wrote:
>
> 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?
>

I have a similar case.
As an x86 system, some of the recipes/changes are referenced from
Intel-BMC, which is not upstreamed.
Currently, we had patches related to UART routing and
phosphor-node-manager-proxy.
The UART routing patches are being upstreamed thanks to Troy.
The change to node-manager is related to the HW design difference, and
due to the fact that phosphor-node-manager-proxy is in Intel-BMC, we
can not really make the patch upstream.

How do we handle such cases?

-- 
BRs,
Lei YU

Re: [External] Re: New test for patches in openbmc/openbmc

[Ed Tanous]

On Fri, Sep 24, 2021 at 4:06 AM Lei Yu <yulei.sh@bytedance.com> wrote:
>
> On Fri, Sep 24, 2021 at 6:29 PM Thang Nguyen
> <thang@amperemail.onmicrosoft.com> wrote:
> >
> > 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?
> >
>
> I have a similar case.
> As an x86 system, some of the recipes/changes are referenced from
> Intel-BMC, which is not upstreamed.
> Currently, we had patches related to UART routing and
> phosphor-node-manager-proxy.
> The UART routing patches are being upstreamed thanks to Troy.
> The change to node-manager is related to the HW design difference, and
> due to the fact that phosphor-node-manager-proxy is in Intel-BMC, we
> can not really make the patch upstream.

I'm not following why that's preventing upstreaming.  If
node-manager-proxy is something you need on your systems, I don't see
a reason why we would avoid cleaning it up and upstreaming it, but I
have no details on what this patch is, or what it does, so it's really
hard to talk in concrete terms about how to proceed next.

>
> How do we handle such cases?
>
> --
> BRs,
> Lei YU

Re: [External] Re: New test for patches in openbmc/openbmc

[Lei Yu]

> > I have a similar case.
> > As an x86 system, some of the recipes/changes are referenced from
> > Intel-BMC, which is not upstreamed.
> > Currently, we had patches related to UART routing and
> > phosphor-node-manager-proxy.
> > The UART routing patches are being upstreamed thanks to Troy.
> > The change to node-manager is related to the HW design difference, and
> > due to the fact that phosphor-node-manager-proxy is in Intel-BMC, we
> > can not really make the patch upstream.
>
> I'm not following why that's preventing upstreaming.  If
> node-manager-proxy is something you need on your systems, I don't see
> a reason why we would avoid cleaning it up and upstreaming it, but I
> have no details on what this patch is, or what it does, so it's really
> hard to talk in concrete terms about how to proceed next.

node-manager-proxy is in Intel-BMC, so we really need Intel to
upstream it into openbmc.

-- 
BRs,
Lei YU

Re: [External] Re: New test for patches in openbmc/openbmc

[Lei YU]

It's noticed that the `repotest` is enabled in CI and we got CI
failure due to node-manager's patch:
https://gerrit.openbmc-project.xyz/c/openbmc/openbmc/+/47673

I know the right way is to ask Intel to upstream the node-manager and
fix the issues we met.
But in reality it's not easy and it takes time for Intel to upstream a
repo (and it depends on Intel to decide whether or not to upstream)

@Ed Do we really want to reject such patches?

On Tue, Sep 28, 2021 at 4:37 PM Lei Yu <yulei.sh@bytedance.com> wrote:
>
> > > I have a similar case.
> > > As an x86 system, some of the recipes/changes are referenced from
> > > Intel-BMC, which is not upstreamed.
> > > Currently, we had patches related to UART routing and
> > > phosphor-node-manager-proxy.
> > > The UART routing patches are being upstreamed thanks to Troy.
> > > The change to node-manager is related to the HW design difference, and
> > > due to the fact that phosphor-node-manager-proxy is in Intel-BMC, we
> > > can not really make the patch upstream.
> >
> > I'm not following why that's preventing upstreaming.  If
> > node-manager-proxy is something you need on your systems, I don't see
> > a reason why we would avoid cleaning it up and upstreaming it, but I
> > have no details on what this patch is, or what it does, so it's really
> > hard to talk in concrete terms about how to proceed next.
>
> node-manager-proxy is in Intel-BMC, so we really need Intel to
> upstream it into openbmc.
>
> --
> BRs,
> Lei YU

Re: [External] Re: New test for patches in openbmc/openbmc

[Ed Tanous]

On Fri, Oct 8, 2021 at 1:31 AM Lei YU <mine260309@gmail.com> wrote:
>
> It's noticed that the `repotest` is enabled in CI and we got CI
> failure due to node-manager's patch:
> https://gerrit.openbmc-project.xyz/c/openbmc/openbmc/+/47673
>
> I know the right way is to ask Intel to upstream the node-manager and
> fix the issues we met.
> But in reality it's not easy and it takes time for Intel to upstream a
> repo (and it depends on Intel to decide whether or not to upstream)

If this is something you need, there's no need to wait for Intel, as
that application already has an Apache 2 license.  You are free to
upstream it and maintain it yourself if you don't want to wait for
intel.

>
>
> @Ed Do we really want to reject such patches?

I don't want to reject patches, I want to see them on master in a way
that things can be changed as needs evolve.  This patch is a perfect
example of something that, had we taken the small amount of time to
upstream this small daemon, wouldn't have even been an issue now as
sdbusplus needs to make a very minor change.  As-is, we're effectively
2 levels of fork deep (openbmc upstream -> intel-bmc -> openbmc
upstream only for bytedance systems, which is the source of the
problem, not this patch itself.

>
> On Tue, Sep 28, 2021 at 4:37 PM Lei Yu <yulei.sh@bytedance.com> wrote:
> >
> > > > I have a similar case.
> > > > As an x86 system, some of the recipes/changes are referenced from
> > > > Intel-BMC, which is not upstreamed.
> > > > Currently, we had patches related to UART routing and
> > > > phosphor-node-manager-proxy.
> > > > The UART routing patches are being upstreamed thanks to Troy.
> > > > The change to node-manager is related to the HW design difference, and
> > > > due to the fact that phosphor-node-manager-proxy is in Intel-BMC, we
> > > > can not really make the patch upstream.
> > >
> > > I'm not following why that's preventing upstreaming.  If
> > > node-manager-proxy is something you need on your systems, I don't see
> > > a reason why we would avoid cleaning it up and upstreaming it, but I
> > > have no details on what this patch is, or what it does, so it's really
> > > hard to talk in concrete terms about how to proceed next.
> >
> > node-manager-proxy is in Intel-BMC, so we really need Intel to
> > upstream it into openbmc.
> >
> > --
> > BRs,
> > Lei YU

Re: [External] Re: New test for patches in openbmc/openbmc

[Lei Yu]

On Sat, Oct 9, 2021 at 1:35 AM Ed Tanous <edtanous@google.com> wrote:
>
> On Fri, Oct 8, 2021 at 1:31 AM Lei YU <mine260309@gmail.com> wrote:
> >
> > It's noticed that the `repotest` is enabled in CI and we got CI
> > failure due to node-manager's patch:
> > https://gerrit.openbmc-project.xyz/c/openbmc/openbmc/+/47673
> >
> > I know the right way is to ask Intel to upstream the node-manager and
> > fix the issues we met.
> > But in reality it's not easy and it takes time for Intel to upstream a
> > repo (and it depends on Intel to decide whether or not to upstream)
>
> If this is something you need, there's no need to wait for Intel, as
> that application already has an Apache 2 license.  You are free to
> upstream it and maintain it yourself if you don't want to wait for
> intel.
>
> >
> >
> > @Ed Do we really want to reject such patches?
>
> I don't want to reject patches, I want to see them on master in a way
> that things can be changed as needs evolve.  This patch is a perfect
> example of something that, had we taken the small amount of time to
> upstream this small daemon, wouldn't have even been an issue now as
> sdbusplus needs to make a very minor change.  As-is, we're effectively

Totally agree. We have already asked Intel to upstream the
node-manager, let's wait for the feedback.

> 2 levels of fork deep (openbmc upstream -> intel-bmc -> openbmc
> upstream only for bytedance systems, which is the source of the
> problem, not this patch itself.

True. But as an Intel x86 platform, the repo is needed and in the
current state, the patch has to be added. Otherwise the g220a build is
broken.
Is it OK to ignore the repotest CI failure and just merge the patch in
meta-bytedance layer?
(Be noted that it's not trying to make a bad example, but only trying
to fix the broken build)

Re: [External] Re: New test for patches in openbmc/openbmc

[Ed Tanous]

On Fri, Oct 8, 2021 at 7:18 PM Lei Yu <yulei.sh@bytedance.com> wrote:
>
> On Sat, Oct 9, 2021 at 1:35 AM Ed Tanous <edtanous@google.com> wrote:
> >
> > On Fri, Oct 8, 2021 at 1:31 AM Lei YU <mine260309@gmail.com> wrote:
> > >
> > > It's noticed that the `repotest` is enabled in CI and we got CI
> > > failure due to node-manager's patch:
> > > https://gerrit.openbmc-project.xyz/c/openbmc/openbmc/+/47673
> > >
> > > I know the right way is to ask Intel to upstream the node-manager and
> > > fix the issues we met.
> > > But in reality it's not easy and it takes time for Intel to upstream a
> > > repo (and it depends on Intel to decide whether or not to upstream)
> >
> > If this is something you need, there's no need to wait for Intel, as
> > that application already has an Apache 2 license.  You are free to
> > upstream it and maintain it yourself if you don't want to wait for
> > intel.
> >
> > >
> > >
> > > @Ed Do we really want to reject such patches?
> >
> > I don't want to reject patches, I want to see them on master in a way
> > that things can be changed as needs evolve.  This patch is a perfect
> > example of something that, had we taken the small amount of time to
> > upstream this small daemon, wouldn't have even been an issue now as
> > sdbusplus needs to make a very minor change.  As-is, we're effectively
>
> Totally agree. We have already asked Intel to upstream the
> node-manager, let's wait for the feedback.'

Let's not wait any longer.  The code is licensed appropriately, is
already open source, and in total, is smaller than a number of single
patchsets I've seen in recent history (it would probably classify as a
medium patchset).  Just open a review to add the code to an existing
repository, or request a new repository along with a design doc.  If
you want this to live in dbus-sensors, I'm fine with that, just make
sure we have maintainers that can test on their systems;  It seems
like an ok fit given it's another entity-manager enabled sensor app,
although I don't have a strong opinion between the two options.


>
> > 2 levels of fork deep (openbmc upstream -> intel-bmc -> openbmc
> > upstream only for bytedance systems, which is the source of the
> > problem, not this patch itself.
>
> True. But as an Intel x86 platform, the repo is needed and in the
> current state, the patch has to be added. Otherwise the g220a build is
> broken.
> Is it OK to ignore the repotest CI failure and just merge the patch in
> meta-bytedance layer?
> (Be noted that it's not trying to make a bad example, but only trying
> to fix the broken build)

Re: New test for patches in openbmc/openbmc

[Ed Tanous]

On Fri, Sep 24, 2021 at 3:29 AM Thang Nguyen
<thang@amperemail.onmicrosoft.com> wrote:
>
> 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?

I don't have great experience in u-boot patches, or our OpenBMC
procedures there.  If nobody replies you directly here, I'd start a
discussion in the kernel-and-uboot track on discord and see what they
say to do next.

>
> 2. After completing patch removal from meta- folder, can we still add
> patch into meta- folder in the local repo?

As always, you are free to do whatever you like locally, although I
would recommend against creating a large number of patches, and treat
it as a temporary measure at worst.

> 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.

Assuming your follow through on upstreaming, that sounds like a
reasonable development model.

>
>
> 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@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