Re: [PATCH v3 1/2] riscv: Introduce support for hardware break/watchpoints

[Conor Dooley]

On Mon, Feb 23, 2026 at 10:19:17AM +0530, Himanshu Chauhan wrote:
> +static void init_sbi_dbtr(void)
> +{
> +	unsigned long tdata1;
> +	struct sbiret ret;
> +
> +	if (sbi_probe_extension(SBI_EXT_DBTR) <= 0) {
> +		pr_warn("%s: SBI_EXT_DBTR is not supported\n", __func__);

This is going to run an all systems, right? A pr_warn() seems
inappropriate, given that not supporting this extension isn't a problem.
If you want to produce warnings, only do it for < 0 return values and
maybe print the actual error code too?

> +		dbtr_total_num = 0;
> +		goto done;
> +	}
> +
> +	ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS,
> +			0, 0, 0, 0, 0, 0);
> +	if (ret.error) {
> +		pr_warn("%s: Failed to detect triggers\n", __func__);
> +		dbtr_total_num = 0;
> +		goto done;
> +	}
> +
> +	tdata1 = 0;
> +	tdata1 = RV_DBTR_SET_TDATA1_TYPE(tdata1, RV_DBTR_TRIG_MCONTROL6);
> +
> +	ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS,
> +			tdata1, 0, 0, 0, 0, 0);
> +	if (ret.error) {
> +		pr_warn("%s: failed to detect mcontrol6 triggers\n", __func__);
> +	} else if (!ret.value) {
> +		pr_warn("%s: type 6 triggers not available\n", __func__);
> +	} else {
> +		dbtr_total_num = ret.value;
> +		dbtr_type = RV_DBTR_TRIG_MCONTROL6;
> +		pr_warn("%s: mcontrol6 trigger available.\n", __func__);
> +		goto done;
> +	}
> +
> +	/* fallback to type 2 triggers if type 6 is not available */
> +
> +	tdata1 = 0;
> +	tdata1 = RV_DBTR_SET_TDATA1_TYPE(tdata1, RV_DBTR_TRIG_MCONTROL);
> +
> +	ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS,
> +			tdata1, 0, 0, 0, 0, 0);
> +	if (ret.error) {
> +		pr_warn("%s: failed to detect mcontrol triggers\n", __func__);
> +	} else if (!ret.value) {
> +		pr_warn("%s: type 2 triggers not available\n", __func__);
> +	} else {
> +		dbtr_total_num = ret.value;
> +		dbtr_type = RV_DBTR_TRIG_MCONTROL;
> +		goto done;
> +	}
> +
> +done:
> +	dbtr_init = 1;
> +}

> +
> +void hw_breakpoint_pmu_read(struct perf_event *bp)
> +{
> +	/* TODO */
> +}
> +
> +void clear_ptrace_hw_breakpoint(struct task_struct *tsk)
> +{
> +	/* TODO */
> +}
> +
> +void flush_ptrace_hw_breakpoint(struct task_struct *tsk)
> +{
> +	/* TODO */
> +}

This isn't an RFC, why does it have TODOs?

Attachment: signature.asc
Description: PGP signature