linux-kernel - Re: [PATCH 11/15] PCI: pci-epf-test: Use pci_epc_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190212150718.GA28306@e107981-ln.cambridge.arm.com>
Date:   Tue, 12 Feb 2019 15:07:18 +0000
From:   Lorenzo Pieralisi <lorenzo.pieralisi@....com>
To:     Kishon Vijay Abraham I <kishon@...com>
Cc:     Bjorn Helgaas <bhelgaas@...gle.com>,
        Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
        Alan Douglas <adouglas@...ence.com>,
        Shawn Lin <shawn.lin@...k-chips.com>,
        Jingoo Han <jingoohan1@...il.com>,
        Heiko Stuebner <heiko@...ech.de>,
        Cyrille Pitchen <cyrille.pitchen@...e-electrons.com>,
        Jia-Ju Bai <baijiaju1990@...il.com>,
        linux-omap@...r.kernel.org, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-rockchip@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 11/15] PCI: pci-epf-test: Use pci_epc_get_features to get
 EPC features

On Mon, Jan 07, 2019 at 12:11:44PM +0530, Kishon Vijay Abraham I wrote:

[...]

>  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 msix_capable = false;
> +	bool msi_capable = true;
>  
>  	if (WARN_ON_ONCE(!epc))
>  		return -EINVAL;
>  
> -	if (epc->features & EPC_FEATURE_NO_LINKUP_NOTIFIER)
> -		epf_test->linkup_notifier = false;
> -	else
> -		epf_test->linkup_notifier = true;
> -
> -	epf_test->msix_available = epc->features & EPC_FEATURE_MSIX_AVAILABLE;
> +	epc_features = pci_epc_get_features(epc, epf->func_no);

I think it would work out better if struct pci_epc_features was
allocated in the caller (stack) and pci_epc_get_features() take a
pointer parameter to it rather than the callee and the callee would just
have to fill it out, this also removes data in the driver that is not
really useful.

Is there any other reason behind the current design choice ?

Thanks,
Lorenzo

> +	if (!epc_features) {
> +		linkup_notifier = epc_features->linkup_notifier;
> +		msix_capable = epc_features->msix_capable;
> +		msi_capable = epc_features->msi_capable;
> +		test_reg_bar = pci_epc_get_first_free_bar(epc_features);
> +		pci_epf_configure_bar(epf, epc_features);
> +	}
>  
> -	epf_test->test_reg_bar = EPC_FEATURE_GET_BAR(epc->features);
> +	epf_test->test_reg_bar = test_reg_bar;
>  
>  	ret = pci_epc_write_header(epc, epf->func_no, header);
>  	if (ret) {
> @@ -492,13 +509,15 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>  	if (ret)
>  		return ret;
>  
> -	ret = pci_epc_set_msi(epc, epf->func_no, epf->msi_interrupts);
> -	if (ret) {
> -		dev_err(dev, "MSI configuration failed\n");
> -		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 (epf_test->msix_available) {
> +	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");
> @@ -506,7 +525,7 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>  		}
>  	}
>  
> -	if (!epf_test->linkup_notifier)
> +	if (!linkup_notifier)
>  		queue_work(kpcitest_workqueue, &epf_test->cmd_handler.work);
>  
>  	return 0;
> @@ -523,17 +542,6 @@ static int pci_epf_test_probe(struct pci_epf *epf)
>  {
>  	struct pci_epf_test *epf_test;
>  	struct device *dev = &epf->dev;
> -	const struct pci_epf_device_id *match;
> -	struct pci_epf_test_data *data;
> -	enum pci_barno test_reg_bar = BAR_0;
> -	bool linkup_notifier = true;
> -
> -	match = pci_epf_match_device(pci_epf_test_ids, epf);
> -	data = (struct pci_epf_test_data *)match->driver_data;
> -	if (data) {
> -		test_reg_bar = data->test_reg_bar;
> -		linkup_notifier = data->linkup_notifier;
> -	}
>  
>  	epf_test = devm_kzalloc(dev, sizeof(*epf_test), GFP_KERNEL);
>  	if (!epf_test)
> @@ -541,8 +549,6 @@ static int pci_epf_test_probe(struct pci_epf *epf)
>  
>  	epf->header = &test_header;
>  	epf_test->epf = epf;
> -	epf_test->test_reg_bar = test_reg_bar;
> -	epf_test->linkup_notifier = linkup_notifier;
>  
>  	INIT_DELAYED_WORK(&epf_test->cmd_handler, pci_epf_test_cmd_handler);
>  
> -- 
> 2.17.1
>