Re: [PATCH v3 4/6] PCI: endpoint: pci-epf-test: Add doorbell test support

[Niklas Cassel <cassel@kernel.org>]

On Fri, Oct 18, 2024 at 11:13:34AM -0400, Frank Li wrote:
> On Fri, Oct 18, 2024 at 04:01:05PM +0200, Niklas Cassel wrote:
> > > +	/* Only revid >=1 support RC-to-EP Door bell */
> > > +	ret = epf->header->revid > 0 ?  pci_epf_alloc_doorbell(epf, 1) : -EINVAL;
> >
> > I really, really don't like this idea.
> >
> > This means that you would need to write a revid > 1 in configfs to test this.
> > I also don't think that it is right that pci-epf-test takes ownership of "rev".
> >
> > How about something like this instead:
> >
> > My thinking is that you add a doorbell_capable struct member to epc_features,
> > and then populate CAPS_DOORBELL_SUPPORT based on epc_features in
> > pci_epf_test_init_caps() (similar to how my proposal sets CAPS_MSI_SUPPORT).
> 
> The primary issue is that the doorbell is not a capability of the EPC
> itself; rather, it's a capability of the entire system that requires an
> external MSI/ITS controller. The CAPS_DOORBELL_SUPPORT should handle this
> feature. Even we needn't CAPS_DOORBELL_SUPPORT, just call
> pci_epf_alloc_doorbell(), if error return, means not support DOORBELL.

Well, the idea is that CAPS_DOORBELL_SUPPORT bit is to tell the host side
driver (pci-endpoint-test.c) that the EPF supports doorbell.

In other words, if pcitest -B is executed, but the EP (pci-epf-test) does
not set the CAPS_DOORBELL_SUPPORT bit to one in the CAPS register,
the host side driver (pci-endpoint-test.c) can error out immediately,
no need to even trying to send any command to the EP.

(We can do the same with MSI and MSI-X, no need to send a command to the EP
if the EP has CAPS (CAPS_MAGIC is set), but does not indicate support for
MSI/MSI-X.)


> To use the doorbell functionality, the revid can clearly inform users that
> this feature breaks previous compatibility. Users will need to update the
> host-side driver, PID/VID values, and the pcitest tools accordingly.

I still really don't like the revid idea.


> One potential problem is that if the EPC supports CAPS_DOORBELL_SUPPORT,
> but the user continues to use older PID/VID values to enable EPF testing,
> the pcitest tool may treat the doorbell BAR as a normal BAR. This could
> lead to confusion for users as to why their system breaks after a kernel
> update.

How about we add a new pcitest --set-doorbell-mode option
(that is similar to pcitest -i which sets the interrupt mode to use).

That way, we can do:
./pcitest --set-doorbell-mode 1
(This will enable doorbell for e.g. BAR0, pci-epf-test will call
pci_epf_alloc_doorbell() when receiving this command from the RC side.
The command will return failure if pci_epf_alloc_doorbell() returned failure.)

./pcitest -B
(This will perform the doorbell test)

./pcitest --set-doorbell-mode 0
(This will disable the doorbell for BAR0,
so it will again not trigger IRQs when BAR0 is written,
and pcitest's tests to read/write the BARs will again behave as expected.)

(We probably also need another option pcitest --get-doorbell-mode.)

I think this should solve all your concerns. Thoughts?


Kind regards,
Niklas