Messages in this thread | | | From | "Tian, Kevin" <> | Subject | RE: [PATCH] iommufd/selftest: Fix dirty_bitmap tests | Date | Fri, 17 Nov 2023 03:15:25 +0000 |
| |
> From: Robin Murphy <robin.murphy@arm.com> > Sent: Friday, November 17, 2023 1:44 AM > > On 16/11/2023 5:28 pm, Joao Martins wrote: > > On 16/11/2023 16:52, Robin Murphy wrote: > >> The ASSERT_EQ() macro sneakily expands to two statements, so the loop > >> here needs braces to ensure it captures both and actually terminates the > >> test upon failure. > > > > Ugh > > > >> Where these tests are currently failing on my arm64 > >> machine, this reduces the number of logged lines from a rather > >> unreasonable ~197,000 down to 10. While we're at it, we can also clean > >> up the tautologous "count" calculations whose assertions can never fail > >> unless mathematics and/or the C language become fundamentally broken. > >> > >> Fixes: a9af47e382a4 ("iommufd/selftest: Test > IOMMU_HWPT_GET_DIRTY_BITMAP") > >> Signed-off-by: Robin Murphy <robin.murphy@arm.com> > > > > I was going to say that the second assert is useful, but we are already test > the > > number of bits we set against what the mock domain set after > > mock_domain_set_dirty(). So the second is redundantly testing the same, > and can > > be removed as you are doing. Thanks for fixing this. > > Yeah, it's still effectively just counting half the number of loop > iterations executed, but since there's no control flow that could exit > the loop early and still reach the assertion, it must always be true > following the previous assertion that out_dirty == nr == nbits/2. > > > I would suggest the subject to: > > > > iommufd/selftest: Fix _test_mock_dirty_bitmaps() > > > > Because dirty-bitmap tests seems to imply the whole fixture, which covers > more > > than the bitmaps. > > Sure, that sounds reasonable. Jason, Kevin, would you want a v2 for that > or could it be fixed up when applying? >
Jason can help fix it when applying.
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
| |