linux-kernel - Re: [RFC PATCH v2 3/3] soc: sprd: Add Spreadtrum special bits updating 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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 15 Apr 2020 17:36:07 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Baolin Wang <baolin.wang7@...il.com>
Cc:     Lee Jones <lee.jones@...aro.org>, Mark Brown <broonie@...nel.org>,
        Orson Zhai <orsonzhai@...il.com>,
        Lyra Zhang <zhang.lyra@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH v2 3/3] soc: sprd: Add Spreadtrum special bits
 updating support

On Mon, Apr 13, 2020 at 8:14 AM Baolin Wang <baolin.wang7@...il.com> wrote:
>
> The spreadtrum platform uses a special set/clear method to update
> registers' bits, which can remove the race of updating the global
> registers between the multiple subsystems. Thus we can register
> a physical regmap bus into syscon core to support this.
>
> Signed-off-by: Baolin Wang <baolin.wang7@...il.com>

I'd hope to avoid complicating the syscon driver further for this.
Have you tried to use something other than syscon here to
provide the regmap?

> +#define SPRD_REG_SET_OFFSET    0x1000
> +#define SPRD_REG_CLR_OFFSET    0x2000
> +
> +/*
> + * The Spreadtrum platform defines a special set/clear method to update
> + * registers' bits, which means it can write values to the register's SET
> + * address (offset is 0x1000) to set bits, and write values to the register's
> + * CLEAR address (offset is 0x2000) to clear bits.
> + *
> + * This set/clear method can help to remove the race of accessing the global
> + * registers between the multiple subsystems instead of using hardware
> + * spinlocks.
> + */
> +static int sprd_syscon_update_bits(void *context, unsigned int reg,
> +                                  unsigned int mask, unsigned int val)
> +{
> +       void __iomem *base = context;
> +       unsigned int set, clr;
> +
> +       set = val & mask;
> +       clr = ~set & mask;
> +
> +       if (set)
> +               writel(set, base + reg + SPRD_REG_SET_OFFSET);
> +
> +       if (clr)
> +               writel(clr, base + reg + SPRD_REG_CLR_OFFSET);
> +
> +       return 0;
> +}

Regarding the implementation: Doesn't this introduce a new race
between setting and clearing bits if you do both at the same time?

This may not be a problem if you never do.

> +static int sprd_syscon_init(void)
> +{
> +       syscon_register_phys_regmap_bus(&sprd_syscon_regmap);
> +
> +       return 0;
> +}
> +core_initcall_sync(sprd_syscon_init);

I don't think this part can be done at all: If you load the module on a
generic kernel running on a random other platform, it will break as
there is no check at all to ensure the platform is compatible.

The same thing happens on a platform that may have multiple
syscon nodes, when not all of them use the same register layout.

The only sane way that I can see would be to do it based on
properties of the syscon node itself.

        Arnd