Skip to content

include: zephyr: drivers: Add flash_get_page_layout() syscall #53324

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

duda-patryk
Copy link
Contributor

Each flash layout entry describes continuous part of flash with the same page size (page is a smallest erasable unit of flash). This way, the flash layout is represented in an efficient way e.g. if all flash pages have the same size, the whole flash will be described in one entry.

Introduced function provides direct access to the flash layout representation.

Best regards,
Patryk

Each flash layout entry describes continuous part of flash with the same
page size (page is a smallest erasable unit of flash). This way, the
flash layout is represented in an efficient way e.g. if all flash pages
have the same size, the whole flash will be described in one entry.

Introduced function provides direct access to the flash layout
representation.

Signed-off-by: Patryk Duda <[email protected]>
@duda-patryk duda-patryk force-pushed the pdk_flash_get_page_layout branch from 01b1625 to 4257acd Compare December 23, 2022 12:42
Copy link
Contributor

@de-nordic de-nordic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this is good idea.
You just expose pointer to kernel/driver structure that may at some point happen in protected part of memory and cause protection fault when iterated over in user space.

@duda-patryk
Copy link
Contributor Author

Actually there are couple of examples of this:

  • flash_get_parameters() also returns pointer to internal driver structure.
  • flash_page_foreach() is not marked as __syscall. It seems like userspace is calling api->page_layout() directly (and accessing driver structures).

But I understand your point. What do you think about adding flash_layout_foreach() marked as __syscall?

@de-nordic
Copy link
Contributor

de-nordic commented Jan 3, 2023

Actually there are couple of examples of this:

* `flash_get_parameters()` also returns pointer to internal driver structure.

* `flash_page_foreach()` is not marked as `__syscall`. It seems like userspace is calling `api->page_layout()` directly (and accessing driver structures).

Yes we have problems there. The flash_get_parameters should probably get modified to do copyout to provided buffer; worth thinking it over.

But I understand your point. What do you think about adding flash_layout_foreach() marked as __syscall?

I was considering that, but got two problems with this:

  1. this will elevate execution of user code to system space;
  2. if you make the callback CPU heavy you will end up spending a lot of time in syscall iterating over flash.

The foreach case is quite rare and there are three main uses now 1) get flash size, 2) check flash layout to figure out whether it is uniform, because FS needs that, 3) list all pages for some reason (shell?)

These shouldn't happen too often and can probably take performance hit in querying each page info individually.

Some time ago I proposed this #36484 which gets rid of the layout array and instead directly asks drivers for information on flash size or page info. Unfortunately it is in review limbo.
Above makes foreach less efficient, due to multiple syscalls to get info for each page independently, but allows to, for example, to query external flash devices for size without hardcoding it into the driver.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants