Messages in this thread | | | Subject | Re: [PATCH 18 of 38] x86: unify pci iommu setup and allow swiotlb to compile for 32 bit | From | Ian Campbell <> | Date | Thu, 27 Nov 2008 17:14:47 +0000 |
| |
On Thu, 2008-11-27 at 12:43 +0900, FUJITA Tomonori wrote: > 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.
Yes, I think adding a comment to the existing code (option a) would be best. I actually have a small queue of other fixes which make swiotlb work properly for x86 PAE and HighMem but they are not particularly well baked at the moment. I'll include a patch to add a comment in that series.
Ian.
| |