| Message ID | 20240528063533.26723-1-yung-chuan.liao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND] soundwire: fix usages of device_get_named_child_node() | expand |
> 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
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
>>> 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
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!
>>>>> 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
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); } /*