| Message ID | 1673428065-22356-1-git-send-email-quic_mojha@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | pstore/ram: Rework logic for detecting ramoops | expand |
Thanks for the patch Mukesh! I don't have a DT hardware at hand right now, so cannot test this one myself. I'll just provide a (really) minor feedback, something to address in a potential V2 or even in merge time, see below. On 11/01/2023 06:07, 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 I'd write "Also, some" instead of upper "Some". > 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 Same here, maybe "So, add". Really minor nits, though! Cheers, Guilherme
Hi Guilherme, On 1/11/2023 6:31 PM, Guilherme G. Piccoli wrote: > Thanks for the patch Mukesh! I don't have a DT hardware at hand right > now, so cannot test this one myself. I'll just provide a (really) minor > feedback, something to address in a potential V2 or even in merge time, > see below. > > > On 11/01/2023 06:07, 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 > > I'd write "Also, some" instead of upper "Some". > >> 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 > > Same here, maybe "So, add". > Thanks for the review. Will fix it along with other comments. Also, let me know if we need to change binding or document update as well. -Mukesh > Really minor nits, though! > Cheers > > > Guilherme
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@quicinc.com> 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; > + } 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? -Kees > > - pdata->mem_size = resource_size(res); > - pdata->mem_address = res->start; > /* > * Setting "unbuffered" is deprecated and will be ignored if > * "mem_type" is also specified. > -- > 2.7.4 >
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@quicinc.com> > > 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
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; + } + } else { + pdata->mem_size = resource_size(res); + pdata->mem_address = res->start; } - pdata->mem_size = resource_size(res); - pdata->mem_address = res->start; /* * Setting "unbuffered" is deprecated and will be ignored if * "mem_type" is also specified.
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@quicinc.com> --- fs/pstore/ram.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)