[PATCH v1 06/24] pci: pci-uclass: Add multi entry support for memory regions

[Tom Rini]

On Sun, Aug 23, 2020 at 11:41:41AM +0200, Stefan Roese wrote:
> Hi Simon,
> Hi Tom,
> 
> On 22.08.20 17:09, Simon Glass wrote:
> > Hi Stefan,
> > 
> > On Fri, 14 Aug 2020 at 05:40, Stefan Roese <sr at denx.de> wrote:
> > > 
> > > Hi Simon,
> > > 
> > > On 04.08.20 17:05, Simon Glass wrote:
> > > 
> > > <snip>
> > > 
> > > > > > > > > Changes in v1:
> > > > > > > > > - Change patch subject
> > > > > > > > > - Enhance Kconfig help descrition
> > > > > > > > > - Use if() instead of #if
> > > > > > > > > 
> > > > > > > > >      drivers/pci/Kconfig      | 10 ++++++++++
> > > > > > > > >      drivers/pci/pci-uclass.c |  9 ++++++---
> > > > > > > > >      2 files changed, 16 insertions(+), 3 deletions(-)
> > > > > > > > 
> > > > > > > > This needs an update to a sandbox test to handle this behaviour.
> > > > > > > 
> > > > > > > Okay. But how should I handle all these defconfig changes with regard
> > > > > > > to the other patches in this series, introducing multiple new PCI
> > > > > > > related Kconfig options. With 3 new Kconfig options, all permutations
> > > > > > > would lead to 8 (2 ^ 3) different defconfig files. This does not
> > > > > > > scale.
> > > > > > > 
> > > > > > > I might be missing something here though - perhaps this is easier to
> > > > > > > achieve.
> > > > > > 
> > > > > > For sandbox, turn on all options and then add a new PCI bus that uses
> > > > > > this functionality. If there are lots of combinations you could add 8
> > > > > > new buses, but I'm hoping that isn't necessary?
> > > > > 
> > > > > If I turn on all new options, sandbox will run with these new options
> > > > > enabled. I don't know with with implications, as it usually runs with
> > > > > the "normal" PCI related Kconfig options. Also the "normal" PCI
> > > > > defconfig (e.g. CONFIG_PCI_REGION_MULTI_ENTRY etc disabled) will not
> > > > > be tested any more via the sandbox tests. So you get either a test for
> > > > > the new Kconfig option enabled or disabled this way.
> > > > > 
> > > > > Do you really want me to do this?
> > > > 
> > > > So the Kconfig completely changes the implementation of PCI? That
> > > > doesn't make it very testable, as you say.
> > > > 
> > > > Instead, I think the Kconfig should enable the option, then use one of
> > > > three ways to select the option:
> > > > 
> > > > - a device tree property (on sandbox particularly)
> > > > - compatible string (where the property is not appropriate
> > > > - setting a flag in PCI bus (where a driver requires the option be selected)
> > > > 
> > > > That way you can write a test for the new feature in sandbox, without
> > > > deleting all the other tests.
> > > 
> > > Coming back to this issue after some time - sorry for the delay.
> > > 
> > > I'm not sure, if I understand this correctly. Do you suggest that the
> > > driver code (in this case pci-uclass.c) should be extended to support
> > > this (sandbox) testing support?
> > 
> > Not really. I see these things as features that drivers can enable /
> > disable depending on their needs. If that is correct, then sandbox is
> > no different form other drivers, except that perhaps it might support
> > all combinations rather than just one.
> > 
> > > 
> > > If yes, I really think that this is counterproductive. As we added (at
> > > least some of) the Kconfig options explicitly, to not add code to
> > > pci-uclass.c in the "normal case". So adding code to e.g. check a device
> > > tree property or a compatible string would increase the code size again.
> > 
> > How about some flags in struct pci_controller?
> > 
> > > 
> > > If not, I'm still unsure how you would like to test the "normal case",
> > > e.g. with CONFIG_PCI_REGION_MULTI_ENTRY disabled, and with it enabled
> > > without adding more sandbox build targets, with all the Kconfig options
> > > permutations. As the extra code (in pci-uclass) is either included or
> > > not in the sandbox binary.
> > 
> > Kconfig enables and disables the feature by adding the code. But we
> > can still have a flag to determine whether it is used by a particular
> > driver. That way we can keep our test coverage.
> > 
> > > 
> > > But after adding one test for the first of these pci-uclass related
> > > patches, I do have a general comment on this. I find it quite complex
> > > and time consuming to add these tests. Don't get me wrong, I agree in
> > > general, that having tests in U-Boot is very good. But enforcing tests
> > > for each and every new feature addition in drivers (layers) like PCI
> > > seems a bit too much to me. For example new features like the "pci:
> > > pci-uclass: Add support for Single-Root I/O Virtualization" would mean
> > > AFAIU, that I need to write some emulation code for such a PCI device
> > > and also some testing driver matching such a device, since we have no
> > > real hardware like this in sandbox. This would result in much more
> > > complex code for this test & emulation compared to the driver change /
> > > extension.
> > 
> > Yes but it is not that hard. There are a few PCI emulation devices in
> > U-Boot and these form the basis of existing tests. Before this, we
> > really had no tests for PCI and even the behaviour was largely
> > undefined. Your ability to convert the regions[] array to a
> > dynamically allocated array is partly thanks to these tets.
> > 
> > > 
> > > To sum it up, I'm asking if you still think that adding tests for all
> > > those PCI driver extensions is really necessary for upstream acceptance?
> > > What's your opinion on this? Do you understand my position on this?
> > 
> > I don't want to be silly about it, but in general if we add new
> > features, particularly to core features, I think there should be
> > tests.
> 
> As mentioned before, I agree in general. But we should be not too
> strict here IMHO, to enforce new tests for each and every U-Boot
> addition. It's probably not easy to draw the line here to decide,
> when and when not to enforce tests. Perhaps this should reflect the
> complexitiy of the test code and also the user count of the new
> U-Boot code / features (here its solely Octeon TX/TX2).
> 
> > Apart from correctness, they also define the behaviour of the
> > code, in many cases. The test you added in one patch looks good, and
> > doesn't look too complicated.
> 
> Yes, that one was quite simple. But emulating special PCI device
> capabilities to enable such virtual testings will be much more complex,
> AFAICT. If it would be "easy", I would not argue with you on this and
> just implement these tests and be done with it. ;) But frankly, I have
> no real idea (without digging much deeper into this) on how to add these
> emulation codes and tests in a way, that they completely test all the
> feature additions. Please note that I'm not the original author of these
> additions.
> 
> > Of course it is more than zero work. But
> > so much of the refactoring we do in U-Boot these days would be much
> > harder and error-prone without these tests.
> > 
> > +Tom Rini who might want to weigh in.
> 
> Tom, are you okay with going forward with these PCIe related patches,
> without adding test cases for all PCI feature additions? Which would
> mean to add very special PCI device emulation and test drivers for
> such devices? The PCI extensions are not that intrusive and AFAICT
> Simon is okay with all of them in general, only would like to have tests
> for all driver extensions.

For complex hardware specific things like this and testing I think we'll
have the most luck by, when possible and I'm not sure we do enough
today, enable features and tests on qemu* machines and get it that way,
rather than writing new emulators for sandbox.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200823/47dc3f9a/attachment.sig>