Re: [PATCH] pstore/ram: Rework logic for detecting ramoops

[Mukesh Ojha]

Hi Kees,

Thanks for your comments.

On 1/13/2023 3:09 AM, Kees Cook wrote:
> On Wed, Jan 11, 2023 at 02:37:45PM +0530, Mukesh Ojha wrote:
> > The reserved memory region for ramoops is assumed to be at a fixed
> > and known location when read from the devicetree. This is not desirable
> > in environments where it is preferred the region to be dynamically
> > allocated at runtime, as opposed to being fixed at compile time.
> >
> > Also, Some of the platforms might be still expecting dedicated
> > memory region for ramoops node where the region is known
> > beforehand and platform_get_resource() is used in that case.
> >
> > So, Add logic to detect the start and size of the ramoops memory
> > region by looking up reserved memory region with
> > of_reserved_mem_lookup() when platform_get_resource() failed.
> >
> > Signed-off-by: Mukesh Ojha <quic_mojha@xxxxxxxxxxx>
>
> Thanks for the patch! Notes below...
>
> > ---
> >   fs/pstore/ram.c | 19 ++++++++++++++-----
> >   1 file changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> > index ade66db..e4bbba1 100644
> > --- a/fs/pstore/ram.c
> > +++ b/fs/pstore/ram.c
> > @@ -20,6 +20,7 @@
> >   #include <linux/compiler.h>
> >   #include <linux/of.h>
> >   #include <linux/of_address.h>
> > +#include <linux/of_reserved_mem.h>
> >   
> >   #include "internal.h"
> >   #include "ram_internal.h"
> > @@ -643,6 +644,7 @@ static int ramoops_parse_dt(struct platform_device *pdev,
> >   {
> >   	struct device_node *of_node = pdev->dev.of_node;
> >   	struct device_node *parent_node;
> > +	struct reserved_mem *rmem;
> >   	struct resource *res;
> >   	u32 value;
> >   	int ret;
> > @@ -651,13 +653,20 @@ static int ramoops_parse_dt(struct platform_device *pdev,
> >   
> >   	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >   	if (!res) {
> > -		dev_err(&pdev->dev,
> > -			"failed to locate DT /reserved-memory resource\n");
> > -		return -EINVAL;
> > +		rmem = of_reserved_mem_lookup(of_node);
> > +		if (rmem) {
> > +			pdata->mem_size = rmem->size;
> > +			pdata->mem_address = rmem->base;
> > +		} else {
> > +			dev_err(&pdev->dev,
> > +				"failed to locate DT /reserved-memory resource\n");
> > +			return -EINVAL;
> > +		}
>
> Since the "else" case returns, the traditional code pattern is to leave
> the other case "inline" (an indented), like so:
>
> 		if (!rmem) {
> 			dev_err(&pdev->dev,
> 				"failed to locate DT /reserved-memory resource\n");
> 			return -EINVAL;
> 		}
> 		pdata->mem_size = rmem->size;
> 		pdata->mem_address = rmem->base;

Fixed it in v2.

> > +	} else {
> > +		pdata->mem_size = resource_size(res);
> > +		pdata->mem_address = res->start;
> >   	}
>
> Since this change the potential interface with DT, can you also update
> the documentation in:
>
> Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
>
> Or maybe my understanding of DT parsing is lacking and this change is
> doing something slightly different?

Have updated the docs in v2;

-Mukesh