Re: [PATCH v3] rust: platform: add Io support

[Rob Herring]

On Wed, Dec 11, 2024 at 06:00:31PM -0300, Daniel Almeida wrote:
> Hi Danilo,
> 
> > On 11 Dec 2024, at 15:36, Danilo Krummrich <dakr@xxxxxxxxxx> wrote:
> > 
> > On Wed, Dec 11, 2024 at 02:51:56PM -0300, Daniel Almeida wrote:
> >> Add support for iomem regions by providing a struct IoMem abstraction
> >> for the platform bus. This will request a memory region and remap it
> >> into a kernel virtual address using ioremap(). The underlying reads and
> >> writes are performed by struct Io, which can be derived from the IoRaw
> >> embedded in platform::IoMem.
> >> 
> >> This is the equivalent of pci::Bar for the platform bus.
> >> 
> >> Memory-mapped I/O devices are common, and this patch offers a way to
> >> program them in Rust, usually by reading and writing to their
> >> memory-mapped register set.
> >> 
> >> Both sized and unsized versions are exposed. Sized allocations use
> >> `ioremap_resource_sized` and specify their size at compile time. Reading
> >> and writing to IoMem is infallible in this case and no extra runtime
> >> checks are performed. Unsized allocations have to check the offset
> >> against the regions's max length at runtime and so return Result.
> >> 
> >> Lastly, like pci::Bar, platform::IoMem uses the Devres abstraction to
> >> revoke access to the region if the device is unbound or the underlying
> >> resource goes out of scope.
> >> 
> >> Signed-off-by: Daniel Almeida <daniel.almeida@xxxxxxxxxxxxx>
> >> ---
> >> The PCI/Platform abstractions are in-flight and can be found at:
> >> 
> >> https://lore.kernel.org/rust-for-linux/20241210224947.23804-1-dakr@xxxxxxxxxx/
> >> ---
> >> Changes in v3:
> >> - Rebased on top of v5 for the PCI/Platform abstractions
> >> - platform_get_resource is now called only once when calling ioremap
> >> - Introduced a platform::Resource type, which is bound to the lifetime of the
> >>  platform Device 
> >> - Allow retrieving resources from the platform device either by index or
> >>  name
> >> - Make request_mem_region() optional
> >> - Use resource.name() in request_mem_region
> >> - Reword the example to remove an unaligned, out-of-bounds offset
> >> - Update the safety requirements of platform::IoMem
> >> 
> >> Changes in v2:
> >> - reworked the commit message
> >> - added missing request_mem_region call (Thanks Alice, Danilo)
> >> - IoMem::new() now takes the platform::Device, the resource number and
> >>  the name, instead of an address and a size (thanks, Danilo)
> >> - Added a new example for both sized and unsized versions of IoMem.
> >> - Compiled the examples using kunit.py (thanks for the tip, Alice!)
> >> - Removed instances of `foo as _`. All `as` casts now spell out the actual
> >>  type.
> >> - Now compiling with CLIPPY=1 (I realized I had forgotten, sorry)
> >> - Rebased on top of rust-next to check for any warnings given the new
> >>  unsafe lints.
> >> ---
> >> rust/bindings/bindings_helper.h |   1 +
> >> rust/helpers/io.c               |  17 ++++
> >> rust/kernel/platform.rs         | 209 +++++++++++++++++++++++++++++++++++++++-
> >> 3 files changed, 226 insertions(+), 1 deletion(-)

[...]

> >> +    unsafe fn new(resource: &Resource<'_>, exclusive: bool) -> Result<Self> {
> >> +        let size = resource.size();
> >> +        if size == 0 {
> >> +            return Err(ENOMEM);
> >> +        }
> >> +
> >> +        let res_start = resource.start();
> >> +
> >> +        // SAFETY:
> >> +        // - `res_start` and `size` are read from a presumably valid `struct resource`.
> >> +        // - `size` is known not to be zero at this point.
> >> +        // - `resource.name()` returns a valid C string.
> >> +        let mem_region =
> >> +            unsafe { bindings::request_mem_region(res_start, size, resource.name().as_char_ptr()) };
> > 
> > This should only be called if exclusive == true, right?
> 
> Yes (oops)
> 
> > 
> > Btw. what's the use-case for non-exclusive access? Shouldn't we rather support
> > partial exclusive mappings?
> 
> Rob pointed out that lots of drivers do not call `request_mem_region` in his review for v2, which
> Is why I added support for non-exclusive access.
> 
> What do you mean by `partial exclusive mappings` ?

I dug into the history of this some and I may be misremembering where 
the problem is exactly. The issue for DT is we can't call 
platform_device_add() because it calls insert_resource() and that may 
fail. Now I'm not sure if the drivers in overlapping case have to avoid 
calling request_mem_region(). I think so...

Here's some relevant commits:

8171d5f7bf26 Revert "of/platform: Use platform_device interface"
b6d2233f2916 of/platform: Use platform_device interface
02bbde7849e6 Revert "of: use platform_device_add"
aac73f34542b of: use platform_device_add

There was another attempt to address this here[1], but I don't think 
anything ever landed. 

Grant mentioned mpc5200 ethernet and mdio as one example overlapping. 
Indeed it does:

                ethernet@3000 {
                        compatible = "fsl,mpc5200-fec";
                        reg = <0x3000 0x400>;
                        local-mac-address = [ 00 00 00 00 00 00 ];
                        interrupts = <2 5 0>;
                        phy-handle = <&phy0>;
                };

                mdio@3000 {
                        #address-cells = <1>;
                        #size-cells = <0>;
                        compatible = "fsl,mpc5200-mdio";
                        reg = <0x3000 0x400>;   // fec range, since we need to setup fec interrupts
                        interrupts = <2 5 0>;   // these are for "mii command finished", not link changes & co.

                        phy0: ethernet-phy@0 {
                                reg = <0>;
                        };
                };

The FEC driver does request_mem_region(), but the MDIO driver does not.

Rob

[1] https://lore.kernel.org/all/20150607140138.026C4C412C8@xxxxxxxxxxxxxxxxxxx/