Received-SPF: pass (google.com: domain of linux-kernel+bounces-99580-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 11 Mar 2024 22:54:28 +0100
From: Niklas Cassel <cassel@kernel.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: Jingoo Han <jingoohan1@gmail.com>,
	Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
	Lorenzo Pieralisi <lpieralisi@kernel.org>,
	Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
	Rob Herring <robh@kernel.org>, Bjorn Helgaas <bhelgaas@google.com>,
	Marek Vasut <marek.vasut+renesas@gmail.com>,
	Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>,
	Thierry Reding <thierry.reding@gmail.com>,
	Jonathan Hunter <jonathanh@nvidia.com>,
	Kishon Vijay Abraham I <kishon@ti.com>,
	Vidya Sagar <vidyas@nvidia.com>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Richard Zhu <hongxing.zhu@nxp.com>,
	Lucas Stach <l.stach@pengutronix.de>,
	Shawn Guo <shawnguo@kernel.org>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	Fabio Estevam <festevam@gmail.com>,
	NXP Linux Team <linux-imx@nxp.com>,
	Minghuan Lian <minghuan.Lian@nxp.com>,
	Mingkai Hu <mingkai.hu@nxp.com>, Roy Zang <roy.zang@nxp.com>,
	Kunihiko Hayashi <hayashi.kunihiko@socionext.com>,
	Masami Hiramatsu <mhiramat@kernel.org>,
	Kishon Vijay Abraham I <kishon@kernel.org>,
	Jesper Nilsson <jesper.nilsson@axis.com>,
	Srikanth Thokala <srikanth.thokala@intel.com>,
	linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-renesas-soc@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-tegra@vger.kernel.org, linux-omap@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, linuxppc-dev@lists.ozlabs.org,
	linux-arm-kernel@axis.com, Frank Li <Frank.Li@nxp.com>
Subject: Re: [PATCH v9 07/10] PCI: dwc: ep: Remove "core_init_notifier" flag
Message-ID: <Ze99lLhe2GqIqMgl@ryzen>
References: <20240304-pci-dbi-rework-v9-0-29d433d99cda@linaro.org>
 <20240304-pci-dbi-rework-v9-7-29d433d99cda@linaro.org>
 <ZesRk5Dg4KEASD3U@ryzen>
 <20240311144559.GA2504@thinkpad>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240311144559.GA2504@thinkpad>

On Mon, Mar 11, 2024 at 08:15:59PM +0530, Manivannan Sadhasivam wrote:
> > 
> > I would say that it is the following change that breaks things:
> > 
> > > -	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!
> 
> > 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.


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


Kind regards,
Niklas