RE: [PATCH RFC 2/5] usb:cdns3: Add pci to platform driver wrapper

[Pawel Laszczak <pawell@cadence.com>]

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

Ok, I will remove unlikely.
>
>> +		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?

We don't need.
Such function doesn't exist.

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

This is ok for me.

>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

thanks Dan
regards,
pawel