The values are already validated by clock_ts_to_ct. In my opinion, we can drop those checks to avoid checking them twice.

That was my first impulse. OTOH, there's no guarantee that clock_ts_to_ct will continue to validate them the same way that this function does (and you'll notice that it doesn't validate year quite the same way). And since these are only assertions, they don't even get included in production builds, so I left them in.

Good catch.

clock_ts_to_ct should always return a valid clocktime if it succeeds. If the vrtc isn't able to handle a valid clocktime, it shouldn't ask for it.

The asserts are already included. So, it's fine to keep them. We could just think of removing these unneccessary checks while we're already here.

What do you mean by "keep the asserts", but "remove the unneccessary checks"? What's the difference between checks and asserts?

could this be reached from setting the date via the command line? If so, then it shouldn't panic.
Also, if this is in the boot path, it will keep us from booting at all.

This is wrong. It can never be 60 here. time_t has no ability to represent a leap second since POSIX does not define an encoding for it, let alone a unique one (There's a formula in the standard, but it's specifically not for leap seconds).

So how does this fail? Especially since we truncate the year a few lines below to 25500 years.
Since this is vendor code (I know it's in the wrong place in the tree for that, but it is), and since there's other bugs, I'd suggest leaving it alone and reporting it upstream.

Panic here means we can't boot.

panic here means we can't boot.

No. It does not. In fact, it cannot because time_t doesn't encode leap seconds. There's no defined time_t encoding for a leap second.

59. Though this can never fire after the rsec = rsec % 60

As far as I can tell, no. I think only BHyve uses this. And this panic isn't new; there was always a panic in this path, at least in INVARIANTS builds.

Allright, I'll adjust all of the assertions and checks to exclude the possibility of leap seconds.

Even though the RTC can't be set to a year greater than 10,000 AD, the system clock can be. So getnanouptime could return 1.4 billion AD or something like that. In that case, the clocktime structure won't be initialized. And even though there's no precedent for this function returning an error, there is for its caller. So I think it's ok to return an error here. The only alternatives would be to panic, or to zero the structure, or something like that.
Do you know where the upstream source is?

Yeah, but it paniced before this change too, at least when INVARIANTS were on. It's easy enough to fix though, so I'll do it.

Same as above, this line paniced before too. And I'll fix it too.

As a matter of fact, it should be impossible for any of the checks to fail except for the year one. I'll remove them all, but restore the KASSERTs.

Sry, for being unclear. We can think of removing the asserts because we're touching this code but if we keep them, it's fine.