Re: [PATCH 2/5] Powerpc/hw-breakpoint: Refactor hw_breakpoint_arch_parse()
From: Christophe Leroy <hidden>
Date: 2019-06-18 06:50:57
Also in:
lkml
Le 18/06/2019 à 06:27, Ravi Bangoria a écrit :
Move feature availability check at the start of the function. Rearrange comment to it's associated code. Use hw->address and hw->len in the 512 bytes boundary check(to write if statement in a single line). Add spacing between code blocks.
Are those cosmetic changes in the boundary check worth it since they disappear in the final patch ? Christophe
quoted hunk ↗ jump to hunk
Signed-off-by: Ravi Bangoria <redacted> --- arch/powerpc/kernel/hw_breakpoint.c | 34 +++++++++++++++-------------- 1 file changed, 18 insertions(+), 16 deletions(-)diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c index 1908e4fcc132..36bcf705df65 100644 --- a/arch/powerpc/kernel/hw_breakpoint.c +++ b/arch/powerpc/kernel/hw_breakpoint.c@@ -133,10 +133,13 @@ int hw_breakpoint_arch_parse(struct perf_event *bp, const struct perf_event_attr *attr, struct arch_hw_breakpoint *hw) { - int ret = -EINVAL, length_max; + int length_max; + + if (!ppc_breakpoint_available()) + return -ENODEV; if (!bp) - return ret; + return -EINVAL; hw->type = HW_BRK_TYPE_TRANSLATE; if (attr->bp_type & HW_BREAKPOINT_R)@@ -145,34 +148,33 @@ int hw_breakpoint_arch_parse(struct perf_event *bp, hw->type |= HW_BRK_TYPE_WRITE; if (hw->type == HW_BRK_TYPE_TRANSLATE) /* must set alteast read or write */ - return ret; + return -EINVAL; + if (!attr->exclude_user) hw->type |= HW_BRK_TYPE_USER; if (!attr->exclude_kernel) hw->type |= HW_BRK_TYPE_KERNEL; if (!attr->exclude_hv) hw->type |= HW_BRK_TYPE_HYP; + hw->address = attr->bp_addr; hw->len = attr->bp_len; - /* - * Since breakpoint length can be a maximum of HW_BREAKPOINT_LEN(8) - * and breakpoint addresses are aligned to nearest double-word - * HW_BREAKPOINT_ALIGN by rounding off to the lower address, the - * 'symbolsize' should satisfy the check below. - */ - if (!ppc_breakpoint_available()) - return -ENODEV; length_max = 8; /* DABR */ if (dawr_enabled()) { length_max = 512 ; /* 64 doublewords */ - /* DAWR region can't cross 512 boundary */ - if ((attr->bp_addr >> 9) != - ((attr->bp_addr + attr->bp_len - 1) >> 9)) + /* DAWR region can't cross 512 bytes boundary */ + if ((hw->address >> 9) != ((hw->address + hw->len - 1) >> 9)) return -EINVAL; } - if (hw->len > - (length_max - (hw->address & HW_BREAKPOINT_ALIGN))) + + /* + * Since breakpoint length can be a maximum of length_max and + * breakpoint addresses are aligned to nearest double-word + * HW_BREAKPOINT_ALIGN by rounding off to the lower address, + * the 'symbolsize' should satisfy the check below. + */ + if (hw->len > (length_max - (hw->address & HW_BREAKPOINT_ALIGN))) return -EINVAL; return 0; }