[Cbe-oss-dev] [patch 07/16] powerpc and x86_64: Initial port of PCIe endpoint device driver for Axon

[Tim Schimke]

Thanks for the helpful comments.

Arnd Bergmann <arnd.bergmann at de.ibm.com> wrote on 03/12/2008 10:57:38 PM:

> On Saturday 08 March 2008, Murali Iyer wrote:
> > +void axon_initialize_dacs_sysfs(struct axon *axon_priv)
> > +{
>
> What does this function have to do with sysfs? It seems to just
> set some generic variables per device that happen to also be exported
> through sysfs.
>
Correct, will be renamed.


> The request size limits seem rather strange to me. User space should not
> assume that you can map anything of this size into the IOMMU. On most
> systems, the IOMMU has a total size of just a few megabytes, shared by
> all devices, and it's certainly not just architecture dependent.
>
> E.g. when you have a System p host system, the limit will be around 128MB
> for all of them. It may be good to start with a really small size here,
> e.g. 16MB, and require the (root) user to increase it to a system
specific
> reasonable setting that can be enforced by the device driver, so we don't
> crash the system because of exhausted IOMMU space.
>
Maybe we should investigate ways to make the limit configurable such that
large
values will work on systems which support it, but it limits itself more
nicely
on platforms with limited resources. Needs more thought ....


>
> I'm not sure I understand what this function is used for, but
> you'll have to audit this when going to a more recent kernel,
> as you can no longer use 'sg + i' for indexing the sg list entry
> with chained scatterlists.
>
This is still being developed on a kernel without the new scatterlist APIs,
there are a number of changes required to move ahead. This is one.


> > +
> > +   if (length == 0) {
> > +      rc = -ETOOSMALL;
> > +      pr_info("axon_register_mr: Request length 0 is invalid\n");
> > +      return rc;
> > +   }
>
> User supplied values should normally not cause a kernel log message at
> level higher than pr_debug -- it easily causes problems with
syslogdflooding.
>
Eventually this need to change to pr_debug, they are only pr_info to aid
in initial debug of user code.


> There is nothing wrong with mutex_lock_interruptible returning
-ERESTARTSYS,
> so this certainly should not cause a log message.
>
Will fix.


> It looks like you pass the user page directly into the iommu mapping,
which
> might be highly dangerous for anything except page cache or hugepages.
> How is this code expected to deal with SPU local store pages while the
> SPU is context switched?
>
SPU local store pages are not supported. Is there a check that could be
added
to detect this and reject the request?


> As mentioned a few times before, sg_dma_address() should return the
> address with the right offset as it came from dma_map_sg, and not
> require local_phys_to_plb5() here.
>
This is the translation required to convert the address to the form needed
internally by the AXON DMA controller HW. It is done here to move the
translation
time off of the DMA setup path. Arguably it should be done by the DMA
controller
code instead, but this performs slightly better.


> You shouldn't need to include the other header here,
> just add a forward declaration for the structures in there,
> like
>
> struct axon;
> struct axon_priv;
>
Will fix.


Tim