[PATCH RESEND] soundwire: fix usages of device_get_named_child_node()

[PATCH RESEND] soundwire: fix usages of device_get_named_child_node()

[Bard Liao]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

The documentation for device_get_named_child_node() mentions this
important point:

"
The caller is responsible for calling fwnode_handle_put() on the
returned fwnode pointer.
"

Add fwnode_handle_put() to avoid leaked references.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/amd_manager.c     |  3 +++
 drivers/soundwire/intel_auxdevice.c |  6 +++++-
 drivers/soundwire/mipi_disco.c      | 30 +++++++++++++++++++++++------
 3 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index 20d94bcfc9b4..795e223f7e5c 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -571,6 +571,9 @@ static int sdw_master_read_amd_prop(struct sdw_bus *bus)
 	amd_manager->wake_en_mask = wake_en_mask;
 	fwnode_property_read_u32(link, "amd-sdw-power-mode", &power_mode_mask);
 	amd_manager->power_mode_mask = power_mode_mask;
+
+	fwnode_handle_put(link);
+
 	return 0;
 }
 
diff --git a/drivers/soundwire/intel_auxdevice.c b/drivers/soundwire/intel_auxdevice.c
index 17cf27e6ea73..18517121cc89 100644
--- a/drivers/soundwire/intel_auxdevice.c
+++ b/drivers/soundwire/intel_auxdevice.c
@@ -155,8 +155,10 @@ static int sdw_master_read_intel_prop(struct sdw_bus *bus)
 		SDW_MASTER_QUIRKS_CLEAR_INITIAL_PARITY;
 
 	intel_prop = devm_kzalloc(bus->dev, sizeof(*intel_prop), GFP_KERNEL);
-	if (!intel_prop)
+	if (!intel_prop) {
+		fwnode_handle_put(link);
 		return -ENOMEM;
+	}
 
 	/* initialize with hardware defaults, in case the properties are not found */
 	intel_prop->doaise = 0x1;
@@ -184,6 +186,8 @@ static int sdw_master_read_intel_prop(struct sdw_bus *bus)
 		intel_prop->dodse,
 		intel_prop->dods);
 
+	fwnode_handle_put(link);
+
 	return 0;
 }
 
diff --git a/drivers/soundwire/mipi_disco.c b/drivers/soundwire/mipi_disco.c
index 55a9c51c84c1..e5d9df26d4dc 100644
--- a/drivers/soundwire/mipi_disco.c
+++ b/drivers/soundwire/mipi_disco.c
@@ -66,8 +66,10 @@ int sdw_master_read_prop(struct sdw_bus *bus)
 		prop->clk_freq = devm_kcalloc(bus->dev, prop->num_clk_freq,
 					      sizeof(*prop->clk_freq),
 					      GFP_KERNEL);
-		if (!prop->clk_freq)
+		if (!prop->clk_freq) {
+			fwnode_handle_put(link);
 			return -ENOMEM;
+		}
 
 		fwnode_property_read_u32_array(link,
 				"mipi-sdw-clock-frequencies-supported",
@@ -92,8 +94,10 @@ int sdw_master_read_prop(struct sdw_bus *bus)
 		prop->clk_gears = devm_kcalloc(bus->dev, prop->num_clk_gears,
 					       sizeof(*prop->clk_gears),
 					       GFP_KERNEL);
-		if (!prop->clk_gears)
+		if (!prop->clk_gears) {
+			fwnode_handle_put(link);
 			return -ENOMEM;
+		}
 
 		fwnode_property_read_u32_array(link,
 					       "mipi-sdw-supported-clock-gears",
@@ -116,6 +120,8 @@ int sdw_master_read_prop(struct sdw_bus *bus)
 	fwnode_property_read_u32(link, "mipi-sdw-command-error-threshold",
 				 &prop->err_threshold);
 
+	fwnode_handle_put(link);
+
 	return 0;
 }
 EXPORT_SYMBOL(sdw_master_read_prop);
@@ -197,8 +203,10 @@ static int sdw_slave_read_dpn(struct sdw_slave *slave,
 						    dpn[i].num_words,
 						    sizeof(*dpn[i].words),
 						    GFP_KERNEL);
-			if (!dpn[i].words)
+			if (!dpn[i].words) {
+				fwnode_handle_put(node);
 				return -ENOMEM;
+			}
 
 			fwnode_property_read_u32_array(node,
 					"mipi-sdw-port-wordlength-configs",
@@ -236,8 +244,10 @@ static int sdw_slave_read_dpn(struct sdw_slave *slave,
 						       dpn[i].num_channels,
 						       sizeof(*dpn[i].channels),
 						 GFP_KERNEL);
-			if (!dpn[i].channels)
+			if (!dpn[i].channels) {
+				fwnode_handle_put(node);
 				return -ENOMEM;
+			}
 
 			fwnode_property_read_u32_array(node,
 					"mipi-sdw-channel-number-list",
@@ -251,8 +261,10 @@ static int sdw_slave_read_dpn(struct sdw_slave *slave,
 					dpn[i].num_ch_combinations,
 					sizeof(*dpn[i].ch_combinations),
 					GFP_KERNEL);
-			if (!dpn[i].ch_combinations)
+			if (!dpn[i].ch_combinations) {
+				fwnode_handle_put(node);
 				return -ENOMEM;
+			}
 
 			fwnode_property_read_u32_array(node,
 					"mipi-sdw-channel-combination-list",
@@ -274,6 +286,8 @@ static int sdw_slave_read_dpn(struct sdw_slave *slave,
 
 		/* TODO: Read audio mode */
 
+		fwnode_handle_put(node);
+
 		i++;
 	}
 
@@ -348,10 +362,14 @@ int sdw_slave_read_prop(struct sdw_slave *slave)
 		prop->dp0_prop = devm_kzalloc(&slave->dev,
 					      sizeof(*prop->dp0_prop),
 					      GFP_KERNEL);
-		if (!prop->dp0_prop)
+		if (!prop->dp0_prop) {
+			fwnode_handle_put(port);
 			return -ENOMEM;
+		}
 
 		sdw_slave_read_dp0(slave, port, prop->dp0_prop);
+
+		fwnode_handle_put(port);
 	}
 
 	/*
-- 
2.40.1

Re: [PATCH RESEND] soundwire: fix usages of device_get_named_child_node()

[Markus Elfring]

> Add fwnode_handle_put() to avoid leaked references.

Are you going to respond also to my previous patch review
in more constructive ways?
https://lore.kernel.org/lkml/eb15ab0a-e416-4ae9-98bb-610fdc04492c@web.de/
https://lkml.org/lkml/2024/4/29/493

Regards,
Markus

Re: [PATCH RESEND] soundwire: fix usages of device_get_named_child_node()

[Pierre-Louis Bossart]

On 5/28/24 10:19, Markus Elfring wrote:
>> Add fwnode_handle_put() to avoid leaked references.
> 
> Are you going to respond also to my previous patch review
> in more constructive ways?
> https://lore.kernel.org/lkml/eb15ab0a-e416-4ae9-98bb-610fdc04492c@web.de/
> https://lkml.org/lkml/2024/4/29/493

Sorry about that, both Bard and I missed your comments.

On the Fixes tag: I made a deliberate choice to add all the fixes in one
patch, to show that the usage was copy-pasted and done 'wrong' in
multiple places. That makes it really hard to add a Fixes tag since the
different uses were added in a time interval of about 5 years.
We could split and have multiple patches if that was desired, but I
would still not include a Fixes tag since the leaked references are not
that bad, we read the Manager properties on probe, and the peripheral
properties are generally not used by codec drivers, so it's unlikely
that any user will ever see a problem that requires a backport in
linux-stable. This problem was found by reading the documentation while
adding new things, not by any user report or test failure.

The error flow was revisited and hardened in a follow-up patchset which
also adds new properties for MIPI DisCo 2.1 spec [1], we'll share the
patches in this kernel cycle.

[1] https://github.com/thesofproject/linux/pull/4857

Re: soundwire: fix usages of device_get_named_child_node()

[Markus Elfring]

>>> Add fwnode_handle_put() to avoid leaked references.
>>
>> Are you going to respond also to my previous patch review
>> in more constructive ways?
>> https://lore.kernel.org/lkml/eb15ab0a-e416-4ae9-98bb-610fdc04492c@web.de/
>> https://lkml.org/lkml/2024/4/29/493
>
> Sorry about that, both Bard and I missed your comments.

How could this happen?


> On the Fixes tag: I made a deliberate choice to add all the fixes in one
> patch, to show that the usage was copy-pasted and done 'wrong' in
> multiple places. That makes it really hard to add a Fixes tag since the
> different uses were added in a time interval of about 5 years.

Is it interesting how the affected software components evolved in the meantime?


> We could split and have multiple patches if that was desired, but I
> would still not include a Fixes tag since the leaked references are not
> that bad, we read the Manager properties on probe, and the peripheral
> properties are generally not used by codec drivers, so it's unlikely
> that any user will ever see a problem that requires a backport in linux-stable.
…

I became curious how the exception handling will be completed here.

* Do you still care for the usage of goto chains?
  https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources

* How do you think about to increase the application of scope-based resource management?
  https://elixir.bootlin.com/linux/v6.10-rc1/source/include/linux/cleanup.h#L124


Regards,
Markus

Re: soundwire: fix usages of device_get_named_child_node()

[Vinod Koul]

On 28-05-24, 18:22, Markus Elfring wrote:
> >>> Add fwnode_handle_put() to avoid leaked references.
> >>
> >> Are you going to respond also to my previous patch review
> >> in more constructive ways?
> >> https://lore.kernel.org/lkml/eb15ab0a-e416-4ae9-98bb-610fdc04492c@web.de/
> >> https://lkml.org/lkml/2024/4/29/493
> >
> > Sorry about that, both Bard and I missed your comments.
> 
> How could this happen?

Becuase your emails go to dev/null for most of people!

-- 
~Vinod

Re: soundwire: fix usages of device_get_named_child_node()

[Markus Elfring]

>>>>> Add fwnode_handle_put() to avoid leaked references.
>>>>
>>>> Are you going to respond also to my previous patch review
>>>> in more constructive ways?
>>>> https://lore.kernel.org/lkml/eb15ab0a-e416-4ae9-98bb-610fdc04492c@web.de/
>>>> https://lkml.org/lkml/2024/4/29/493
>>>
>>> Sorry about that, both Bard and I missed your comments.
>>
>> How could this happen?
>
> Becuase your emails go to dev/null for most of people!

I am aware of some communication challenges.

Is it interesting that my enquiry for this “PATCH RESEND” triggered
a constructive response finally?

Regards,
Markus