linux-kernel - Re: [PATCH 2/2 v5] regmap: mmio: Support accelerared noinc operations

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:   Tue, 30 Aug 2022 19:35:19 +0300
From:   Andy Shevchenko <andriy.shevchenko@...el.com>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     Mark Brown <broonie@...nel.org>, linux-kernel@...r.kernel.org,
        Brian Cain <bcain@...cinc.com>, linux-hexagon@...r.kernel.org,
        Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH 2/2 v5] regmap: mmio: Support accelerared noinc operations

On Tue, Aug 16, 2022 at 10:48:32PM +0200, Linus Walleij wrote:
> Use the newly added callback for accelerated noinc MMIO
> to provide writesb, writesw, writesl, writesq, readsb, readsw,
> readsl and readsq.
> 
> A special quirk is needed to deal with big endian regmaps: there
> are no accelerated operations defined for big endian, so fall
> back to calling the big endian operations itereatively for this
> case.
> 
> The Hexagon architecture turns out to have an incomplete
> <asm/io.h>: writesb() is not implemented. Fix this by doing
> what other architectures do: include <asm-generic/io.h> into
> the <asm/io.h> file.

Wonderful!

So, I have seen a recent blow up by kernel bot due to Alpha issues on these
accessors.

Also see below for the further followups.

...

> +	if (!IS_ERR(ctx->clk)) {
> +		ret = clk_enable(ctx->clk);
> +		if (ret < 0)
> +			return ret;
> +	}

It's a new place of the duplicating check, can we have a helper for that?

...

> +	/*
> +	 * There are no native, assembly-optimized write single register
> +	 * operations for big endian, so fall back to emulation if this
> +	 * is needed. (Single bytes are fine, they are not affected by
> +	 * endianness.)
> +	 */

Wouldn't be faster to memdup() / swap / use corresponding IO accessor?

...

> +	/*
> +	 * There are no native, assembly-optimized write single register
> +	 * operations for big endian, so fall back to emulation if this
> +	 * is needed. (Single bytes are fine, they are not affected by
> +	 * endianness.)
> +	 */
> +	if (ctx->big_endian && (ctx->val_bytes > 1)) {
> +		switch (ctx->val_bytes) {
> +		case 2:
> +		{
> +			u16 *valp = (u16 *)val;
> +			for (i = 0; i < val_count; i++)
> +				valp[i] = swab16(valp[i]);
> +			break;
> +		}
> +		case 4:
> +		{
> +			u32 *valp = (u32 *)val;
> +			for (i = 0; i < val_count; i++)
> +				valp[i] = swab32(valp[i]);
> +			break;
> +		}
> +#ifdef CONFIG_64BIT
> +		case 8:
> +		{
> +			u64 *valp = (u64 *)val;
> +			for (i = 0; i < val_count; i++)
> +				valp[i] = swab64(valp[i]);
> +			break;
> +		}
> +#endif
> +		default:
> +			ret = -EINVAL;
> +			break;
> +		}
> +	}

So, two questions here:

1) can we use helpers from include/linux/byteorder/generic.h, such as
   cpu_to_be32_array()/be32_to_cpu_array()?

2) have you considered using memcpy_toio() / memcpy_fromio() and why
   it's not okay to use them?

...

> +
> +

Single blank line is enough.

> +out_clk:
> +	if (!IS_ERR(ctx->clk))
> +		clk_disable(ctx->clk);
> +
> +	return ret;
> +
> +	return 0;

Seems like misrebase? I believe this has to be fixed.

-- 
With Best Regards,
Andy Shevchenko