Re: [cdi-devel] [RFC PATCH] mem.h: Handling memory areas

[Kevin Wolf <kevin@xxxxxxxxxx>]

On Wed, Jan 27, 2010 at 09:51:24PM +1000, Matthew Iselin wrote:
> On Wed, Jan 27, 2010 at 7:54 AM, Kevin Wolf <kevin@xxxxxxxxxx> wrote:
> > At this point, we have two possibilities: The kernel can just pass the
> > unchanged memory area down and the driver needs to make it fit its
> > requrements (this is what I was trying to achieve with
> > cdi_mem_require_flags). The only alternative that I see is to specify
> > the required flags for each memory area parameter of any callback
> > function in the driver struct. I personally consider this really awkward.
> 
> I must have misunderstood your initial explanation of
> cdi_mem_require_flags. In light of the information in this newest
> mail...
> 
> So cdi_mem_require_flags would take a buffer that might be passed from
> an external source, and ensure that the buffer matches the given
> flags? If so that's a good idea and my only nitpick would be to ask
> for a rename ;)

Yes, I think this is what I meant. I'm open for suggestions for a better
name.

> >> Again, cdi_mem_require_flags probably shouldn't perform a copy.
> >>
> >> It just sounds like the interface could be cleaned up a bit, and
> >> especially could be modified to reduce the amount of copying that the
> >> CDI implementation might have to do. Drivers at least should know what
> >> they need from the very beginning.
> >
> > The whole thing was made to avoid copies in the first place. But there
> > are cases where you just need to copy data. So what I tried is to
> > introduce functions which transparently do the copy or are a no-op when
> > no copy is needed.
> 
> Right. I think this is one spot where the semantics of the functions
> need to be unambiguously defined (very clearly ;) )... Documentation
> here should be very in-depth in order to avoid misunderstandings like
> the one I experienced.

Unfortunately, it seems when I try to explain things very clearly it's
almost guaranteed that you are going to misunderstand them. ;-)

Maybe you could modify the patch so it fits your idea of a
well-documented clean header?

> > By the way, one more thing that I wasn't sure about is if it's really
> > helpful to restrict the whole thing to a page granularity. Maybe this
> > should just be a flag CDI_MEM_PAGE_ALIGN which could be used by drivers
> > whenever it's helpful to use complete pages. Or maybe leave it
> > completely to the OS.
> 
> I'd say leave it up to the driver to ask for a page-aligned area. In
> fact, a better idea would be to have alignments on different word
> boundaries, as some devices are more than happy to accept buffers
> which are not page aligned but rather aligned on an n-word or n-byte
> boundary.

That's a very good point, actually. Should we reserve some bits in the
flags for it, make it another parameter or even a new function for
allocating/requiring aligned memory?

Another point that came to my mind today is that maybe we shouldn't have
a single paddr in the memory area, but maintain a scatter/gather list of
physical addresses.

Kevin