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