diff mbox series

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

Message ID 20220622025031.51812-1-yoshihiro.shimoda.uh@renesas.com (mailing list archive)
State Superseded
Headers show
Series [v2] PCI: endpoint: Fix WARN() when an endpoint driver is removed | expand

Commit Message

Yoshihiro Shimoda June 22, 2022, 2:50 a.m. UTC
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(-)

Comments

Vidya Sagar June 22, 2022, 7:10 a.m. UTC | #1
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
>
Manivannan Sadhasivam June 22, 2022, 8:49 a.m. UTC | #2
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
>
Yoshihiro Shimoda June 23, 2022, 12:19 a.m. UTC | #3
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
> >
> 
> --
> மணிவண்ணன் சதாசிவம்
diff mbox series

Patch

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