sound/pci/ctxfi/ctdaio.c: duplicate function removal question

sound/pci/ctxfi/ctdaio.c: duplicate function removal question

[Ethan Carter Edwards]

Hello all,

First of all, happy holidays. 

I was browsing the ctdaio.c code and I noticed a lot of 
duplicate code and functions, specifically:

dao_set_{right,left}_input and
dao_clear_{right,left}_input functions.

The functions are pretty much identical. They only 
differ in the side (left, right). What was the original
idea in doing this? Wouldn't it make more since to just
have an ENUM (left, right) as an argument that would 
determine the side and just reduce the function to 
dao_set_input and dao_clear_input. 

I would be more than happy to send in a patch doing
these changes, but before I did I wanted to ask if
there was a reason the code was written in this way.
I am pretty new to kernel development.

Thanks,
Ethan
--
Ethan Carter Edwards
CompTIA A+, Security+, and ISC2 (CC)
Ham Radio: AE4CE
Website: https://ethancedwards.com

Re: sound/pci/ctxfi/ctdaio.c: duplicate function removal question

[Takashi Iwai]

On Wed, 25 Dec 2024 00:16:17 +0100,
Ethan Carter Edwards wrote:
> 
> Hello all,
> 
> First of all, happy holidays. 
> 
> I was browsing the ctdaio.c code and I noticed a lot of 
> duplicate code and functions, specifically:
> 
> dao_set_{right,left}_input and
> dao_clear_{right,left}_input functions.
> 
> The functions are pretty much identical. They only 
> differ in the side (left, right). What was the original
> idea in doing this? Wouldn't it make more since to just
> have an ENUM (left, right) as an argument that would 
> determine the side and just reduce the function to 
> dao_set_input and dao_clear_input. 
> 
> I would be more than happy to send in a patch doing
> these changes, but before I did I wanted to ask if
> there was a reason the code was written in this way.
> I am pretty new to kernel development.

Yes, the code simplification would be appreciated.
You can create a common function and call with the left/right
argument from each callback or change the call pattern from ctatc.c.


thanks,

Takashi

Re: sound/pci/ctxfi/ctdaio.c: duplicate function removal question

[David Laight]

On Tue, 24 Dec 2024 23:16:17 +0000
Ethan Carter Edwards <ethan@ethancedwards.com> wrote:

> Hello all,
> 
> First of all, happy holidays. 
> 
> I was browsing the ctdaio.c code and I noticed a lot of 
> duplicate code and functions, specifically:
> 
> dao_set_{right,left}_input and
> dao_clear_{right,left}_input functions.
> 
> The functions are pretty much identical. They only 
> differ in the side (left, right). What was the original
> idea in doing this? Wouldn't it make more since to just
> have an ENUM (left, right) as an argument that would 
> determine the side and just reduce the function to 
> dao_set_input and dao_clear_input.

Hmmm... you'd have a lot of conditionals inside the function.

They also look like a memory leak just waiting to happen.
I guess that an earlier implementation used a separate kmalloc()
for each imappers[].

Why is imappers[] an array of pointers not an array of the items?
Each is just 8 bytes plus a 'list_head' (2 pointers?).

	David

> 
> I would be more than happy to send in a patch doing
> these changes, but before I did I wanted to ask if
> there was a reason the code was written in this way.
> I am pretty new to kernel development.
> 
> Thanks,
> Ethan
> --
> Ethan Carter Edwards
> CompTIA A+, Security+, and ISC2 (CC)
> Ham Radio: AE4CE
> Website: https://ethancedwards.com
> 
> 

Re: sound/pci/ctxfi/ctdaio.c: duplicate function removal question

[Takashi Iwai]

On Sun, 29 Dec 2024 10:02:28 +0100,
David Laight wrote:
> 
> On Tue, 24 Dec 2024 23:16:17 +0000
> Ethan Carter Edwards <ethan@ethancedwards.com> wrote:
> 
> > Hello all,
> > 
> > First of all, happy holidays. 
> > 
> > I was browsing the ctdaio.c code and I noticed a lot of 
> > duplicate code and functions, specifically:
> > 
> > dao_set_{right,left}_input and
> > dao_clear_{right,left}_input functions.
> > 
> > The functions are pretty much identical. They only 
> > differ in the side (left, right). What was the original
> > idea in doing this? Wouldn't it make more since to just
> > have an ENUM (left, right) as an argument that would 
> > determine the side and just reduce the function to 
> > dao_set_input and dao_clear_input.
> 
> Hmmm... you'd have a lot of conditionals inside the function.
> 
> They also look like a memory leak just waiting to happen.
> I guess that an earlier implementation used a separate kmalloc()
> for each imappers[].
> 
> Why is imappers[] an array of pointers not an array of the items?
> Each is just 8 bytes plus a 'list_head' (2 pointers?).

AFAIUC, it's a setup of a chained element, so no leak there as of
now.

I agree with that the code is unnecessarily complex, though.


thanks,

Takashi

Re: sound/pci/ctxfi/ctdaio.c: duplicate function removal question

[David Laight]

On Sun, 29 Dec 2024 10:10:09 +0100
Takashi Iwai <tiwai@suse.de> wrote:

> On Sun, 29 Dec 2024 10:02:28 +0100,
> David Laight wrote:
> > 
> > On Tue, 24 Dec 2024 23:16:17 +0000
> > Ethan Carter Edwards <ethan@ethancedwards.com> wrote:
> >   
> > > Hello all,
> > > 
> > > First of all, happy holidays. 
> > > 
> > > I was browsing the ctdaio.c code and I noticed a lot of 
> > > duplicate code and functions, specifically:
> > > 
> > > dao_set_{right,left}_input and
> > > dao_clear_{right,left}_input functions.
> > > 
> > > The functions are pretty much identical. They only 
> > > differ in the side (left, right). What was the original
> > > idea in doing this? Wouldn't it make more since to just
> > > have an ENUM (left, right) as an argument that would 
> > > determine the side and just reduce the function to 
> > > dao_set_input and dao_clear_input.  
> > 
> > Hmmm... you'd have a lot of conditionals inside the function.
> > 
> > They also look like a memory leak just waiting to happen.
> > I guess that an earlier implementation used a separate kmalloc()
> > for each imappers[].
> > 
> > Why is imappers[] an array of pointers not an array of the items?
> > Each is just 8 bytes plus a 'list_head' (2 pointers?).  
> 
> AFAIUC, it's a setup of a chained element, so no leak there as of
> now.

Unless someone calls the functions in the wrong order.
And that seems to be outside the control of this code.

	David

> 
> I agree with that the code is unnecessarily complex, though.
> 
> 
> thanks,
> 
> Takashi

Re: sound/pci/ctxfi/ctdaio.c: duplicate function removal question

[Takashi Iwai]

On Sun, 29 Dec 2024 11:57:22 +0100,
David Laight wrote:
> 
> On Sun, 29 Dec 2024 10:10:09 +0100
> Takashi Iwai <tiwai@suse.de> wrote:
> 
> > On Sun, 29 Dec 2024 10:02:28 +0100,
> > David Laight wrote:
> > > 
> > > On Tue, 24 Dec 2024 23:16:17 +0000
> > > Ethan Carter Edwards <ethan@ethancedwards.com> wrote:
> > >   
> > > > Hello all,
> > > > 
> > > > First of all, happy holidays. 
> > > > 
> > > > I was browsing the ctdaio.c code and I noticed a lot of 
> > > > duplicate code and functions, specifically:
> > > > 
> > > > dao_set_{right,left}_input and
> > > > dao_clear_{right,left}_input functions.
> > > > 
> > > > The functions are pretty much identical. They only 
> > > > differ in the side (left, right). What was the original
> > > > idea in doing this? Wouldn't it make more since to just
> > > > have an ENUM (left, right) as an argument that would 
> > > > determine the side and just reduce the function to 
> > > > dao_set_input and dao_clear_input.  
> > > 
> > > Hmmm... you'd have a lot of conditionals inside the function.
> > > 
> > > They also look like a memory leak just waiting to happen.
> > > I guess that an earlier implementation used a separate kmalloc()
> > > for each imappers[].
> > > 
> > > Why is imappers[] an array of pointers not an array of the items?
> > > Each is just 8 bytes plus a 'list_head' (2 pointers?).  
> > 
> > AFAIUC, it's a setup of a chained element, so no leak there as of
> > now.
> 
> Unless someone calls the functions in the wrong order.
> And that seems to be outside the control of this code.

Could you elaborate?  The chain element gets released also by the
destructor, dao_rsc_uninit().  And this is no exported function to be
used by other drivers, but an internal one only for ctxfi driver.


thanks,

Takashi

Re: sound/pci/ctxfi/ctdaio.c: duplicate function removal question

[David Laight]

On Sun, 29 Dec 2024 13:03:56 +0100
Takashi Iwai <tiwai@suse.de> wrote:

> On Sun, 29 Dec 2024 11:57:22 +0100,
> David Laight wrote:
> > 
> > On Sun, 29 Dec 2024 10:10:09 +0100
> > Takashi Iwai <tiwai@suse.de> wrote:
> >   
> > > On Sun, 29 Dec 2024 10:02:28 +0100,
> > > David Laight wrote:  
> > > > 
> > > > On Tue, 24 Dec 2024 23:16:17 +0000
> > > > Ethan Carter Edwards <ethan@ethancedwards.com> wrote:
> > > >     
> > > > > Hello all,
> > > > > 
> > > > > First of all, happy holidays. 
> > > > > 
> > > > > I was browsing the ctdaio.c code and I noticed a lot of 
> > > > > duplicate code and functions, specifically:
> > > > > 
> > > > > dao_set_{right,left}_input and
> > > > > dao_clear_{right,left}_input functions.
> > > > > 
> > > > > The functions are pretty much identical. They only 
> > > > > differ in the side (left, right). What was the original
> > > > > idea in doing this? Wouldn't it make more since to just
> > > > > have an ENUM (left, right) as an argument that would 
> > > > > determine the side and just reduce the function to 
> > > > > dao_set_input and dao_clear_input.    
> > > > 
> > > > Hmmm... you'd have a lot of conditionals inside the function.
> > > > 
> > > > They also look like a memory leak just waiting to happen.
> > > > I guess that an earlier implementation used a separate kmalloc()
> > > > for each imappers[].
> > > > 
> > > > Why is imappers[] an array of pointers not an array of the items?
> > > > Each is just 8 bytes plus a 'list_head' (2 pointers?).    
> > > 
> > > AFAIUC, it's a setup of a chained element, so no leak there as of
> > > now.  
> > 
> > Unless someone calls the functions in the wrong order.
> > And that seems to be outside the control of this code.  
> 
> Could you elaborate?  The chain element gets released also by the
> destructor, dao_rsc_uninit().  And this is no exported function to be
> used by other drivers, but an internal one only for ctxfi driver.

The code is just too horrid :-)
I missed that when dao_set_right_input() calls dao->ops->clear_right_input()
it is just calling the static function just below.

I think the two 'clear' functions could be combined as:

static int dao_clear_input(struct dao *dao, unsigned int start, unsigned int end)
{
	struct imapper *to_free = dao->imappers[start];
	unsigned int i;

	if (!to_free)
		return 0;
	for (i = start; i < end; i++) {
		dao->mgr->imap_delete(dao->mgr, dao->imappers[i]);
		dao->imappers[i] = NULL;
	}

	kfree(to_free);
	return 0;
}

static int dao_clear_left_input(struct dao *dao)
{
	return dao_clear_input(dao, 0, dao->rscl.msr);
}

static int dao_clear_right_input(struct dao *dao)
{
	return dao_clear_input(dao, dao->rscl.msr, dao->rscl.msr + dao->rscr.msr);
}

Although I'm missing any other code that actually indexes that imappers[] array.
So I suspect only [0] and [dao->rscl.msr] are ever used??

Stanger things happen in the set functions.

	David

> 
> 
> thanks,
> 
> Takashi

Re: sound/pci/ctxfi/ctdaio.c: duplicate function removal question

[Ethan Carter Edwards]

On 24/12/29 01:34PM, David Laight wrote:
> On Sun, 29 Dec 2024 13:03:56 +0100
> Takashi Iwai <tiwai@suse.de> wrote:
> 
> > On Sun, 29 Dec 2024 11:57:22 +0100,
> > David Laight wrote:
> > >
> > > On Sun, 29 Dec 2024 10:10:09 +0100
> > > Takashi Iwai <tiwai@suse.de> wrote:
> > >
> > > > On Sun, 29 Dec 2024 10:02:28 +0100,
> > > > David Laight wrote:
> > > > >
> > > > > On Tue, 24 Dec 2024 23:16:17 +0000
> > > > > Ethan Carter Edwards <ethan@ethancedwards.com> wrote:
> > > > >
> > > > > > Hello all,
> > > > > >
> > > > > > First of all, happy holidays.
> > > > > >
> > > > > > I was browsing the ctdaio.c code and I noticed a lot of
> > > > > > duplicate code and functions, specifically:
> > > > > >
> > > > > > dao_set_{right,left}_input and
> > > > > > dao_clear_{right,left}_input functions.
> > > > > >
> > > > > > The functions are pretty much identical. They only
> > > > > > differ in the side (left, right). What was the original
> > > > > > idea in doing this? Wouldn't it make more since to just
> > > > > > have an ENUM (left, right) as an argument that would
> > > > > > determine the side and just reduce the function to
> > > > > > dao_set_input and dao_clear_input.
> > > > >
> > > > > Hmmm... you'd have a lot of conditionals inside the function.
> > > > >
> > > > > They also look like a memory leak just waiting to happen.
> > > > > I guess that an earlier implementation used a separate kmalloc()
> > > > > for each imappers[].
> > > > >
> > > > > Why is imappers[] an array of pointers not an array of the items?
> > > > > Each is just 8 bytes plus a 'list_head' (2 pointers?).
> > > >
> > > > AFAIUC, it's a setup of a chained element, so no leak there as of
> > > > now.
> > >
> > > Unless someone calls the functions in the wrong order.
> > > And that seems to be outside the control of this code.
> >
> > Could you elaborate?  The chain element gets released also by the
> > destructor, dao_rsc_uninit().  And this is no exported function to be
> > used by other drivers, but an internal one only for ctxfi driver.
> 
> The code is just too horrid :-)
> I missed that when dao_set_right_input() calls dao->ops->clear_right_input()
> it is just calling the static function just below.
> 
> I think the two 'clear' functions could be combined as:
> 
> static int dao_clear_input(struct dao *dao, unsigned int start, unsigned int end)
> {
> 	struct imapper *to_free = dao->imappers[start];
> 	unsigned int i;
> 
> 	if (!to_free)
> 		return 0;
> 	for (i = start; i < end; i++) {
> 		dao->mgr->imap_delete(dao->mgr, dao->imappers[i]);
> 		dao->imappers[i] = NULL;
> 	}
> 
> 	kfree(to_free);
> 	return 0;
> }
> 
> static int dao_clear_left_input(struct dao *dao)
> {
> 	return dao_clear_input(dao, 0, dao->rscl.msr);
> }
> 
> static int dao_clear_right_input(struct dao *dao)
> {
> 	return dao_clear_input(dao, dao->rscl.msr, dao->rscl.msr + dao->rscr.msr);
> }
> 
> Although I'm missing any other code that actually indexes that imappers[] array.
> So I suspect only [0] and [dao->rscl.msr] are ever used??
> 
> Stanger things happen in the set functions.
> 
> 	David
> 

Hey David,

I used large parts of this email (with some modifications) to submit my
patch [1]. Do I have your permission to add Co-developed-by: and
Signed-off-by: to it? I want to give you proper credit and attribution
and I figured I should ask before putting your name on something. 

Thanks

[1] https://lore.kernel.org/lkml/rwfdemj7ic7rnxnycnv6sxixtfmyqnoi7u6wgulzpsjf7qm6oh@xwkyteg6hak4/T/#t
-- 
Ethan Carter Edwards
CompTIA A+, Security+, and ISC2 (CC)
Ham Radio: AE4CE
Website: https://ethancedwards.com
GPG: 2E51 F618 39D1 FA94 7A73  00C2 34C0 4305 D581 DBFE