Messages in this thread | ![/](/images/icornerl.gif) | | Date | Thu, 22 Jun 2023 10:26:35 -0700 | From | Isaac Manjarres <> | Subject | Re: [PATCH] pstore/ram: Add support for dynamically allocated ramoops memory regions |
| |
On Wed, Jun 21, 2023 at 10:15:45PM -0700, Kees Cook wrote: > On Wed, Jun 21, 2023 at 09:47:26PM -0700, John Stultz 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 for the region to be dynamically > > > allocated early during boot (i.e. the memory region is defined with > > > the "alloc-ranges" property instead of the "reg" property). > > > > > > > Thanks for sending this out, Isaac! > > > > Apologies, I've forgotten much of the details around dt bindings here, > > so forgive my questions: > > If the memory is dynamically allocated from a specific range, is it > > guaranteed to be consistently the same address boot to boot? > > > > > Since ramoops regions are part of the reserved-memory devicetree > > > node, they exist in the reserved_mem array. This means that the > > > of_reserved_mem_lookup() function can be used to retrieve the > > > reserved_mem structure for the ramoops region, and that structure > > > contains the base and size of the region, even if it has been > > > dynamically allocated. > > > > I think this is answering my question above, but it's a little opaque, > > so I'm not sure. > > Yeah, I had exactly the same question: will this be the same > boot-to-boot?
Hi Kees,
Thank you for taking a look at this patch and for your review! When the alloc-ranges property is used to describe a memory region, the memory region will always be allocated within that range, but it's not guaranteed to be allocated at the same base address across reboots.
I had proposed re-wording the end of the commit message in my response to John as follows:
"...and that structure contains the address of the base of the region that was allocated at boot anywhere within the range specified by the "alloc-ranges" devicetree property."
Does that clarify things better?
> > > > > Thus invoke of_reserved_mem_lookup() in case the call to > > > platform_get_resource() fails in order to support dynamically > > > allocated ramoops memory regions. > > > > > > Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org> > > > Signed-off-by: Mukesh Ojha <mojha@codeaurora.org> > > > Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org> > > > Signed-off-by: Isaac J. Manjarres <isaacmanjarres@google.com> > > I think this should have "Co-developed-by:"s for each person, since this > isn't explicitly a S-o-B chain...
Noted. I'll fix this up for v2 of the patch.
> > > @@ -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); > > > > Nit: you could keep rmem scoped locally here. > > > > Otherwise the code looks sane, I just suspect the commit message could > > be more clear in explaining the need/utility of the dts entry using > > alloc-ranges. > > I haven't looked closely at the API here, but does this need a "put" > like the "get" stuff? (I assume not, given the "lookup" is on a node...)
No, it doesn't need a put, since of_reserved_mem_lookup() doesn't acquire a reference to anything.
Thanks, Isaac
| ![\](/images/icornerr.gif) |