Re: [PATCH v8 2/2] rust: add dma coherent allocator abstraction.

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Jan 31, 2025 at 08:47:54AM +0100, Greg KH wrote:
> On Thu, Jan 30, 2025 at 01:24:37PM -0400, Jason Gunthorpe wrote:
> > On Thu, Jan 30, 2025 at 05:11:43PM +0100, Greg KH wrote:
> > > On Thu, Jan 30, 2025 at 11:46:46AM -0400, Jason Gunthorpe wrote:
> > > > On Thu, Jan 30, 2025 at 02:19:16PM +0100, Philipp Stanner wrote:
> > > > > would some sort of official statement by the "entire community"
> > > > > reassure you that the burden of keeping Rust abstractions working with
> > > > > any changes on the C side rests entirely on the Rust side's
> > > > > shoulders?
> > > > 
> > > > You'd have to reconcile that with the recent event where Linus defered
> > > > the MM pull request and some C patches were dropped because of rust
> > > > kbuild bugs:
> > > > 
> > > > https://lore.kernel.org/linux-mm/CAHk-=whddBhfi5DUi370W3pYs+z3r2E7KYuHjwR=a1eRig5Gxg@mail.gmail.com/
> > > > 
> > > > It seems to me the message is now crystal clear, and the opposite of
> > > > what you claim.
> > > > 
> > > > All PRs to Linus must not break the rust build and the responsibilty
> > > > for that falls to all the maintainers. If the Rust team is not quick
> > > > enough to resolve any issues during the development window then
> > > > patches must be dropped before sending PRs, or Linus will refuse the
> > > > PR.
> > > > 
> > > > Effectively this seems to imply that patches changing some of the C
> > > > API cannot be merged by maintainers unless accompanied by matching
> > > > Rust hunks.
> > > > 
> > > > If there are different instructions to maintainers I would be
> > > > interested to know.
> > > 
> > > That's not the case, the one you point at above was a tooling issue that
> > > people missed due to the holidays.  Fixing it up was simple enough and
> > > people did so and moved on.
> > 
> > Regardless of holidays, you seem to be saying that Linus should have
> > accepted Andrew's PR and left rust with build failures?
> 
> I can't answer for Linus, sorry.  

Then I think we need a clear statement from Linus how he will be
working. If he is build testing rust or not.

Without that I don't think the Rust team should be saying "any changes
on the C side rests entirely on the Rust side's shoulders".

It is clearly not the process if Linus is build testing rust and
rejecting PRs that fail to build.

> But a generic "hey, this broke our
> working toolchain builds" is something that is much much much different

It broke some builds, it sounded like the typical rust build was not
effected because it used the same version of clang for C code and
bindgen. Linus was mixing gcc and clang in his build.

> than "an api changed so I now have to turn off this driver in my build"
> issue.

I tested this theory, allnoconfig x86 build, enable 64 bit, rust and
PCI only. *NO* Rust drivers enabled.

Make a hypothetical C API change:

       --- a/include/asm-generic/pci_iomap.h
       +++ b/include/asm-generic/pci_iomap.h
       @@ -18,7 +18,7 @@ extern void __iomem *pci_iomap_range(struct pci_dev *dev, int bar,
	extern void __iomem *pci_iomap_wc_range(struct pci_dev *dev, int bar,
					       unsigned long offset,
					       unsigned long maxlen);
       -extern void pci_iounmap(struct pci_dev *dev, void __iomem *);
       +extern void pci_iounmap(struct pci_dev *dev, void __iomem *, unsigned int flags);
	/* Create a virtual mapping cookie for a port on a given PCI device.
	 * Do not call this directly, it exists to make it easier for architectures
	 * to override */
       diff --git a/lib/iomap.c b/lib/iomap.c
       index 4f8b31baa5752a..5b63063a1a811f 100644
       --- a/lib/iomap.c
       +++ b/lib/iomap.c
       @@ -421,7 +421,7 @@ EXPORT_SYMBOL(ioport_unmap);
	#ifdef CONFIG_PCI
	/* Hide the details if this is a MMIO or PIO address space and just do what
	 * you expect in the correct way. */
       -void pci_iounmap(struct pci_dev *dev, void __iomem * addr)
       +void pci_iounmap(struct pci_dev *dev, void __iomem * addr, unsigned int flags)
	{
	       IO_COND(addr, /* nothing */, iounmap(addr));
	}

And the result is..

       error[E0061]: this function takes 3 arguments but 2 arguments were supplied
	  --> ../rust/kernel/pci.rs:320:13
	   |
       320 |             bindings::pci_iounmap(pdev.as_raw(), ioptr as _);
	   |             ^^^^^^^^^^^^^^^^^^^^^--------------------------- an argument of type `u32` is missing
	   |
       note: function defined here
	  --> /tmp/x/build/rust/bindings/bindings_generated.rs:3:1444598
	   |
       3   | ...> * mut ffi :: c_void ; } extern "C" { pub fn pci_iounmap (dev : * mut pci_dev , arg1 : * mut ffi :: c_v...
	   |                                                  ^^^^^^^^^^^
       help: provide the argument
	   |
       320 |             bindings::pci_iounmap(pdev.as_raw(), ioptr as _, /* u32 */);
	   |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Build fails.

Can't seem to fix this using kconfig without turning off CONFIG_RUST,
exactly the same outcome as Uros's case.

Doesn't seem "much much much different" to me.

This shouldn't be surprising. It doesn't work like C. Rust builds the
PCI bindings always once CONFIG_PCI is turned on. It doesn't matter if
no rust driver is being built that consumes those bindings. It won't
work like staging does where you can just turn off one driver.

Jason