Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Benjamin Bara <bbara93@gmail.com>
To:     peterz@infradead.org
Cc:     bbara93@gmail.com, benjamin.bara@skidata.com,
        dmitry.osipenko@collabora.com, jonathanh@nvidia.com,
        lee@kernel.org, linux-i2c@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-tegra@vger.kernel.org,
        rafael.j.wysocki@intel.com, richard.leitner@linux.dev,
        stable@vger.kernel.org, treding@nvidia.com, wsa@kernel.org
Subject: Re: [PATCH v3 2/4] i2c: core: run atomic i2c xfer when !preemptible
Date:   Tue,  4 Apr 2023 16:06:42 +0200
Message-Id: <20230404140642.550919-1-bbara93@gmail.com>
In-Reply-To: <20230404082255.GU4253@hirez.programming.kicks-ass.net>
References: <20230404082255.GU4253@hirez.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Tue, 4 Apr 2023 at 10:23, Peter Zijlstra <peterz@infradead.org> wrote:
> You can mostly forget about CONFIG_PREEMPT=n (or more specifically
> CONFIG_PREMPT_COUNT=n) things that work for PREEMPT typically also work
> for !PREEMPT.

Thanks for the clarification!

> The question here seems to be if i2c_in_atomic_xfer_mode() should have
> an in_atomic() / !preemptible() check, right? IIUC Wolfram doesn't like
> it being used outside of extra special cicumstances?

Sorry for expressing things more complicated than needed.
Yes, exactly. Wolfram expressed considerations of adding preemptible() as
check, as the now existing irq_disabled() check is lost with !PREEMPT. I tried
to clarify that the check seems to be there for "performance reasons" and that
the check essentially was !preemptible() before v5.2, so nothing should break.

However, what came to my mind during my "research", is that it might make sense
to handle all i2c transfers atomically when in a post RUNNING state (namely
HALT, POWER_OFF, RESTART, SUSPEND). The outcome would basically be the same as
with this patch series applied, but I guess the reasoning behind it would be
simplified: If in a restart handler, the i2c transfer is atomic, independent of
current IRQ or preemption state. Currently, the state depends on from where the
handler is triggered. As you have stated, most of the i2c transfers in the
mentioned states will just update single bits for power-off/sleep/suspend, so
IMHO nothing where not using a DMA would matter from a performance point of
view. But there might be other reasons for not doing so - I guess in this case
the provided patch is fine (at least from my side).