From mboxrd@z Thu Jan 1 00:00:00 1970 From: Arnd Bergmann Subject: Re: [RFC PATCH 10/12] soc: qcom: ipa: data path Date: Thu, 15 Nov 2018 06:48:36 -0800 Message-ID: References: <20181107003250.5832-1-elder@linaro.org> <20181107003250.5832-11-elder@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Alex Elder Cc: David Miller , Bjorn Andersson , Ilias Apalodimas , Networking , DTML , linux-arm-msm@vger.kernel.org, linux-soc@vger.kernel.org, Linux ARM , Linux Kernel Mailing List , syadagir@codeaurora.org, mjavid@codeaurora.org, Rob Herring , Mark Rutland List-Id: linux-arm-msm@vger.kernel.org On Wed, Nov 14, 2018 at 7:31 PM Alex Elder wrote: > > On 11/7/18 8:55 AM, Arnd Bergmann wrote: > > On Wed, Nov 7, 2018 at 1:33 AM Alex Elder wrote: > >> > >> This patch contains "ipa_dp.c", which includes the bulk of the data > >> path code. There is an overview in the code of how things operate, > >> but there are already plans to rework this portion of the driver. > >> > >> In particular: > >> - Interrupt handling will be replaced with a threaded interrupt > >> handler. Currently handling occurs in a combination of > >> interrupt and workqueue context, and this requires locking > >> and atomic operations for proper synchronization. > > > > You probably don't want to use just a threaded IRQ handler to > > start the poll function, that would still require an extra indirection. > > That's a really good point. However I think that the path I'll > take to *getting* to scheduling the poll in interrupt context > will use a threaded interrupt handler. I'm hoping that will > allow me to simplify the code in steps. > > The main reason for this split between working in interrupt > context when possible, but pushing to a workqueue when not, is > to allow IPA clock(s) to be turned off. Enabling the clocks > is a blocking operation, so can't' be done in the top half > interrupt handler. The thought was it would be best to work > in interrupt context--if the clock was already active--but > to defer to a workqueue to turn the clock on if necessary. > > The result requires locking and duplication of code that I > find to be pretty confusing--and hard to reason about. I > have been planning to re-do things to be better suited to > NAPI, and knowing that, I haven't given the data path as > much attention as some of the rest. Right, that sounds like a good plan: start making it use a threaded IRQ handler first to clean up the code, and then think about optimizing the NAPI wakeup once that works reliably. I think what you can do here eventually is to have a combined threaded/non-threaded IRQ handler, where the threaded handler can do everything it needs to do, and the non-threaded handler does only one thing: if all conditions are met for entering the NAPI handler (waiting for rx/tx IRQ, clocks enabled, ...) we call napi_schedule(), otherwise defer to the threaded handler. Arnd From mboxrd@z Thu Jan 1 00:00:00 1970 From: arnd@arndb.de (Arnd Bergmann) Date: Thu, 15 Nov 2018 06:48:36 -0800 Subject: [RFC PATCH 10/12] soc: qcom: ipa: data path In-Reply-To: References: <20181107003250.5832-1-elder@linaro.org> <20181107003250.5832-11-elder@linaro.org> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wed, Nov 14, 2018 at 7:31 PM Alex Elder wrote: > > On 11/7/18 8:55 AM, Arnd Bergmann wrote: > > On Wed, Nov 7, 2018 at 1:33 AM Alex Elder wrote: > >> > >> This patch contains "ipa_dp.c", which includes the bulk of the data > >> path code. There is an overview in the code of how things operate, > >> but there are already plans to rework this portion of the driver. > >> > >> In particular: > >> - Interrupt handling will be replaced with a threaded interrupt > >> handler. Currently handling occurs in a combination of > >> interrupt and workqueue context, and this requires locking > >> and atomic operations for proper synchronization. > > > > You probably don't want to use just a threaded IRQ handler to > > start the poll function, that would still require an extra indirection. > > That's a really good point. However I think that the path I'll > take to *getting* to scheduling the poll in interrupt context > will use a threaded interrupt handler. I'm hoping that will > allow me to simplify the code in steps. > > The main reason for this split between working in interrupt > context when possible, but pushing to a workqueue when not, is > to allow IPA clock(s) to be turned off. Enabling the clocks > is a blocking operation, so can't' be done in the top half > interrupt handler. The thought was it would be best to work > in interrupt context--if the clock was already active--but > to defer to a workqueue to turn the clock on if necessary. > > The result requires locking and duplication of code that I > find to be pretty confusing--and hard to reason about. I > have been planning to re-do things to be better suited to > NAPI, and knowing that, I haven't given the data path as > much attention as some of the rest. Right, that sounds like a good plan: start making it use a threaded IRQ handler first to clean up the code, and then think about optimizing the NAPI wakeup once that works reliably. I think what you can do here eventually is to have a combined threaded/non-threaded IRQ handler, where the threaded handler can do everything it needs to do, and the non-threaded handler does only one thing: if all conditions are met for entering the NAPI handler (waiting for rx/tx IRQ, clocks enabled, ...) we call napi_schedule(), otherwise defer to the threaded handler. Arnd