[Xen-devel] Re: [PATCH 08/11] ttm: Provide DMA aware TTM page pool code.

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

> > +static struct dma_page *__ttm_dma_alloc_page(struct dma_pool *pool)
> > +{
> > +   struct dma_page *d_page;
> > +
> > +   d_page = kmalloc(sizeof(struct dma_page), GFP_KERNEL);
> > +   if (!d_page)
> > +           return NULL;
> > +
> > +   d_page->vaddr = dma_alloc_coherent(pool->dev, pool->size,
> > +                                      &d_page->dma,
> > +                                      pool->gfp_flags);
> > +   d_page->p = virt_to_page(d_page->vaddr);
> > +   if (!d_page->vaddr) {
> > +           kfree(d_page);
> > +           d_page = NULL;
> > +   }
> 
> Move d_page->p = virt_to_page(d_page->vaddr); after if (!d_page->vaddr)
> block.

Duh! Yes.

.. snip..
> > +#if 0
> > +   if (nr_free > 1) {
> > +           pr_debug("%s: (%s:%d) Attempting to free %d (%d) pages\n",
> > +                   pool->dev_name, pool->name, current->pid,
> > +                   npages_to_free, nr_free);
> > +   }
> > +#endif

What is your feeling on those #if 0? I was not sure to keep them - they are 
useful
when debugging, but not so much during run-time? Rip them out and I can just
keep them in my 'debug' patch queue in case things go wrong?

Or perhas do it (rip 'em out) in 3 months time-frame?

.. snip..
> > +static struct dma_pool *ttm_dma_find_pool(struct device *dev,
> > +                                     enum pool_type type)
> > +{
> > +   struct dma_pool *pool, *tmp, *found = NULL;
> > +
> > +   if (type == IS_UNDEFINED)
> > +           return found;
> > +   /* NB: We iterate on the 'struct dev' which has no spinlock, but
> > +    * it does have a kref which we have taken. */
> 
> I fail to see where we kref dev.

Ah, I should document that more extensivly. That is done way way
earlier. As in in the path of the initialization of the driver:

drm_pci_init
  for non-KMS calls pci_dev_get()

  for KMS calls pci_register_driver..
      which calls 'driver_register' which called 'device_register'

And then during teardown (so unbind on sysfs), it ends up calling the devres
deconstructors which cleans up the 'struct device' dev_res, - in our case
ttm_dma_pool_release. However the nice thing is at that point of time all of
the calls to the TTM have quiseced so nobody is calling ttm for this device
anymore.

Let me stick this in the comment section.

> See comment above, otherwise:
> Reviewed-by: Jerome Glisse <jglisse@xxxxxxxxxx>

Great! Thank you! 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel