linux-kernel - Re: [PATCH v2] ALSA: ctxfi: Simplify dao_clear_{left,right}

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20241230181153.7a825fa7@dsl-u17-10>
Date: Mon, 30 Dec 2024 18:11:53 +0000
From: David Laight <david.laight.linux@...il.com>
To: Ethan Carter Edwards <ethan@...ancedwards.com>
Cc: tiwai@...e.de, tiwai@...e.com, perex@...ex.cz, arnd@...db.de,
 linux-sound@...r.kernel.org, linux-kernel@...r.kernel.org,
 ryan_richards@...ativelabs.com, wychay@....creative.com
Subject: Re: [PATCH v2] ALSA: ctxfi: Simplify dao_clear_{left,right}_input()
 functions

On Mon, 30 Dec 2024 15:32:58 +0000
Ethan Carter Edwards <ethan@...ancedwards.com> wrote:

> There was a lote of code duplication in the dao_clear_left_input() and
> dao_clear_right_input() functions. A new function, dao_clear_input(),
> was created and now the left and right functions call it instead of
> repeating themselves.
> 
> Link: https://lore.kernel.org/lkml/NyKCr2VHK_xCQDwNxFKKx2LVd2d_AC2f2j4eAvnD9uRPtb50i2AruCLOp6mHxsGiyYJ0Tgd3Z50Oy1JTi5gPhjd2WQM2skrv7asp3fLl8HU=@ethancedwards.com/
> 
> Signed-off-by: Ethan Carter Edwards <ethan@...ancedwards.com>
> Co-developed-by: David Laight <david.laight.linux@...il.com>
> ---
>  Changes in V2: Fixed formatting caused by email client. Sorry!
>  sound/pci/ctxfi/ctdaio.c | 50 ++++++++++++++--------------------------
>  1 file changed, 17 insertions(+), 33 deletions(-)
> 
> diff --git a/sound/pci/ctxfi/ctdaio.c b/sound/pci/ctxfi/ctdaio.c
> index 83aaf9441ef3..d46542d0b454 100644
> --- a/sound/pci/ctxfi/ctdaio.c
> +++ b/sound/pci/ctxfi/ctdaio.c
> @@ -211,52 +211,36 @@ static int dao_set_right_input(struct dao *dao, struct rsc *input)
>  	return 0;
>  }
>  
> -static int dao_clear_left_input(struct dao *dao)
> +static int dao_clear_input(struct dao *dao, unsigned int start, unsigned int end)
>  {
> -	struct imapper *entry;
> -	struct daio *daio = &dao->daio;
> -	int i;
> +	struct imapper *to_free = dao->imappers[start];
> +	unsigned int i;
>  
> -	if (!dao->imappers[0])
> +	if (!to_free)
>  		return 0;
> -
> -	entry = dao->imappers[0];
> -	dao->mgr->imap_delete(dao->mgr, entry);
> -	/* Program conjugate resources */
> -	for (i = 1; i < daio->rscl.msr; i++) {
> -		entry = dao->imappers[i];
> -		dao->mgr->imap_delete(dao->mgr, entry);
> +	for (i = start; i < end; i++) {
> +		dao->mgr->imap_delete(dao->mgr, dao->imappers[i]);
>  		dao->imappers[i] = NULL;
>  	}
>  
> -	kfree(dao->imappers[0]);
> -	dao->imappers[0] = NULL;
> -
> +	kfree(to_free);
>  	return 0;
>  }
>  
> -static int dao_clear_right_input(struct dao *dao)
> -{
> -	struct imapper *entry;
> -	struct daio *daio = &dao->daio;
> -	int i;
>  
> -	if (!dao->imappers[daio->rscl.msr])
> -		return 0;
> +static int dao_clear_left_input(struct dao *dao)
> +{
> +	u32 offset = dao->daio.rscl.msr;
>  
> -	entry = dao->imappers[daio->rscl.msr];
> -	dao->mgr->imap_delete(dao->mgr, entry);
> -	/* Program conjugate resources */
> -	for (i = 1; i < daio->rscr.msr; i++) {
> -		entry = dao->imappers[daio->rscl.msr + i];
> -		dao->mgr->imap_delete(dao->mgr, entry);
> -		dao->imappers[daio->rscl.msr + i] = NULL;
> -	}
> +	return dao_clear_input(dao, 0, 0 + offset);

I really wouldn't bother with the temporary variable.
It is also a 'length' not an offset and you can't decide on the type.
'unsigned int' is fine.

But I suspect it really is worth checking if that *imappers[] is needed at all.
(Rename and see where the build breaks!)
You might just need struct imappers *imappers_left and *imappers_right to hold
base of the kmalloc()ed block.
At that point the functions collapse down even more.

	David

> +}
>  
> -	kfree(dao->imappers[daio->rscl.msr]);
> -	dao->imappers[daio->rscl.msr] = NULL;
> +static int dao_clear_right_input(struct dao *dao)
> +{
> +	u32 start = dao->daio.rscl.msr;
> +	u32 offset = dao->daio.rscr.msr;
>  
> -	return 0;
> +	return dao_clear_input(dao, start, start + offset);
>  }
>  
>  static const struct dao_rsc_ops dao_ops = {