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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20111101135139.GC22652@phenom.dumpdata.com>
Date:	Tue, 1 Nov 2011 09:51:39 -0400
From:	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
To:	Jerome Glisse <j.glisse@...il.com>
Cc:	linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
	thellstrom@...are.com, thomas@...pmail.org, airlied@...hat.com,
	xen-devel@...ts.xensource.com, j.glisse@...hat.com,
	bskeggs@...hat.com
Subject: Re: [PATCH 08/11] ttm: Provide DMA aware TTM page pool code.

> > +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@...hat.com>

Great! Thank you! 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/