Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1588350008-8143-1-git-send-email-alan.mikhak@sifive.com> <20200507214234.GA5449@bogus>
In-Reply-To: <20200507214234.GA5449@bogus>
From:   Alan Mikhak <alan.mikhak@sifive.com>
Date:   Mon, 11 May 2020 10:26:33 -0700
Message-ID: <CABEDWGygtfK0-dd7VUKH2K1FWy8KweG8o7bRjNFQAXfWHkT8fA@mail.gmail.com>
Subject: Re: [PATCH] PCI: endpoint: functions/pci-epf-test: Enable picking DMA
 channel by name
To:     Rob Herring <robh@kernel.org>
Cc:     linux-pci <linux-pci@vger.kernel.org>,
        linux-kernel@vger.kernel.org,
        Kishon Vijay Abraham I <kishon@ti.com>,
        lorenzo.pieralisi@arm.com, Bjorn Helgaas <bhelgaas@google.com>,
        Ulf Hansson <ulf.hansson@linaro.org>, sebott@linux.ibm.com,
        efremov@linux.com, vidyas@nvidia.com,
        Paul Walmsley <paul.walmsley@sifive.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, May 7, 2020 at 2:42 PM Rob Herring <robh@kernel.org> wrote:
>
> On Fri, May 01, 2020 at 09:20:08AM -0700, Alan Mikhak wrote:
> > From: Alan Mikhak <alan.mikhak@sifive.com>
> >
> > Modify pci_epf_test_init_dma_chan() to call dma_request_channel() with a
> > filter function to pick DMA channel by name, if desired.
> >
> > Add a new filter function pci_epf_test_pick_dma_chan() which takes a name
> > string as an optional parameter. If desired name is specified, the filter
> > function checks the name of each DMA channel candidate against the desired
> > name. If no match, the filter function rejects the candidate channel.
> > Otherwise, the candidate channel is accepted. If optional name parameter
> > is null or an empty string, filter function picks the first DMA channel
> > candidate, thereby preserving the existing behavior of pci-epf-test.
> >
> > Currently, pci-epf-test picks the first suitable DMA channel. Adding a
> > filter function enables a developer to modify the optional parameter
> > during debugging by providing the name of a desired DMA channel. This is
> > useful during debugging because it allows different DMA channels to be
> > exercised.
> >
> > Adding a filter function also takes one step toward modifying pcitest to
> > allow the user to choose a DMA channel by providing a name string at the
> > command line when issuing the -d parameter for DMA transfers.
>
> This mostly looks fine, but needs to be part of a series giving it a
> user.

Thanks Rob for your comments.

I do have other changes in mind that build on this patch to give this
filter function a user other than its current caller that is passing a
NULL pointer as the filter parameter. I can hold it back until then.
However, those changes are more involved and may take longer to implement
and review. In the meantime, maybe this can be accepted independently as
an interim measure to aid debugging during development. If this patch were
to be applied to the codebase, it becomes much simpler to manually edit
the code during development and replace the NULL with a string such as
"dma0chan1" until a command line option becomes available.

This patch is also not necessarily related to my other patch about
supporting slave dma transfers. Even when working on machines that just
have platform dma channels and no slave dma channels, I experience the
same limitation where I cannot exercise any channel other than the one
picked by default.

>
> > Signed-off-by: Alan Mikhak <alan.mikhak@sifive.com>
> > ---
> >  drivers/pci/endpoint/functions/pci-epf-test.c | 24 ++++++++++++++++++++----
> >  1 file changed, 20 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > index 60330f3e3751..043916d3ab5f 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > @@ -149,10 +149,26 @@ static int pci_epf_test_data_transfer(struct pci_epf_test *epf_test,
> >  }
> >
> >  /**
> > - * pci_epf_test_init_dma_chan() - Function to initialize EPF test DMA channel
> > - * @epf_test: the EPF test device that performs data transfer operation
> > + * pci_epf_test_pick_dma_chan() - Filter DMA channel based on desired criteria
> > + * @chan: the DMA channel to examine
> >   *
> > - * Function to initialize EPF test DMA channel.
> > + * Filter DMA channel candidates by matching against an optional desired name.
> > + * Pick first candidate channel if desired name is not specified.
> > + * Reject candidate channel if its name does not match the desired name.
> > + */
> > +static bool pci_epf_test_pick_dma_chan(struct dma_chan *chan, void *name)
> > +{
> > +     if (name && strlen(name) && strcmp(dma_chan_name(chan), name))
>
> Doesn't this cause warning with 'name' being void*?

I compiled this patch for riscv and x86_64 but didn't see any warning. I will
address your concern by posting a v2 patch.

>
>
> > +             return false;
> > +
> > +     return true;
> > +}
> > +
> > +/**
> > + * pci_epf_test_init_dma_chan() - Helper to initialize EPF DMA channel
> > + * @epf: the EPF device that has to perform the data transfer operation
> > + *
> > + * Helper to initialize EPF DMA channel.
> >   */
> >  static int pci_epf_test_init_dma_chan(struct pci_epf_test *epf_test)
> >  {
> > @@ -165,7 +181,7 @@ static int pci_epf_test_init_dma_chan(struct pci_epf_test *epf_test)
> >       dma_cap_zero(mask);
> >       dma_cap_set(DMA_MEMCPY, mask);
> >
> > -     dma_chan = dma_request_chan_by_mask(&mask);
> > +     dma_chan = dma_request_channel(mask, pci_epf_test_pick_dma_chan, NULL);
> >       if (IS_ERR(dma_chan)) {
> >               ret = PTR_ERR(dma_chan);
> >               if (ret != -EPROBE_DEFER)
> > --
> > 2.7.4
> >