Re: [PATCH v3 3/4] drivers: firmware: add riscv SSE support

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1944 bytes --]

On Thu, Jan 23, 2025 at 11:52:35AM +0100, Clément Léger wrote:
> On 16/01/2025 14:58, Conor Dooley wrote:
> >> +static int sse_sbi_event_func(struct sse_event *event, unsigned long func)
> >> +{
> >> +	struct sbiret ret;
> >> +	u32 evt = event->evt;
> >> +
> >> +	ret = sbi_ecall(SBI_EXT_SSE, func, evt, 0, 0, 0, 0, 0);
> >> +	if (ret.error)
> >> +		pr_debug("Failed to execute func %lx, event %x, error %ld\n",
> >> +			 func, evt, ret.error);
> > 
> > Why's this only at a debug level?
> 
> That's only really meaningful for debugging, this error is often
> reported to the upper level and ends up to the final caller. I don't
> think we should be too verbose for such drivers, but rather propagate
> the error. If one wants to debug, then, just enable DEBUG.
> 
> But that's only my opinion, if you'd prefer all pr_debug to be either
> removed or changed to pr_err(), I'll do it.

Nah, you can leave it as is, you know better than I about how helpful it
would be as an error.

> >> +int sse_event_enable(struct sse_event *event)
> >> +{
> >> +	int ret = 0;
> >> +	struct sse_per_cpu_evt cpu_evt;
> >> +
> >> +	scoped_guard(mutex, &sse_mutex) {
> >> +		cpus_read_lock();
> >> +		if (sse_event_is_global(event->evt)) {
> >> +			ret = sse_sbi_enable_event(event);
> >> +		} else {
> >> +			cpu_evt.event = event;
> >> +			atomic_set(&cpu_evt.error, 0);
> >> +			cpu_evt.func = SBI_SSE_EVENT_ENABLE;
> >> +			on_each_cpu(sse_event_per_cpu_func, &cpu_evt, 1);
> >> +			ret = atomic_read(&cpu_evt.error);
> >> +			if (ret) {
> >> +				cpu_evt.func = SBI_SSE_EVENT_DISABLE;
> >> +				on_each_cpu(sse_event_per_cpu_func, &cpu_evt,
> >> +					    1);
> > 
> > nit: this should fit on one line, no?
> 
> the trailing ; is above 80 characters. But if you are ok with 100 char,
> I can go for it.

The slight improvement in readability trumps the slight increase over 80
characters every time for me.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]