[PATCH v2 0/4] staging: vchiq: Rework child platform device (un)register

[PATCH v2 0/4] staging: vchiq: Rework child platform device (un)register

[Umang Jain]

V2 series for addressing the TODO item:
        "Get rid of all non essential global structures and create a proper
         per device structure"
This series:
- Fixes a platform device leak (1/4)
- Simplifies vchiq_register_child (2/4 and 3/4)
- drops global references for child platform devices and prepares for
  addition of new child devices in future (4/4)

v1: https://lore.kernel.org/all/20221220084404.19280-1-umang.jain@ideasonboard.com/

Umang Jain (4):
  staging: vc04_services: Stop leaking platform device on error path
  staging: vchiq: Do not assign default dma_mask explicitly
  staging: vchiq: Simplify platform devices registration
  staging: vchiq: Rework child platform device (un)register

 .../interface/vchiq_arm/vchiq_arm.c           | 40 ++++++++++---------
 1 file changed, 21 insertions(+), 19 deletions(-)

-- 
2.38.1

[PATCH v2 1/4] staging: vc04_services: Stop leaking platform device on error path

[Umang Jain]

vchiq driver registers the child platform devices in
vchiq_register_child(). However, in the registration error code path,
currently the driver is leaking platform devices by not destroying the
return platform device. Plug this leak using platform_device_put() as
mentioned in the documentation for platform_device_register().

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index dc33490ba7fb..fc7ea7ba97b2 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1779,6 +1779,7 @@ vchiq_register_child(struct platform_device *pdev, const char *name)
 	child = platform_device_register_full(&pdevinfo);
 	if (IS_ERR(child)) {
 		dev_warn(&pdev->dev, "%s not registered\n", name);
+		platform_device_put(child);
 		child = NULL;
 	}
 
-- 
2.38.1

Re: [PATCH v2 1/4] staging: vc04_services: Stop leaking platform device on error path

[Stefan Wahren]

Hi Umang,

Am 22.12.22 um 20:14 schrieb Umang Jain:
> vchiq driver registers the child platform devices in
> vchiq_register_child(). However, in the registration error code path,
> currently the driver is leaking platform devices by not destroying the
> return platform device. Plug this leak using platform_device_put() as
> mentioned in the documentation for platform_device_register().
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>   drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> index dc33490ba7fb..fc7ea7ba97b2 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -1779,6 +1779,7 @@ vchiq_register_child(struct platform_device *pdev, const char *name)
>   	child = platform_device_register_full(&pdevinfo);
>   	if (IS_ERR(child)) {
>   		dev_warn(&pdev->dev, "%s not registered\n", name);
> +		platform_device_put(child);
why should we free an error?
>   		child = NULL;
>   	}
>   

Re: [PATCH v2 1/4] staging: vc04_services: Stop leaking platform device on error path

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Fri, Dec 23, 2022 at 12:44:57AM +0530, Umang Jain wrote:
> vchiq driver registers the child platform devices in
> vchiq_register_child(). However, in the registration error code path,
> currently the driver is leaking platform devices by not destroying the
> return platform device. Plug this leak using platform_device_put() as
> mentioned in the documentation for platform_device_register().
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> index dc33490ba7fb..fc7ea7ba97b2 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -1779,6 +1779,7 @@ vchiq_register_child(struct platform_device *pdev, const char *name)
>  	child = platform_device_register_full(&pdevinfo);
>  	if (IS_ERR(child)) {
>  		dev_warn(&pdev->dev, "%s not registered\n", name);
> +		platform_device_put(child);

If IS_ERR(child), what do you expect platform_device_put(child) to do ?
And have you read the implementation of platform_device_register_full()
?

>  		child = NULL;
>  	}
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/4] staging: vc04_services: Stop leaking platform device on error path

[Umang Jain]

Hi,

On 12/23/22 2:00 AM, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> On Fri, Dec 23, 2022 at 12:44:57AM +0530, Umang Jain wrote:
>> vchiq driver registers the child platform devices in
>> vchiq_register_child(). However, in the registration error code path,
>> currently the driver is leaking platform devices by not destroying the
>> return platform device. Plug this leak using platform_device_put() as
>> mentioned in the documentation for platform_device_register().
>>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>   drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> index dc33490ba7fb..fc7ea7ba97b2 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> @@ -1779,6 +1779,7 @@ vchiq_register_child(struct platform_device *pdev, const char *name)
>>   	child = platform_device_register_full(&pdevinfo);
>>   	if (IS_ERR(child)) {
>>   		dev_warn(&pdev->dev, "%s not registered\n", name);
>> +		platform_device_put(child);
> If IS_ERR(child), what do you expect platform_device_put(child) to do ?
> And have you read the implementation of platform_device_register_full()
> ?

Errr, yeah - it is handling the platform_device_put() as well. Stupid me!

(dropping this patch for v3)
>
>>   		child = NULL;
>>   	}
>>   

[PATCH v2 2/4] staging: vchiq: Do not assign default dma_mask explicitly

[Umang Jain]

The vchiq driver assigns DMA_BIT_MASK(32) to the child platform devices
it registers. By default, platform_device_register_* helpers assign
DMA_BIT_MASK(32) dma_mask to the platform devices it registers. It is
unnecessary to define it explicitly in struct platform_device_info
hence, drop this explicit dma_mask assignment.

This will help simplying the vchiq_register_child() going forwards.
No functional changes intended in this patch.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index fc7ea7ba97b2..3c4766375daa 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1774,7 +1774,6 @@ vchiq_register_child(struct platform_device *pdev, const char *name)
 	pdevinfo.parent = &pdev->dev;
 	pdevinfo.name = name;
 	pdevinfo.id = PLATFORM_DEVID_NONE;
-	pdevinfo.dma_mask = DMA_BIT_MASK(32);
 
 	child = platform_device_register_full(&pdevinfo);
 	if (IS_ERR(child)) {
-- 
2.38.1

[PATCH v2 3/4] staging: vchiq: Simplify platform devices registration

[Umang Jain]

The child platform devices registered by the vchiq driver currently
populates a struct platform_device_info with a name and parent device
and uses platform_device_register_full() to its registration.

It can be simplified by using platform_device_register_data() directly
(which encapsulates populating the platform_device_info struct and a
platform_device_register_full() call in itself).

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../vc04_services/interface/vchiq_arm/vchiq_arm.c      | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 3c4766375daa..ba34e4d603d4 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1766,16 +1766,10 @@ MODULE_DEVICE_TABLE(of, vchiq_of_match);
 static struct platform_device *
 vchiq_register_child(struct platform_device *pdev, const char *name)
 {
-	struct platform_device_info pdevinfo;
 	struct platform_device *child;
 
-	memset(&pdevinfo, 0, sizeof(pdevinfo));
-
-	pdevinfo.parent = &pdev->dev;
-	pdevinfo.name = name;
-	pdevinfo.id = PLATFORM_DEVID_NONE;
-
-	child = platform_device_register_full(&pdevinfo);
+	child = platform_device_register_data(&pdev->dev, name, PLATFORM_DEVID_NONE,
+					      NULL, 0);
 	if (IS_ERR(child)) {
 		dev_warn(&pdev->dev, "%s not registered\n", name);
 		platform_device_put(child);
-- 
2.38.1

[PATCH v2 4/4] staging: vchiq: Rework child platform device (un)register

[Umang Jain]

This patch reworks how the child platform devices are (un)registered
by the vchiq driver. It drops the global references to the child platform
devices thereby reducing the scope of platform device access to probe()
function only. It does so, by maintaining an array of platform device
names and registering each of them through vchiq_register_child().
In addition to that, any new child platform device can be
(un)regsitered easily by just appending to the child platform devices'
array.

For platform device unregisterion, device_for_each_child() helper is
used to call vchiq_unregister_child() on each of the child platform
device of vchiq driver.

This is part of an effort to address TODO item "Get rid of all non
essential global structures and create a proper per device structure"

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_arm.c           | 28 ++++++++++++-------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index ba34e4d603d4..d04dbea833ac 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -65,9 +65,6 @@ int vchiq_susp_log_level = VCHIQ_LOG_ERROR;
 DEFINE_SPINLOCK(msg_queue_spinlock);
 struct vchiq_state g_state;
 
-static struct platform_device *bcm2835_camera;
-static struct platform_device *bcm2835_audio;
-
 struct vchiq_drvdata {
 	const unsigned int cache_line_size;
 	struct rpi_firmware *fw;
@@ -1763,7 +1760,7 @@ static const struct of_device_id vchiq_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, vchiq_of_match);
 
-static struct platform_device *
+static void
 vchiq_register_child(struct platform_device *pdev, const char *name)
 {
 	struct platform_device *child;
@@ -1773,10 +1770,18 @@ vchiq_register_child(struct platform_device *pdev, const char *name)
 	if (IS_ERR(child)) {
 		dev_warn(&pdev->dev, "%s not registered\n", name);
 		platform_device_put(child);
-		child = NULL;
 	}
+}
 
-	return child;
+static int
+vchiq_unregister_child(struct device *dev, void *data)
+{
+	struct platform_device *pdev;
+
+	pdev = to_platform_device(dev);
+	platform_device_unregister(pdev);
+
+	return 0;
 }
 
 static int vchiq_probe(struct platform_device *pdev)
@@ -1784,6 +1789,10 @@ static int vchiq_probe(struct platform_device *pdev)
 	struct device_node *fw_node;
 	const struct of_device_id *of_id;
 	struct vchiq_drvdata *drvdata;
+	const char *const vchiq_devices[] = {
+		"bcm2835_audio",
+		"bcm2835-camera",
+	};
 	int err;
 
 	of_id = of_match_node(vchiq_of_match, pdev->dev.of_node);
@@ -1826,8 +1835,8 @@ static int vchiq_probe(struct platform_device *pdev)
 		goto error_exit;
 	}
 
-	bcm2835_camera = vchiq_register_child(pdev, "bcm2835-camera");
-	bcm2835_audio = vchiq_register_child(pdev, "bcm2835_audio");
+	for (unsigned int i = 0; i < ARRAY_SIZE(vchiq_devices); i++)
+		vchiq_register_child(pdev, vchiq_devices[i]);
 
 	return 0;
 
@@ -1839,8 +1848,7 @@ static int vchiq_probe(struct platform_device *pdev)
 
 static int vchiq_remove(struct platform_device *pdev)
 {
-	platform_device_unregister(bcm2835_audio);
-	platform_device_unregister(bcm2835_camera);
+	device_for_each_child(&pdev->dev, NULL, vchiq_unregister_child);
 	vchiq_debugfs_deinit();
 	vchiq_deregister_chrdev();
 
-- 
2.38.1

Re: [PATCH v2 0/4] staging: vchiq: Rework child platform device (un)register

[Stefan Wahren]

Hi Umang,

Am 22.12.22 um 20:14 schrieb Umang Jain:
> V2 series for addressing the TODO item:
>          "Get rid of all non essential global structures and create a proper
>           per device structure"
> This series:
> - Fixes a platform device leak (1/4)
> - Simplifies vchiq_register_child (2/4 and 3/4)
> - drops global references for child platform devices and prepares for
>    addition of new child devices in future (4/4)

the whole problem i see with this series is that kernel module support 
is broken. So i'm not able to test everything.

Did you did just test driver removable?

>
> v1: https://lore.kernel.org/all/20221220084404.19280-1-umang.jain@ideasonboard.com/
>
> Umang Jain (4):
>    staging: vc04_services: Stop leaking platform device on error path
>    staging: vchiq: Do not assign default dma_mask explicitly
>    staging: vchiq: Simplify platform devices registration
>    staging: vchiq: Rework child platform device (un)register
>
>   .../interface/vchiq_arm/vchiq_arm.c           | 40 ++++++++++---------
>   1 file changed, 21 insertions(+), 19 deletions(-)
>