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@xxxxxx> wrote:
> 
> > From: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
> > 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@xxxxxxxxxxxxxxxxxxxxx>
> > ---
> >  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
> > 
> 
>