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

[Meghana Malladi]

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