[v3,02/18] remoteproc: qcom: Move minidump specific data to qcom_minidump.h
Commit Message
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.
There is no change in functional behavior after this.
Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
drivers/remoteproc/qcom_common.c | 56 +---------------------------------
include/soc/qcom/qcom_minidump.h | 66 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 67 insertions(+), 55 deletions(-)
create mode 100644 include/soc/qcom/qcom_minidump.h
Comments
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.
Best regards,
Krzysztof
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.
-Mukesh
>
> Best regards,
> Krzysztof
>
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.
Best regards,
Krzysztof
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@quicinc.com/
-- Mukesh
>
> Best regards,
> Krzysztof
>
On 04/05/2023 14:26, Mukesh Ojha wrote:
>
>
> 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 .
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.
>
> 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?
Best regards,
Krzysztof
On 5/4/2023 6:06 PM, Krzysztof Kozlowski wrote:
> On 04/05/2023 14:26, Mukesh Ojha wrote:
>>
>>
>> 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 .
>
> 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.
>
>>
>> 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 ?
-- Mukesh
>
>
> Best regards,
> Krzysztof
>
On 04/05/2023 14:57, Mukesh Ojha wrote:
>
>
> On 5/4/2023 6:06 PM, Krzysztof Kozlowski wrote:
>> On 04/05/2023 14:26, Mukesh Ojha wrote:
>>>
>>>
>>> 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 .
>>
>> 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.
>>
>>>
>>> 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
@@ -18,6 +18,7 @@
#include <linux/slab.h>
#include <linux/soc/qcom/mdt_loader.h>
#include <linux/soc/qcom/smem.h>
+#include <soc/qcom/qcom_minidump.h>
#include "remoteproc_internal.h"
#include "qcom_common.h"
@@ -26,61 +27,6 @@
#define to_smd_subdev(d) container_of(d, struct qcom_rproc_subdev, subdev)
#define to_ssr_subdev(d) container_of(d, struct qcom_rproc_ssr, subdev)
-#define MAX_NUM_OF_SS 10
-#define MAX_REGION_NAME_LENGTH 16
-#define SBL_MINIDUMP_SMEM_ID 602
-#define MINIDUMP_REGION_VALID ('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' << 0)
-#define MINIDUMP_SS_ENCR_DONE ('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0)
-#define MINIDUMP_SS_ENABLED ('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
-
-/**
- * struct minidump_region - Minidump region
- * @name : Name of the region to be dumped
- * @seq_num: : Use to differentiate regions with same name.
- * @valid : This entry to be dumped (if set to 1)
- * @address : Physical address of region to be dumped
- * @size : Size of the region
- */
-struct minidump_region {
- char name[MAX_REGION_NAME_LENGTH];
- __le32 seq_num;
- __le32 valid;
- __le64 address;
- __le64 size;
-};
-
-/**
- * struct minidump_subsystem - Subsystem's SMEM Table of content
- * @status : Subsystem toc init status
- * @enabled : if set to 1, this region would be copied during coredump
- * @encryption_status: Encryption status for this subsystem
- * @encryption_required : Decides to encrypt the subsystem regions or not
- * @region_count : Number of regions added in this subsystem toc
- * @regions_baseptr : regions base pointer of the subsystem
- */
-struct minidump_subsystem {
- __le32 status;
- __le32 enabled;
- __le32 encryption_status;
- __le32 encryption_required;
- __le32 region_count;
- __le64 regions_baseptr;
-};
-
-/**
- * struct minidump_global_toc - Global Table of Content
- * @status : Global Minidump init status
- * @md_revision : Minidump revision
- * @enabled : Minidump enable status
- * @subsystems : Array of subsystems toc
- */
-struct minidump_global_toc {
- __le32 status;
- __le32 md_revision;
- __le32 enabled;
- struct minidump_subsystem subsystems[MAX_NUM_OF_SS];
-};
-
struct qcom_ssr_subsystem {
const char *name;
struct srcu_notifier_head notifier_list;
new file mode 100644
@@ -0,0 +1,66 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Qualcomm minidump shared data structures and macros
+ *
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#ifndef _QCOM_MINIDUMP_H_
+#define _QCOM_MINIDUMP_H_
+
+#define MAX_NUM_OF_SS 10
+#define MAX_REGION_NAME_LENGTH 16
+#define SBL_MINIDUMP_SMEM_ID 602
+#define MINIDUMP_REGION_VALID ('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' << 0)
+#define MINIDUMP_SS_ENCR_DONE ('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0)
+#define MINIDUMP_SS_ENABLED ('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
+
+/**
+ * struct minidump_region - Minidump region
+ * @name : Name of the region to be dumped
+ * @seq_num: : Use to differentiate regions with same name.
+ * @valid : This entry to be dumped (if set to 1)
+ * @address : Physical address of region to be dumped
+ * @size : Size of the region
+ */
+struct minidump_region {
+ char name[MAX_REGION_NAME_LENGTH];
+ __le32 seq_num;
+ __le32 valid;
+ __le64 address;
+ __le64 size;
+};
+
+/**
+ * struct minidump_subsystem - Subsystem's SMEM Table of content
+ * @status : Subsystem toc init status
+ * @enabled : if set to 1, this region would be copied during coredump
+ * @encryption_status: Encryption status for this subsystem
+ * @encryption_required : Decides to encrypt the subsystem regions or not
+ * @region_count : Number of regions added in this subsystem toc
+ * @regions_baseptr : regions base pointer of the subsystem
+ */
+struct minidump_subsystem {
+ __le32 status;
+ __le32 enabled;
+ __le32 encryption_status;
+ __le32 encryption_required;
+ __le32 region_count;
+ __le64 regions_baseptr;
+};
+
+/**
+ * struct minidump_global_toc - Global Table of Content
+ * @status : Global Minidump init status
+ * @md_revision : Minidump revision
+ * @enabled : Minidump enable status
+ * @subsystems : Array of subsystems toc
+ */
+struct minidump_global_toc {
+ __le32 status;
+ __le32 md_revision;
+ __le32 enabled;
+ struct minidump_subsystem subsystems[MAX_NUM_OF_SS];
+};
+
+#endif /* _QCOM_MINIDUMP_H_ */