Re: [PATCH 4/4] sample/reserved_mem: Introduce a sample of struct page and dio support to no-map rmem

[Li Chen]

Hi Arnd,
 ---- On Mon, 11 Jul 2022 21:28:10 +0800  Arnd Bergmann <arnd@xxxxxxxx> wrote --- 
 > On Mon, Jul 11, 2022 at 2:24 PM Li Chen <me@linux.beauty> wrote:
 > >
 > > From: Li Chen <lchen@xxxxxxxxxxxxx>
 > >
 > > This sample driver shows how to build struct pages support to no-map rmem.
 > >
 > > Signed-off-by: Li Chen <lchen@xxxxxxxxxxxxx>
 > 
 > Not sure what a sample driver helps here if there are no actual users in-tree.
 > 
 > It would make more sense to merge the driver that wants to actually use this
 > first, and then add the additional feature.

Totally agree, but we plan to start rewriting our video driver in a long time, it has many legacy codes and I need to rewrite a lot of codes to migrate to v4l2.
That's why I also submit a sample driver here: to make the review progress easier and don't need reviewers to read video driver codes.

 > > +/*
 > > + * dts example
 > > + * rmem: rmem@1 {
 > > + *                     compatible = "shared-dma-pool";
 > > + *                     no-map;
 > > + *                     size = <0x0 0x20000000>;
 > > + *             };
 > > + * perf {
 > > + *             compatible = "example,rmem";
 > > + *             memory-region = <&rmem>;
 > > + *     };
 > 
 > The problem here is that the DT is meant to describe the platform in an OS
 > independent way, so having a binding that just corresponds to a user space
 > interface is not a good abstraction.

Gotcha, but IMO dts + rmem is the only choice for our use case. In our real case, we use reg instead of size to specify the physical address, so memremap cannot be used.

 > 
 > > +       vaddr = reserved_mem_memremap_pages(dev, rmem);
 > > +       if (IS_ERR_OR_NULL(vaddr))
 > > +               return PTR_ERR(vaddr);
 > 
 >  Using IS_ERR_OR_NULL() is usually an indication of a bad interface.
 > 
 > For the reserved_mem_memremap_pages(), you should decide whether to return
 > NULL on error or an error pointer, but not both.

Thanks, will fix in v2.

 > 
 >        Arnd
 >