Re: [PATCH v3 02/18] remoteproc: qcom: Move minidump specific data to qcom_minidump.h

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 5/4/2023 5:33 PM, Krzysztof Kozlowski wrote:
On 04/05/2023 13:58, Mukesh Ojha wrote:


On 5/4/2023 5:08 PM, Krzysztof Kozlowski wrote:
On 03/05/2023 19:02, Mukesh Ojha wrote:
Move minidump specific data types and macros to a separate internal
header(qcom_minidump.h) so that it can be shared among different
Qualcomm drivers.

No, this is not internal header. You moved it to global header.

There is no reason driver internals should be exposed to other unrelated
subsystems.


There is no change in functional behavior after this.

It is. You made all these internal symbols available to others.



This comes without justification why other drivers needs to access
private and internal data. It does not look correct design. NAK.

Thanks for catching outdated commit text, will fix the commit with
more descriptive reasoning.

It has to be global so that co-processor minidump and apss minidump can
share data structure and they are lying in different directory.


Then you should not share all the internals of memory layout but only
few pieces necessary to talk with minidump driver. The minidump driver
should organize everything how it wants.

These are core data structure which is shared with boot firmware and the
one's are moved here all are required by minidump driver .

If you follow here[1], i raised by concern to make this particular one's
as private and later to avoid confusion went with single header.
But if others agree, I will keep the one that get shared with minidump
as separate one or if relative path of headers are allowed that can make
it private between these drivers(which i don't think, will be allowed or
recommended).

[1]
https://lore.kernel.org/lkml/3df1ec27-7e4d-1f84-ff20-94e8ea91c86f@xxxxxxxxxxx/

-- Mukesh

Best regards,
Krzysztof




[Index of Archives]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Photo Sharing]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux