| Message ID | 1683133352-10046-1-git-send-email-quic_mojha@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
On 3.05.2023 19:02, Mukesh Ojha wrote: > This enable dynamic reserve memory for Qualcomm ramoops device, > Which will used to save ramoops frontend data and this region > gets dumped on crash via Qualcomm's minidump infrastructure. > qcom_pstore_minidump is the associated driver for this node. > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > --- > arch/arm64/boot/dts/qcom/sm8450.dtsi | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi > index 595533a..92d023f 100644 > --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi > @@ -614,6 +614,17 @@ > reg = <0x0 0xed900000 0x0 0x3b00000>; > no-map; > }; > + > + qcom_ramoops_md_region:qcom_ramoops_md { Missing space after ':' node names should not contain underscores minidump { or ramoops { would probably be better names for this node > + alloc-ranges = <0x0 0x00000000 0xffffffff 0xffffffff>; > + size = <0x0 0x200000>; > + no-map; > + }; > + }; > + > + qcom_ramoops_md { Node names should be generic (e.g. ramdump or something) and should not contain underscores. Konrad > + compatible = "qcom,sm8450-ramoops-minidump", "qcom,ramoops-minidump"; > + memory-region = <&qcom_ramoops_md_region>; > }; > > smp2p-adsp {
On 03/05/2023 19:02, Mukesh Ojha wrote: > Previous patches add the Qualcomm minidump driver support, so > lets enable minidump config so that it can be used by kernel > clients. > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> This patchset is split too much. Defconfig change is one change. Not two or three. > --- > arch/arm64/configs/defconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig > index a24609e..831c942 100644 > --- a/arch/arm64/configs/defconfig > +++ b/arch/arm64/configs/defconfig > @@ -1250,6 +1250,7 @@ CONFIG_QCOM_STATS=m > CONFIG_QCOM_WCNSS_CTRL=m > CONFIG_QCOM_APR=m > CONFIG_QCOM_ICC_BWMON=m > +CONFIG_QCOM_MINIDUMP=y This must be a module. Best regards, Krzysztof
On 03/05/2023 19:02, Mukesh Ojha wrote: > Minidump is a best effort mechanism to collect useful and predefined data > for first level of debugging on end user devices running on Qualcomm SoCs. > It is built on the premise that System on Chip (SoC) or subsystem part of > SoC crashes, due to a range of hardware and software bugs. Hence, the > ability to collect accurate data is only a best-effort. The data collected > could be invalid or corrupted, data collection itself could fail, and so on. You organized the patch in a way making it very hard for us to review. I see mixed remoteproc, then soc, then defconfig (!!!), then remote proc, then soc, then bindings (! they must be before usage...), then dts (which should be the last), then soc then dts then... You see the point. Bindings, docs, changes organized by subsystem. Then DTS as separate patchset with a link to this one. If you have bisectability issues then it's a hint something is wrongly organized or done and must be fixed anyway. Best regards, Krzysztof
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 4:53 PM, Krzysztof Kozlowski wrote: > On 03/05/2023 19:02, Mukesh Ojha wrote: >> Previous patches add the Qualcomm minidump driver support, so >> lets enable minidump config so that it can be used by kernel >> clients. >> >> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > > This patchset is split too much. Defconfig change is one change. Not two > or three. > >> --- >> arch/arm64/configs/defconfig | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig >> index a24609e..831c942 100644 >> --- a/arch/arm64/configs/defconfig >> +++ b/arch/arm64/configs/defconfig >> @@ -1250,6 +1250,7 @@ CONFIG_QCOM_STATS=m >> CONFIG_QCOM_WCNSS_CTRL=m >> CONFIG_QCOM_APR=m >> CONFIG_QCOM_ICC_BWMON=m >> +CONFIG_QCOM_MINIDUMP=y > > This must be a module. Why do you think this should be a module ? Is it because, it is lying here among others '=m' ? Or you have some other reasoning ? like it is for qcom specific soc and can not be used outside ? but that is not true for all configs mentioned here. The reason behind making it as '=y' was, to collect information from core kernel data structure as well as the information like percpu data, run queue, irq stat kind of information on kernel crash on a target running some perf configuration(android phone). -- Mukesh > > 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 13:45, Mukesh Ojha wrote: > > > On 5/4/2023 4:53 PM, Krzysztof Kozlowski wrote: >> On 03/05/2023 19:02, Mukesh Ojha wrote: >>> Previous patches add the Qualcomm minidump driver support, so >>> lets enable minidump config so that it can be used by kernel >>> clients. >>> >>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> >> >> This patchset is split too much. Defconfig change is one change. Not two >> or three. >> >>> --- >>> arch/arm64/configs/defconfig | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig >>> index a24609e..831c942 100644 >>> --- a/arch/arm64/configs/defconfig >>> +++ b/arch/arm64/configs/defconfig >>> @@ -1250,6 +1250,7 @@ CONFIG_QCOM_STATS=m >>> CONFIG_QCOM_WCNSS_CTRL=m >>> CONFIG_QCOM_APR=m >>> CONFIG_QCOM_ICC_BWMON=m >>> +CONFIG_QCOM_MINIDUMP=y >> >> This must be a module. > > Why do you think this should be a module ? > > Is it because, it is lying here among others '=m' ? Because we want and insist on everything being a module. That's the generic rule. There are exceptions, so if this justifies being an exception, please bring appropriate arguments. > > Or you have some other reasoning ? like it is for qcom specific > soc and can not be used outside ? but that is not true for > all configs mentioned here. > > The reason behind making it as '=y' was, to collect information from > core kernel data structure as well as the information like percpu data, > run queue, irq stat kind of information on kernel crash on a target > running some perf configuration(android phone). I don't understand why =m stops you from all that. What's more, I don't understand why do you refer to the Android here. This is a development and debugging Linux defconfig, not Android reference config for vendors... Best regards, Krzysztof
On 03/05/2023 19:02, Mukesh Ojha wrote: > Expand MD_* as MINIDUMP_* which makes more sense than the > abbreviation. > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > --- > drivers/remoteproc/qcom_common.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c > index a0d4238..805e525 100644 > --- a/drivers/remoteproc/qcom_common.c > +++ b/drivers/remoteproc/qcom_common.c > @@ -29,9 +29,9 @@ > #define MAX_NUM_OF_SS 10 > #define MAX_REGION_NAME_LENGTH 16 > #define SBL_MINIDUMP_SMEM_ID 602 > -#define MD_REGION_VALID ('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' << 0) > -#define MD_SS_ENCR_DONE ('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0) > -#define MD_SS_ENABLED ('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0) > +#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) You remove it in the next patch, so no, don't touch the line for trivial cleanup and immediately remove it. 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 5/4/2023 6:02 PM, Krzysztof Kozlowski wrote: > On 04/05/2023 13:45, Mukesh Ojha wrote: >> >> >> On 5/4/2023 4:53 PM, Krzysztof Kozlowski wrote: >>> On 03/05/2023 19:02, Mukesh Ojha wrote: >>>> Previous patches add the Qualcomm minidump driver support, so >>>> lets enable minidump config so that it can be used by kernel >>>> clients. >>>> >>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> >>> >>> This patchset is split too much. Defconfig change is one change. Not two >>> or three. >>> >>>> --- >>>> arch/arm64/configs/defconfig | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig >>>> index a24609e..831c942 100644 >>>> --- a/arch/arm64/configs/defconfig >>>> +++ b/arch/arm64/configs/defconfig >>>> @@ -1250,6 +1250,7 @@ CONFIG_QCOM_STATS=m >>>> CONFIG_QCOM_WCNSS_CTRL=m >>>> CONFIG_QCOM_APR=m >>>> CONFIG_QCOM_ICC_BWMON=m >>>> +CONFIG_QCOM_MINIDUMP=y >>> >>> This must be a module. >> >> Why do you think this should be a module ? >> >> Is it because, it is lying here among others '=m' ? > > Because we want and insist on everything being a module. That's the > generic rule. There are exceptions, so if this justifies being an > exception, please bring appropriate arguments. > >> >> Or you have some other reasoning ? like it is for qcom specific >> soc and can not be used outside ? but that is not true for >> all configs mentioned here. >> >> The reason behind making it as '=y' was, to collect information from >> core kernel data structure as well as the information like percpu data, >> run queue, irq stat kind of information on kernel crash on a target >> running some perf configuration(android phone). > > I don't understand why =m stops you from all that. How do i get kernel symbol address from a modules can we use kallsyms_lookup_name from modules ? --Mukesh What's more, I don't > understand why do you refer to the Android here. This is a development > and debugging Linux defconfig, not Android reference config for vendors... > > 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
On 04/05/2023 16:43, Mukesh Ojha wrote: > > > On 5/4/2023 6:02 PM, Krzysztof Kozlowski wrote: >> On 04/05/2023 13:45, Mukesh Ojha wrote: >>> >>> >>> On 5/4/2023 4:53 PM, Krzysztof Kozlowski wrote: >>>> On 03/05/2023 19:02, Mukesh Ojha wrote: >>>>> Previous patches add the Qualcomm minidump driver support, so >>>>> lets enable minidump config so that it can be used by kernel >>>>> clients. >>>>> >>>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> >>>> >>>> This patchset is split too much. Defconfig change is one change. Not two >>>> or three. >>>> >>>>> --- >>>>> arch/arm64/configs/defconfig | 1 + >>>>> 1 file changed, 1 insertion(+) >>>>> >>>>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig >>>>> index a24609e..831c942 100644 >>>>> --- a/arch/arm64/configs/defconfig >>>>> +++ b/arch/arm64/configs/defconfig >>>>> @@ -1250,6 +1250,7 @@ CONFIG_QCOM_STATS=m >>>>> CONFIG_QCOM_WCNSS_CTRL=m >>>>> CONFIG_QCOM_APR=m >>>>> CONFIG_QCOM_ICC_BWMON=m >>>>> +CONFIG_QCOM_MINIDUMP=y >>>> >>>> This must be a module. >>> >>> Why do you think this should be a module ? >>> >>> Is it because, it is lying here among others '=m' ? >> >> Because we want and insist on everything being a module. That's the >> generic rule. There are exceptions, so if this justifies being an >> exception, please bring appropriate arguments. >> >>> >>> Or you have some other reasoning ? like it is for qcom specific >>> soc and can not be used outside ? but that is not true for >>> all configs mentioned here. >>> >>> The reason behind making it as '=y' was, to collect information from >>> core kernel data structure as well as the information like percpu data, >>> run queue, irq stat kind of information on kernel crash on a target >>> running some perf configuration(android phone). >> >> I don't understand why =m stops you from all that. > > How do i get kernel symbol address from a modules > can we use kallsyms_lookup_name from modules ? You allow it to be a module in patch #4, so I think you solved it, right? Otherwise it could not be a module? Anyway, where do you use kallsyms_lookup_name()? I cannot find it in your patch. Best regards, Krzysztof
On 03/05/2023 19:02, Mukesh Ojha wrote: > This driver was inspired from the fact pstore ram region should be > fixed and boot firmware need to have awarness about this region, > so that it will be persistent across boot. But, there are many > QCOM SoC which does not support warm boot from hardware but they > have minidump support from the software, and for them, there is > no need of this pstore ram region to be fixed, but at the same > time have interest in the pstore frontends. So, this driver > get the dynamic reserved region from the ram and register the > ramoops platform device. > > +---------+ +---------+ +--------+ +---------+ > | console | | pmsg | | ftrace | | dmesg | > +---------+ +---------+ +--------+ +---------+ > | | | | > | | | | > +------------------------------------------+ > | > \ / > +----------------+ > (1) |pstore frontends| > +----------------+ > | > \ / > +------------------- + > (2) | pstore backend(ram)| > +--------------------+ > | > \ / > +--------------------+ > (3) |qcom_pstore_minidump| > +--------------------+ > | > \ / > +---------------+ > (4) | qcom_minidump | > +---------------+ > > This driver will route all the pstore front data to the stored > in qcom pstore reserved region and the reason of showing an > arrow from (3) to (4) as qcom_pstore_minidump driver will register > all the available frontends region with qcom minidump driver > in upcoming patch. > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > --- > drivers/soc/qcom/Kconfig | 11 +++ > drivers/soc/qcom/Makefile | 1 + > drivers/soc/qcom/qcom_pstore_minidump.c | 116 ++++++++++++++++++++++++++++++++ > 3 files changed, 128 insertions(+) > create mode 100644 drivers/soc/qcom/qcom_pstore_minidump.c > > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig > index 15c931e..afdc634 100644 > --- a/drivers/soc/qcom/Kconfig > +++ b/drivers/soc/qcom/Kconfig > @@ -293,4 +293,15 @@ config QCOM_MINIDUMP > these selective regions will be dumped instead of the entire DDR. > This saves significant amount of time and/or storage space. > > +config QCOM_PSTORE_MINIDUMP > + tristate "Pstore support for QCOM Minidump" > + depends on ARCH_QCOM > + depends on PSTORE_RAM > + depends on QCOM_MINIDUMP > + help > + Enablement of this driver ensures that ramoops region can be anywhere > + reserved in ram instead of being fixed address which needs boot firmware > + awareness. So, this driver creates plaform device and registers available > + frontend region with the Qualcomm's minidump driver. > + > endmenu > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile > index 1ebe081..02d30d7 100644 > --- a/drivers/soc/qcom/Makefile > +++ b/drivers/soc/qcom/Makefile > @@ -34,3 +34,4 @@ obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) += kryo-l2-accessors.o > obj-$(CONFIG_QCOM_ICC_BWMON) += icc-bwmon.o > obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += ice.o > obj-$(CONFIG_QCOM_MINIDUMP) += qcom_minidump.o > +obj-$(CONFIG_QCOM_PSTORE_MINIDUMP) += qcom_pstore_minidump.o > diff --git a/drivers/soc/qcom/qcom_pstore_minidump.c b/drivers/soc/qcom/qcom_pstore_minidump.c > new file mode 100644 > index 0000000..8d58500 > --- /dev/null > +++ b/drivers/soc/qcom/qcom_pstore_minidump.c > @@ -0,0 +1,116 @@ > +// SPDX-License-Identifier: GPL-2.0-only > + > +/* > + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/of_reserved_mem.h> > +#include <linux/platform_device.h> > +#include <linux/pstore_ram.h> > +#include <soc/qcom/qcom_minidump.h> > + > +struct qcom_ramoops_config { > + unsigned long record_size; > + unsigned long console_size; > + unsigned long ftrace_size; > + unsigned long pmsg_size; > + unsigned int mem_type; > + unsigned int flags; > + int max_reason; > +}; > + > +struct qcom_ramoops_dd { > + struct ramoops_platform_data qcom_ramoops_pdata; > + struct platform_device *ramoops_pdev; > +}; > + > +static struct qcom_ramoops_config default_ramoops_config = { Cannot this be const? > + .mem_type = 2, > + .record_size = 0x0, > + .console_size = 0x200000, > + .ftrace_size = 0x0, > + .pmsg_size = 0x0, > +}; > + > +static struct qcom_ramoops_dd *qcom_rdd; Drop file scope variable. It's not even used. > +static int qcom_ramoops_probe(struct platform_device *pdev) > +{ > + struct device_node *of_node = pdev->dev.of_node; > + struct device_node *node; > + const struct qcom_ramoops_config *cfg; > + struct ramoops_platform_data *pdata; > + struct reserved_mem *rmem; > + long ret; > + > + node = of_parse_phandle(of_node, "memory-region", 0); > + if (!node) > + return -ENODEV; > + > + rmem = of_reserved_mem_lookup(node); > + of_node_put(node); > + if (!rmem) { > + dev_err(&pdev->dev, "failed to locate DT /reserved-memory resource\n"); > + return -EINVAL; > + } > + > + qcom_rdd = devm_kzalloc(&pdev->dev, sizeof(*qcom_rdd), GFP_KERNEL); > + if (!qcom_rdd) > + return -ENOMEM; > + > + cfg = of_device_get_match_data(&pdev->dev); > + if (!cfg) { > + dev_err(&pdev->dev, "failed to get supported matched data\n"); > + return -ENOENT; > + } > + > + pdata = &qcom_rdd->qcom_ramoops_pdata; > + pdata->mem_size = rmem->size; > + pdata->mem_address = rmem->base; > + pdata->mem_type = cfg->mem_type; > + pdata->record_size = cfg->record_size; > + pdata->console_size = cfg->console_size; > + pdata->ftrace_size = cfg->ftrace_size; > + pdata->pmsg_size = cfg->pmsg_size; > + pdata->max_reason = KMSG_DUMP_PANIC; > + > + qcom_rdd->ramoops_pdev = platform_device_register_data(NULL, "ramoops", -1, > + pdata, sizeof(*pdata)); > + if (IS_ERR(qcom_rdd->ramoops_pdev)) { > + ret = PTR_ERR(qcom_rdd->ramoops_pdev); > + dev_err(&pdev->dev, "could not create platform device: %ld\n", ret); > + qcom_rdd->ramoops_pdev = NULL; > + } > + > + return ret; > +} > + > +static int qcom_ramoops_remove(struct platform_device *pdev) Use instead .remove_new callback. > +{ > + platform_device_unregister(qcom_rdd->ramoops_pdev); > + qcom_rdd->ramoops_pdev = NULL; > + > + return 0; > +} > + > +static const struct of_device_id qcom_ramoops_of_match[] = { > + { .compatible = "qcom,sm8450-ramoops-minidump", .data = &default_ramoops_config }, You don't need this entry. > + { .compatible = "qcom,ramoops-minidump", .data = &default_ramoops_config }, > + {} > +}; > + > +MODULE_DEVICE_TABLE(of, qcom_ramoops_of_match); Blank line goes after the MODULE_DEVICE_TABLE, not before. Best regards, Krzysztof
On Wed, 03 May 2023 22:32:14 +0530, Mukesh Ojha wrote: > Minidump is a best effort mechanism to collect useful and predefined data > for first level of debugging on end user devices running on Qualcomm SoCs. > It is built on the premise that System on Chip (SoC) or subsystem part of > SoC crashes, due to a range of hardware and software bugs. Hence, the > ability to collect accurate data is only a best-effort. The data collected > could be invalid or corrupted, data collection itself could fail, and so on. > > [...] Applied, thanks! [01/18] remoteproc: qcom: Expand MD_* as MINIDUMP_* commit: 318da1371246fdc1806011a27138175cfb078687 Best regards,
On Sat, Jul 15, 2023 at 03:13:34PM -0700, Bjorn Andersson wrote: > > On Wed, 03 May 2023 22:32:14 +0530, Mukesh Ojha wrote: > > Minidump is a best effort mechanism to collect useful and predefined data > > for first level of debugging on end user devices running on Qualcomm SoCs. > > It is built on the premise that System on Chip (SoC) or subsystem part of > > SoC crashes, due to a range of hardware and software bugs. Hence, the > > ability to collect accurate data is only a best-effort. The data collected > > could be invalid or corrupted, data collection itself could fail, and so on. > > > > [...] > > Applied, thanks! > > [01/18] remoteproc: qcom: Expand MD_* as MINIDUMP_* > commit: 318da1371246fdc1806011a27138175cfb078687 > Krzysztof asked for modifications on this patch. > Best regards, > -- > Bjorn Andersson <andersson@kernel.org>
On 17/07/2023 03:15, Mathieu Poirier wrote: > On Sat, Jul 15, 2023 at 03:13:34PM -0700, Bjorn Andersson wrote: >> >> On Wed, 03 May 2023 22:32:14 +0530, Mukesh Ojha wrote: >>> Minidump is a best effort mechanism to collect useful and predefined data >>> for first level of debugging on end user devices running on Qualcomm SoCs. >>> It is built on the premise that System on Chip (SoC) or subsystem part of >>> SoC crashes, due to a range of hardware and software bugs. Hence, the >>> ability to collect accurate data is only a best-effort. The data collected >>> could be invalid or corrupted, data collection itself could fail, and so on. >>> >>> [...] >> >> Applied, thanks! >> >> [01/18] remoteproc: qcom: Expand MD_* as MINIDUMP_* >> commit: 318da1371246fdc1806011a27138175cfb078687 >> > > Krzysztof asked for modifications on this patch. I guess it is fine, no big issue. Best regards, Krzysztof
On Sun, Jul 16, 2023 at 07:15:57PM -0600, Mathieu Poirier wrote: > On Sat, Jul 15, 2023 at 03:13:34PM -0700, Bjorn Andersson wrote: > > > > On Wed, 03 May 2023 22:32:14 +0530, Mukesh Ojha wrote: > > > Minidump is a best effort mechanism to collect useful and predefined data > > > for first level of debugging on end user devices running on Qualcomm SoCs. > > > It is built on the premise that System on Chip (SoC) or subsystem part of > > > SoC crashes, due to a range of hardware and software bugs. Hence, the > > > ability to collect accurate data is only a best-effort. The data collected > > > could be invalid or corrupted, data collection itself could fail, and so on. > > > > > > [...] > > > > Applied, thanks! > > > > [01/18] remoteproc: qcom: Expand MD_* as MINIDUMP_* > > commit: 318da1371246fdc1806011a27138175cfb078687 > > > > Krzysztof asked for modifications on this patch. > Krzysztof pointed out that there was no reason to trivially modify the defines and then immediately start moving things around. I agree with this, but as the consensus was that the rest of the series needs more work, I find this to be a sensible cleanup, getting rid of the cryptic "MD_" abbreviation. Regards, Bjorn
diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c index 0d2209c..767e84a 100644 --- a/drivers/watchdog/qcom-wdt.c +++ b/drivers/watchdog/qcom-wdt.c @@ -12,6 +12,7 @@ #include <linux/platform_device.h> #include <linux/watchdog.h> #include <linux/of_device.h> +#include <linux/panic.h> enum wdt_reg { WDT_RST, @@ -114,12 +115,28 @@ static int qcom_wdt_set_pretimeout(struct watchdog_device *wdd, return qcom_wdt_start(wdd); } +static void qcom_wdt_bite_on_panic(struct qcom_wdt *wdt) +{ + writel(0, wdt_addr(wdt, WDT_EN)); + writel(1, wdt_addr(wdt, WDT_BITE_TIME)); + writel(1, wdt_addr(wdt, WDT_RST)); + writel(QCOM_WDT_ENABLE, wdt_addr(wdt, WDT_EN)); + + wmb(); + + while(1) + udelay(1); +} + static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action, void *data) { struct qcom_wdt *wdt = to_qcom_wdt(wdd); u32 timeout; + if (in_panic) + qcom_wdt_bite_on_panic(wdt); + /* * Trigger watchdog bite: * Setup BITE_TIME to be 128ms, and enable WDT. diff --git a/include/linux/panic.h b/include/linux/panic.h index 979b776..f913629 100644 --- a/include/linux/panic.h +++ b/include/linux/panic.h @@ -22,6 +22,7 @@ extern int panic_on_oops; extern int panic_on_unrecovered_nmi; extern int panic_on_io_nmi; extern int panic_on_warn; +extern bool in_panic; extern unsigned long panic_on_taint; extern bool panic_on_taint_nousertaint; diff --git a/kernel/panic.c b/kernel/panic.c index 487f5b0..714f7f4 100644 --- a/kernel/panic.c +++ b/kernel/panic.c @@ -65,6 +65,8 @@ static unsigned int warn_limit __read_mostly; int panic_timeout = CONFIG_PANIC_TIMEOUT; EXPORT_SYMBOL_GPL(panic_timeout); +bool in_panic = false; +EXPORT_SYMBOL_GPL(in_panic); #define PANIC_PRINT_TASK_INFO 0x00000001 #define PANIC_PRINT_MEM_INFO 0x00000002 @@ -261,6 +263,7 @@ void panic(const char *fmt, ...) int old_cpu, this_cpu; bool _crash_kexec_post_notifiers = crash_kexec_post_notifiers; + in_panic = true; if (panic_on_warn) { /* * This thread may hit another WARN() in the panic path.
Minidump is a best effort mechanism to collect useful and predefined data for first level of debugging on end user devices running on Qualcomm SoCs. It is built on the premise that System on Chip (SoC) or subsystem part of SoC crashes, due to a range of hardware and software bugs. Hence, the ability to collect accurate data is only a best-effort. The data collected could be invalid or corrupted, data collection itself could fail, and so on. Qualcomm devices in engineering mode provides a mechanism for generating full system ramdumps for post mortem debugging. But in some cases it's however not feasible to capture the entire content of RAM. The minidump mechanism provides the means for selecting which snippets should be included in the ramdump. The core of minidump feature is part of Qualcomm's boot firmware code. It initializes shared memory (SMEM), which is a part of DDR and allocates a small section of SMEM to minidump table i.e also called global table of content (G-ToC). Each subsystem (APSS, ADSP, ...) has their own table of segments to be included in the minidump and all get their reference from G-ToC. Each segment/region has some details like name, physical address and it's size etc. and it could be anywhere scattered in the DDR. Existing upstream Qualcomm remoteproc driver[1] already supports minidump feature for remoteproc instances like ADSP, MODEM, ... where predefined selective segments of subsystem region can be dumped as part of coredump collection which generates smaller size artifacts compared to complete coredump of subsystem on crash. [1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/remoteproc/qcom_common.c#n142 In addition to managing and querying the APSS minidump description, the Linux driver maintains a ELF header in a segment. This segment gets updated with section/program header whenever a new entry gets registered. Patch 1/18 is very trivial change. Patch 2/18 moves the minidump specific data structure and macro to qcom_minidump.h so that (4/18) qcom minidump driver can use. Patch 3/18 documents qualcomm minidump guide for users. Patch 4/18 implements qualcomm minidump kernel driver and exports symbol which other minidump kernel client can use. Patch 5/18 add pending region support for the clients who came for registration before minidump. Patch 6/18 add update region support for registered clients. Patch 7/18 enables the qualcomm minidump driver. Patch 8/18 Use the exported symbol from minidump driver in qcom_common for querying minidump descriptor for a subsystem. Patch 9-13 add qcom dynamic ramoops region support via a driver which adds ramoops platform device and also register existing pstore frontend regions. Patch 14-18 are not new and has already been through 6 versions and reason of adding here is for minidump testing purpose and it will be rebased automatically along with new version of minidump series. Testing of the patches has been done on sm8450 target after enabling config like CONFIG_PSTORE_RAM and CONFIG_PSTORE_CONSOLE and once the device boots up. Try crashing it via devmem2 0xf11c000(this is known to create xpu violation and and put the device in download mode) on command prompt. I have added download patch here numbered from 14/18 to 18/18 Earlier download mode setting patches were sent separately https://lore.kernel.org/lkml/1680076012-10785-1-git-send-email-quic_mojha@quicinc.com/ Default storage type is set to via USB, so minidump would be downloaded with the help of x86_64 machine (running PCAT tool) attached to Qualcomm device which has backed minidump boot firmware support (more can be found patch 3/18) Below patch [1] is to warm reset Qualcomm device which has upstream qcom watchdog driver support. After applying all patches, we can boot the device and can execute following command. echo mini > /sys/module/qcom_scm/parameters/download_mode echo c > /proc/sysrq-trigger This will make the device go to download mode and collect the minidump on to the attached x86 machine running the Qualcomm PCAT tool(This comes as part Qualcomm package manager kit). After that we will see a bunch of predefined registered region as binary blobs files starts with md_* downloaded on the x86 machine on given location in PCAT tool from the target device. A sample client example to dump a linux region has been given in patch 3/18 and as well as can be seen in patch 12/18. [1] --------------------------->8------------------------------------- commit f1124ccebd47550b4c9627aa162d9cdceba2b76f Author: Mukesh Ojha <quic_mojha@quicinc.com> Date: Thu Mar 16 14:08:35 2023 +0530 do not merge: watchdog bite on panic Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> -------------------------------------------------------------------------- Changes in v3: - Addressed most of the comments by Srini on v2 and refactored the minidump driver. - Added platform device support - Unregister region support. - Added update region for clients. - Added pending region support. - Modified the documentation guide accordingly. - Added qcom_pstore_ramdump client driver which happen to add ramoops platform device and also registers ramoops region with minidump. - Added download mode patch series with this minidump series. https://lore.kernel.org/lkml/1680076012-10785-1-git-send-email-quic_mojha@quicinc.com/ Changes in v2: https://lore.kernel.org/lkml/1679491817-2498-1-git-send-email-quic_mojha@quicinc.com/ - Addressed review comment made by [quic_tsoni/bmasney] to add documentation. - Addressed comments made by [srinivas.kandagatla] - Dropped pstore 6/6 from the last series, till i get conclusion to get pstore region in minidump. - Fixed issue reported by kernel test robot. Changes in v1: https://lore.kernel.org/lkml/1676978713-7394-1-git-send-email-quic_mojha@quicinc.com/ Mukesh Ojha (18): remoteproc: qcom: Expand MD_* as MINIDUMP_* remoteproc: qcom: Move minidump specific data to qcom_minidump.h docs: qcom: Add qualcomm minidump guide soc: qcom: Add Qualcomm minidump kernel driver soc: qcom: minidump: Add pending region registration support soc: qcom: minidump: Add update region support arm64: defconfig: Enable Qualcomm minidump driver remoterproc: qcom: refactor to leverage exported minidump symbol soc: qcom: Add qcom's pstore minidump driver support dt-bindings: reserved-memory: Add qcom,ramoops-minidump binding arm64: dts: qcom: sm8450: Add Qualcomm ramoops minidump node soc: qcom: Register pstore frontend region with minidump arm64: defconfig: Enable Qualcomm pstore minidump client driver firmware: qcom_scm: provide a read-modify-write function pinctrl: qcom: Use qcom_scm_io_update_field() firmware: scm: Modify only the download bits in TCSR register firmware: qcom_scm: Refactor code to support multiple download mode firmware: qcom_scm: Add multiple download mode support Documentation/admin-guide/qcom_minidump.rst | 246 +++++++ .../reserved-memory/qcom,ramoops-minidump.yaml | 69 ++ arch/arm64/boot/dts/qcom/sm8450.dtsi | 11 + arch/arm64/configs/defconfig | 2 + drivers/firmware/Kconfig | 11 - drivers/firmware/qcom_scm.c | 88 ++- drivers/pinctrl/qcom/pinctrl-msm.c | 11 +- drivers/remoteproc/qcom_common.c | 75 +-- drivers/soc/qcom/Kconfig | 25 + drivers/soc/qcom/Makefile | 2 + drivers/soc/qcom/qcom_minidump.c | 724 +++++++++++++++++++++ drivers/soc/qcom/qcom_pstore_minidump.c | 194 ++++++ drivers/soc/qcom/smem.c | 8 + include/linux/firmware/qcom/qcom_scm.h | 2 + include/soc/qcom/qcom_minidump.h | 130 ++++ 15 files changed, 1503 insertions(+), 95 deletions(-) create mode 100644 Documentation/admin-guide/qcom_minidump.rst create mode 100644 Documentation/devicetree/bindings/reserved-memory/qcom,ramoops-minidump.yaml create mode 100644 drivers/soc/qcom/qcom_minidump.c create mode 100644 drivers/soc/qcom/qcom_pstore_minidump.c create mode 100644 include/soc/qcom/qcom_minidump.h