Re: [meta-ti][master/kirkstone][PATCH 8/8] recipes-bsp: Do not use MACHINE_ARCH when package is not machine specific

[Denys Dmytriyenko <denis@denix.org>]

On Thu, Nov 02, 2023 at 09:57:11AM -0500, Ryan Eatmon wrote:
> 
> 
> On 10/26/2023 9:00 AM, Andrew Davis wrote:
> >On 10/25/23 10:27 PM, Denys Dmytriyenko wrote:
> >>On Wed, Oct 25, 2023 at 11:56:30AM -0500, Andrew Davis via
> >>lists.yoctoproject.org wrote:
> >>>Signed-off-by: Andrew Davis <afd@ti.com>
> >>>---
> >>>  meta-ti-bsp/recipes-bsp/cadence-mhdp-fw/cadence-mhdp-fw_git.bb | 2 --
> >>>  meta-ti-bsp/recipes-bsp/cnm-wave-fw/cnm-wave-fw_git.bb         | 2 --
> >>>  meta-ti-bsp/recipes-bsp/cpsw9g-eth-fw/cpsw9g-eth-fw_git.bb     | 1 -
> >>>  meta-ti-bsp/recipes-bsp/goodix-fw/goodix-fw_git.bb             | 2 --
> >>>  meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x-sr2_git.bb   | 2 --
> >>>  meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x_git.bb       | 2 --
> >>>  meta-ti-bsp/recipes-bsp/pruhsr-fw/pruhsr-fw-am65x-sr2_git.bb   | 2 --
> >>>  meta-ti-bsp/recipes-bsp/prusw-fw/prusw-fw-am65x-sr2_git.bb     | 2 --
> >>>  meta-ti-bsp/recipes-bsp/ti-img-encode-decode/vxd-dec-fw_git.bb | 2 --
> >>>  meta-ti-bsp/recipes-bsp/vis-fw/vis_01.50.07.15.bb              | 1 -
> >>>  meta-ti-bsp/recipes-bsp/vpdma-fw/vpdma-fw_03-2012.bb           | 1 -
> >>>  11 files changed, 19 deletions(-)
> >>
> >>Overall I agree and fully support the first 7 patches in this series.
> >>
> >>But for this last one I wanted to open a discussion.
> >>
> >
> >I try to sort my series in least-to-most likely to be controversial, I was
> >just wondering how far we would get down the list, glad we got to 8 :)
> >
> >>On one hand I understand the desire to make components as
> >>generic as possible
> >>and reduce the number of machine-specific components to a bare minimum.
> >>
> >>But on another hand, marking the resulting package as
> >>machine-specific when it
> >>has a short list of compatible machines is a standard practice.
> >>The reason is
> >>that the list of compatible machines controls only compile time
> >>filtering, but
> >>doesn't have any effect on run time. And marking packages as
> >>machine specific
> >>helps with that. That closes the loophole of installing
> >>incompatible packages.
> >>
> >>For example, first recipe below specifies that Cadence MHDP firmware is
> >>compatible with 3 J7 platforms only (or their SoC families, to be exact).
> >>But w/o marking resulting binary package as machine-specific (therefore
> >>producing separate packages for those platforms), there will be a single
> >>generic Aarch64 package made. And there's no protection from installing
> >>this generic package on non-compatible platforms, like J7200 or AM65xx,
> >>either manullay or by pulling it into a rootfs for those incompatible
> >>platforms.
> >>
> >>And you normally want to prevent this for regular components. But I guess
> >>this doesn't fully apply to FW images that are loaded by corresponding
> >>drivers anyway. Moreover, there's no compilation involved, just packaging
> >>the binary blob.
> >>
> >>In that case, should we also remove COMPATIBLE_MACHINE from
> >>these firmware
> >>recipes?
> >>
> >
> >That is exactly where I was going to go next. These firmware packages are
> >not technically incompatible with the other machines.
> >
> >We just use COMPATIBLE_MACHINE checks here to keep us from
> >accidentily bundling
> >them with images where they wouldn't add any value (which you did
> >with prusw-fw
> >which is what stated me thinking on all this). But since they don't break
> >anything either, forcing them to be machine specific seemed like
> >overkill also.
> >
> >Only place where we still need this is firmware recipes that only
> >ship some of
> >the firmware based on machine (see prueth-fw for instance). I'd like to get
> >that cleaned up next. If we only want some of the firmware then we
> >should split
> >it into different packages (prueth-fw-am57xx, etc.) and only
> >install the one
> >we want for that platform.
> 
> Denys, does Andrew's response address your concerns?

Well, I do believe we are generally in agreement here. But I'm not sure if 
that means we should merge patch #8 as is and address COMPATIBLE_MACHINE 
changes later, or rework the patch to address that with MACHINE_ARCH together?

-- 
Denys