diff mbox series

[v5,09/17] pstore/ram: Use dynamic ramoops reserve resource

Message ID 1694290578-17733-10-git-send-email-quic_mojha@quicinc.com (mailing list archive)
State New
Headers show
Series Add Qualcomm Minidump kernel driver related support | expand

Commit Message

Mukesh Ojha Sept. 9, 2023, 8:16 p.m. UTC
As dynamic ramoops command line parsing is now added, so
lets add the support in ramoops driver to get the resource
structure and add it during platform device registration.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 fs/pstore/ram.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Pavan Kondeti Sept. 11, 2023, 5:33 a.m. UTC | #1
On Sun, Sep 10, 2023 at 01:46:10AM +0530, Mukesh Ojha wrote:
> As dynamic ramoops command line parsing is now added, so
> lets add the support in ramoops driver to get the resource
> structure and add it during platform device registration.
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>  fs/pstore/ram.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 

Documentation/admin-guide/ramoops.rst might need an update as well.

> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index 2f625e1fa8d8..e73fbbc1b5b5 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -913,13 +913,17 @@ static void __init ramoops_register_dummy(void)
>  
>  	/*
>  	 * Prepare a dummy platform data structure to carry the module
> -	 * parameters. If mem_size isn't set, then there are no module
> -	 * parameters, and we can skip this.
> +	 * parameters. If mem_size isn't set, check for dynamic ramoops
> +	 * size and extract the information if it is set.
>  	 */
> -	if (!mem_size)
> +	if (!mem_size && !dyn_ramoops_res.end)
>  		return;
>  
>  	pr_info("using module parameters\n");
> +	if (dyn_ramoops_res.end) {
> +		mem_size = resource_size(&dyn_ramoops_res);
> +		mem_address = dyn_ramoops_res.start;
> +	}
>  
>  	memset(&pdata, 0, sizeof(pdata));
>  	pdata.mem_size = mem_size;

You might want to add "arch_" prefix to dyn_ramoops resource so that it
would be clear that it is coming from arch code. This code needs to
guard against arch not supplying this.

Thanks,
Pavan
Mukesh Ojha Sept. 11, 2023, 10:51 a.m. UTC | #2
On 9/11/2023 11:03 AM, Pavan Kondeti wrote:
> On Sun, Sep 10, 2023 at 01:46:10AM +0530, Mukesh Ojha wrote:
>> As dynamic ramoops command line parsing is now added, so
>> lets add the support in ramoops driver to get the resource
>> structure and add it during platform device registration.
>>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> ---
>>   fs/pstore/ram.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
> 
> Documentation/admin-guide/ramoops.rst might need an update as well.

I have said in the cover-letter under changes in v5, it is open for
comment and not yet documented it yet.

> 
>> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
>> index 2f625e1fa8d8..e73fbbc1b5b5 100644
>> --- a/fs/pstore/ram.c
>> +++ b/fs/pstore/ram.c
>> @@ -913,13 +913,17 @@ static void __init ramoops_register_dummy(void)
>>   
>>   	/*
>>   	 * Prepare a dummy platform data structure to carry the module
>> -	 * parameters. If mem_size isn't set, then there are no module
>> -	 * parameters, and we can skip this.
>> +	 * parameters. If mem_size isn't set, check for dynamic ramoops
>> +	 * size and extract the information if it is set.
>>   	 */
>> -	if (!mem_size)
>> +	if (!mem_size && !dyn_ramoops_res.end)
>>   		return;
>>   
>>   	pr_info("using module parameters\n");
>> +	if (dyn_ramoops_res.end) {
>> +		mem_size = resource_size(&dyn_ramoops_res);
>> +		mem_address = dyn_ramoops_res.start;
>> +	}
>>   
>>   	memset(&pdata, 0, sizeof(pdata));
>>   	pdata.mem_size = mem_size;
> 
> You might want to add "arch_" prefix to dyn_ramoops resource so that it
> would be clear that it is coming from arch code. This code needs to
> guard against arch not supplying this.

Sure, thanks for pointing this.
Agree, if we finally decide to keep it in arch/arm64 .

-Mukesh
> 
> Thanks,
> Pavan
Pavan Kondeti Sept. 12, 2023, 12:39 a.m. UTC | #3
On Mon, Sep 11, 2023 at 04:21:44PM +0530, Mukesh Ojha wrote:
> 
> 
> On 9/11/2023 11:03 AM, Pavan Kondeti wrote:
> > On Sun, Sep 10, 2023 at 01:46:10AM +0530, Mukesh Ojha wrote:
> > > As dynamic ramoops command line parsing is now added, so
> > > lets add the support in ramoops driver to get the resource
> > > structure and add it during platform device registration.
> > > 
> > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> > > ---
> > >   fs/pstore/ram.c | 10 +++++++---
> > >   1 file changed, 7 insertions(+), 3 deletions(-)
> > > 
> > 
> > Documentation/admin-guide/ramoops.rst might need an update as well.
> 
> I have said in the cover-letter under changes in v5, it is open for
> comment and not yet documented it yet.
> 
Sure.

To easy on the reviewers, the under cut portion of a specific patch could be
used to add footer notes like TODO/Testing etc. In this case, I was lazy to 
read the loong cover letter posted in this series ;-)

Thanks,
Pavan
Mukesh Ojha Sept. 12, 2023, 9:46 a.m. UTC | #4
On 9/12/2023 6:09 AM, Pavan Kondeti wrote:
> On Mon, Sep 11, 2023 at 04:21:44PM +0530, Mukesh Ojha wrote:
>>
>>
>> On 9/11/2023 11:03 AM, Pavan Kondeti wrote:
>>> On Sun, Sep 10, 2023 at 01:46:10AM +0530, Mukesh Ojha wrote:
>>>> As dynamic ramoops command line parsing is now added, so
>>>> lets add the support in ramoops driver to get the resource
>>>> structure and add it during platform device registration.
>>>>
>>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>>>> ---
>>>>    fs/pstore/ram.c | 10 +++++++---
>>>>    1 file changed, 7 insertions(+), 3 deletions(-)
>>>>
>>>
>>> Documentation/admin-guide/ramoops.rst might need an update as well.
>>
>> I have said in the cover-letter under changes in v5, it is open for
>> comment and not yet documented it yet.
>>
> Sure.
> 
> To easy on the reviewers, the under cut portion of a specific patch could be
> used to add footer notes like TODO/Testing etc. In this case, I was lazy to
> read the loong cover letter posted in this series ;-)

I have seen it, will comment related to particular patch under --- .
Thanks for suggestion.

-Mukesh

> 
> Thanks,
> Pavan
diff mbox series

Patch

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 2f625e1fa8d8..e73fbbc1b5b5 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -913,13 +913,17 @@  static void __init ramoops_register_dummy(void)
 
 	/*
 	 * Prepare a dummy platform data structure to carry the module
-	 * parameters. If mem_size isn't set, then there are no module
-	 * parameters, and we can skip this.
+	 * parameters. If mem_size isn't set, check for dynamic ramoops
+	 * size and extract the information if it is set.
 	 */
-	if (!mem_size)
+	if (!mem_size && !dyn_ramoops_res.end)
 		return;
 
 	pr_info("using module parameters\n");
+	if (dyn_ramoops_res.end) {
+		mem_size = resource_size(&dyn_ramoops_res);
+		mem_address = dyn_ramoops_res.start;
+	}
 
 	memset(&pdata, 0, sizeof(pdata));
 	pdata.mem_size = mem_size;