| Message ID | 1683133352-10046-3-git-send-email-quic_mojha@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v3,01/18] remoteproc: qcom: Expand MD_* as MINIDUMP_* | expand |
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
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_ */
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