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;
From:   Pawel Laszczak <pawell@cadence.com>
To:     Dan Carpenter <dan.carpenter@oracle.com>
CC:     "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "balbi@kernel.org" <balbi@kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "ben.dooks@codethink.co.uk" <ben.dooks@codethink.co.uk>,
        "colin.king@canonical.com" <colin.king@canonical.com>,
        "rogerq@ti.com" <rogerq@ti.com>,
        "peter.chen@nxp.com" <peter.chen@nxp.com>,
        "weiyongjun1@huawei.com" <weiyongjun1@huawei.com>,
        Jayshri Dajiram Pawar <jpawar@cadence.com>,
        Rahul Kumar <kurahul@cadence.com>,
        Sanket Parmar <sparmar@cadence.com>
Subject: RE: [PATCH RFC 2/5] usb:cdns3: Add pci to platform driver wrapper
Thread-Topic: [PATCH RFC 2/5] usb:cdns3: Add pci to platform driver wrapper
Thread-Index: AQHWS3YeujIotZUd8kOCCktMnZwmq6jqxmUAgAAx8KA=
Date:   Fri, 26 Jun 2020 15:10:32 +0000
Message-ID: <DM6PR07MB5529E239FBA41BDDB52CAFC4DD930@DM6PR07MB5529.namprd07.prod.outlook.com>
References: <20200626045450.10205-1-pawell@cadence.com>
 <20200626045450.10205-3-pawell@cadence.com> <20200626114057.GD2571@kadam>
In-Reply-To: <20200626114057.GD2571@kadam>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 3b58b5b8-7867-453a-68f8-08d819e3123d
X-MS-Exchange-CrossTenant-originalarrivaltime: 26 Jun 2020 15:10:32.5681
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: d36035c5-6ce6-4662-a3dc-e762e61ae4c9
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 4aWsxF77644pgL/BjSd7UZ5vkzFnOgiKVSCWj+OQ3ePQZYGYHh+iJWjASVHay3HMiPme83ENGBRuFY7/QzarD6cfDBl8Se7QYN1s2wLByEE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR07MB7324
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.216,18.0.687
 definitions=2020-06-26_08:2020-06-26,2020-06-26 signatures=0
X-Proofpoint-Spam-Details: rule=outbound_check_notspam policy=outbound_check score=0 phishscore=0
 mlxlogscore=556 priorityscore=1501 malwarescore=0 clxscore=1015
 impostorscore=0 lowpriorityscore=0 spamscore=0 bulkscore=0 mlxscore=0
 adultscore=0 suspectscore=0 cotscore=-2147483648 classifier=spam adjust=0
 reason=mlx scancount=1 engine=8.12.0-2004280000
 definitions=main-2006260107
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


>
>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 =3D 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 =3D=3D pdev->devfn) {
>> +		func =3D 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 !=3D PCI_DEV_FN_HOST_DEVICE &&
>> +		    pdev->devfn !=3D PCI_DEV_FN_OTG))
>> +		return -EINVAL;
>> +
>> +	func =3D cdnsp_get_second_fun(pdev);
>> +	if (unlikely(!func))
>> +		return -EINVAL;
>> +
>> +	if (func->class =3D=3D PCI_CLASS_SERIAL_USB_XHCI ||
>> +	    pdev->class =3D=3D 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 =3D -EINVAL;
>		goto put_pci;
>
>> +
>> +	err =3D 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 =3D pci_get_drvdata(func);
>> +	} else {
>> +		wrap =3D kzalloc(sizeof(*wrap), GFP_KERNEL);
>> +		if (!wrap) {
>> +			pci_disable_device(pdev);
>> +			return -ENOMEM;
>
>goto disable_pci;
>
>> +		}
>> +	}
>> +
>> +	res =3D wrap->dev_res;
>> +
>> +	/* For GADGET device function number is 0. */
>> +	if (pdev->devfn =3D=3D 0) {
>> +		/* Function 0: host(BAR_0) + device(BAR_1).*/
>> +		dev_dbg(&pdev->dev, "Initialize Device resources\n");
>> +		res[RES_DEV_ID].start =3D pci_resource_start(pdev, PCI_BAR_DEV);
>> +		res[RES_DEV_ID].end =3D   pci_resource_end(pdev, PCI_BAR_DEV);
>> +		res[RES_DEV_ID].name =3D "dev";
>> +		res[RES_DEV_ID].flags =3D IORESOURCE_MEM;
>> +		dev_dbg(&pdev->dev, "USBSS-DEV physical base addr: %pa\n",
>> +			&res[RES_DEV_ID].start);
>> +
>> +		res[RES_HOST_ID].start =3D pci_resource_start(pdev, PCI_BAR_HOST);
>> +		res[RES_HOST_ID].end =3D pci_resource_end(pdev, PCI_BAR_HOST);
>> +		res[RES_HOST_ID].name =3D "xhci";
>> +		res[RES_HOST_ID].flags =3D 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 =3D pdev->irq;
>> +		wrap->dev_res[RES_IRQ_HOST_ID].name =3D "host";
>> +		wrap->dev_res[RES_IRQ_HOST_ID].flags =3D IORESOURCE_IRQ;
>> +
>> +		/* Interrupt device. It's the same as for HOST. */
>> +		wrap->dev_res[RES_IRQ_PERIPHERAL_ID].start =3D pdev->irq;
>> +		wrap->dev_res[RES_IRQ_PERIPHERAL_ID].name =3D "peripheral";
>> +		wrap->dev_res[RES_IRQ_PERIPHERAL_ID].flags =3D IORESOURCE_IRQ;
>> +	} else {
>> +		res[RES_DRD_ID].start =3D pci_resource_start(pdev, PCI_BAR_OTG);
>> +		res[RES_DRD_ID].end =3D   pci_resource_end(pdev, PCI_BAR_OTG);
>> +		res[RES_DRD_ID].name =3D "otg";
>> +		res[RES_DRD_ID].flags =3D 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 =3D pdev->irq;
>> +		wrap->dev_res[RES_IRQ_OTG_ID].name =3D "otg";
>> +		wrap->dev_res[RES_IRQ_OTG_ID].flags =3D IORESOURCE_IRQ;
>> +	}
>> +
>> +	if (pci_is_enabled(func)) {
>> +		/* Set up platform device info. */
>> +		memset(&plat_info, 0, sizeof(plat_info));
>> +		plat_info.parent =3D &pdev->dev;
>> +		plat_info.fwnode =3D pdev->dev.fwnode;
>> +		plat_info.name =3D PLAT_DRIVER_NAME;
>> +		plat_info.id =3D pdev->devfn;
>> +		wrap->devfn  =3D pdev->devfn;
>> +		plat_info.res =3D wrap->dev_res;
>> +		plat_info.num_res =3D ARRAY_SIZE(wrap->dev_res);
>> +		plat_info.dma_mask =3D pdev->dma_mask;
>> +		/* Register platform device. */
>> +		wrap->plat_dev =3D platform_device_register_full(&plat_info);
>> +		if (IS_ERR(wrap->plat_dev)) {
>> +			pci_disable_device(pdev);
>> +			err =3D PTR_ERR(wrap->plat_dev);
>> +			kfree(wrap);
>
>		err =3D 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 =3D cdnsp_get_second_fun(pdev);
>> +
>> +	wrap =3D (struct cdnsp_wrap *)pci_get_drvdata(pdev);
>> +	if (wrap->devfn =3D=3D 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