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

[Himanshu Chauhan]

On Tue, Feb 24, 2026 at 1:33 PM Conor Dooley <conor.dooley@xxxxxxxxxxxxx> wrote:
>
> On Tue, Feb 24, 2026 at 09:30:24AM +0530, Himanshu Chauhan wrote:
> > Hi,
> >
> > On Mon, Feb 23, 2026 at 3:55 PM Conor Dooley <conor.dooley@xxxxxxxxxxxxx> wrote:
> > >
> > > 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?
> >
> > It's correct that the absence of the extension is not a problem but an
> > informational message can be helpful.
> > So, I can use pr_info or pr_info_once for this with proper error code. Ok?
>
> IMO, you shouldn't be printing unless there's an actual error. If every
> extension we had support for in the kernel printed a message when it was
> absent, our logs would be saturated.

Accepted.

>
> >
> > Regards
> > Himanshu
>
> Did you miss the comment at the end about the remaining TODOs?

No. As I mentioned in the cover letter, the ptrace support is not
implemented here. I am actively working on it and these are
implemented in ptrace work.
The test is done using the perf events directly. The second patch in
this patch set has the test application.

Regards
Himanshu

>
> Cheers,
> Conor.
>
> >
> > >
> > > > +             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?