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
