TTY_IO: Remove unnecessary kmalloc casts

PATCH In review Ahmed S. Darwish 5 participants 15 messages Jan 5, 2007 – Jan 6, 2007 Char and misc drivers

lore

Share & tools

lore

Ahmed S. Darwish

author

2007-01-05 06:36 UTC #0 lore raw

PATCH 1/1 TTY_IO: Remove unnecessary kmalloc casts

Remove unnecessary kmalloc casts in drivers/char/tty_io.c

Signed-off-by Ahmed Darwish <[email protected]>

drivers/char/tty_io.c +4 -8

index 47a6eac..97f54b0 100644
@@ -1932,16 +1932,14 @@ static int init_dev(struct tty_driver *driver, int idx,
 	}
 
 	if (!*tp_loc) {
-		tp = (struct ktermios *) kmalloc(sizeof(struct ktermios),
-						GFP_KERNEL);
+		tp = kmalloc(sizeof(struct ktermios), GFP_KERNEL);
 		if (!tp)
 			goto free_mem_out;
 		*tp = driver->init_termios;
 	}
 
 	if (!*ltp_loc) {
-		ltp = (struct ktermios *) kmalloc(sizeof(struct ktermios),
-						 GFP_KERNEL);
+		ltp = kmalloc(sizeof(struct ktermios), GFP_KERNEL);
 		if (!ltp)
 			goto free_mem_out;
 		memset(ltp, 0, sizeof(struct ktermios));

@@ -1965,16 +1963,14 @@ static int init_dev(struct tty_driver *driver, int idx,
 		}
 
 		if (!*o_tp_loc) {
-			o_tp = (struct ktermios *)
-				kmalloc(sizeof(struct ktermios), GFP_KERNEL);
+			o_tp = kmalloc(sizeof(struct ktermios), GFP_KERNEL);
 			if (!o_tp)
 				goto free_mem_out;
 			*o_tp = driver->other->init_termios;
 		}
 
 		if (!*o_ltp_loc) {
-			o_ltp = (struct ktermios *)
-				kmalloc(sizeof(struct ktermios), GFP_KERNEL);
+			o_ltp = kmalloc(sizeof(struct ktermios), GFP_KERNEL);
 			if (!o_ltp)
 				goto free_mem_out;
 			memset(o_ltp, 0, sizeof(struct ktermios));

Signature · 2 lines

Ahmed S. Darwish
http://darwish-07.blogspot.com

Robert P. J. Day

2007-01-05 07:02 UTC #1 lore raw

Quoted 1 line · Ahmed S. Darwish

Remove unnecessary kmalloc casts in drivers/char/tty_io.c

rather than remove these casts a file or two at a time, why not just do them all at once and submit a single patch? there aren't that many of them:

grep -Er "\([^\)\*]+\*\) ?k[cmz]alloc ?\(" .

rday

Ahmed S. Darwish

author

2007-01-05 07:14 UTC #2 lore raw

Quoted 7 lines · Robert P. J. Day

Quoted 1 line · Ahmed S. Darwish
Remove unnecessary kmalloc casts in drivers/char/tty_io.c
rather than remove these casts a file or two at a time, why not just do them all at once and submit a single patch? there aren't that many of them:

OK, Thanks for the tip ..

Signature · 2 lines

Ahmed S. Darwish
http://darwish-07.blogspot.com

Currently I'm dropping this patch and writing a big patch to remove all the k[mzc]alloc castings in the 20-rc3 tree as suggested by Mr. Robert Day. I think this will be better done in another patch to let every patch do one single thing. right ?

Signature · 2 lines

Ahmed S. Darwish
http://darwish-07.blogspot.com

Robert P. J. Day

2007-01-05 10:24 UTC #5 lore raw

Quoted 28 lines · Ahmed S. Darwish

Quoted 19 lines · Rolf Eike Beer
Quoted 3 lines · Ahmed S. Darwish
Remove unnecessary kmalloc casts in drivers/char/tty_io.c
Signed-off-by Ahmed Darwish <[email protected]>
if (!*ltp_loc) {
- ltp = (struct ktermios *) kmalloc(sizeof(struct ktermios),
- GFP_KERNEL);
 +		ltp = kmalloc(sizeof(struct ktermios), GFP_KERNEL);
                      ^^^^^^^
  		if (!ltp)
  			goto free_mem_out;
  		memset(ltp, 0, sizeof(struct ktermios));
                ^^^^^^
kzalloc
if (!*o_ltp_loc) {
- o_ltp = (struct ktermios *)
- kmalloc(sizeof(struct ktermios), GFP_KERNEL);
Quoted 7 lines
 +			o_ltp = kmalloc(sizeof(struct ktermios), GFP_KERNEL);
                                ^^^^^^^
  			if (!o_ltp)
  				goto free_mem_out;
  			memset(o_ltp, 0, sizeof(struct ktermios));
                        ^^^^^^
kzalloc

Quoted 4 lines

Currently I'm dropping this patch and writing a big patch to remove all the k[mzc]alloc castings in the 20-rc3 tree as suggested by Mr. Robert Day. I think this will be better done in another patch to let every patch do one single thing. right ?

almost. as i've learned (the hard way), while each patch should logically accomplish one thing, while you're there, you might as well clean up other issues *at the same locations*.

in this case, as mr. beer suggests, you should also check if this represents a kmalloc->kzalloc cleanup (there's lots of those), and also see if you can replace one of these:

sizeof(struct blah)

with one of these:

sizeof(*blahptr)

according to the CodingStyle guide.

rday

p.s. just FYI, i have a patch that does most of this, but i was going to hold off submitting it until 2.6.20 had arrived. but if you want to take a shot at it, it's all yours.

Pekka Enberg

2007-01-05 10:39 UTC #6 lore raw

Quoted 11 lines · Robert P. J. Day

in this case, as mr. beer suggests, you should also check if this represents a kmalloc->kzalloc cleanup (there's lots of those), and also see if you can replace one of these:
sizeof(struct blah)
with one of these:
sizeof(*blahptr)
according to the CodingStyle guide.

Unfortunately, not all maintainers like sizeof() changes, so you're better off leaving them as-is.

Alan

2007-01-05 10:40 UTC #7 lore raw

In reply to Robert P. J. Day

Quoted 8 lines

represents a kmalloc->kzalloc cleanup (there's lots of those), and also see if you can replace one of these:
sizeof(struct blah)
with one of these:
sizeof(*blahptr)

Patches that do this will get rejected by the tty maintainer in favour of the clarity of the sizeof(struct xyz) format 8)

Ahmed - if you can send me a patch for the tty_io/tty_ioctl code which switches to kzalloc where it makes sense and removes un-needed casts I'll review it and push the bits that look sane upstream.

Alan

Robert P. J. Day

2007-01-05 10:46 UTC #8 lore raw

Quoted 11 lines · Alan

Quoted 8 lines
represents a kmalloc->kzalloc cleanup (there's lots of those), and also see if you can replace one of these:
sizeof(struct blah)
with one of these:
sizeof(*blahptr)
Patches that do this will get rejected by the tty maintainer in favour of the clarity of the sizeof(struct xyz) format 8)

ok, i was just going by the recommendations of the CodingStyle guide.

rday

Ahmed S. Darwish

author

2007-01-05 11:04 UTC #9 lore raw

In reply to Alan

Quoted 5 lines · Alan

Ahmed - if you can send me a patch for the tty_io/tty_ioctl code which switches to kzalloc where it makes sense and removes un-needed casts I'll review it and push the bits that look sane upstream.
Alan

OK I'll try this now.
Are you Mr. Alan Cox. ?. Sorry, "Alan" alone is ambiguous.

Signature · 2 lines

Ahmed S. Darwish
http://darwish-07.blogspot.com

Ahmed S. Darwish

author

2007-01-06 02:37 UTC #10 lore raw

In reply to Robert P. J. Day

Quoted 10 lines · rday

Quoted 3 lines
Ahmed S. Darwish wrote:
Remove unnecessary kmalloc casts in drivers/char/tty_io.c
Signed-off-by Ahmed Darwish <[email protected]>
rday
p.s. just FYI, i have a patch that does most of this, but i was going to hold off submitting it until 2.6.20 had arrived. but if you want to take a shot at it, it's all yours.

OK, then I should stop sending new patches related to that matter to avoid patch conflicts. right ?

Signature · 2 lines

Ahmed S. Darwish
http://darwish-07.blogspot.com

Robert P. J. Day

2007-01-06 07:41 UTC #11 lore raw

Quoted 5 lines · Ahmed S. Darwish

Quoted 5 lines · rday
Quoted 3 lines
Ahmed S. Darwish wrote:
Remove unnecessary kmalloc casts in drivers/char/tty_io.c
Signed-off-by Ahmed Darwish <[email protected]>

Quoted 8 lines

Quoted 5 lines
rday
p.s. just FYI, i have a patch that does most of this, but i was going to hold off submitting it until 2.6.20 had arrived. but if you want to take a shot at it, it's all yours.
OK, then I should stop sending new patches related to that matter to avoid patch conflicts. right ?

no, no, go ahead. i have enough to do to keep me busy.

rday

Rolf Eike Beer

2007-01-05 10:25 UTC #12 lore raw

In reply to Ahmed S. Darwish

Am Freitag, 5. Januar 2007 11:06 schrieb Ahmed S. Darwish:

Quoted 28 lines · Rolf Eike Beer

Quoted 19 lines
Quoted 3 lines · Ahmed S. Darwish
Remove unnecessary kmalloc casts in drivers/char/tty_io.c
Signed-off-by Ahmed Darwish <[email protected]>
if (!*ltp_loc) {
- ltp = (struct ktermios *) kmalloc(sizeof(struct ktermios),
- GFP_KERNEL);
 +		ltp = kmalloc(sizeof(struct ktermios), GFP_KERNEL);
                      ^^^^^^^
  		if (!ltp)
  			goto free_mem_out;
  		memset(ltp, 0, sizeof(struct ktermios));
                ^^^^^^
kzalloc
if (!*o_ltp_loc) {
- o_ltp = (struct ktermios *)
- kmalloc(sizeof(struct ktermios), GFP_KERNEL);
Quoted 7 lines
 +			o_ltp = kmalloc(sizeof(struct ktermios), GFP_KERNEL);
                                ^^^^^^^
  			if (!o_ltp)
  				goto free_mem_out;
  			memset(o_ltp, 0, sizeof(struct ktermios));
                        ^^^^^^
kzalloc

Quoted 2 lines

Currently I'm dropping this patch and writing a big patch to remove all the k[mzc]alloc castings in the 20-rc3 tree as suggested by Mr. Robert Day.

One big patch for the whole kernel will not work anyway. You have to split it up to allow subsystems to integrate them in their own trees. With one big patch you would get collisions all over the tree causing the complete patch to get dropped. Also CC subsystem maintainers on their parts. And please send the patches as replies to the first one as it cleans up readability of lkml a lot :)

Quoted 2 lines

I think this will be better done in another patch to let every patch do one single thing. right ?

Yes. But I would suggest starting with the kmalloc()->kzalloc() things. When you do this conversions just remove the casts of the lines you're touching. This will reduce the size of the complete thing avoiding two rather trivial patches touching the same line twice.

Eike

Ahmed S. Darwish

author

2007-01-05 10:33 UTC #13 lore raw

Quoted 6 lines · Rolf Eike Beer

One big patch for the whole kernel will not work anyway. You have to split it up to allow subsystems to integrate them in their own trees. With one big patch you would get collisions all over the tree causing the complete patch to get dropped. Also CC subsystem maintainers on their parts. And please send the patches as replies to the first one as it cleans up readability of lkml a lot :)

Oops, Just read this warning after sending the (big) patch. Sorry It's my first patch :). I'll split it and do as written. Thanks alot :).

Quoted 9 lines

Quoted 2 lines
I think this will be better done in another patch to let every patch do one single thing. right ?
Yes. But I would suggest starting with the kmalloc()->kzalloc() things. When you do this conversions just remove the casts of the lines you're touching. This will reduce the size of the complete thing avoiding two rather trivial patches touching the same line twice.
Eike

OK. In progress

Signature · 2 lines

Ahmed S. Darwish
http://darwish-07.blogspot.com

Rolf Eike Beer

2007-01-05 11:53 UTC #14 lore raw

Am Freitag, 5. Januar 2007 11:32 schrieb Ahmed S. Darwish:

Quoted 10 lines · Rolf Eike Beer

Quoted 6 lines
One big patch for the whole kernel will not work anyway. You have to split it up to allow subsystems to integrate them in their own trees. With one big patch you would get collisions all over the tree causing the complete patch to get dropped. Also CC subsystem maintainers on their parts. And please send the patches as replies to the first one as it cleans up readability of lkml a lot :)
Oops, Just read this warning after sending the (big) patch. Sorry It's my first patch :). I'll split it and do as written. Thanks alot :).

That wasn't meant for resending. If you resend the whole series it's good to start a new thread. But if you have several related patches (usually this [PATCH N/xx] thing) it helps if you either send a 0/xx first describing the whole series or at least sending everything as reply to 1/xx. This way the mail program will help everybody by keeping this things together.

Eike