[PATCH v2] PCI: endpoint: Fix WARN() when an endpoint driver is removed

[PATCH v2] PCI: endpoint: Fix WARN() when an endpoint driver is removed

[Yoshihiro Shimoda]

Add pci_epc_release() for epc->dev.release and move kfree(epc)
to the release function. Otherwise, WARN() happened when a PCIe
endpoint driver is removed.

 Device 'e65d0000.pcie-ep' does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.
 WARNING: CPU: 0 PID: 139 at drivers/base/core.c:2232 device_release+0x78/0x8c

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 Changes from v1:
 - Move kfree(epc) to the release function.
 - Revised the commit description.
 https://lore.kernel.org/all/20220621121147.3971001-1-yoshihiro.shimoda.uh@renesas.com/

 drivers/pci/endpoint/pci-epc-core.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
index 3bc9273d0a08..2542196e8c3d 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -724,7 +724,6 @@ void pci_epc_destroy(struct pci_epc *epc)
 {
 	pci_ep_cfs_remove_epc_group(epc->group);
 	device_unregister(&epc->dev);
-	kfree(epc);
 }
 EXPORT_SYMBOL_GPL(pci_epc_destroy);
 
@@ -746,6 +745,11 @@ void devm_pci_epc_destroy(struct device *dev, struct pci_epc *epc)
 }
 EXPORT_SYMBOL_GPL(devm_pci_epc_destroy);
 
+static void pci_epc_release(struct device *dev)
+{
+	kfree(to_pci_epc(dev));
+}
+
 /**
  * __pci_epc_create() - create a new endpoint controller (EPC) device
  * @dev: device that is creating the new EPC
@@ -779,6 +783,7 @@ __pci_epc_create(struct device *dev, const struct pci_epc_ops *ops,
 	device_initialize(&epc->dev);
 	epc->dev.class = pci_epc_class;
 	epc->dev.parent = dev;
+	epc->dev.release = pci_epc_release;
 	epc->ops = ops;
 
 	ret = dev_set_name(&epc->dev, "%s", dev_name(dev));
-- 
2.25.1

Re: [PATCH v2] PCI: endpoint: Fix WARN() when an endpoint driver is removed

[Vidya Sagar]

Verified on Tegra194 platform.

Tested-by: Vidya Sagar <vidyas@nvidia.com>

On 6/22/2022 8:20 AM, Yoshihiro Shimoda wrote:
> External email: Use caution opening links or attachments
> 
> 
> Add pci_epc_release() for epc->dev.release and move kfree(epc)
> to the release function. Otherwise, WARN() happened when a PCIe
> endpoint driver is removed.
> 
>   Device 'e65d0000.pcie-ep' does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.
>   WARNING: CPU: 0 PID: 139 at drivers/base/core.c:2232 device_release+0x78/0x8c
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>   Changes from v1:
>   - Move kfree(epc) to the release function.
>   - Revised the commit description.
>   https://lore.kernel.org/all/20220621121147.3971001-1-yoshihiro.shimoda.uh@renesas.com/
> 
>   drivers/pci/endpoint/pci-epc-core.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index 3bc9273d0a08..2542196e8c3d 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -724,7 +724,6 @@ void pci_epc_destroy(struct pci_epc *epc)
>   {
>          pci_ep_cfs_remove_epc_group(epc->group);
>          device_unregister(&epc->dev);
> -       kfree(epc);
>   }
>   EXPORT_SYMBOL_GPL(pci_epc_destroy);
> 
> @@ -746,6 +745,11 @@ void devm_pci_epc_destroy(struct device *dev, struct pci_epc *epc)
>   }
>   EXPORT_SYMBOL_GPL(devm_pci_epc_destroy);
> 
> +static void pci_epc_release(struct device *dev)
> +{
> +       kfree(to_pci_epc(dev));
> +}
> +
>   /**
>    * __pci_epc_create() - create a new endpoint controller (EPC) device
>    * @dev: device that is creating the new EPC
> @@ -779,6 +783,7 @@ __pci_epc_create(struct device *dev, const struct pci_epc_ops *ops,
>          device_initialize(&epc->dev);
>          epc->dev.class = pci_epc_class;
>          epc->dev.parent = dev;
> +       epc->dev.release = pci_epc_release;
>          epc->ops = ops;
> 
>          ret = dev_set_name(&epc->dev, "%s", dev_name(dev));
> --
> 2.25.1
> 

Re: [PATCH v2] PCI: endpoint: Fix WARN() when an endpoint driver is removed

[Manivannan Sadhasivam]

On Wed, Jun 22, 2022 at 11:50:31AM +0900, Yoshihiro Shimoda wrote:
> Add pci_epc_release() for epc->dev.release and move kfree(epc)
> to the release function. Otherwise, WARN() happened when a PCIe
> endpoint driver is removed.

So you have mentioned why you are adding the release callback but not justified
the move of kfree() to release callback.

The commit message should clearly state what the patch does and why.

You can use something like below:

Since there is no release callback defined for the PCI EPC device, the below
warning is thrown by driver core when a PCI endpoint driver is removed:

  Device 'e65d0000.pcie-ep' does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.
  WARNING: CPU: 0 PID: 139 at drivers/base/core.c:2232 device_release+0x78/0x8c

Hence, add the release callback and also move the kfree(epc) from
pci_epc_destroy() so that the epc memory is freed when all references are
dropped.

> 
>  Device 'e65d0000.pcie-ep' does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.
>  WARNING: CPU: 0 PID: 139 at drivers/base/core.c:2232 device_release+0x78/0x8c
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

With the commit message fixed,

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> ---
>  Changes from v1:
>  - Move kfree(epc) to the release function.
>  - Revised the commit description.
>  https://lore.kernel.org/all/20220621121147.3971001-1-yoshihiro.shimoda.uh@renesas.com/
> 
>  drivers/pci/endpoint/pci-epc-core.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index 3bc9273d0a08..2542196e8c3d 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -724,7 +724,6 @@ void pci_epc_destroy(struct pci_epc *epc)
>  {
>  	pci_ep_cfs_remove_epc_group(epc->group);
>  	device_unregister(&epc->dev);
> -	kfree(epc);
>  }
>  EXPORT_SYMBOL_GPL(pci_epc_destroy);
>  
> @@ -746,6 +745,11 @@ void devm_pci_epc_destroy(struct device *dev, struct pci_epc *epc)
>  }
>  EXPORT_SYMBOL_GPL(devm_pci_epc_destroy);
>  
> +static void pci_epc_release(struct device *dev)
> +{
> +	kfree(to_pci_epc(dev));
> +}
> +
>  /**
>   * __pci_epc_create() - create a new endpoint controller (EPC) device
>   * @dev: device that is creating the new EPC
> @@ -779,6 +783,7 @@ __pci_epc_create(struct device *dev, const struct pci_epc_ops *ops,
>  	device_initialize(&epc->dev);
>  	epc->dev.class = pci_epc_class;
>  	epc->dev.parent = dev;
> +	epc->dev.release = pci_epc_release;
>  	epc->ops = ops;
>  
>  	ret = dev_set_name(&epc->dev, "%s", dev_name(dev));
> -- 
> 2.25.1
> 

-- 
மணிவண்ணன் சதாசிவம்

RE: [PATCH v2] PCI: endpoint: Fix WARN() when an endpoint driver is removed

[Yoshihiro Shimoda]

Hi Manivannan,

> From: Manivannan Sadhasivam, Sent: Wednesday, June 22, 2022 5:49 PM
> 
> On Wed, Jun 22, 2022 at 11:50:31AM +0900, Yoshihiro Shimoda wrote:
> > Add pci_epc_release() for epc->dev.release and move kfree(epc)
> > to the release function. Otherwise, WARN() happened when a PCIe
> > endpoint driver is removed.
> 
> So you have mentioned why you are adding the release callback but not justified
> the move of kfree() to release callback.
> 
> The commit message should clearly state what the patch does and why.
> 
> You can use something like below:
> 
> Since there is no release callback defined for the PCI EPC device, the below
> warning is thrown by driver core when a PCI endpoint driver is removed:
> 
>   Device 'e65d0000.pcie-ep' does not have a release() function, it is broken and must be fixed. See
> Documentation/core-api/kobject.rst.
>   WARNING: CPU: 0 PID: 139 at drivers/base/core.c:2232 device_release+0x78/0x8c
> 
> Hence, add the release callback and also move the kfree(epc) from
> pci_epc_destroy() so that the epc memory is freed when all references are
> dropped.

Thank you for your suggestion! This description looks the best to me.

> >
> >  Device 'e65d0000.pcie-ep' does not have a release() function, it is broken and must be fixed. See
> Documentation/core-api/kobject.rst.
> >  WARNING: CPU: 0 PID: 139 at drivers/base/core.c:2232 device_release+0x78/0x8c
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
> With the commit message fixed,
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thank you!

Best regards,
Yoshihiro Shimoda

> Thanks,
> Mani
> 
> > ---
> >  Changes from v1:
> >  - Move kfree(epc) to the release function.
> >  - Revised the commit description.
> >
> >
> >  drivers/pci/endpoint/pci-epc-core.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> > index 3bc9273d0a08..2542196e8c3d 100644
> > --- a/drivers/pci/endpoint/pci-epc-core.c
> > +++ b/drivers/pci/endpoint/pci-epc-core.c
> > @@ -724,7 +724,6 @@ void pci_epc_destroy(struct pci_epc *epc)
> >  {
> >  	pci_ep_cfs_remove_epc_group(epc->group);
> >  	device_unregister(&epc->dev);
> > -	kfree(epc);
> >  }
> >  EXPORT_SYMBOL_GPL(pci_epc_destroy);
> >
> > @@ -746,6 +745,11 @@ void devm_pci_epc_destroy(struct device *dev, struct pci_epc *epc)
> >  }
> >  EXPORT_SYMBOL_GPL(devm_pci_epc_destroy);
> >
> > +static void pci_epc_release(struct device *dev)
> > +{
> > +	kfree(to_pci_epc(dev));
> > +}
> > +
> >  /**
> >   * __pci_epc_create() - create a new endpoint controller (EPC) device
> >   * @dev: device that is creating the new EPC
> > @@ -779,6 +783,7 @@ __pci_epc_create(struct device *dev, const struct pci_epc_ops *ops,
> >  	device_initialize(&epc->dev);
> >  	epc->dev.class = pci_epc_class;
> >  	epc->dev.parent = dev;
> > +	epc->dev.release = pci_epc_release;
> >  	epc->ops = ops;
> >
> >  	ret = dev_set_name(&epc->dev, "%s", dev_name(dev));
> > --
> > 2.25.1
> >
> 
> --
> மணிவண்ணன் சதாசிவம்