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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250124-banker-unbeaten-aef71ed58867@spud>
Date: Fri, 24 Jan 2025 14:15:43 +0000
From: Conor Dooley <conor@...nel.org>
To: Clément Léger <cleger@...osinc.com>
Cc: Paul Walmsley <paul.walmsley@...ive.com>,
	Palmer Dabbelt <palmer@...belt.com>,
	linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	Himanshu Chauhan <hchauhan@...tanamicro.com>,
	Anup Patel <apatel@...tanamicro.com>,
	Xu Lu <luxu.kernel@...edance.com>,
	Atish Patra <atishp@...shpatra.org>
Subject: Re: [PATCH v3 3/4] drivers: firmware: add riscv SSE support

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.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)