Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 26 Jun 2020 14:40:58 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Pawel Laszczak <pawell@cadence.com>
Cc:     gregkh@linuxfoundation.org, robh+dt@kernel.org,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        balbi@kernel.org, devicetree@vger.kernel.org,
        ben.dooks@codethink.co.uk, colin.king@canonical.com, rogerq@ti.com,
        peter.chen@nxp.com, weiyongjun1@huawei.com, jpawar@cadence.com,
        kurahul@cadene.com, sparmar@cadence.com
Subject: Re: [PATCH RFC 2/5] usb:cdns3: Add pci to platform driver wrapper
Message-ID: <20200626114057.GD2571@kadam>
References: <20200626045450.10205-1-pawell@cadence.com>
 <20200626045450.10205-3-pawell@cadence.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200626045450.10205-3-pawell@cadence.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Jun 26, 2020 at 06:54:47AM +0200, Pawel Laszczak wrote:
> +static struct pci_dev *cdnsp_get_second_fun(struct pci_dev *pdev)
> +{
> +	struct pci_dev *func;
> +
> +	/*
> +	 * Gets the second function.
> +	 * It's little tricky, but this platform has two function.
> +	 * The fist keeps resources for Host/Device while the second
> +	 * keeps resources for DRD/OTG.
> +	 */
> +	func = pci_get_device(pdev->vendor, pdev->device, NULL);
> +	if (unlikely(!func))

Delete all likely/unlikely annotations.  Likely and unlikely annotations
make the code less readable.  We are willing to sacrifice readability on
fast paths.

They're only supposed to be used where they're supported by benchmarking.
Probably it's pretty tricky to benchmark probe.  The other rule of thumb
is don't add them to drivers.  Another thing to consider is that this
error path is probably obvious enough for the compiler to figure out
without help.

> +		return NULL;
> +
> +	if (func->devfn == pdev->devfn) {
> +		func = pci_get_device(pdev->vendor, pdev->device, func);
> +		if (unlikely(!func))
> +			return NULL;
> +	}
> +
> +	return func;
> +}
> +
> +static int cdnsp_pci_probe(struct pci_dev *pdev,
> +			   const struct pci_device_id *id)
> +{
> +	struct platform_device_info plat_info;
> +	struct cdnsp_wrap *wrap;
> +	struct resource *res;
> +	struct pci_dev *func;
> +	int err;
> +
> +	/*
> +	 * For GADGET/HOST PCI (devfn) function number is 0,
> +	 * for OTG PCI (devfn) function number is 1.
> +	 */
> +	if (!id || (pdev->devfn != PCI_DEV_FN_HOST_DEVICE &&
> +		    pdev->devfn != PCI_DEV_FN_OTG))
> +		return -EINVAL;
> +
> +	func = cdnsp_get_second_fun(pdev);
> +	if (unlikely(!func))
> +		return -EINVAL;
> +
> +	if (func->class == PCI_CLASS_SERIAL_USB_XHCI ||
> +	    pdev->class == PCI_CLASS_SERIAL_USB_XHCI)
> +		return -EINVAL;


Do we need call pci_put_device(func) before returning?

		ret = -EINVAL;
		goto put_pci;

> +
> +	err = pcim_enable_device(pdev);
> +	if (err) {
> +		dev_err(&pdev->dev, "Enabling PCI device has failed %d\n", err);
> +		return err;

		goto put_pci;

> +	}
> +
> +	pci_set_master(pdev);
> +
> +	if (pci_is_enabled(func)) {
> +		wrap = pci_get_drvdata(func);
> +	} else {
> +		wrap = kzalloc(sizeof(*wrap), GFP_KERNEL);
> +		if (!wrap) {
> +			pci_disable_device(pdev);
> +			return -ENOMEM;

goto disable_pci;

> +		}
> +	}
> +
> +	res = wrap->dev_res;
> +
> +	/* For GADGET device function number is 0. */
> +	if (pdev->devfn == 0) {
> +		/* Function 0: host(BAR_0) + device(BAR_1).*/
> +		dev_dbg(&pdev->dev, "Initialize Device resources\n");
> +		res[RES_DEV_ID].start = pci_resource_start(pdev, PCI_BAR_DEV);
> +		res[RES_DEV_ID].end =   pci_resource_end(pdev, PCI_BAR_DEV);
> +		res[RES_DEV_ID].name = "dev";
> +		res[RES_DEV_ID].flags = IORESOURCE_MEM;
> +		dev_dbg(&pdev->dev, "USBSS-DEV physical base addr: %pa\n",
> +			&res[RES_DEV_ID].start);
> +
> +		res[RES_HOST_ID].start = pci_resource_start(pdev, PCI_BAR_HOST);
> +		res[RES_HOST_ID].end = pci_resource_end(pdev, PCI_BAR_HOST);
> +		res[RES_HOST_ID].name = "xhci";
> +		res[RES_HOST_ID].flags = IORESOURCE_MEM;
> +		dev_dbg(&pdev->dev, "USBSS-XHCI physical base addr: %pa\n",
> +			&res[RES_HOST_ID].start);
> +
> +		/* Interrupt for XHCI, */
> +		wrap->dev_res[RES_IRQ_HOST_ID].start = pdev->irq;
> +		wrap->dev_res[RES_IRQ_HOST_ID].name = "host";
> +		wrap->dev_res[RES_IRQ_HOST_ID].flags = IORESOURCE_IRQ;
> +
> +		/* Interrupt device. It's the same as for HOST. */
> +		wrap->dev_res[RES_IRQ_PERIPHERAL_ID].start = pdev->irq;
> +		wrap->dev_res[RES_IRQ_PERIPHERAL_ID].name = "peripheral";
> +		wrap->dev_res[RES_IRQ_PERIPHERAL_ID].flags = IORESOURCE_IRQ;
> +	} else {
> +		res[RES_DRD_ID].start = pci_resource_start(pdev, PCI_BAR_OTG);
> +		res[RES_DRD_ID].end =   pci_resource_end(pdev, PCI_BAR_OTG);
> +		res[RES_DRD_ID].name = "otg";
> +		res[RES_DRD_ID].flags = IORESOURCE_MEM;
> +		dev_dbg(&pdev->dev, "CDNSP-DRD physical base addr: %pa\n",
> +			&res[RES_DRD_ID].start);
> +
> +		/* Interrupt for OTG/DRD. */
> +		wrap->dev_res[RES_IRQ_OTG_ID].start = pdev->irq;
> +		wrap->dev_res[RES_IRQ_OTG_ID].name = "otg";
> +		wrap->dev_res[RES_IRQ_OTG_ID].flags = IORESOURCE_IRQ;
> +	}
> +
> +	if (pci_is_enabled(func)) {
> +		/* Set up platform device info. */
> +		memset(&plat_info, 0, sizeof(plat_info));
> +		plat_info.parent = &pdev->dev;
> +		plat_info.fwnode = pdev->dev.fwnode;
> +		plat_info.name = PLAT_DRIVER_NAME;
> +		plat_info.id = pdev->devfn;
> +		wrap->devfn  = pdev->devfn;
> +		plat_info.res = wrap->dev_res;
> +		plat_info.num_res = ARRAY_SIZE(wrap->dev_res);
> +		plat_info.dma_mask = pdev->dma_mask;
> +		/* Register platform device. */
> +		wrap->plat_dev = platform_device_register_full(&plat_info);
> +		if (IS_ERR(wrap->plat_dev)) {
> +			pci_disable_device(pdev);
> +			err = PTR_ERR(wrap->plat_dev);
> +			kfree(wrap);

		err = PTR_ERR(wrap->plat_dev);
		goto free_wrap;

Except, do we really want to kfree(wrap)?  It looks like it came from
pci_get_drvdata().

> +			return err;
> +		}
> +	}
> +
> +	pci_set_drvdata(pdev, wrap);
> +	return err;

free_wrap:
	if (!pci_is_enabled(func))
		kfree(wrap);
disable_pci:
	pci_disable_device(pdev);
put_pci:
	pci_put_device(func);

> +}
> +
> +static void cdnsp_pci_remove(struct pci_dev *pdev)
> +{
> +	struct cdnsp_wrap *wrap;
> +	struct pci_dev *func;
> +
> +	func = cdnsp_get_second_fun(pdev);
> +
> +	wrap = (struct cdnsp_wrap *)pci_get_drvdata(pdev);
> +	if (wrap->devfn == pdev->devfn)
> +		platform_device_unregister(wrap->plat_dev);
> +
> +	if (!pci_is_enabled(func))
> +		kfree(wrap);

pci_put_device(func);

> +}
> +

regards,
dan carpenter