On 04/05/2023 14:57, Mukesh Ojha wrote:

On 5/4/2023 6:06 PM, Krzysztof Kozlowski wrote:

quoted

On 04/05/2023 14:26, Mukesh Ojha wrote:

quoted

On 5/4/2023 5:33 PM, Krzysztof Kozlowski wrote:

quoted

On 04/05/2023 13:58, Mukesh Ojha wrote:

quoted

On 5/4/2023 5:08 PM, Krzysztof Kozlowski wrote:

quoted

On 03/05/2023 19:02, Mukesh Ojha wrote:

quoted

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.

quoted

There is no change in functional behavior after this.

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

quoted

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 .

I am not sure if I understand correctly. If they are all required by
minidump driver, then this must not be in include, but stay with
minidump. Remoteproc then should not touch it.

I don't understand why internals of minidump should be important for
remoteproc. If they are, means you broken encapsulation.

quoted

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

Let's be specific: why MD_REGION_VALID must be available for remoteproc
or any other driver after introducing qcom minidump driver?

Forget about this driver for a moment.

I am not sure  how much you know about existing qcom_minidump()
implementation and why is it there in first place in remoteproc
code in driver/remoteproc/qcom_common.c

The idea is, remoteproc co-processor like adsp/cdsp etc. may have their
static predefined region (segments) to be collected on their crash which 
is what exactly existing qcom_minidump() is doing.

Now, after this minidump series, APSS (linux) will have it's
own of collecting linux client region independent of whether
remoteproc minidump collection.

I think, are you hinting to move all minidump related code from 
remoteproc to qcom_minidump driver, is this what are you trying
to say ?

Close, not all but the ones not necessary to identify the
regions/storage/layout. If some variable about this
region/storage/layout is the same everywhere, it means it's basically a
property of qcom minidump and you have just exposed it to consumers
breaking encapsulation.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel