lkml.org 
[lkml]   [2020]   [Jun]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH RFC 2/5] usb:cdns3: Add pci to platform driver wrapper
    Date

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

    \
     
     \ /
      Last update: 2020-06-26 17:12    [W:7.330 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site