Re: [PATCH] selftests: pci: pci-selftest: add support for PCI endpoint driver test

['Manivannan Sadhasivam' <manivannan.sadhasivam@linaro.org>]

On Tue, Dec 27, 2022 at 10:45:26AM +0530, Aman Gupta/FDS SW /SSIR/Engineer/Samsung Electronics wrote:
> 
> 
> > -----Original Message-----
> > From: Shuah Khan [mailto:skhan@linuxfoundation.org]
> > Sent: 23 December 2022 22:01
> > To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Cc: Aman Gupta <aman1.gupta@samsung.com>; shradha.t@samsung.com;
> > pankaj.dubey@samsung.com; kishon@ti.com; lpieralisi@kernel.org;
> > kw@linux.com; shuah@kernel.org; Bjorn Helgaas <helgaas@kernel.org>;
> > linux-pci@vger.kernel.org; linux-kselftest@vger.kernel.org; Padmanabhan
> > Rajanbabu <p.rajanbabu@samsung.com>; Shuah Khan
> > <skhan@linuxfoundation.org>
> > Subject: Re: [PATCH] selftests: pci: pci-selftest: add support for PCI endpoint
> > driver test
> > 
> > On 12/23/22 08:02, Manivannan Sadhasivam wrote:
> > > On Thu, Dec 22, 2022 at 10:49:48AM -0700, Shuah Khan wrote:
> > >> On 12/22/22 10:45, Manivannan Sadhasivam wrote:
> > >>> On Thu, Dec 22, 2022 at 09:58:30AM -0700, Shuah Khan wrote:
> > >>>> On 10/6/22 23:39, Aman Gupta wrote:
> > >>>>> This patch enables the support to perform selftest on PCIe
> > >>>>> endpoint driver present in the system. The following tests are
> > >>>>> currently performed by the selftest utility
> > >>>>>
> > >>>>> 1. BAR Tests (BAR0 to BAR5)
> > >>>>> 2. MSI Interrupt Tests (MSI1 to MSI32) 3. Read Tests (For 1, 1024,
> > >>>>> 1025, 1024000, 1024001 Bytes) 4. Write Tests (For 1, 1024, 1025,
> > >>>>> 1024000, 1024001 Bytes) 5. Copy Tests (For 1, 1024, 1025, 1024000,
> > >>>>> 1024001 Bytes)
> > >>>>>
> > >>>>> Signed-off-by: Aman Gupta <aman1.gupta@samsung.com>
> > >>>>> Signed-off-by: Padmanabhan Rajanbabu
> > <p.rajanbabu@samsung.com>
> > >>>>
> > >>>> Adding Bjorn Helgaas to the thread.
> > >>>>
> > >>>> Adding pcit test under selftests is good. There is another pcitest
> > >>>> under tools/pci. I would like to see if the existing code in
> > >>>> tools/pci/pcitest.c can be leveraged.
> > >>>>
> > >>>> As part of this test work, also look into removing tools/pci so we
> > >>>> don't have to maintain duplicate code in two places.
> > >>>>
> > >>>
> > >>> It has been agreed in a thread with Greg [1] to {re}move the tests
> > >>> under tools/pci and utilize the kselftest.
> > >>>
> > >>
> > >> Inline with what I am suggesting. However, I don't see either move or
> > >> delete of tools/pci in the patch?
> > >>
> > >> The first patch could start with git mv of the existing files and
> > >> then make changes to preserver the history.
> > >>
> > >
> > > Right. This patch was posted independently of the series [1] that I
> > > submitted to fix the return values of IOCTL calls used in
> > > drivers/misc/pci_endpoint_test.c driver.
> > >
> > > Then in that series, it was decided to move the existing test to
> > > kselftest. So, I suggested Aman Gupta [2] to integrate my latest
> > > patches, add the kselftest patch on top, then remove the existing test
> > under tools/pci.
> > >
> > > The kselftest patch can also move the driver first and then make the
> > > change as you suggested. Either way it is fine by me.
> > >
> > 
> > As I mentioned in my previous email, I prefer to see the move as the first
> > patch and then changes on top. This preserves the history and cleaner.
> > 
> > thanks,
> > -- Shuah
> > 
> 
> Hi Shuah,
> 
> Thanks for review and suggestion. I understand that we would like to reuse and preserve the history of tools/pci/pcietest.c. So we have two approaches:
> 
> 1: Using git mv command move existing code from tools/pci/ to tools/testing/selftest/drivers/pci/ and then update the file to convert to kselftest framework. I thought about this but after movement, when we move it to kselftest format it is going to be huge churn and we will be having modification in almost all lines.
> 
> 2: Develop kselftest based driver in tools/testing/selftest/drivers/pci/ and eventually delete existing file from tools/pci/ folder providing justification in commit message.
> 
> From my viewpoint, going with the second approach makes more sense because if almost complete file is getting modified, and it will make the review process complex and anyways there is not much code reusability.  
> Please let me know if you have any other thought process or if I am missing anything to understand your approach.
> 

As Bjorn and Shuah said, I presume you are working on option 1.

Thanks,
Mani

> Thanks,
> Aman Gupta
> > 
> 
> 

-- 
மணிவண்ணன் சதாசிவம்