request_region() on sound/oss/wavfront.c.

Kernel-janitors Domen Puncer 2 participants 4 messages May 22, 2004 – May 23, 2004 Kernel janitors

Share & tools

Domen Puncer

author

2004-05-22 22:43 UTC #0 lore raw

Quoted 10 lines · Gustavo Franco

Hi list,
[Patch aplied against 2.6.6-bk9 - compiles cleanly. ]
I've added two error checks for request_region() calls on the source and replaced a check_region() with request_region().The release_region() calls seems to be ok, but let me known if i'm missing something.
Comments?

You request same region twice...

Quoted 2 lines

+++ sound/oss/wavfront.c 2004-05-22 18:28:38.000000000 -0300
+ if (!request_region (io_base, 16, "wavefront")) {

Few lines later there is a:

	dev.base = io_base;

So this will print ugly error messages.

Review

Quoted 2 lines

(quoted diff) +2 -0

+       if(!request_region (dev.base+2, 6, "wavefront synth")) {
+               if(!request_region (dev.base+8, 8, "wavefront fx")) {

IMHO, you can just get rid of latter 2 request_region's. And don't forget about release_region.

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Domen Puncer

author

2004-05-22 23:11 UTC #1 lore raw

Quoted 5 lines · Domen Puncer

Quoted 2 lines · Gustavo Franco
+++ sound/oss/wavfront.c 2004-05-22 18:28:38.000000000 -0300
+ if (!request_region (io_base, 16, "wavefront")) {
Few lines later there is a:

Quoted 7 lines

	dev.base = io_base;
So this will print ugly error messages.
Review
Quoted 2 lines
(quoted diff) +2 -0
+       if(!request_region (dev.base+2, 6, "wavefront synth")) {
+               if(!request_region (dev.base+8, 8, "wavefront fx")) {
IMHO, you can just get rid of latter 2 request_region's.

Ehm, no, damn. Comment above check_region states that device can take up 8 ports.

So if there is another device on base+8 this wont work correctly (if that device registred it's region before, even the old code wont work ok).

Quoted 1 line

And don't forget about release_region.

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Gustavo Franco

2004-05-23 01:12 UTC #2 lore raw

In reply to Domen Puncer

Quoted 7 lines · Domen Puncer

Quoted 4 lines · Domen Puncer
On 22/05/04 18:54 -0300, Gustavo Franco wrote:

Quoted 9 lines

Quoted 9 lines
Quoted 4 lines
+++ sound/oss/wavfront.c 2004-05-22 18:28:38.000000000 -0300
+ if (!request_region (io_base, 16, "wavefront")) {
Few lines later there is a:
	dev.base = io_base;
So this will print ugly error messages.

Sorry, but do you mean KERN_ERR ones?

Quoted 10 lines

Quoted 9 lines
Review
Quoted 6 lines
(quoted diff) +2 -0
+       if(!request_region (dev.base+2, 6, "wavefront synth")) {
+               if(!request_region (dev.base+8, 8, "wavefront fx")) {
IMHO, you can just get rid of latter 2 request_region's.

Quoted 8 lines

Ehm, no, damn. Comment above check_region states that device can take up 8 ports.
So if there is another device on base+8 this wont work correctly (if that device registred it's region before, even the old code wont work ok).

What? Can you summarize your two messages? Check that both request_region() cited by you above were not added by me, i've added a return check only.

Thanks,
--
Gustavo Franco

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Domen Puncer

author

2004-05-23 07:09 UTC #3 lore raw

In reply to Domen Puncer

Quoted 18 lines · Gustavo Franco

Quoted 7 lines · Domen Puncer
Quoted 4 lines · Domen Puncer
On 22/05/04 18:54 -0300, Gustavo Franco wrote:
Quoted 9 lines
Quoted 9 lines
Quoted 4 lines
+++ sound/oss/wavfront.c 2004-05-22 18:28:38.000000000 -0300
+ if (!request_region (io_base, 16, "wavefront")) {
Few lines later there is a:
	dev.base = io_base;
So this will print ugly error messages.

Quoted 1 line

Sorry, but do you mean KERN_ERR ones?

I mean error messeges below request_region's that will fail.

Quoted 13 lines

Quoted 13 lines
Quoted 9 lines
Review
Quoted 6 lines
(quoted diff) +2 -0
+       if(!request_region (dev.base+2, 6, "wavefront synth")) {
+               if(!request_region (dev.base+8, 8, "wavefront fx")) {
IMHO, you can just get rid of latter 2 request_region's.
Ehm, no, damn. Comment above check_region states that device can take up 8 ports.

Quoted 8 lines

Quoted 5 lines
So if there is another device on base+8 this wont work correctly (if that device registred it's region before, even the old code wont work ok).
What? Can you summarize your two messages? Check that both request_region() cited
by you above were not added by me, i've added a return check only.

Yes, i know that, but the request_region you added causes the other two to fail. (You can't request same region twice)
It seems right to me to request_region(base, 8) where check_region is now, and another (base+8, 8) before fx detection.

Quoted 10 lines

Thanks,
--
Gustavo Franco
_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors