| Message ID | 3727de04-7993-4b81-80c0-adb40b847307@web.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | crypto: sun8i-ce-hash - Refine exception handling in sun8i_ce_hash_run() | expand |
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 >
>> 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
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 > > > >
> 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
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,
> 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
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; }