Re: [PATCH 2/2] reset: toshiba: introduce a TC9564 SoC reset driver

[Alex Elder <elder@riscstar.com>]

On 10/12/26 11:54 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] reset: tc9564: hardware state initialized after reset controller registration
> - [High] reset: tc9564: active consumers forcibly reset during driver removal
> --
> 
> commit 504236856b96e78b9064b4d58c3ef7ba166341e3
> Author: Alex Elder <elder@riscstar.com>
> 
> reset: toshiba: introduce a TC9564 SoC reset driver
> 
> Defines a new platform driver that manages reset signals implemented by the
> Toshiba TC9564 SoC. The state of the resets is controlled via MMIO to a
> region managed by a system controller.
> 
>> diff --git a/drivers/reset/reset-tc9564.c b/drivers/reset/reset-tc9564.c
>> new file mode 100644
>> index 0000000000000..0243c2e0efd54
>> --- /dev/null
>> +++ b/drivers/reset/reset-tc9564.c
> 
> [ ... ]
> 
>> +static int tc9564_reset_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct tc9564_resets *resets;
>> +
>> +	if (!dev_of_node(dev))
>> +		return dev_err_probe(dev, -EINVAL, "no devicetree node\n");
>> +
>> +	resets = tc9564_reset_init(dev);
>> +	if (IS_ERR(resets))
>> +		return dev_err_probe(dev, PTR_ERR(resets),
>> +				     "failed to initialize resets\n");
>> +
>> +	/* Force all resets to be initially asserted */
>> +	tc9564_reset_assert_all(resets);
> 
> [Severity: High]
> Does this cause a race condition with consumer drivers?

I hadn't thought of that, but yes, this is a good point.
We would like to ensure this is the initial state, but I will
rework this so that all defined resets will be asserted
initially by "directly" writing registers through the regmap,
rather than doing so using the reset API.

> 
> tc9564_reset_init() globally registers the reset controller via
> devm_reset_controller_register(). Once registered, consumers can
> immediately probe and deassert their reset lines.
> 
> If a consumer driver deasserts its reset right after registration,
> will this subsequent call to tc9564_reset_assert_all() overwrite the
> consumer's hardware state and unexpectedly yank running hardware back
> into reset?
> 
>> +
>> +	platform_set_drvdata(pdev, resets);
>> +
>> +	return 0;
>> +}
>> +
>> +static void tc9564_reset_remove(struct platform_device *pdev)
>> +{
>> +	struct tc9564_resets *resets = platform_get_drvdata(pdev);
>> +
>> +	/* Leave all resets asserted when done */
>> +	tc9564_reset_assert_all(resets);
> 
> [Severity: High]
> Does this unexpectedly assert resets for active consumers?

I suppose that's possible, and it's easy enough to simply
omit this remove callback (and the assert-all call).

I will adjust this for version 2.

					-Alex

> 
> Since the reset controller is registered using devm_reset_controller_register(),
> the devres subsystem will unregister the controller only after this remove
> callback completes.
> 
> This means the controller is still active and consumers might be actively
> using their deasserted reset lines when tc9564_reset_assert_all() is called
> here. Should the driver coordinate with consumers or unregister itself first
> before forcing all lines into reset?
> 
> [ ... ]
>