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

[Conor Dooley]

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.

> 
> Regards
> Himanshu

Did you miss the comment at the end about the remaining TODOs?

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?

Attachment: signature.asc
Description: PGP signature