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

[Meghana Malladi <m-malladi@ti.com>]

On 11/12/24 21:16, Dan Carpenter wrote:
> 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
> ZjQcmQRYFpfptBannerStart
> This message was sent from outside of Texas Instruments.
> Do not click links or open attachments unless you recognize the source 
> of this email and know the content is safe.
> Report Suspicious
> <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/G3vK!uldqfRcOdo0RqyXEHNnjPxku43QuA2sRmrlczDVj-denyMX3qWPEeHokm6IS-fNmWZGSvK3Wn7nSFeNotanVMDOTlZZjZ8Ausf9AkMk$>
> ZjQcmQRYFpfptBannerEnd
> 
> 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.
> 

Yeah ok, I will fix this as part of this patch.

>>  	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://urldefense.com/v3/__https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/__;!!G3vK!T5VCna8tMLVZNSL49zSwJOQBnoAQEa2xqXUUsIYY78CYm5mEH2wAdMX9CDEfMHXWsjTn0sG4mwKevVIOrgfAuQ$  <https://urldefense.com/v3/__https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/__;!!G3vK!T5VCna8tMLVZNSL49zSwJOQBnoAQEa2xqXUUsIYY78CYm5mEH2wAdMX9CDEfMHXWsjTn0sG4mwKevVIOrgfAuQ$>
> 

I agree that current error handling is all over the place. But I wasn't 
sure what would be the cleanest approach here. Thanks for sharing the 
blog, I have looked into it and looks very promising. I will try this 
approach and get back to you.

thanks & regards,
Meghana Malladi.

> regards,
> dan carpenter
>