Linux system internal refactoring

[glneo]

Set of interface cleanups for the Linux system. Mostly removing dead code. Should result in no functional change.

[arnopo]

This code is legacy code. some of the APIs can be used by AMD on using the libmetal in Linux user space
Need to be approved by @tnmysh

[tnmysh]

OpenAMP is one of the client of Libmetal library but not only client of libmetal library. Libmetal can be used by other userspace drivers as well. I see usage of libmetal here on github wide search: https://github.com/Xilinx/embeddedsw/blob/c9a0ee31b2a14cbcfcb56ca369037319b4ad4847/XilinxProcessorIPLib/drivers/rfdc/src/xrfdc_sinit.c#L150

While I agree metal_linux_get_device_property is specific to linux, removing it as it is will break backward compatibility for userspace drivers that are using them (may be not OpenAMP). Ideally "metal_device_get_property" should be provided, and metal_linux_get_device_property could be its linux implementation. Then proper deprecation method need to be followed where users should be given enough time to switch to new API and then linux_get_device_property can be deprecated.

I am on leave right now, but this can be discussed once I am back.
For other APIs same, can be discussed individually.

[github-actions]

This pull request has been marked as a stale pull request because it has been open (more than) 45 days with no activity.

[glneo]

I'll drop the patch that removes metal_linux_get_device_property from this series for now. But we should circle back to it later, perhaps if that XilinxProcessorIPLib project you linked makes use of it still then it could be moved into that project so we can deprecate it out of here. That way libmetal doesn't turn into a dumping ground for miscellaneous helper functions, instead keeping it a more focused and generic HAL.

[edmooring]

My big concern is that there are users of libmetal outside of the OpenAMP library. There are several within Xilinx/AMD, and potentially more that we don't know about.

I would prefer to see these API changes go through the deprecation process, so potential outside users have at least some warning.

[glneo]

I would prefer to see these API changes go through the deprecation process, so potential outside users have at least some warning.

These functions are not exposed externally as they are wrapped in an ifdef check for METAL_INTERNAL. This is only to be defined when building libmetal. If anyone defines that in their build to get access to these lib internal-only function then they are abusing the API this library provides. So external user already have a warning not to use these function, it is METAL_INTERNAL.

[github-actions]

This pull request has been marked as a stale pull request because it has been open (more than) 45 days with no activity.

[wmamills]

Discussed 2024/09/05: Need to move this stuff to a legacy header and mark it obsolete.
Legacy header & code will only be used if a LEGACY define is defined at build time.
The default for LEGACY is debatable but if enabled by default the developer should get deprecation warnings

[glneo]

As stated in the above comment (2 months ago): These functions are not exposed externally as they are wrapped in an ifdef check for METAL_INTERNAL.

There is nothing to add to this deprecation file as no external facing API is being deprecated. We are only removing internal use only code. This is a completely internal refactor/cleanup.

[arnopo]

As stated in the above comment (2 months ago): These functions are not exposed externally as they are wrapped in an ifdef check for METAL_INTERNAL.

There is nothing to add to this deprecation file as no external facing API is being deprecated. We are only removing internal use only code. This is a completely internal refactor/cleanup.

I mainly agree with @glneo's analysis. However, I suppose that these functions were initially added by AMD for a reason. This point should be clarified to determine if the functions can be removed or if they are not truly internal.

[tnmysh]

As stated in the above comment (2 months ago): These functions are not exposed externally as they are wrapped in an ifdef check for METAL_INTERNAL.
There is nothing to add to this deprecation file as no external facing API is being deprecated. We are only removing internal use only code. This is a completely internal refactor/cleanup.

I mainly agree with @glneo's analysis. However, I suppose that these functions were initially added by AMD for a reason. This point should be clarified to determine if the functions can be removed or if they are not truly internal.

Correct. I am not sure why they were exported, it is pretty old code. Just for reference, libmetal is not only used for OpenAMP backend, but also used in a standalone fashion. I agree with commits presented in this PR. However, I see functions are exported via sys.h header, and it's not really possible to know how these are being used by users. Hence, as per code of conduct, we should follow deprecation process.

From next time, we can make sure not to export functions that are internal to libmetal. Once functions are exported, it is assumed that users are using them.
That is why we should provide option to enable this functionality, and later it can be removed at once.

[arnopo]

As stated in the above comment (2 months ago): These functions are not exposed externally as they are wrapped in an ifdef check for METAL_INTERNAL.
There is nothing to add to this deprecation file as no external facing API is being deprecated. We are only removing internal use only code. This is a completely internal refactor/cleanup.

I mainly agree with @glneo's analysis. However, I suppose that these functions were initially added by AMD for a reason. This point should be clarified to determine if the functions can be removed or if they are not truly internal.

Correct. I am not sure why they were exported, it is pretty old code. Just for reference, libmetal is not only used for OpenAMP backend, but also used in a standalone fashion. I agree with commits presented in this PR. However, I see functions are exported via sys.h header, and it's not really possible to know how these are being used by users. Hence, as per code of conduct, we should follow deprecation process.

From next time, we can make sure not to export functions that are internal to libmetal. Once functions are exported, it is assumed that users are using them. That is why we should provide option to enable this functionality, and later it can be removed at once.

The point highlighted by @glneo is that these functions are under the specific METAL_INTERNAL condition, so we should consider them as "non-exported." I would be in favor of first confirming whether AMD uses them outside of libmetal (as they are the owners of this part of the legacy code).

• If AMD uses them, we have to move them out of the METAL_INTERNAL condition.
• If not used, we should consider them as they are and remove the useless functions. I would also suggest that we create internal_xxx.h files to separate internal and exported APIs.

[glneo]

I would also suggest that we create internal_xxx.h files to separate internal and exported APIs.

That is what I was originally working on when I came across these unused functions, moving the remaining used internal functions out of the exported headers is the next PR in my queue :)

[tnmysh]

LGTM.