Date: Thu, 27 Nov 2008 12:43:52 +0900
To: Ian.Campbell@citrix.com
Cc: fujita.tomonori@lab.ntt.co.jp, jeremy@goop.org, mingo@elte.hu,
       linux-kernel@vger.kernel.org, xen-devel@lists.xensource.com,
       x86@kernel.org
Subject: Re: [PATCH 18 of 38] x86: unify pci iommu setup and allow swiotlb
	to	compile for 32 bit
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
In-Reply-To: <1227692209.9425.117.camel@zakaz.uk.xensource.com>
References: <1227526897.9425.38.camel@zakaz.uk.xensource.com>
	<20081126115344B.fujita.tomonori@lab.ntt.co.jp>
	<1227692209.9425.117.camel@zakaz.uk.xensource.com>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Message-Id: <20081127124329X.fujita.tomonori@lab.ntt.co.jp>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1428
Lines: 29

On Wed, 26 Nov 2008 09:36:49 +0000
Ian Campbell <Ian.Campbell@citrix.com> wrote:

> On Wed, 2008-11-26 at 11:53 +0900, FUJITA Tomonori wrote:
> > 
> > > +     BUG_ON(max_slots > 1UL << (BITS_PER_LONG - IO_TLB_SHIFT));
> > 
> > How can this BUG_ON happen? Using u64 for the mask is fine though.
> 
> It covers the cases where the previous code would have overflowed. It
> can't happen right now because although mask is 64 bits the value
> assigned to it is currently sizeof(unsigned long). If someone changes
> the type of that field then we would start seeing unexpected values.

If someone changes dma_get_seg_boundary to return a u64 value instead
of unsigned long, this BUG_ON could happen on 32bit architectures. But
you don't need to trigger BUG_ON for it. max_slots > 1UL <<
(BITS_PER_LONG - IO_TLB_SHIFT) should be fine for
iommu_is_span_boundary().

Anyway, this is minor but would it be nice to make sure that anyone
can easily understand the code without digging into the git log?

a) dropping this patch and adding some comments how the code works
(especially about the overflow on 32bit architectures).

b) removing the BUG_ON in this patch and adding some comments.

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