On Jan 28, 2015, at 08:35, Ian Lepore <i...@freebsd.org> wrote: > > On Wed, 2015-01-28 at 16:08 +0000, Ruslan Bukin wrote: >> Author: br >> Date: Wed Jan 28 16:08:07 2015 >> New Revision: 277835 >> URL: https://svnweb.freebsd.org/changeset/base/277835 >> >> Log: >> Add ARMv7 performance monitoring counters. >> >> Differential Revision: https://reviews.freebsd.org/D1687 >> Reviewed by: rpaulo >> Sponsored by: DARPA, AFRL >> >> Added: >> head/sys/dev/hwpmc/hwpmc_armv7.c (contents, props changed) >> head/sys/dev/hwpmc/hwpmc_armv7.h (contents, props changed) >> Modified: >> head/lib/libpmc/libpmc.c >> head/sys/arm/arm/intr.c >> head/sys/arm/include/pmc_mdep.h >> head/sys/arm/ti/files.ti >> head/sys/conf/files.arm >> head/sys/dev/hwpmc/hwpmc_arm.c >> head/sys/dev/hwpmc/pmc_events.h >> head/sys/sys/pmc.h > > This was in phabricator for review for 27 hours before it got committed, > that's not enough time to allow people to actually review it. That > would be not enough time for even a simple change, let alone over a > thousand of lines of code. It certainly wasn't reviewed by those > actively working on arm pmc stuff recently (gnn and to a lesser degree, > me). > > Just from a quick glance at the part that wasn't truncated, I notice all > the inline asm stuff is wrong -- it duplicates what's already available > in cpu-v6.h.
I do agree that reviewers weren't given enough time and phabricator seems to make that worse by saying the code is "ready to land" (we're trying to change that). In my defense, I only reviewed this because I implemented the original XScale PMC. I also didn't know who else was working on ARM PMC, so I couldn't warn Ruslan. Andrew was the one that added the ARM group to the review, so maybe he wasn't aware of it either. -- Rui Paulo _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"