[PATCH net v4 1/2] net: ti: icssg-prueth: Fix firmware load sequence.

[Dan Carpenter]

On Wed, Dec 11, 2024 at 07:29:40PM +0530, Meghana Malladi wrote:
> -static int prueth_emac_start(struct prueth *prueth, struct prueth_emac *emac)
> +static int prueth_emac_start(struct prueth *prueth, int slice)
>  {
>  	struct icssg_firmwares *firmwares;
>  	struct device *dev = prueth->dev;
> -	int slice, ret;
> +	int ret;
>  
>  	if (prueth->is_switch_mode)
>  		firmwares = icssg_switch_firmwares;
> @@ -177,16 +177,6 @@ static int prueth_emac_start(struct prueth *prueth, struct prueth_emac *emac)
>  	else
>  		firmwares = icssg_emac_firmwares;
>  
> -	slice = prueth_emac_slice(emac);
> -	if (slice < 0) {
> -		netdev_err(emac->ndev, "invalid port\n");
> -		return -EINVAL;
> -	}
> -
> -	ret = icssg_config(prueth, emac, slice);
> -	if (ret)
> -		return ret;
> -
>  	ret = rproc_set_firmware(prueth->pru[slice], firmwares[slice].pru);
>  	ret = rproc_boot(prueth->pru[slice]);

This isn't introduced by this patch but eventually Colin King is going to
get annoyed with you for setting ret twice in a row.

>  	if (ret) {
> @@ -208,7 +198,6 @@ static int prueth_emac_start(struct prueth *prueth, struct prueth_emac *emac)
>  		goto halt_rtu;
>  	}
>  
> -	emac->fw_running = 1;
>  	return 0;
>  
>  halt_rtu:
> @@ -220,6 +209,78 @@ static int prueth_emac_start(struct prueth *prueth, struct prueth_emac *emac)
>  	return ret;
>  }
>  
> +static int prueth_emac_common_start(struct prueth *prueth)
> +{
> +	struct prueth_emac *emac;
> +	int ret = 0;
> +	int slice;
> +
> +	if (!prueth->emac[ICSS_SLICE0] && !prueth->emac[ICSS_SLICE1])
> +		return -EINVAL;
> +
> +	/* clear SMEM and MSMC settings for all slices */
> +	memset_io(prueth->msmcram.va, 0, prueth->msmcram.size);
> +	memset_io(prueth->shram.va, 0, ICSSG_CONFIG_OFFSET_SLICE1 * PRUETH_NUM_MACS);
> +
> +	icssg_class_default(prueth->miig_rt, ICSS_SLICE0, 0, false);
> +	icssg_class_default(prueth->miig_rt, ICSS_SLICE1, 0, false);
> +
> +	if (prueth->is_switch_mode || prueth->is_hsr_offload_mode)
> +		icssg_init_fw_offload_mode(prueth);
> +	else
> +		icssg_init_emac_mode(prueth);
> +
> +	for (slice = 0; slice < PRUETH_NUM_MACS; slice++) {
> +		emac = prueth->emac[slice];
> +		if (emac) {
> +			ret |= icssg_config(prueth, emac, slice);
> +			if (ret)
> +				return ret;

Here we return directly.

> +		}
> +		ret |= prueth_emac_start(prueth, slice);

Here we continue.  Generally, I would expect there to be some clean up
on this error path like this:

		ret = prueth_emac_start(prueth, slice);
		if (ret)
			goto unwind_slices;

	...

	return 0;

unwind_slices:
	while (--slice >= 0)
		prueth_emac_stop(prueth, slice);

	return ret;

I dread to see how the cleanup is handled on this path...

Ok.  I've looked at it and, nope, it doesn't work.  This is freed in
prueth_emac_common_stop() but partial allocations are not freed.
Also the prueth_emac_stop() is open coded as three calls to
rproc_shutdown() which is ugly.

I've written a blog which describes a system for writing error
handling code.  If each function cleans up after itself by freeing
its own partial allocations then you don't need to have a variable
like "prueth->prus_running = 1;" to track how far the allocation
process went before failing.
https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/

regards,
dan carpenter