Received-SPF: pass (google.com: domain of linux-kernel+bounces-47039-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240130124828.14678-1-brgl@bgdev.pl> <20240130124828.14678-11-brgl@bgdev.pl>
In-Reply-To: <20240130124828.14678-11-brgl@bgdev.pl>
From: Linus Walleij <linus.walleij@linaro.org>
Date: Wed, 31 Jan 2024 21:01:25 +0100
Message-ID: <CACRpkdZyaqwbxvsLtXPHSX=6jyFPYSxA9n+qWakdhGKmo+L9fw@mail.gmail.com>
Subject: Re: [PATCH 10/22] gpio: reinforce desc->flags handling
To: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Kent Gibson <warthog618@gmail.com>, Alex Elder <elder@linaro.org>, 
	Geert Uytterhoeven <geert+renesas@glider.be>, "Paul E . McKenney" <paulmck@kernel.org>, 
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Wolfram Sang <wsa@the-dreams.de>, 
	linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Jan 30, 2024 at 1:48=E2=80=AFPM Bartosz Golaszewski <brgl@bgdev.pl>=
 wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> We now removed the gpio_lock spinlock and modified the places
> previously protected by it to handle desc->flags access in a consistent
> way. Let's improve other places that were previously unprotected by
> reading the flags field of gpio_desc once and using the stored value for
> logic consistency. If we need to modify the field, let's also write it
> back once with a consistent value resulting from the function's logic.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
(...)

I have a trouble with this one:

gpiochip_find_base_unlocked()
> +       unsigned long flags;
(...)
> +       flags =3D READ_ONCE(desc->flags);
(...)
> +       if (test_bit(FLAG_OPEN_DRAIN, &flags) &&
> +           test_bit(FLAG_IS_OUT, &flags))
>                 return 0;
(...)
> +       assign_bit(FLAG_IS_OUT, &flags, !ret);
> +       WRITE_ONCE(desc->flags, flags);

I unerstand the atomicity of each operation here, but ... if what you want
to protect is modifications from other CPUs, how do we know that another
CPU isn't coming in and reading and modifying and assigning
another flag inbetween these operations while the value is only
stored in the CPU-local flags variable?

Same with gpiod_direction_output().

To me it seems like maybe you need to actually protect the desc->flags
with the SRCU struct in these cases? (and not only use it for the
label protection then).

An alternative is maybe to rewrite the code with test_and_set().

But as you say it is currently unprotected, I just wonder if this really
adds any protection.

Yours,
Linus Walleij