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

[Simon Glass]

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. 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. 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.

Regards,
SImon