Change in coreboot[master]: cbfs: Make cbfs_default_props() hidden again

[Aaron Durbin (Code Review)]

Aaron Durbin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36867 )

Change subject: cbfs: Make cbfs_default_props() hidden again
......................................................................

cbfs: Make cbfs_default_props() hidden again

All the users that were needing cbfs_default_props() to remove
code duplication are fixed. Therefore, there's no need to expose
the symbol globally any longer.

Change-Id: Id84402b54ec1688e0c0adfa5c8fd12ce68ab8634
Signed-off-by: Aaron Durbin 
---
M src/include/cbfs.h
M src/lib/cbfs.c
2 files changed, 1 insertion(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/36867/1

diff --git a/src/include/cbfs.h b/src/include/cbfs.h
index 60129d3..540a481 100644
--- a/src/include/cbfs.h
+++ b/src/include/cbfs.h
@@ -73,9 +73,6 @@
    size_t size;
 };

-/* Default CBFS locator .locate() callback that locates "COREBOOT" region. */
-int cbfs_default_props(struct cbfs_props *props);
-
 /* Return < 0 on error otherwise props are filled out accordingly. */
 int cbfs_boot_region_properties(struct cbfs_props *props);

diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c
index fbe6e43..44cda21 100644
--- a/src/lib/cbfs.c
+++ b/src/lib/cbfs.c
@@ -297,7 +297,7 @@
 }

/* The default locator to find the CBFS in the "COREBOOT" FMAP region. */
-int cbfs_default_props(struct cbfs_props *props)
+static int cbfs_default_props(struct cbfs_props *props)
 {
    struct region region;

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36867 )

Change subject: cbfs: Make cbfs_default_props() hidden again
......................................................................

Patch Set 1:

Why not leave it like this for the next time someone may need it? (Technically, for the apollolake implementation it would also be a bit cleaner to call into this rather than reimplement it.)

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36867 )

Change subject: cbfs: Make cbfs_default_props() hidden again
......................................................................

Patch Set 1:
> Patch Set 1:
>
> Why not leave it like this for the next time someone may need it? (Technically, for the apollolake implementation it would also be a bit cleaner to call into this rather than reimplement it.)

I was just trying to clean up the surface area. I'll make apollolake use it.

Another question for you:
I looked at this for a few mins earlier. We're currently utilizing offset/size properties. Should we make this API fill out a region device?

int (*locate)(const struct region_device *boot_ro, struct region_device *cbfs_rdev);

the only other place we use offset/size directly is in the coreboot table formation, but I was wondering if there's any added benefit to the change. One side effect would be that rdev_chain() has to be done within each of the callbacks instead of cbfs_boot_locate().

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36867 )

Change subject: cbfs: Make cbfs_default_props() hidden again
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> >
> > Why not leave it like this for the next time someone may need it? (Technically, for the apollolake implementation it would also be a bit cleaner to call into this rather than reimplement it.)
>
> I was just trying to clean up the surface area. I'll make apollolake use it.
>
> Another question for you:
> I looked at this for a few mins earlier. We're currently utilizing offset/size properties. Should we make this API fill out a region device?
>
> int (*locate)(const struct region_device *boot_ro, struct region_device *cbfs_rdev);
>
> the only other place we use offset/size directly is in the coreboot table formation, but I was wondering if there's any added benefit to the change. One side effect would be that rdev_chain() has to be done within each of the callbacks instead of cbfs_boot_locate().

diff looks like this:

diff --git a/src/soc/intel/apollolake/mmap_boot.c b/src/soc/intel/apollolake/mmap_boot.c
index 631a834160..8243dc4d1f 100644
--- a/src/soc/intel/apollolake/mmap_boot.c
+++ b/src/soc/intel/apollolake/mmap_boot.c
@@ -102,14 +102,14 @@ const struct region_device *boot_device_ro(void)
 static int iafw_boot_region_properties(struct cbfs_props *props)
 {
        struct region *real_dev_reg;
-       struct region regn;

-       /* use fmap to locate CBFS area */
-       if (fmap_locate_area("COREBOOT", &regn))
+       if (cbfs_default_props(props))
                return -1;

-       props->offset = region_offset(&regn);
-       props->size = region_sz(&regn);
+       struct region regn = {
+               .offset = props->offset,
+               .size = props->size,
+       };

/* Check that we are within the memory mapped area. It's too
           easy to forget the SRAM mapping when crafting an FMAP file. */

It feels off/gross to me. Thoughts?

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36867 )

Change subject: cbfs: Make cbfs_default_props() hidden again
......................................................................

Patch Set 1:
> Another question for you:
> I looked at this for a few mins earlier. We're currently utilizing offset/size properties. Should we make this API fill out a region device?

How about just replacing struct cbfs_props with struct region everywhere? They're the exact same struct on the inside, I don't understand why they have different names anyway. Then your apollolake problem should go away because you can call region_is_subregion() directly on the thing cbfs_default_props() fills out.

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36867 )

Change subject: cbfs: Make cbfs_default_props() hidden again
......................................................................

Patch Set 1:
> Patch Set 1:
> > Another question for you:
> > I looked at this for a few mins earlier. We're currently utilizing offset/size properties. Should we make this API fill out a region device?
>
> How about just replacing struct cbfs_props with struct region everywhere? They're the exact same struct on the inside, I don't understand why they have different names anyway. Then your apollolake problem should go away because you can call region_is_subregion() directly on the thing cbfs_default_props() fills out.

Ya. I thought about that as well earlier. I forgot why I separated the types to begin with. I can't remember. I can try and do that next.

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36867 )

Change subject: cbfs: Make cbfs_default_props() hidden again
......................................................................

Patch Set 1:
> How about just replacing struct cbfs_props with struct region everywhere? They're the exact same struct on the inside, I don't understand why they have different names anyway. Then your apollolake problem should go away because you can call region_is_subregion() directly on the thing cbfs_default_props() fills out.

On the other hand, yes, just having it fill out the region_device directly may also be good. Passing raw regions around with the implicit assumption that they point to boot_device_ro() seems a bit unclean if we could have region and device tied together instead.

I don't think the API would look like you described though, I think it would just be

int (*locate)(struct region_device *cbfs_rdev);

The backing storage of the region device comes from the leaf function (from the FMAP code calling boot_device_ro()). cbfs_boot_locate() wouldn't have to call boot_device_ro() at all anymore.

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36867 )

Change subject: cbfs: Make cbfs_default_props() hidden again
......................................................................

Patch Set 1:
> Patch Set 1:
> > How about just replacing struct cbfs_props with struct region everywhere? They're the exact same struct on the inside, I don't understand why they have different names anyway. Then your apollolake problem should go away because you can call region_is_subregion() directly on the thing cbfs_default_props() fills out.
>
> On the other hand, yes, just having it fill out the region_device directly may also be good. Passing raw regions around with the implicit assumption that they point to boot_device_ro() seems a bit unclean if we could have region and device tied together instead.
>
> I don't think the API would look like you described though, I think it would just be
>
> int (*locate)(struct region_device *cbfs_rdev);
>
> The backing storage of the region device comes from the leaf function (from the FMAP code calling boot_device_ro()). cbfs_boot_locate() wouldn't have to call boot_device_ro() at all anymore.

Ya. I wasn't thinking about that we'd be just utilizing the FMAP code. That certainly makes sense. I'll throw something together.

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36867 )

Change subject: cbfs: Make cbfs_default_props() hidden again
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> > > How about just replacing struct cbfs_props with struct region everywhere? They're the exact same struct on the inside, I don't understand why they have different names anyway. Then your apollolake problem should go away because you can call region_is_subregion() directly on the thing cbfs_default_props() fills out.
> >
> > On the other hand, yes, just having it fill out the region_device directly may also be good. Passing raw regions around with the implicit assumption that they point to boot_device_ro() seems a bit unclean if we could have region and device tied together instead.
> >
> > I don't think the API would look like you described though, I think it would just be
> >
> > int (*locate)(struct region_device *cbfs_rdev);
> >
> > The backing storage of the region device comes from the leaf function (from the FMAP code calling boot_device_ro()). cbfs_boot_locate() wouldn't have to call boot_device_ro() at all anymore.
>
> Ya. I wasn't thinking about that we'd be just utilizing the FMAP code. That certainly makes sense. I'll throw something together.

Here's what I came up with: https://review.coreboot.org/c/coreboot/+/36939

[Aaron Durbin (Code Review)]

Aaron Durbin has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/36867 )

Change subject: cbfs: Make cbfs_default_props() hidden again
......................................................................

Abandoned