Re: [PATCH 4/4] PCI: pci-epf-test: Add support to defer core initialization

[Vidya Sagar]

On 11/27/2019 2:50 PM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On 13/11/19 2:38 PM, Vidya Sagar wrote:
> > Add support to defer core initialization and to receive a notifier
> > when core is ready to accommodate platforms where core is not for
> > initialization untile reference clock from host is available.
> >
> > Signed-off-by: Vidya Sagar <vidyas@xxxxxxxxxx>
> > ---
> >   drivers/pci/endpoint/functions/pci-epf-test.c | 114 ++++++++++++------
> >   1 file changed, 77 insertions(+), 37 deletions(-)
> >
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > index bddff15052cc..068024fab544 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > @@ -360,18 +360,6 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
> >   			   msecs_to_jiffies(1));
> >   }
> >   
> > -static int pci_epf_test_notifier(struct notifier_block *nb, unsigned long val,
> > -				 void *data)
> > -{
> > -	struct pci_epf *epf = container_of(nb, struct pci_epf, nb);
> > -	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> > -
> > -	queue_delayed_work(kpcitest_workqueue, &epf_test->cmd_handler,
> > -			   msecs_to_jiffies(1));
> > -
> > -	return NOTIFY_OK;
> > -}
> > -
> >   static void pci_epf_test_unbind(struct pci_epf *epf)
> >   {
> >   	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> > @@ -428,6 +416,78 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
> >   	return 0;
> >   }
> >   
> > +static int pci_epf_test_core_init(struct pci_epf *epf)
> > +{
> > +	struct pci_epf_header *header = epf->header;
> > +	const struct pci_epc_features *epc_features;
> > +	struct pci_epc *epc = epf->epc;
> > +	struct device *dev = &epf->dev;
> > +	bool msix_capable = false;
> > +	bool msi_capable = true;
> > +	int ret;
> > +
> > +	epc_features = pci_epc_get_features(epc, epf->func_no);
> > +	if (epc_features) {
> > +		msix_capable = epc_features->msix_capable;
> > +		msi_capable = epc_features->msi_capable;
> > +	}
> > +
> > +	ret = pci_epc_write_header(epc, epf->func_no, header);
> > +	if (ret) {
> > +		dev_err(dev, "Configuration header write failed\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = pci_epf_test_set_bar(epf);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (msi_capable) {
> > +		ret = pci_epc_set_msi(epc, epf->func_no, epf->msi_interrupts);
> > +		if (ret) {
> > +			dev_err(dev, "MSI configuration failed\n");
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	if (msix_capable) {
> > +		ret = pci_epc_set_msix(epc, epf->func_no, epf->msix_interrupts);
> > +		if (ret) {
> > +			dev_err(dev, "MSI-X configuration failed\n");
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int pci_epf_test_notifier(struct notifier_block *nb, unsigned long val,
> > +				 void *data)
> > +{
> > +	struct pci_epf *epf = container_of(nb, struct pci_epf, nb);
> > +	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> > +	int ret;
> > +
> > +	switch (val) {
> > +	case CORE_INIT:
> > +		ret = pci_epf_test_core_init(epf);
> > +		if (ret)
> > +			return NOTIFY_BAD;
> > +		break;
> > +
> > +	case LINK_UP:
> > +		queue_delayed_work(kpcitest_workqueue, &epf_test->cmd_handler,
> > +				   msecs_to_jiffies(1));
> > +		break;
> > +
> > +	default:
> > +		dev_err(&epf->dev, "Invalid EPF test notifier event\n");
> > +		return NOTIFY_BAD;
> > +	}
> > +
> > +	return NOTIFY_OK;
> > +}
> > +
> >   static int pci_epf_test_alloc_space(struct pci_epf *epf)
> >   {
> >   	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> > @@ -496,12 +556,11 @@ static int pci_epf_test_bind(struct pci_epf *epf)
> >   {
> >   	int ret;
> >   	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> > -	struct pci_epf_header *header = epf->header;
> >   	const struct pci_epc_features *epc_features;
> >   	enum pci_barno test_reg_bar = BAR_0;
> >   	struct pci_epc *epc = epf->epc;
> > -	struct device *dev = &epf->dev;
> >   	bool linkup_notifier = false;
> > +	bool skip_core_init = false;
> >   	bool msix_capable = false;
> >   	bool msi_capable = true;
> >   
> > @@ -511,6 +570,7 @@ static int pci_epf_test_bind(struct pci_epf *epf)
> >   	epc_features = pci_epc_get_features(epc, epf->func_no);
> >   	if (epc_features) {
> >   		linkup_notifier = epc_features->linkup_notifier;
> > +		skip_core_init = epc_features->skip_core_init;
> >   		msix_capable = epc_features->msix_capable;
> >   		msi_capable = epc_features->msi_capable;
>
> Are these used anywhere in this function?
Nope. I'll remove them.

> >   		test_reg_bar = pci_epc_get_first_free_bar(epc_features);
> > @@ -520,34 +580,14 @@ static int pci_epf_test_bind(struct pci_epf *epf)
> >   	epf_test->test_reg_bar = test_reg_bar;
> >   	epf_test->epc_features = epc_features;
> >   
> > -	ret = pci_epc_write_header(epc, epf->func_no, header);
> > -	if (ret) {
> > -		dev_err(dev, "Configuration header write failed\n");
> > -		return ret;
> > -	}
> > -
> >   	ret = pci_epf_test_alloc_space(epf);
> >   	if (ret)
> >   		return ret;
> >   
> > -	ret = pci_epf_test_set_bar(epf);
> > -	if (ret)
> > -		return ret;
> > -
> > -	if (msi_capable) {
> > -		ret = pci_epc_set_msi(epc, epf->func_no, epf->msi_interrupts);
> > -		if (ret) {
> > -			dev_err(dev, "MSI configuration failed\n");
> > -			return ret;
> > -		}
> > -	}
> > -
> > -	if (msix_capable) {
> > -		ret = pci_epc_set_msix(epc, epf->func_no, epf->msix_interrupts);
> > -		if (ret) {
> > -			dev_err(dev, "MSI-X configuration failed\n");
> > +	if (!skip_core_init) {
> > +		ret = pci_epf_test_core_init(epf);
> > +		if (ret)
> >   			return ret;
> > -		}
> >   	}
> >   
> >   	if (linkup_notifier) {
>
> This could as well be moved to pci_epf_test_core_init().
Yes, but I would like to keep only the code that touches hardware in pci_epf_test_core_init()
to minimize the time it takes to execute it. Is there any strong reason to move it? if not,
I would prefer to leave it here in this function itself.

> Thanks
> Kishon