On Mon, Mar 11, 2024 at 10:54:28PM +0100, Niklas Cassel wrote:

On Mon, Mar 11, 2024 at 08:15:59PM +0530, Manivannan Sadhasivam wrote:

quoted

quoted

I would say that it is the following change that breaks things:

quoted

-	if (!core_init_notifier) {
-		ret = pci_epf_test_core_init(epf);
-		if (ret)
-			return ret;
-	}
-

Since without this code, pci_epf_test_core_init() will no longer be called,
as there is currently no one that calls epf->core_init() for a EPF driver
after it has been bound. (For drivers that call dw_pcie_ep_init_notify() in
.probe())

Thanks a lot for testing, Niklas!

quoted

I guess one way to solve this would be for the EPC core to keep track of
the current EPC "core state" (up/down). If the core is "up" at EPF .bind()
time, notify the EPF driver directly after .bind()?

Yeah, that's a good solution. But I think it would be better if the EPC caches
all events if the EPF drivers are not available and dispatch them once the bind
happens for each EPF driver. Even though INIT_COMPLETE is the only event that is
getting generated before bind() now, IMO it is better to add provision to catch
other events also.

Wdyt?

I'm not sure.
What if the EPF goes up/down/up, it seems a bit silly to send all those
events to the EPF driver that will alloc+free+alloc.

Do we know for sure that we will want to store + replay events other than
INIT_COMPLETE?

And how many events should we store?


Until we can think of a good reason which events other than UP/DOWN we
can to store, I think that just storing the state as an integer in
struct pci_epc seems simpler.

Hmm, makes sense.

Or I guess we could continue with a flag in struct pci_epc_features,
like has_perst_notifier, which would then require the EPC driver to
call both epc_notify_core_up() and epc_notify_core_down() when receiving
the PERST deassert/assert.
For a driver without the flag set, the EPC core would call
.epc_notify_core_up() after bind. (And .epc_notify_core_down() would never
be called, or it could call it before unbind().)
That way an EPF driver itself would not need any different handling
(all callbacks would always come, either triggered by an EPC driver that
has PERST GPIO irq, or triggered by the EPC core for a driver that lacks
a PERST GPIO).

For simplicity, I've just used a flag in 'struct pci_epc' to track the core_init
and call the callback during bind().

But the series has grown big, so I decided to split it into two. One to address
the DBI access issue and also remove the 'core_init_notifier' flag and another
one to make EPF drivers more robust to handle the host reboot scenario.

- Mani

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel