In D32579#735327, @jrtc27 wrote:

Presumably the transform isn't hard to write for someone who knows coccinelle, but the issue is knowing whether a string is from a trusted source or not (in the sense that we can trust it to have a NUL byte; strlcpy is safe if you know there's a NUL in the object but just cannot guarantee a bound on its length). For that you'd presumably need taint tracking via annotations on every data structure.

I like it. Is there a coccinelle script we can run on the whole tree?

nice catch.

Looks good to me. Any other places we make this mistake?

I'm removing mips in a few weeks. If there is a hassle due to mips, it won't be a hassle for long. Unless I've misread this, I don't see this change being MFCd.

I have other comments on the makefile since it seems to confuse machine and machine_arch in the terminology. But that's beyond the scope of this change...

Removed core from reviewer. This tripped over the GPL check in herald for SPDX lines, but the OR MIT means it's fine and doesn't require approval (which is why I removed core blocking rather than approved it with my core hat on).

In D32512#733680, @cperciva wrote:

In D32512#733610, @cperciva wrote:

Rather than having a callback after 1 second, can you leave the final synchronization happening in the SYSINIT? I realize this will slow things down (by preventing anything else from happening during that 1 second interval) but I will be able to speed up the calibration dramatically via repeated measurements.

Sure, I can do this, and that would side-step some of the discussion above. Can you explain your approach in more detail (or provide a pointer to a patch)?

Conceptually: rather than plotting two points on a graph, drawing a line between them, and calculating the slope, I was plotting a few thousand points on a graph and drawing the best-fit line through them. How long it takes will depend on how fast the underlying timer is but I was getting < 1 ppm accuracy in ~10 ms..

In D32512#733515, @markj wrote:

In D32512#733514, @imp wrote:

In D32512#733502, @markj wrote:

In D32512#733493, @imp wrote:

What's the frequency error you see in early boot?

From the PIT loop you mean? I can't remember offhand but the value was clearly wrong. I'll recheck later today.

For either that or the CPUID leaf values. During early boot, the frequency error can be large (even a few percent, as much as maybe 10% would be fine for device initialization, etc), but once we start 'timekeeping' for real, it needs to be < 100-200ppm and ideally < 10-20ppm (though at this level of accuracy, thermal effects start to introduce a fair amount of noise).

This was mentioned in the review description, though admittedly it is too verbose, sorry: I get 1600MHz from CPUID and ~1608MHz from late calibration (and ntpd reports an error of 5-20ppm with the latter). I suspect 1600MHz is good enough for early boot, indeed.

In D32512#733502, @markj wrote:

In D32512#733493, @imp wrote:

What's the frequency error you see in early boot?

From the PIT loop you mean? I can't remember offhand but the value was clearly wrong. I'll recheck later today.

Also, Colin has code in this area as well, might want to add him as a reviewer.

What's the frequency error you see in early boot?

Don't know how I missed this in my sweep! Good catch!

Do we need a man page for this?

.PHONY fix

Move to opt-out rather than opt-in list.

Add net*/*.h too
We're at ~250/650 being stand alone in these directories.

Note:

hack for phab

this time with goodfiles.inc <sigh>

updates based on suggestions so far: move to a goodfiles list that's updated by
a makefile target. This makes maintaining the list trivial and so long as we run
the target only when hdrtest is buildable, the list will only ever increase in
size (absent those rare cases we remove files from /usr/include).

I'm thinking .include with a target to regenerate the file... the list would keep things working. Regenerating when you fix something would be easy and thus the list would only grow. No need for marking or filtering.

Better comment... Will harmonize it and the commit message.

Pull in most of brooks' suggested changes
Also augment it a little with cleaning the generated files
Still not sure how the target ${.OBJDIR}/sys gets created, but special code to
create it seems unnecessary.

In D32498#733162, @brooks wrote:

Thinking about @markj's idea, a BADHDRS variable to filter out headers from a generated list wouldn't be hard to maintain.

83581511d9476ef5084f47e3cc379be7191ae866 should have closed this.

In D32498#733144, @markj wrote:

I know this isn't actually part of buildworld
And I'd be keen on knowing whether or not an extra file with all the header files would be a good idea
As well as creating an actual dummy program that might make this a little easier.

So as developers add new headers, we want to ensure that they don't have external dependencies. With the scheme in the proposed diff, they have to manually add their header to this file. I think a script (or makefile I guess) which tests every single header that we install and compares failures against a known-bad list would require the least maintenance. I'm not quite sure what an "extra file with all of the header files" would be for - can't the makefile do something like "HDRS!=find <directories> -name \*.h" and test each one? Or is that too hacky?

I'm torn... on the one hand I like the idea of building this during MK_TESTS=yes because the build will fail if you break it.
On the other hand, I like being able to test the target system.

better comment

If we 'accept' it, then bump the count of items we've cleared.

Thanks for moving this here... It's a lot easier to comment on something like this here than in bz.

Finalize kib's comments

11.4

Looks like const-poisoning to me. :)

In D32161#730514, @hselasky wrote:

I think this check should be in the linux_pci_probe function, not the attach one.

	if (resource_disabled(device_get_name(dev), device_get_unit(dev)))
		return (ENXIO);

Else looks good.

Minor wording tweak, but now it's fine.