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] [thread-next>] [day] [month] [year] [list]
Date: Mon, 12 Feb 2024 15:21:10 +0100
From: Nam Cao <namcao@...utronix.de>
To: "Moritz C. Weber" <mo.c.weber@...il.com>
Cc: marvin24@....de, ac100@...ts.launchpad.net, linux-tegra@...r.kernel.org,
 linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Staging: nvec: nvec: fixed two usleep_range is
 preferred over udelay warnings

On 12/Feb/2024 Moritz C. Weber wrote:
> Fixed a code style issue raised by checkpatch.
> ---
>  drivers/staging/nvec/nvec.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
> index 2823cacde..18c5471d5 100644
> --- a/drivers/staging/nvec/nvec.c
> +++ b/drivers/staging/nvec/nvec.c
> @@ -627,7 +627,7 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)
>  		break;
>  	case 2:		/* first byte after command */
>  		if (status == (I2C_SL_IRQ | RNW | RCVD)) {
> -			udelay(33);
> +			usleep_range(32, 33);
>  			if (nvec->rx->data[0] != 0x01) {
>  				dev_err(nvec->dev,
>  					"Read without prior read command\n");
> @@ -714,7 +714,7 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)
>  	 * We experience less incomplete messages with this delay than without
>  	 * it, but we don't know why. Help is appreciated.
>  	 */
> -	udelay(100);
> +	usleep_range(99, 100);
>  
>  	return IRQ_HANDLED;
>  }

I have zero knowledge about this driver, but nvec_interrupt() seems to be
a hard interrupt handler, and sleeping in an interrupt handler is a big no
no. So I think this change breaks the driver.

Delaying like the driver is currently doing doesn't break things, but it is
not very nice because this is interrupt handler and the processor cannot
switch to other tasks, so delaying is wasting processor's cycles here. The
better fix would be to figure out how to remove the delay entirely, or
switch to threaded interrupt handler and then we can use usleep_range() in
there, but you need actual hardware to test such changes.

Best regards,
Nam



Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ