diff mbox series

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

Message ID 1683133352-10046-3-git-send-email-quic_mojha@quicinc.com (mailing list archive)
State Handled Elsewhere
Headers show
Series [v3,01/18] remoteproc: qcom: Expand MD_* as MINIDUMP_* | expand

Commit Message

Mukesh Ojha May 3, 2023, 5:02 p.m. UTC
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

Krzysztof Kozlowski May 4, 2023, 11:38 a.m. UTC | #1
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
Mukesh Ojha May 4, 2023, 11:58 a.m. UTC | #2
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
>
Krzysztof Kozlowski May 4, 2023, 12:03 p.m. UTC | #3
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
Mukesh Ojha May 4, 2023, 12:26 p.m. UTC | #4
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
>
Krzysztof Kozlowski May 4, 2023, 12:36 p.m. UTC | #5
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
Mukesh Ojha May 4, 2023, 12:57 p.m. UTC | #6
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
>
Krzysztof Kozlowski May 4, 2023, 3:16 p.m. UTC | #7
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
diff mbox series

Patch

diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
index 805e525..88fc984 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -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;
diff --git a/include/soc/qcom/qcom_minidump.h b/include/soc/qcom/qcom_minidump.h
new file mode 100644
index 0000000..84c8605
--- /dev/null
+++ b/include/soc/qcom/qcom_minidump.h
@@ -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_ */