[BUG] kernel reporting bug when using same major ABI with different minor ABIs  #822

Activity

1. Jiangxinx

   added

   bugSomething isn't workingSomething isn't working

   BYTApplies to Baytrail platformApplies to Baytrail platform

   APLApplies to ApolloLake platformApplies to ApolloLake platform

   WHLApplies to WhiskeyLake platformApplies to WhiskeyLake platform

   on Apr 12, 2019
2. mengdonglin

   assigned

   bardliao

   on Apr 12, 2019
3. lgirdwood commented on Apr 15, 2019

   

   lgirdwood

   on Apr 15, 2019

   Member

   More actions

   @Jiangxinx this is incorrectly being reported as a bug by the kernel. I've updated the title.
   @plbossart fyi.
4. lgirdwood

   changed the title [-][BUG] Error: Incompatible ABI version on all platforms.[/-] [+][BUG] kernel reporting bug when using same major ABI with different minor ABIs [/+] on Apr 15, 2019
5. lgirdwood commented on Apr 15, 2019

   

   lgirdwood

   on Apr 15, 2019

   Member

   More actions

   @bardliao only major ABI updates are incompatible. Minor ABI version changes are backwards compatible. Can you fix this error handling in the kernel.
6. ranj063 commented on Apr 15, 2019

   

   ranj063

   on Apr 15, 2019

   Last edited by ranj063

   Collaborator

   More actions

   @lgirdwood the check for minor ABI was intentional in the kernel as even minor ABI updates could require new tokens in the tplg that may not have previously existed.
7. lgirdwood commented on Apr 16, 2019

   

   lgirdwood

   on Apr 16, 2019

   Member

   More actions

   @ranj063 new or unknown tokens should just be ignored by kernel. The point is that the new feature will work as long as both minor versions are compatible but existing features will work regardless (as long as major ABI is compatible).
8. lgirdwood commented on Apr 16, 2019

   

   lgirdwood

   on Apr 16, 2019

   Last edited by lgirdwood

   Member

   More actions

   @xiulipan to fix this you need

   1. Dont fail on minor ABI differences.
   2. Dont fail on unknown DAI types
   3. Dont fail on unknown component types (not below)
   4. Delete any pipeline that fails to complete

   1,2 and 4 are below. Need a separate patch for each. Build tested only.

   diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c
   index 2b9de1b97447..31454cf57050 100644
   --- a/sound/soc/sof/topology.c
   +++ b/sound/soc/sof/topology.c
   @@ -2790,8 +2790,7 @@ static int sof_link_load(struct snd_soc_component *scomp, int index,
                                           &config);
                   break;
           default:
   -               dev_err(sdev->dev, "error: invalid DAI type %d\n", config.type);
   -               ret = -EINVAL;
   +               dev_warn(sdev->dev, "warn: unknowm DAI type %d\n", config.type);
                   break;
           }
           if (ret < 0)
   @@ -3005,6 +3004,7 @@ int snd_sof_complete_pipeline(struct snd_sof_dev *sdev,
    {
           struct sof_ipc_pipe_ready ready;
           struct sof_ipc_reply reply;
   +       struct sof_ipc_pipe_free free;
           int ret;
    
           dev_dbg(sdev->dev, "tplg: complete pipeline %s id %d\n",
   @@ -3018,8 +3018,27 @@ int snd_sof_complete_pipeline(struct snd_sof_dev *sdev,
           ret = sof_ipc_tx_message(sdev->ipc,
                                    ready.hdr.cmd, &ready, sizeof(ready), &reply,
                                    sizeof(reply));
   -       if (ret < 0)
   -               return ret;
   +
   +       /* pipeline completion may fail for a number of reasons */
   +       if (ret < 0) {
   +
   +               /* delete pipeline from DAPM */
   +               snd_soc_tplg_widget_remove_all(swidget->widget->dapm,
   +                                              swidget->comp_id);
   +
   +               /* dismantle this pipeline to free up DSP resources */
   +               memset(&free, 0, sizeof(free));
   +               free.hdr.size = sizeof(free);
   +               free.hdr.cmd = SOF_IPC_GLB_TPLG_MSG | SOF_IPC_TPLG_PIPE_FREE;
   +               free.comp_id = swidget->comp_id;
   +
   +               ret = sof_ipc_tx_message(sdev->ipc,
   +                                ready.hdr.cmd, &ready, sizeof(ready), &reply,
   +                                sizeof(reply));
   +               if (ret < 0)
   +                       return ret;
   +       }
   +
           return 1;
    }
    
   @@ -3066,8 +3085,7 @@ static int sof_manifest(struct snd_soc_component *scomp, int index,
                            man->priv.data[0], man->priv.data[1],
                            man->priv.data[2], SOF_ABI_MAJOR, SOF_ABI_MINOR,
                            SOF_ABI_PATCH);
   -               if (SOF_ABI_VER(man->priv.data[0], man->priv.data[1],
   -                               man->priv.data[2]) <= SOF_ABI_VERSION)
   +               if (man->priv.data[0] == SOF_ABI_MAJOR)
                           return 0;
           }
           dev_err(sdev->dev,
   
9. bardliao commented on Apr 16, 2019

   

   bardliao

   on Apr 16, 2019

   Collaborator

   More actions

   @ranj063 @plbossart I tried to add new dai token in topology and kernel. Firstly, I checked the value of comp_dai.reserve to make sure my patch works. Secondly, reverted the patch on kernel side and confirmed the topology will ignore it without error. So I think it's fine if we only check ABI major version in topology. And it's FW's duty to make sure it's backward compatible if ABI major version is not changed.
   On the other hand, we will get ipc error if FW or tplg is not compatible with this kernel. Is there any significant difference when we load topology failed with ipc error or ABI checking error?
10. plbossart commented on Apr 16, 2019

    

    plbossart

    on Apr 16, 2019 via email

    Member

    More actions

    On 4/16/19 3:53 AM, Liam Girdwood wrote: @xiulipan <https://github.com/xiulipan> to fix this you need 1. Dont fail on minor ABI differences. 2. Dont fail on unknown DAI types 3. Dont fail on unknown component types (not below) 4. Delete any pipeline that fails to complete

    Sorry mate, I don't agree here. You are essentially removing all error checks on the grounds that a 'minor' ABI needs to be tolerated, without a discussion on what 'Minor' means. If you pass a topology that contains stuff that the kernel cannot functionally handle, there is no point in continuing the parsing. The solution will be broken anyhow, the user will not get what is specified in the topology. Better fail early and fail big than try to continue without e.g. registering a DAI that will be connected later on to other components. I am fine with tolerating fields that build on top of existing components and DAIs. New component types and new DAIs are NOT a minor addition, sorry.
11. xiulipan commented on Apr 16, 2019

    

    xiulipan

    on Apr 16, 2019

    More actions

    @plbossart @lgirdwood
    My suggestion here it to revert the ABI changes on SOF first. And have some discussion about how should we deal with the ABI changes. And then make some written-down rules or guides for ABI and make somebody( I will be volunteer if nobody want. I do not want to see chaos like this anymore caused by ABI) to have manual test to make sure every ABI change is fully reviewed and can be backwards compatible. Also need to keep a time alignment for linux PR and sof PR merging.

    I do not like to keep some un-decided changes that blocks everything now. I do see some PR that is merged without basically test. We can indeed to apply some workaround to have manual test but now we are shorthanded and can not make sure to cover every PR.
12. lgirdwood commented on Apr 17, 2019

    

    lgirdwood

    on Apr 17, 2019

    Member

    More actions

    1. Dont fail on minor ABI differences.
    2. Dont fail on unknown DAI types
    3. Dont fail on unknown component types (not below)
    4. Delete any pipeline that fails to complete

    Sorry mate, I don't agree here. You are essentially removing all error checks on the grounds that a 'minor' ABI needs to be tolerated, without a discussion on what 'Minor' means. If you pass a topology that contains stuff that the kernel cannot functionally handle, there is no point in continuing the parsing.

    Yes there is, I agree the pipeline that contains the new feature will not exist, but other pipelines/components should still be created without failing big. The implications for failing big are no audio whilst there may be audio if we can continue (depending on topology).

    The solution will be broken anyhow, the user will not get what is specified in the topology.

    Yes this will happen, there will be error/warnings in the log showing the ABI delta and missing components. The end result is the same as failing big in that the user needs to update his kernel to get the new feature working, but at least he may have working audio until that happens if we continue.

    Better fail early and fail big than try to continue without e.g. registering a DAI that will be connected later on to other components. I am fine with tolerating fields that build on top of existing components and DAIs. New component types and new DAIs are NOT a minor addition, sorry.

    If we make new DAIs or new components a MAJOR ABI change then we make a rod for our own back.
    Every MAJOR updates would mean.

    1. All SOF releases from all vendors will need to be updated on firmware git otherwise we fail big.
    2. All topologies need to be updated and merged by all distros. Also include Chrome/Android here too.
    3. Kernel ABI update need upstreamed and justified why we are changing MAJOR ABI continually.

    Now lets try and keep 1,2 and 3 above in sync to avoid the angry email from Linus when his laptop audio breaks. i.e. it was working yesterday but no longer works now after updating kernel, FW, topology.

    To further make things harder, we will have different SOF vendors adding new DAIs and components, meaning managing 1,2 and 3 on an ad-hoc and potentially frequent basis.

    New components and DAIs should be MINOR, just like the relationship versioning between kernel, glibc and userspace. New kernel, glibc API features are added via MINOR updates and never break existing userspace. See the man pages, there are lots of references for "only available in kernel x.y onward" or "only in glibc x.y onward". glibc MAJOR has not changed in about 20 years AFAICT.
13. plbossart commented on Apr 17, 2019

    

    plbossart

    on Apr 17, 2019

    Member

    More actions

    @lgirdwood So you are essentially saying that we never use the MAJOR version and never check for MINOR versions either. Why do we have an ABI check then? Nothing is ever incompatible in your description.
14. plbossart commented on Apr 17, 2019

    

    plbossart

    on Apr 17, 2019

    Last edited by plbossart

    Member

    More actions

    I will also argue that someone using a firmware+topology more recent than the kernel does not fall in the category of 'we don't break userspace'. The only compatibility we care about is people updating the kernel and working with older firmware+topology. The case of using a firmware+topology more recent than the kernel is maybe valid for developers, certainly not for distros.
15. plbossart

    closed this as completedin #823on Apr 17, 2019