Newbie idiotic questions.

Newbie idiotic questions.

[Bill Pringlemeir]

I have been looking at the emu10k1 driver and I had a few questions
about general idioms used there.

In a line like this,

[main.c, line 175]

	for (count = 0; count < sizeof(card->digmix) / sizeof(card->digmix[0]); count++) {

Isn't there some sort of `ALEN' macro available, or is this
considered to muddy things by using a macro?

[main.c, line 223]
	if ((card->mpuout = kmalloc(sizeof(struct emu10k1_mpuout), GFP_KERNEL))

Why is the struct type referenced for the allocation size?  Why not,

	if ((card->mpuout = kmalloc(sizeof(card->mpuout), GFP_KERNEL))

This seems to get the size for the actual object being allocated.

[cardmi.c, line 42]

static struct {
	int (*Fn) (struct emu10k1_mpuin *, u8);
} midistatefn[] = {
...

Why aren't all the gobs of constant data in this driver declared as
constant?  Do it give a performance advantage by having the data in a
different MMU section and better cache effects or something?

Thanks for any helpful pointers.  I did read the FAQ.  I am just
wonder if I would get screamed at for changing things like this and
why... so I will probably get yelled at for suggesting them anyway,
but at least I won't have went through the effort.  Now that I have
pointed that out, the will probably irk people even more...

regards,
Bill Pringlemeir.

Re: Newbie idiotic questions.

[Jeff Garzik]

Bill Pringlemeir wrote:
> [main.c, line 175]
> 
>         for (count = 0; count < sizeof(card->digmix) / sizeof(card->digmix[0]); count++) {
> 
> Isn't there some sort of `ALEN' macro available, or is this
> considered to muddy things by using a macro?

Yes, we have array size

> [main.c, line 223]
>         if ((card->mpuout = kmalloc(sizeof(struct emu10k1_mpuout), GFP_KERNEL))
> 
> Why is the struct type referenced for the allocation size?  Why not,
> 
>         if ((card->mpuout = kmalloc(sizeof(card->mpuout), GFP_KERNEL))

because then you would be allocating the size of a pointer, not the size
of a structure


> Why aren't all the gobs of constant data in this driver declared as
> constant?  Do it give a performance advantage by having the data in a
> different MMU section and better cache effects or something?

Marking data const is usually a good idea.

	Jeff


-- 
Jeff Garzik      | Andre the Giant has a posse.
Building 1024    |
MandrakeSoft     |

Re: Newbie idiotic questions.

[Richard B. Johnson]

On 16 Jun 2001, Bill Pringlemeir wrote:

> 
> I have been looking at the emu10k1 driver and I had a few questions
> about general idioms used there.
> 
> In a line like this,
> 
> [main.c, line 175]
> 
> 	for (count = 0; count < sizeof(card->digmix) / sizeof(card->digmix[0]); count++) {
> 
> Isn't there some sort of `ALEN' macro available, or is this
> considered to muddy things by using a macro?

Note that code in drivers range from very nice to awful, and every level
in between.

I generally use a macro called ArraySize(), defined up-front.

> 
> [main.c, line 223]
> 	if ((card->mpuout = kmalloc(sizeof(struct emu10k1_mpuout), GFP_KERNEL))
> 
> Why is the struct type referenced for the allocation size?  Why not,
> 
> 	if ((card->mpuout = kmalloc(sizeof(card->mpuout), GFP_KERNEL))
> 
> This seems to get the size for the actual object being allocated.
> 

Again, you are correct. However, you may not know the history of the
driver. Perhaps at one time the above statement was correct.

> [cardmi.c, line 42]
> 
> static struct {
> 	int (*Fn) (struct emu10k1_mpuin *, u8);
> } midistatefn[] = {
> ...
> 

[...snipped....]



> 
> regards,
> Bill Pringlemeir.


What you should do is supply a patch. Call it a "general cleanup". Send
it first to the maintainer of the driver. If this doesn't work, send it
to linux-kernel. Test your patch well because you may fix something that
breaks something else. 


Cheers,
Dick Johnson

Penguin : Linux version 2.4.1 on an i686 machine (799.53 BogoMips).

"Memory is like gasoline. You use it up when you are running. Of
course you get it all back when you reboot..."; Actual explanation
obtained from the Micro$oft help desk.

Re: Newbie idiotic questions.

[Arnaldo Carvalho de Melo]

Em Sat, Jun 16, 2001 at 09:35:34PM -0400, Richard B. Johnson escreveu:
> > [main.c, line 223]
> > 	if ((card->mpuout = kmalloc(sizeof(struct emu10k1_mpuout), GFP_KERNEL))
> > 
> > Why is the struct type referenced for the allocation size?  Why not,
> > 
> > 	if ((card->mpuout = kmalloc(sizeof(card->mpuout), GFP_KERNEL))
> > 
> > This seems to get the size for the actual object being allocated.
> > 
> 
> Again, you are correct. However, you may not know the history of the
> driver. Perhaps at one time the above statement was correct.

yes, and in fact this should be one of the entries in the kernel Janitor's
TODO list, please take a look at http://bazar.conectiva.com.br/~acme/TODO
and consider submitting some more things to cleanup so that people wanting
to start hacking the kernel can have some easy starting points. Also please
consider reading http://kernel-janitor.sourceforge.net

- Arnaldo

Re: Newbie idiotic questions.

[rjd]

Hi,

Bill Pringlemeir wrote:
> 
> I have been looking at the emu10k1 driver and I had a few questions
> about general idioms used there.

Warning I've not looked at that particular driver and would concider
myself a Linux kernel newbie. 20 years kernel hacking but only 9
months Linux with two drivers under my belt. One of which I'm now
trying to get into the standard tree.


> [main.c, line 175]
> 
> 	for (count = 0; count < sizeof(card->digmix) / sizeof(card->digmix[0]); count++) {
> 
> Isn't there some sort of `ALEN' macro available, or is this
> considered to muddy things by using a macro?

Some writers are good and some not so good. Personally I'd prefer an
ArrayLen style macro but some prefer to see it long hand. My usual style
would be to use the array size #def used to define the array. I'd shoot
whoever wrote digmix[9 * 6 * 2];  OK so I just peeked at the source :-)

I might be tempted to tidy this up whilst I was adding something
significant to the driver but not for the sake of it. If I tidied that
one up I'd have to change all the other occurances in a consistent manner,
seem like a lot of work for little or no gain.


> [main.c, line 223]
> 	if ((card->mpuout = kmalloc(sizeof(struct emu10k1_mpuout), GFP_KERNEL))
> 
> Why is the struct type referenced for the allocation size?  Why not,
> 
> 	if ((card->mpuout = kmalloc(sizeof(card->mpuout), GFP_KERNEL))
> 
> This seems to get the size for the actual object being allocated.

I prefer the struct construct and this also demonstrates why it's best
not to fiddle just for the sake of it. You've changed the allocation
from the size of the struct into the size of a pointer.


> [cardmi.c, line 42]
> 
> static struct {
> 	int (*Fn) (struct emu10k1_mpuin *, u8);
> } midistatefn[] = {
> ...
> 
> Why aren't all the gobs of constant data in this driver declared as
> constant?  Do it give a performance advantage by having the data in a
> different MMU section and better cache effects or something?

const is better but frequently forgotten :-(


> Thanks for any helpful pointers.  I did read the FAQ.  I am just
> wonder if I would get screamed at for changing things like this and
> why... so I will probably get yelled at for suggesting them anyway,
> but at least I won't have went through the effort.  Now that I have
> pointed that out, the will probably irk people even more...

Perhaps the biggest one is when making changes follow the style of the
existing file. Style changes in midstream only add to the confusion.
Don't change layout rules or methods of doing things unless you are
prepared to update the entire module and justify the changes to the
original module author who has every right to be highly critical.



-- 
        Bob Dunlop                      FarSite Communications
        rjd@xyzzy.clara.co.uk           bob.dunlop@farsite.co.uk
        www.xyzzy.clara.co.uk           www.farsite.co.uk

Re: Newbie idiotic questions.

[Daniel Phillips]

On Sunday 17 June 2001 03:32, Jeff Garzik wrote:
> Bill Pringlemeir wrote:
> > Why is the struct type referenced for the allocation size?  Why not,
> >
> >         if ((card->mpuout = kmalloc(sizeof(card->mpuout), GFP_KERNEL))
>
> because then you would be allocating the size of a pointer, not the size
> of a structure

Whoops Jeff, you didn't have your coffee yet:

	struct foo { int a; int b; };
	struct { struct foo foo; } *foobar;

	int main (void)
	{
		printf("%i\n", sizeof(struct foo));
		printf("%i\n", sizeof(foobar->foo));
		printf("%i\n", sizeof(&foobar->foo));
	}

Prints:

	8
	8
	4
--
Daniel

Re: Newbie idiotic questions.

[rjd]

Daniel Phillips wrote:
> 
> > because then you would be allocating the size of a pointer, not the size
> > of a structure
> 
> Whoops Jeff, you didn't have your coffee yet:

Whoops yourself. The following patch brings your example into line with
the driver code. mpuout is a pointer to a structure not the structure itself
as the malloc assignment clearly indicates.

*** x.c-orig	Sun Jun 17 13:19:33 2001
--- x.c	Sun Jun 17 13:19:42 2001
***************
*** 2,8 ****
  
  
  struct foo { int a; int b; };
! struct { struct foo foo; } *foobar;
  
  int main (void)
  {
--- 2,8 ----
  
  
  struct foo { int a; int b; };
! struct { struct foo *foo; } *foobar;
  
  int main (void)
  {


Now Prints:

	8
	4
	4


-- 
        Bob Dunlop                      FarSite Communications
        rjd@xyzzy.clara.co.uk           bob.dunlop@farsite.co.uk
        www.xyzzy.clara.co.uk           www.farsite.co.uk

Re: Newbie idiotic questions.

[Daniel Phillips]

On Sunday 17 June 2001 14:27, rjd@xyzzy.clara.co.uk wrote:
> Daniel Phillips wrote:
> > > because then you would be allocating the size of a pointer, not the
> > > size of a structure
> >
> > Whoops Jeff, you didn't have your coffee yet:
>
> Whoops yourself. The following patch brings your example into line with
> the driver code. mpuout is a pointer to a structure not the structure
> itself as the malloc assignment clearly indicates.

Yep, the only thing left to resolve is whether Jeff had coffee or not. ;-)

-	if ((card->mpuout = kmalloc(sizeof(struct emu10k1_mpuout), GFP_KERNEL))
+	if ((card->mpuout = kmalloc(sizeof(*card->mpuout), GFP_KERNEL))

--
Daniel

Re: Newbie idiotic questions.

[Jeff Garzik]

Daniel Phillips wrote:
> Yep, the only thing left to resolve is whether Jeff had coffee or not. ;-)
> 
> -       if ((card->mpuout = kmalloc(sizeof(struct emu10k1_mpuout), GFP_KERNEL))
> +       if ((card->mpuout = kmalloc(sizeof(*card->mpuout), GFP_KERNEL))

Yeah, this is fine.  The original posted omitted the '*' which was not
fine :)

-- 
Jeff Garzik      | Andre the Giant has a posse.
Building 1024    |
MandrakeSoft     |

Re: Newbie idiotic questions.

[David Flynn]

> Daniel Phillips wrote:
> > Yep, the only thing left to resolve is whether Jeff had coffee or not.
;-)
> >
> > -       if ((card->mpuout = kmalloc(sizeof(struct emu10k1_mpuout),
GFP_KERNEL))
> > +       if ((card->mpuout = kmalloc(sizeof(*card->mpuout), GFP_KERNEL))
>
> Yeah, this is fine.  The original posted omitted the '*' which was not
> fine :)

The only other thing left to ask, is which is easier to read when glancing
through the code, and which is easier to read when maintaining the code.
imho, ist the former for reading the code, i dont know about maintaing the
code since i dont do that, however in my own projects i prefere the former
when maintaing the code.

What are others oppinions on this ?

Thanks,

Dave Flynn

>
> --
> Jeff Garzik      | Andre the Giant has a posse.
> Building 1024    |
> MandrakeSoft     |
>

Re: Newbie idiotic questions.

[Jeff Garzik]

David Flynn wrote:
> 
> > Daniel Phillips wrote:
> > > Yep, the only thing left to resolve is whether Jeff had coffee or not.
> ;-)
> > >
> > > -       if ((card->mpuout = kmalloc(sizeof(struct emu10k1_mpuout),
> GFP_KERNEL))
> > > +       if ((card->mpuout = kmalloc(sizeof(*card->mpuout), GFP_KERNEL))
> >
> > Yeah, this is fine.  The original posted omitted the '*' which was not
> > fine :)
> 
> The only other thing left to ask, is which is easier to read when glancing
> through the code, and which is easier to read when maintaining the code.
> imho, ist the former for reading the code, i dont know about maintaing the
> code since i dont do that, however in my own projects i prefere the former
> when maintaing the code.

It's the preference of the maintainer.  It's a tossup:  using the type
in the kmalloc makes the type being allocated obvious.  But using
sizeof(*var) is a tiny bit more resistant to change.

Neither one sufficiently affects long term maintenance AFAICS, so it's
personal preference, not any sort of kernel standard one way or the
other...

	Jeff


-- 
Jeff Garzik      | Andre the Giant has a posse.
Building 1024    |
MandrakeSoft     |

Re: Newbie idiotic questions.

[Geert Uytterhoeven]

On Sun, 17 Jun 2001, Jeff Garzik wrote:
> David Flynn wrote:
> > > Daniel Phillips wrote:
> > > > Yep, the only thing left to resolve is whether Jeff had coffee or not.
> > ;-)
> > > >
> > > > -       if ((card->mpuout = kmalloc(sizeof(struct emu10k1_mpuout),
> > GFP_KERNEL))
> > > > +       if ((card->mpuout = kmalloc(sizeof(*card->mpuout), GFP_KERNEL))
> > >
> > > Yeah, this is fine.  The original posted omitted the '*' which was not
> > > fine :)
> > 
> > The only other thing left to ask, is which is easier to read when glancing
> > through the code, and which is easier to read when maintaining the code.
> > imho, ist the former for reading the code, i dont know about maintaing the
> > code since i dont do that, however in my own projects i prefere the former
> > when maintaing the code.
> 
> It's the preference of the maintainer.  It's a tossup:  using the type
> in the kmalloc makes the type being allocated obvious.  But using
> sizeof(*var) is a tiny bit more resistant to change.
> 
> Neither one sufficiently affects long term maintenance AFAICS, so it's
> personal preference, not any sort of kernel standard one way or the
> other...

The first one can be made a bit safer against changes by creating a `knew'
macro that behaves like `new' in C++:

| #define knew(type, flags)	(type *)kmalloc(sizeof(type), (flags))

If the types in the assignment don't match, gcc will tell you.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: Newbie idiotic questions.

[Daniel Phillips]

On Sunday 17 June 2001 21:18, Geert Uytterhoeven wrote:
> On Sun, 17 Jun 2001, Jeff Garzik wrote:
> > David Flynn wrote:
> > > > Daniel Phillips wrote:
> > > > > -       if ((card->mpuout = kmalloc(sizeof(struct emu10k1_mpuout), 
GFP_KERNEL))
> > >
> > > > > +       if ((card->mpuout = kmalloc(sizeof(*card->mpuout),
GFP_KERNEL))
> > > >
> > > > Yeah, this is fine.  The original posted omitted the '*' which was
> > > > not fine :)
> > >
> > > The only other thing left to ask, is which is easier to read when
> > > glancing through the code, and which is easier to read when maintaining
> > > the code. imho, ist the former for reading the code, i dont know about
> > > maintaing the code since i dont do that, however in my own projects i
> > > prefere the former when maintaing the code.
> >
> > It's the preference of the maintainer.  It's a tossup:  using the type
> > in the kmalloc makes the type being allocated obvious.  But using
> > sizeof(*var) is a tiny bit more resistant to change.
> >
> > Neither one sufficiently affects long term maintenance AFAICS, so it's
> > personal preference, not any sort of kernel standard one way or the
> > other...
>
> The first one can be made a bit safer against changes by creating a `knew'
>
> macro that behaves like `new' in C++:
> | #define knew(type, flags)	(type *)kmalloc(sizeof(type), (flags))
>
> If the types in the assignment don't match, gcc will tell you.

Well, since we are still beating this one to death, I'd written a "knew" 
macro as well, and put it aside.  It does the assignment for you too:

   #define knew(p) ((p) = (typeof(p)) kmalloc(sizeof(*(p)), GFP_KERNEL))

Example:

 	struct foo { int a; int b; };
	struct { struct foo *foo; } *foobar;

	if (knew(foobar))
		if (knew(foobar->foo))
			printk("foo: %p\n", foobar->foo);

Terse and clear at the same time, and type safe.  I still don't like it much. 

--
Daniel

Re: Newbie idiotic questions.

[Alexander Viro]

On Sun, 17 Jun 2001, Daniel Phillips wrote:

> > macro that behaves like `new' in C++:
> > | #define knew(type, flags)	(type *)kmalloc(sizeof(type), (flags))
> >
> > If the types in the assignment don't match, gcc will tell you.
> 
> Well, since we are still beating this one to death, I'd written a "knew" 
> macro as well, and put it aside.  It does the assignment for you too:
> 
>    #define knew(p) ((p) = (typeof(p)) kmalloc(sizeof(*(p)), GFP_KERNEL))
 
> Terse and clear at the same time, and type safe.  I still don't like it much. 

And ungreppable, not to mention gratitious use of GNU extension.

Re: Newbie idiotic questions.

[Bill Pringlemeir]

>>>>> "Jeff" == Jeff Garzik <jgarzik@mandrakesoft.com> writes:
[snip]
 Jeff> It's the preference of the maintainer.  It's a tossup: using
 Jeff> the type in the kmalloc makes the type being allocated obvious.
 Jeff> But using sizeof(*var) is a tiny bit more resistant to change.

Ok, thanks.  I was looking at fixing an `actual bug' in this driver
and I was wonder what else I could/should do while there.  I didn't
necessarily want to change `sizeof(struct Type)' to `sizeof(*value)'.
I considered that change to be somewhat dubious, even though I like
`sizeof(*value)'.  Of course, I unwittingly demonstrated what dangers
lie in making cosmetic changes [I was on my way to a party at the time
and my girlfriend was calling, excuses, excuses...].

So, It looks like I might fix the actual race condition, post that
diff, fix any other small oddities, post that diff.  If no one
complains, etc I can ask the maintainer.  Of course I will test it
myself as well.

regards,
Bill Pringlemeir.

Re: Newbie idiotic questions.

[Daniel Phillips]

On Sunday 17 June 2001 22:37, Alexander Viro wrote:
> On Sun, 17 Jun 2001, Daniel Phillips wrote:
> > Well, since we are still beating this one to death, I'd written a "knew"
> > macro as well, and put it aside.  It does the assignment for you too:
> >
> >    #define knew(p) ((p) = (typeof(p)) kmalloc(sizeof(*(p)), GFP_KERNEL))
> >
> > Terse and clear at the same time, and type safe.  I still don't like it
> > much.
>
> And ungreppable, not to mention gratitious use of GNU extension.

typeof?  It's rather popular in the kernel already.  Besides, who is going to 
compile this with anything other than gcc?

I don't see your point about greppability.

--
Daniel

Re: Newbie idiotic questions.

[Alexander Viro]

On Sun, 17 Jun 2001, Daniel Phillips wrote:

> typeof?  It's rather popular in the kernel already.  Besides, who is going to 

Really? 5 instances in PPC arch-specific code, 1 (absolutely gratitious)
in drivers/mtd, 2 - in m68k (also useless), 4 - in drivers/video, 2 -
in AFFS and 1 - in netfilter.

I wouldn't call it "rather popular".

> compile this with anything other than gcc?

> 
> I don't see your point about greppability.

You are making the types it is applied to harder to deal with wrt. global
search.

	But the real issue here is that preprocessor is not a way to get
polymorphism. And that would be the only context where typeof might
have any use. Trying to turn C into the things it isn't is always a bad
idea - had been proven many times. starting at least with Bourne shel
(check the v7 sh source if you don't know what I'm refering to).

Re: Newbie idiotic questions.

[Roman Zippel]

Hi,

On Sun, 17 Jun 2001, Alexander Viro wrote:

> > typeof?  It's rather popular in the kernel already.  Besides, who is going to
>
> Really? 5 instances in PPC arch-specific code, 1 (absolutely gratitious)
> in drivers/mtd, 2 - in m68k (also useless), 4 - in drivers/video, 2 -
> in AFFS and 1 - in netfilter.
>
> I wouldn't call it "rather popular".

You should also grep for '__typeof__'. :-)

bye, Roman

Re: Newbie idiotic questions.

[Alexander Viro]

On Mon, 18 Jun 2001, Roman Zippel wrote:

> > I wouldn't call it "rather popular".
> 
> You should also grep for '__typeof__'. :-)

Yeeeccchhh. OK, there is more of that. However, the main user of that
beast is, AFAICS, get_user()/put_user() and their ilk in include/asm-*
The rest looks very bogus...