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
> >
>
>
