On Wed, Nov 13, 2013 at 3:13 PM, Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx> wrote: > Daniel Vetter <daniel@xxxxxxxx> writes: > >> On Tue, Nov 12, 2013 at 07:58:16PM +0200, Mika Kuoppala wrote: >>> v2: check the ioctl pad and flag parameters >>> >>> Signed-off-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxx> >> >> I've merged this to igt, but there are a few small fixups to do on top: >> - We now have the igt_main macro to cut down a bit on boilerplate. >> - I haven't tested it, but I guess inject_hang will cause some *ERROR* >> noise in dmesg. Rule is that igt testcases should only cause info/debug >> level messages, everything else is considered a failure. I think we >> could fix this by setting the stop_rings debugfs value right _after_ the >> hang is injected, to tell the kernel that the hang it'll see is actually >> fake. > > I will take a look what stop_rings do. I just have a feeling > that with this kind of trickery we shoot ourselves in foot some day. Yeah, it's a bit trickery. But as long as we only have on special testcase I think it's ok. If we grow more injected hang tests which aren't simulated with the stop_ring stuff then maybe a new debugfs variable to tell the kernel to expect a fake hang could be useful. But smells like not worth it right now. > Would be good that we get a proper error messages when we submit a real > hanging batches. Would be also good that our test checks that > the ERROR msg was really emitted. I think the check for the reset count is good enough to make sure the hangcheck code works. If we start to check for specific dmesg lines I fear we'll implicitly make them abi. We have uevents as a general signal to userspace that a hang happened (and now the reset_stat ioctl). > Having whitelist of expected '*ERROR*' messages for these kind of tests > is not an option? Thus far we've just tuned down the message to info level for fake hangs. It does make things a bit easier for the test runner since we can just filter for any dmesg noise with a level >= warn and then fail the test. This is what QA does, and with the latest piglit patches I've just pushed also what the piglit testrunner does. >> - The userspace interface checking has two missing spots: a) checking that >> lookup for an invalid ctx id fails with ENOENT b) checking that non-root >> can't read out the default context. For the later it's probably simples >> to fork a 2nd process and drop the CAP_SYS_ADMIN capability in there. > > I check for ENOENT on submitting bad context id there. Did you miss it > or it is not enough? Oops, missed that. I didn't reread the latest version too carefully :( > I will resubmit when I have b) in place. Thanks. Btw I've already pushed your current patch to igt, so just a follow-up patch is required. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
