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/[email protected]/
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
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 <[email protected]>
---
.../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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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;
>> }
>>