diff mbox series

firmware: ti_sci: Mark driver as non removable

Message ID 20230907171700.1922453-1-d-gole@ti.com (mailing list archive)
State New, archived
Headers show
Series firmware: ti_sci: Mark driver as non removable | expand

Commit Message

Dhruva Gole Sept. 7, 2023, 5:17 p.m. UTC
The TI-SCI message protocol provides a way to communicate between
various compute processors with a central system controller entity. It
provides the fundamental device management capability and clock control
in the SOCs that it's used in.

Ensure that the driver isn't unbound by suppressing its bind and unbind
sysfs attributes. As the driver is built-in there is no way to remove
device once bound.

We can also remove the ti_sci_remove call along with the
ti_sci_debugfs_destroy as there are no callers for it any longer.

Fixes: aa276781a64a ("firmware: Add basic support for TI System Control Interface (TI-SCI) protocol")
Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Dhruva Gole <d-gole@ti.com>
---

Cc: Nishanth Menon <nm@ti.com>
Cc: Tero Kristo <kristo@kernel.org>
Cc: Santosh Shilimkar <ssantosh@kernel.org>
Cc: Vibhore Vardhan <vibhore@ti.com>
Cc: Vignesh Raghavendra <vigneshr@ti.com>

Thanks for bringing up the potential issue Uwe, sharing link below for
context:
https://lore.kernel.org/all/20230216083908.mvmydic5lpi3ogo7@pengutronix.de/

This patch is also inspired from the commit bbfa509d632946578d4f
("powerpc/ep8248e: Mark driver as non removable")

base: cd04b2fab5ee (tag: next-20230907) Add linux-next specific files for 20230907

 drivers/firmware/ti_sci.c | 46 +--------------------------------------
 1 file changed, 1 insertion(+), 45 deletions(-)

Comments

Uwe Kleine-König Sept. 7, 2023, 8:54 p.m. UTC | #1
Hello,

On Thu, Sep 07, 2023 at 10:47:00PM +0530, Dhruva Gole wrote:
> The TI-SCI message protocol provides a way to communicate between
> various compute processors with a central system controller entity. It
> provides the fundamental device management capability and clock control
> in the SOCs that it's used in.
> 

Maybe add:

	The remove function failed to do all the necessary cleanup if
	there are registered users. Some things are freed however which
	likely results in an oops later on.

> Ensure that the driver isn't unbound by suppressing its bind and unbind
> sysfs attributes. As the driver is built-in there is no way to remove
> device once bound.
> 
> We can also remove the ti_sci_remove call along with the
> ti_sci_debugfs_destroy as there are no callers for it any longer.
> 
> Fixes: aa276781a64a ("firmware: Add basic support for TI System Control Interface (TI-SCI) protocol")
> Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Dhruva Gole <d-gole@ti.com>

While this isn't the most elegant solution, this is probably the most
feasible one.

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

(with or without the suggested addition to the commit log above).

Best regards
Uwe
Nishanth Menon Sept. 20, 2023, 12:59 p.m. UTC | #2
On 22:47-20230907, Dhruva Gole wrote:
> The TI-SCI message protocol provides a way to communicate between
> various compute processors with a central system controller entity. It
> provides the fundamental device management capability and clock control
> in the SOCs that it's used in.
> 
> Ensure that the driver isn't unbound by suppressing its bind and unbind
> sysfs attributes. As the driver is built-in there is no way to remove
> device once bound.
> 
> We can also remove the ti_sci_remove call along with the
> ti_sci_debugfs_destroy as there are no callers for it any longer.
> 
> Fixes: aa276781a64a ("firmware: Add basic support for TI System Control Interface (TI-SCI) protocol")
> Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Dhruva Gole <d-gole@ti.com>

Add Reported-by and Closes tags

Also please address the comment from Uwe on improving the commit message

[...]
Dhruva Gole Sept. 21, 2023, 9:24 a.m. UTC | #3
On Sep 20, 2023 at 07:59:04 -0500, Nishanth Menon wrote:
> On 22:47-20230907, Dhruva Gole wrote:
> > The TI-SCI message protocol provides a way to communicate between
> > various compute processors with a central system controller entity. It
> > provides the fundamental device management capability and clock control
> > in the SOCs that it's used in.
> > 
> > Ensure that the driver isn't unbound by suppressing its bind and unbind
> > sysfs attributes. As the driver is built-in there is no way to remove
> > device once bound.
> > 
> > We can also remove the ti_sci_remove call along with the
> > ti_sci_debugfs_destroy as there are no callers for it any longer.
> > 
> > Fixes: aa276781a64a ("firmware: Add basic support for TI System Control Interface (TI-SCI) protocol")
> > Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Signed-off-by: Dhruva Gole <d-gole@ti.com>
> 
> Add Reported-by and Closes tags
> 
> Also please address the comment from Uwe on improving the commit message

Sent V2 patch here:
https://lore.kernel.org/all/20230921091025.133130-1-d-gole@ti.com/T/#u

> 
> [...]
> 
> -- 
> Regards,
> Nishanth Menon
> Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D
diff mbox series

Patch

diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c
index 26a37f47f4ca..66c3846c9147 100644
--- a/drivers/firmware/ti_sci.c
+++ b/drivers/firmware/ti_sci.c
@@ -190,19 +190,6 @@  static int ti_sci_debugfs_create(struct platform_device *pdev,
 	return 0;
 }
 
-/**
- * ti_sci_debugfs_destroy() - clean up log debug file
- * @pdev:	platform device pointer
- * @info:	Pointer to SCI entity information
- */
-static void ti_sci_debugfs_destroy(struct platform_device *pdev,
-				   struct ti_sci_info *info)
-{
-	if (IS_ERR(info->debug_region))
-		return;
-
-	debugfs_remove(info->d);
-}
 #else /* CONFIG_DEBUG_FS */
 static inline int ti_sci_debugfs_create(struct platform_device *dev,
 					struct ti_sci_info *info)
@@ -3449,43 +3436,12 @@  static int ti_sci_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int ti_sci_remove(struct platform_device *pdev)
-{
-	struct ti_sci_info *info;
-	struct device *dev = &pdev->dev;
-	int ret = 0;
-
-	of_platform_depopulate(dev);
-
-	info = platform_get_drvdata(pdev);
-
-	if (info->nb.notifier_call)
-		unregister_restart_handler(&info->nb);
-
-	mutex_lock(&ti_sci_list_mutex);
-	if (info->users)
-		ret = -EBUSY;
-	else
-		list_del(&info->node);
-	mutex_unlock(&ti_sci_list_mutex);
-
-	if (!ret) {
-		ti_sci_debugfs_destroy(pdev, info);
-
-		/* Safe to free channels since no more users */
-		mbox_free_channel(info->chan_tx);
-		mbox_free_channel(info->chan_rx);
-	}
-
-	return ret;
-}
-
 static struct platform_driver ti_sci_driver = {
 	.probe = ti_sci_probe,
-	.remove = ti_sci_remove,
 	.driver = {
 		   .name = "ti-sci",
 		   .of_match_table = of_match_ptr(ti_sci_of_match),
+		   .suppress_bind_attrs = true,
 	},
 };
 module_platform_driver(ti_sci_driver);