linux icon indicating copy to clipboard operation
linux copied to clipboard

SoundWire/ASoC/ALSA: fix usages of device_get_named_child_node()

Open plbossart opened this issue 1 year ago • 8 comments

We don't seem to be following the documentation for all access to hierarchical _DSD properties for audio.

plbossart avatar Apr 16 '24 19:04 plbossart

@andy-shev FYI, if you have time to review.

plbossart avatar Apr 16 '24 19:04 plbossart

So, it's mostly technical conversion, the problem is clear, the solution as well. I'm not going to check everything, esp. you already got a response, but on brief view looks reasonable.

andy-shev avatar Apr 17 '24 08:04 andy-shev

In sdw_slave_read_prop() function, below condition is not addressed. nval = hweight32(prop->source_ports); prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval, sizeof(*prop->src_dpn_prop), GFP_KERNEL); if (!prop->src_dpn_prop) return -ENOMEM;

Second condition check:

prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval, sizeof(*prop->sink_dpn_prop), GFP_KERNEL); if (!prop->sink_dpn_prop) return -ENOMEM;

As per our understanding, above one must be valid cases, where we need for put.

vijendarmukunda avatar Apr 17 '24 13:04 vijendarmukunda

In sdw_master_read_amd_prop() function , for below check , fwnode_handle_put(link) addition is missing.

link = device_get_named_child_node(bus->dev, name); if (!link) { dev_err(bus->dev, "Manager node %s not found\n", name); return -EIO; }

no if the get failed there's no need for a put.

plbossart avatar Apr 17 '24 13:04 plbossart

In sdw_master_read_amd_prop() function , for below check , fwnode_handle_put(link) addition is missing. link = device_get_named_child_node(bus->dev, name); if (!link) { dev_err(bus->dev, "Manager node %s not found\n", name); return -EIO; }

no if the get failed there's no need for a put.

Agreed.

vijendarmukunda avatar Apr 17 '24 13:04 vijendarmukunda

In sdw_slave_read_prop() function, below condition is not addressed. nval = hweight32(prop->source_ports); prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval, sizeof(*prop->src_dpn_prop), GFP_KERNEL); if (!prop->src_dpn_prop) return -ENOMEM;

Second condition check:

prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval, sizeof(*prop->sink_dpn_prop), GFP_KERNEL); if (!prop->sink_dpn_prop) return -ENOMEM;

Yes this one has a set of misses, will fix. Thanks for the review.

plbossart avatar Apr 17 '24 13:04 plbossart

In sdw_slave_read_prop() function, below condition is not addressed. nval = hweight32(prop->source_ports); prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval, sizeof(*prop->src_dpn_prop), GFP_KERNEL); if (!prop->src_dpn_prop) return -ENOMEM;

Humm, after re-checking this is fine: the "port" is released above. Please double-check.

Second condition check:

prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval, sizeof(*prop->sink_dpn_prop), GFP_KERNEL); if (!prop->sink_dpn_prop) return -ENOMEM;

Same, for src and sink ports all the references are released in sdw_slave_read_dpn()

So no change for these two comments.

plbossart avatar Apr 17 '24 14:04 plbossart

sdw_slave_read_dpn

Correct. Port references are released in sdw_slave_read_dpn (). No need to add put for these cases.

vijendarmukunda avatar Apr 17 '24 14:04 vijendarmukunda