Initializing automatic __free variables to NULL without need (e.g.
branches with different allocations), followed by actual allocation is
in contrary to explicit coding rules guiding cleanup.h:
"Given that the "__free(...) = NULL" pattern for variables defined at
the top of the function poses this potential interdependency problem the
recommendation is to always define and assign variables in one statement
and not group variable definitions at the top of the function when
__free() is used."
Code does not have a bug, but is less readable and uses discouraged
coding practice, so fix that by moving declaration to the place of
assignment.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
sound/soc/sdca/sdca_functions.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/sound/soc/sdca/sdca_functions.c b/sound/soc/sdca/sdca_functions.c
index 5a1f120487ef..acac066f1d8d 100644
--- a/sound/soc/sdca/sdca_functions.c
+++ b/sound/soc/sdca/sdca_functions.c
@@ -1184,7 +1184,6 @@ static int find_sdca_entity_pde(struct device *dev,
{
static const int mult_delay = 3;
struct sdca_entity_pde *power = &entity->pde;
- u32 *delay_list __free(kfree) = NULL;
struct sdca_pde_delay *delays;
int num_delays;
int i, j;
@@ -1205,7 +1204,8 @@ static int find_sdca_entity_pde(struct device *dev,
return -EINVAL;
}
- delay_list = kcalloc(num_delays, sizeof(*delay_list), GFP_KERNEL);
+ u32 *delay_list __free(kfree) = kcalloc(num_delays, sizeof(*delay_list),
+ GFP_KERNEL);
if (!delay_list)
return -ENOMEM;
@@ -1250,7 +1250,6 @@ static int find_sdca_entity_ge(struct device *dev,
struct sdca_entity *entity)
{
struct sdca_entity_ge *group = &entity->ge;
- u8 *affected_list __free(kfree) = NULL;
u8 *affected_iter;
int num_affected;
int i, j;
@@ -1269,7 +1268,8 @@ static int find_sdca_entity_ge(struct device *dev,
return -EINVAL;
}
- affected_list = kcalloc(num_affected, sizeof(*affected_list), GFP_KERNEL);
+ u8 *affected_list __free(kfree) = kcalloc(num_affected, sizeof(*affected_list),
+ GFP_KERNEL);
if (!affected_list)
return -ENOMEM;
@@ -1495,7 +1495,6 @@ static int find_sdca_entities(struct device *dev, struct sdw_slave *sdw,
struct fwnode_handle *function_node,
struct sdca_function_data *function)
{
- u32 *entity_list __free(kfree) = NULL;
struct sdca_entity *entities;
int num_entities;
int i, ret;
@@ -1517,7 +1516,8 @@ static int find_sdca_entities(struct device *dev, struct sdw_slave *sdw,
if (!entities)
return -ENOMEM;
- entity_list = kcalloc(num_entities, sizeof(*entity_list), GFP_KERNEL);
+ u32 *entity_list __free(kfree) = kcalloc(num_entities, sizeof(*entity_list),
+ GFP_KERNEL);
if (!entity_list)
return -ENOMEM;
@@ -1642,7 +1642,6 @@ static int find_sdca_entity_connection_pde(struct device *dev,
struct sdca_entity *entity)
{
struct sdca_entity_pde *power = &entity->pde;
- u32 *managed_list __free(kfree) = NULL;
struct sdca_entity **managed;
int num_managed;
int i;
@@ -1664,7 +1663,8 @@ static int find_sdca_entity_connection_pde(struct device *dev,
if (!managed)
return -ENOMEM;
- managed_list = kcalloc(num_managed, sizeof(*managed_list), GFP_KERNEL);
+ u32 *managed_list __free(kfree) = kcalloc(num_managed, sizeof(*managed_list),
+ GFP_KERNEL);
if (!managed_list)
return -ENOMEM;
@@ -1961,7 +1961,6 @@ static int find_sdca_clusters(struct device *dev,
struct fwnode_handle *function_node,
struct sdca_function_data *function)
{
- u32 *cluster_list __free(kfree) = NULL;
struct sdca_cluster *clusters;
int num_clusters;
int i, ret;
@@ -1982,7 +1981,8 @@ static int find_sdca_clusters(struct device *dev,
if (!clusters)
return -ENOMEM;
- cluster_list = kcalloc(num_clusters, sizeof(*cluster_list), GFP_KERNEL);
+ u32 *cluster_list __free(kfree) = kcalloc(num_clusters, sizeof(*cluster_list),
+ GFP_KERNEL);
if (!cluster_list)
return -ENOMEM;
@@ -2026,7 +2026,6 @@ static int find_sdca_filesets(struct device *dev, struct sdw_slave *sdw,
{
static const int mult_fileset = 3;
char fileset_name[SDCA_PROPERTY_LENGTH];
- u32 *filesets_list __free(kfree) = NULL;
struct sdca_fdl_set *sets;
int num_sets;
int i, j;
@@ -2041,7 +2040,8 @@ static int find_sdca_filesets(struct device *dev, struct sdw_slave *sdw,
return num_sets;
}
- filesets_list = kcalloc(num_sets, sizeof(u32), GFP_KERNEL);
+ u32 *filesets_list __free(kfree) = kcalloc(num_sets, sizeof(u32),
+ GFP_KERNEL);
if (!filesets_list)
return -ENOMEM;
@@ -2053,7 +2053,6 @@ static int find_sdca_filesets(struct device *dev, struct sdw_slave *sdw,
return -ENOMEM;
for (i = 0; i < num_sets; i++) {
- u32 *fileset_entries __free(kfree) = NULL;
struct sdca_fdl_set *set = &sets[i];
struct sdca_fdl_file *files;
int num_files, num_entries;
@@ -2079,7 +2078,8 @@ static int find_sdca_filesets(struct device *dev, struct sdw_slave *sdw,
if (!files)
return -ENOMEM;
- fileset_entries = kcalloc(num_entries, sizeof(u32), GFP_KERNEL);
+ u32 *fileset_entries __free(kfree) = kcalloc(num_entries, sizeof(u32),
+ GFP_KERNEL);
if (!fileset_entries)
return -ENOMEM;
--
2.51.0On Wed, Dec 03, 2025 at 05:12:40PM +0100, Krzysztof Kozlowski wrote: > Initializing automatic __free variables to NULL without need (e.g. > branches with different allocations), followed by actual allocation is > in contrary to explicit coding rules guiding cleanup.h: > > "Given that the "__free(...) = NULL" pattern for variables defined at > the top of the function poses this potential interdependency problem the > recommendation is to always define and assign variables in one statement > and not group variable definitions at the top of the function when > __free() is used." > > Code does not have a bug, but is less readable and uses discouraged > coding practice, so fix that by moving declaration to the place of > assignment. > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com> > --- Hmm... yeah a fair point. Can't say I love the inline declarations but I guess I will get used to it. I wonder if for consistency we should do something about the __free in find_sdca_init_table as well. One could move the alloc to before the error checks, does risk doing an unecessary alloc but its on the error path so I don't feel like performance matters. Or rather than sizeof(*raw) we could just do sizeof(struct raw_init_write)? But if you would rather leave as is I don't mind either so: Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com> Thanks, Charles
On 04/12/2025 12:26, Charles Keepax wrote: > On Wed, Dec 03, 2025 at 05:12:40PM +0100, Krzysztof Kozlowski wrote: >> Initializing automatic __free variables to NULL without need (e.g. >> branches with different allocations), followed by actual allocation is >> in contrary to explicit coding rules guiding cleanup.h: >> >> "Given that the "__free(...) = NULL" pattern for variables defined at >> the top of the function poses this potential interdependency problem the >> recommendation is to always define and assign variables in one statement >> and not group variable definitions at the top of the function when >> __free() is used." >> >> Code does not have a bug, but is less readable and uses discouraged >> coding practice, so fix that by moving declaration to the place of >> assignment. >> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com> >> --- > > Hmm... yeah a fair point. Can't say I love the inline > declarations but I guess I will get used to it. > > I wonder if for consistency we should do something about > the __free in find_sdca_init_table as well. One could move > the alloc to before the error checks, does risk doing an > unecessary alloc but its on the error path so I don't feel like > performance matters. Or rather than sizeof(*raw) we could just > do sizeof(struct raw_init_write)? If there is a reason to deviate from the declaration+allocation rule, then sure go for it. This is style, so it describes only the preference. But if you do not have a reason, then coding style described in cleanup.h should be used (or don't use cleanup.h). > > But if you would rather leave as is I don't mind either so: Yep. Few sdca-related files use that exception. The find_sdca_init_table() at least has a reason - it uses sizeof(*raw) as you mentioned, so that's why I decided not to change it. Just too much churn. But I want to fix all the cases where '= NULL' is used without any need. > > Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com> > > Thanks, > Charles Best regards, Krzysztof
On Thu, Dec 04, 2025 at 12:59:45PM +0100, Krzysztof Kozlowski wrote: > On 04/12/2025 12:26, Charles Keepax wrote: > > On Wed, Dec 03, 2025 at 05:12:40PM +0100, Krzysztof Kozlowski wrote: > Yep. Few sdca-related files use that exception. The > find_sdca_init_table() at least has a reason - it uses sizeof(*raw) as > you mentioned, so that's why I decided not to change it. Just too much > churn. > > But I want to fix all the cases where '= NULL' is used without any need. Groovy yeah, I am happy with that if you are. Thanks, Charles > > Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
© 2016 - 2025 Red Hat, Inc.