Message-ID: <20230124234744.GA1062727@bhelgaas>
Date:   Tue, 24 Jan 2023 17:47:44 -0600
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Serge Semin <fancer.lancer@...il.com>
Cc:     Serge Semin <Sergey.Semin@...kalelectronics.ru>,
        Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
        Vinod Koul <vkoul@...nel.org>, Rob Herring <robh@...nel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Cai Huoqing <cai.huoqing@...ux.dev>,
        Robin Murphy <robin.murphy@....com>,
        Jingoo Han <jingoohan1@...il.com>, Frank Li <Frank.Li@....com>,
        Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
        Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
        Pavel Parkhomenko <Pavel.Parkhomenko@...kalelectronics.ru>,
        Krzysztof Wilczyński <kw@...ux.com>,
        caihuoqing <caihuoqing@...du.com>,
        Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
        linux-pci@...r.kernel.org, dmaengine@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v9 24/27] dmaengine: dw-edma: Relax driver config settings

On Tue, Jan 24, 2023 at 05:49:41PM +0300, Serge Semin wrote:
> On Mon, Jan 23, 2023 at 10:43:39AM -0600, Bjorn Helgaas wrote:
> > On Sun, Jan 22, 2023 at 03:11:16AM +0300, Serge Semin wrote:
> > > On Fri, Jan 20, 2023 at 04:50:36PM -0600, Bjorn Helgaas wrote:
> > > > On Fri, Jan 13, 2023 at 08:14:06PM +0300, Serge Semin wrote:
> > > > > Since the DW PCIe RP/EP driver is about to be updated to register the DW
> > > > > eDMA-based DMA-engine the drivers build modes must be synchronized.
> > > > > Currently the DW PCIe RP/EP driver is always built as a builtin module.
> > > > > Meanwhile the DW eDMA driver can be built as a loadable module. Thus in
> > > > > the later case the kernel with DW PCIe controllers support will fail to be
> > > > > linked due to lacking the DW eDMA probe/remove symbols. At the same time
> > > > > forcibly selecting the DW eDMA driver from the DW PCIe RP/EP kconfig will
> > > > > effectively eliminate the tristate type of the former driver fixing it to
> > > > > just the builtin kernel module.
> > > > > 
> > > > > Seeing the DW eDMA engine isn't that often met built into the DW PCIe
> > > > > Root-ports and End-points let's convert the DW eDMA driver config to being
> > > > > more flexible instead of just forcibly selecting the DW eDMA kconfig. In
> > > > > order to do that first the DW eDMA PCIe driver config should be converted
> > > > > to being depended from the DW eDMA core config instead of selecting the
> > > > > one. Second the DW eDMA probe and remove symbols should be referenced only
> > > > > if they are reachable by the caller. Thus the user will be able to build
> > > > > the DW eDMA core driver with any type, meanwhile the dependent code will
> > > > > be either restricted to the same build type (e.g. DW eDMA PCIe driver if
> > > > > DW eDMA driver is built as a loadable module) or just won't be able to use
> > > > > the eDMA engine registration/de-registration functionality (e.g. DW PCIe
> > > > > RP/EP driver if DW eDMA driver is built as a loadable module).
> > > > 
> > > > I'm trying to write the merge commit log, and I understand the linking
> > > > issue, but I'm having a hard time figuring out what the user-visible
> > > > scenarios are here.
> > > > 
> > > > I assume there's something that works when CONFIG_PCIE_DW=y and
> > > > CONFIG_DW_EDMA_PCIE=y but does *not* work when CONFIG_PCIE_DW=y and
> > > > CONFIG_DW_EDMA_PCIE=m?
> > > 
> > > No. The DW eDMA code availability (in other words the CONFIG_DW_EDMA
> > > config value) determines whether the corresponding driver (DW PCIe
> > > RP/EP or DW eDMA PCI) is capable to perform the eDMA engine probe
> > > procedure. Additionally both drivers has the opposite dependency from
> > > the DW eDMA code.
> > > |                |     DW PCIe RP/EP    |     DW eDMA PCIe     |
> > > | CONFIG_DW_EDMA +----------------------+----------------------+
> > > |                | Probe eDMA | KConfig | Probe eDMA | Kconfig |
> > > +----------------+------------+---------+------------+---------+
> > > |        y       |     YES    |   y,n   |     YES    |  y,m,n  |
> > > |        m       |     NO     |   y,n   |     YES    |    m,n  |
> > > |        n       |     NO     |   y,n   |     NO     |      n  |
> > > +--------------------------------------------------------------+
> > > 
> > > Basically it means the DW PCIe RP/EP driver will be able to probe the
> > > DW eDMA engine only if the corresponding driver is built into the
> > > kernel. At the same time the DW PCIe RP/EP driver doesn't depend on
> > > the DW eDMA core module config state. The DW eDMA PCIe driver in
> > > opposite depends on the DW eDMA code config state, but will always be
> > > able to probe the DW eDMA engine as long as the corresponding code is
> > > loaded as either a part of the kernel or as a loadable module.
> > > 
> > > > If both scenarios worked the same, I would think the existing
> > > > dw_edma_pcie_probe() would be enough, and you wouldn't need to call
> > > > dw_pcie_edma_detect() from dw_pcie_host_init() and dw_pcie_ep_init().
> > > 
> > > No. These methods have been implemented for the absolutely different
> > > drivers.
> > > dw_edma_pcie_probe() is called for an end-point PCIe-device found on a
> > > PCIe-bus.
> > > dw_pcie_host_init()/dw_pcie_ep_init() and dw_pcie_edma_detect() are
> > > called for a platform-device representing a DW PCIe RP/EP controller.
> > > In other words dw_pcie_edma_detect() and dw_edma_pcie_probe() are in
> > > no means interchangeable.
> > 
> > The question is what the user-visible difference between
> > CONFIG_DW_EDMA_PCIE=y and CONFIG_DW_EDMA_PCIE=m is. 
> 
> There will be no difference between them after this commit is applied
> from the DW eDMA core driver point of view. CONFIG_DW_EDMA_PCIE now
> depends on the CONFIG_DW_EDMA config state (see it's surrounded by
> if/endif in the Kconfig file). Without this patch the
> CONFIG_DW_EDMA_PCIE config determines the CONFIG_DW_EDMA config state
> by forcibly selecting the one. Using the similar approach for the
> CONFIG_PCIE_DW driver I found less attractive because it would have
> effectively converted the CONFIG_DW_EDMA config tristate to boolean.
> 
> That's why instead I decided to convert the CONFIG_DW_EDMA config to
> being independent from any other config value. (See the table in the
> my previous email message.)
> 
> > If there were no
> > difference, dw_pcie_host_init() would not need to call
> > dw_pcie_edma_detect().
> 
> Even if CONFIG_DW_EDMA (not CONFIG_DW_EDMA_PCIE) is set to m or n I
> would have still recommended to call dw_pcie_edma_detect() because the
> method performs the DW eDMA engine auto-detection independently from the DW
> eDMA driver availability. As a result the system log will have a
> number of eDMA detected channels if the engine was really found. It's
> up to the system administrator to make sure that the eDMA driver is
> properly built/loaded then for the engine to be actually available in
> the kernel/system.
>
> > Can you give me a one- or two-sentence merge commit comment that
> > explains why we want to merge this?  "Relax driver config settings"
> > doesn't tell us that.
> 
> "Convert the DW eDMA kconfig to being independently selected by the
> user in order to preserve the module build options flexibility and fix
> the "undefined reference to" error on DW PCIe driver build."

In the commit log, I think "forcibly selecting the DW eDMA driver from
the DW PCIe RP/EP kconfig" actually refers to just the "DW eDMA PCIe"
driver" not the "DW PCIe RP/EP driver," right?

The undefined reference to dw_edma_probe() doesn't actually happen
unless we merge 27/27 without *this* patch, right?  If so, I wouldn't
call this a "fix" because nobody has ever seen the link failure.

OK.  I think this would be much simpler if it were split into two
patches:

  1) Prepare dw_edma_probe() for builtin callers

     When CONFIG_DW_EDMA=m, dw_edma_probe() is built as a module.
     Previously edma.h declared it as extern, which meant that
     builtin callers like dw_pcie_host_init() and dw_pcie_ep_init()
     caused link errors.

     Make it safe for builtin callers to call dw_edma_probe() by using
     IS_REACHABLE() to define a stub when CONFIG_DW_EDMA=m.

     Builtin callers will fail to detect and register eDMA devices
     when CONFIG_DW_EDMA=m but will otherwise work as before.

  2) Make DW_EDMA_PCIE depend on DW_EDMA

     This seems like a good idea and is much nicer than "select
     DW_EDMA", but I think it should be a separate patch since it
     really only relates to dw-edma-pcie.c.  I would use "depends on
     DW_EDMA" instead of adding if/endif around DW_EDMA_PCIE.

What do you think?  Am I still missing something?

Bjorn

Powered by blists - more mailing lists