On Thu, Jan 12, 2023 at 04:21:28PM +1100, Alexey Kardashevskiy wrote:
> "that function" is set_dr_addr_mask() (btw should I rename it to start with
> amd_?
If you really wanna... I mean the code is already doing AMD-specific handling
but sure, it'll be more obvious then that arch_install_hw_breakpoint() does only
AMD-specific masking there under the info->mask test.
> If it is out of bounds, it won't neither set or get. And these 2 helpers are
> used only by the kernel and the callers pass 0..3 and nothing else. BUG_ON()
> would do too, for example.
Yeah, we don't do BUG_ON - look for Linus' colorful explanations why. :)
In any case, I think we should always aim for proper recovery from errors but
this case is not that important so let's leave it at the WARN_ON_ONCE.
> imho having 1,2,3 in the code eliminates the need in a comment and produces
> the exact same end result. But since nobody cares, I'll do it the shorter
> way with just +1 and +2.
Yeah, the more important goal is simplicity. And that pays off when you have to
revisit that code and figure out what it does, later.
> Sure. Out of curiosity - why?^w^w^w^w^ it reduced the vmlinux size by 48
> bytes, nice.
The same answer - simplicity and speed when reading it.
That
if (per_cpu(amd_dr_addr_mask, smp_processor_id())[dr] == mask)
is a bit harder to parse than
if (per_cpu(amd_dr_addr_mask, cpu)[dr] == mask)
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
