[PATCH] crypto: sun8i-ce-hash - Refine exception handling in sun8i_ce_hash_run()

[PATCH] crypto: sun8i-ce-hash - Refine exception handling in sun8i_ce_hash_run()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 9 Apr 2025 13:26:55 +0200

Two if branches contained duplicate source code.
Thus avoid the specification of repeated error code assignments by using
additional labels instead.

This issue was transformed by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c
index ba13fb75c05d..7d31e190bb6a 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c
@@ -399,14 +399,14 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq)
 	}
 	if (len > 0) {
 		dev_err(ce->dev, "remaining len %d\n", len);
-		err = -EINVAL;
-		goto err_unmap_src;
+		goto e_inval_src;
 	}
 	addr_res = dma_map_single(ce->dev, result, digestsize, DMA_FROM_DEVICE);
 	cet->t_dst[0].addr = desc_addr_val_le32(ce, addr_res);
 	cet->t_dst[0].len = cpu_to_le32(digestsize / 4);
 	if (dma_mapping_error(ce->dev, addr_res)) {
 		dev_err(ce->dev, "DMA map dest\n");
+e_inval_src:
 		err = -EINVAL;
 		goto err_unmap_src;
 	}
@@ -428,16 +428,15 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq)
 		j = hash_pad(bf, 2 * bs, j, byte_count, false, bs);
 		break;
 	}
-	if (!j) {
-		err = -EINVAL;
-		goto err_unmap_result;
-	}
+	if (!j)
+		goto e_inval_result;

 	addr_pad = dma_map_single(ce->dev, buf, j * 4, DMA_TO_DEVICE);
 	cet->t_src[i].addr = desc_addr_val_le32(ce, addr_pad);
 	cet->t_src[i].len = cpu_to_le32(j);
 	if (dma_mapping_error(ce->dev, addr_pad)) {
 		dev_err(ce->dev, "DMA error on padding SG\n");
+e_inval_result:
 		err = -EINVAL;
 		goto err_unmap_result;
 	}
--
2.49.0

Re: [PATCH] crypto: sun8i-ce-hash - Refine exception handling in sun8i_ce_hash_run()

[Andre Przywara]

On Wed, 9 Apr 2025 13:43:39 +0200
Markus Elfring <Markus.Elfring@web.de> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 9 Apr 2025 13:26:55 +0200
> 
> Two if branches contained duplicate source code.
> Thus avoid the specification of repeated error code assignments by using
> additional labels instead.

Is that really useful? I think the current code reads easier, with the
usual pattern of setting the error code and the goto'ing out.
Now there is one rather opaque label it goes to, so a reader doesn't see
the error code immediately. And it really just saves one line per case
here. Plus the added danger that future changes might break this again.

And then there is the oddity that it jumps *into* an "if" branch, which
looks odd, I think typically we goto the end of the function, outside of
any other statements.

Cheers,
Andre

> This issue was transformed by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c
> index ba13fb75c05d..7d31e190bb6a 100644
> --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c
> +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c
> @@ -399,14 +399,14 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq)
>  	}
>  	if (len > 0) {
>  		dev_err(ce->dev, "remaining len %d\n", len);
> -		err = -EINVAL;
> -		goto err_unmap_src;
> +		goto e_inval_src;
>  	}
>  	addr_res = dma_map_single(ce->dev, result, digestsize, DMA_FROM_DEVICE);
>  	cet->t_dst[0].addr = desc_addr_val_le32(ce, addr_res);
>  	cet->t_dst[0].len = cpu_to_le32(digestsize / 4);
>  	if (dma_mapping_error(ce->dev, addr_res)) {
>  		dev_err(ce->dev, "DMA map dest\n");
> +e_inval_src:
>  		err = -EINVAL;
>  		goto err_unmap_src;
>  	}
> @@ -428,16 +428,15 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq)
>  		j = hash_pad(bf, 2 * bs, j, byte_count, false, bs);
>  		break;
>  	}
> -	if (!j) {
> -		err = -EINVAL;
> -		goto err_unmap_result;
> -	}
> +	if (!j)
> +		goto e_inval_result;
> 
>  	addr_pad = dma_map_single(ce->dev, buf, j * 4, DMA_TO_DEVICE);
>  	cet->t_src[i].addr = desc_addr_val_le32(ce, addr_pad);
>  	cet->t_src[i].len = cpu_to_le32(j);
>  	if (dma_mapping_error(ce->dev, addr_pad)) {
>  		dev_err(ce->dev, "DMA error on padding SG\n");
> +e_inval_result:
>  		err = -EINVAL;
>  		goto err_unmap_result;
>  	}
> --
> 2.49.0
> 

Re: crypto: sun8i-ce-hash - Refine exception handling in sun8i_ce_hash_run()

[Markus Elfring]

>> Two if branches contained duplicate source code.
>> Thus avoid the specification of repeated error code assignments by using
>> additional labels instead.
…
> Now there is one rather opaque label it goes to, so a reader doesn't see
> the error code immediately. And it really just saves one line per case
> here. …
I imagine that such a code refinement can occasionally matter.

Regards,
Markus

Re: [PATCH] crypto: sun8i-ce-hash - Refine exception handling in sun8i_ce_hash_run()

[Jernej Škrabec]

Dne sreda, 9. april 2025 ob 14:36:10 Srednjeevropski poletni čas je Andre Przywara napisal(a):
> On Wed, 9 Apr 2025 13:43:39 +0200
> Markus Elfring <Markus.Elfring@web.de> wrote:
> 
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Wed, 9 Apr 2025 13:26:55 +0200
> > 
> > Two if branches contained duplicate source code.
> > Thus avoid the specification of repeated error code assignments by using
> > additional labels instead.
> 
> Is that really useful? I think the current code reads easier, with the
> usual pattern of setting the error code and the goto'ing out.
> Now there is one rather opaque label it goes to, so a reader doesn't see
> the error code immediately. And it really just saves one line per case
> here. Plus the added danger that future changes might break this again.
> 
> And then there is the oddity that it jumps *into* an "if" branch, which
> looks odd, I think typically we goto the end of the function, outside of
> any other statements.

I'm not a fan of this patch either. As Andre said, current code is easier to
read and such optimizations are more for compiler to make than us.

Best regards,
Jernej

> 
> Cheers,
> Andre
> 
> > This issue was transformed by using the Coccinelle software.
> > 
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
> >  drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c
> > index ba13fb75c05d..7d31e190bb6a 100644
> > --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c
> > +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c
> > @@ -399,14 +399,14 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq)
> >  	}
> >  	if (len > 0) {
> >  		dev_err(ce->dev, "remaining len %d\n", len);
> > -		err = -EINVAL;
> > -		goto err_unmap_src;
> > +		goto e_inval_src;
> >  	}
> >  	addr_res = dma_map_single(ce->dev, result, digestsize, DMA_FROM_DEVICE);
> >  	cet->t_dst[0].addr = desc_addr_val_le32(ce, addr_res);
> >  	cet->t_dst[0].len = cpu_to_le32(digestsize / 4);
> >  	if (dma_mapping_error(ce->dev, addr_res)) {
> >  		dev_err(ce->dev, "DMA map dest\n");
> > +e_inval_src:
> >  		err = -EINVAL;
> >  		goto err_unmap_src;
> >  	}
> > @@ -428,16 +428,15 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq)
> >  		j = hash_pad(bf, 2 * bs, j, byte_count, false, bs);
> >  		break;
> >  	}
> > -	if (!j) {
> > -		err = -EINVAL;
> > -		goto err_unmap_result;
> > -	}
> > +	if (!j)
> > +		goto e_inval_result;
> > 
> >  	addr_pad = dma_map_single(ce->dev, buf, j * 4, DMA_TO_DEVICE);
> >  	cet->t_src[i].addr = desc_addr_val_le32(ce, addr_pad);
> >  	cet->t_src[i].len = cpu_to_le32(j);
> >  	if (dma_mapping_error(ce->dev, addr_pad)) {
> >  		dev_err(ce->dev, "DMA error on padding SG\n");
> > +e_inval_result:
> >  		err = -EINVAL;
> >  		goto err_unmap_result;
> >  	}
> > --
> > 2.49.0
> > 
> 
> 

Re: crypto: sun8i-ce-hash - Refine exception handling in sun8i_ce_hash_run()

[Markus Elfring]

> I'm not a fan of this patch either. As Andre said, current code is easier to
> read

I dare to propose a small source code adjustment according to another
software design option.


>      and such optimizations are more for compiler to make than us.
Do you know any compiler versions which would support the presented
transformation directly?

Regards,
Markus

Re: crypto: sun8i-ce-hash - Refine exception handling in sun8i_ce_hash_run()

[Julian Calaby]

Hi Markus,

On Wed, Apr 9, 2025 at 10:47 PM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> >> Two if branches contained duplicate source code.
> >> Thus avoid the specification of repeated error code assignments by using
> >> additional labels instead.
> …
> > Now there is one rather opaque label it goes to, so a reader doesn't see
> > the error code immediately. And it really just saves one line per case
> > here. …
> I imagine that such a code refinement can occasionally matter.

Just because you imagine that such a code refinement might matter,
doesn't mean it's actually useful.

1. this is making the code significantly less readable to save 1 line.
2. gotos into control blocks are weird at best and problematic and
confusing at worst. There's a reason why nobody writes code like this.
3. this sort of tail merging is something a compiler would apply
automatically when optimising for size and it can do a much better job
of this than you can.

I note that you said you did this using the Coccinelle software. Is
the semantic patch something you're trying to get upstream at part of
coccicheck? If so, could you please get that semantic patch merged
before posting these patches?

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: crypto: sun8i-ce-hash - Refine exception handling in sun8i_ce_hash_run()

[Markus Elfring]

> I note that you said you did this using the Coccinelle software. Is
> the semantic patch something you're trying to get upstream at part of
> coccicheck?

Not yet.

It might become more interesting to achieve wider applications of similar
source code analyses and transformations.

Several SmPL script variants were published which are also still waiting
on constructive review for coccicheck extensions.

Regards,
Markus