Skip to contentSkip to conversation

staging/comedi/drivers: release allocated I/O region if alloc_private fails

PATCH In review Devendra Naga 3 participants 5 messages Jul 1, 2012 – Jul 3, 2012 Staging

lore
Share & tools
lore

Devendra Naga

author
#0 lore raw

PATCH 1/2 staging/comedi/drivers: release allocated I/O region if alloc_private fails

if alloc_private fail, we wont' release the I/O region, which was request_region 'ed.

release the allocated I/O region if alloc_private fails.

Signed-off-by Devendra Naga <[email protected]>
---
 drivers/staging/comedi/drivers/fl512.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/staging/comedi/drivers/fl512.c +3 -1
index d1da809..52e6d14 100644
@@ -125,8 +125,10 @@ static int fl512_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 	}
 	dev->iobase = iobase;
 	dev->board_name = "fl512";
-	if (alloc_private(dev, sizeof(struct fl512_private)) < 0)
+	if (alloc_private(dev, sizeof(struct fl512_private)) < 0) {
+		release_region(iobase, FL512_SIZE);
 		return -ENOMEM;
+	}
 
 #if DEBUG
 	printk(KERN_DEBUG "malloc ok\n");
Signature · 1 line
1.7.9.5
#1 lore raw
Quoted 16 lines · Devendra Naga

if alloc_private fail, we wont' release the I/O region, which was request_region 'ed.

release the allocated I/O region if alloc_private fails.

Signed-off-by Devendra Naga <[email protected]>
---
  drivers/staging/comedi/drivers/fl512.c |    4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)
drivers/staging/comedi/drivers/fl512.c +0 -0
index d1da809..52e6d14 100644
@@ -125,8 +125,10 @@ static int fl512_attach(struct comedi_device *dev, struct comedi_devconfig *it)
  	}

drivers/staging/comedi/drivers/fl512.c static int fl512_attach(struct comedi_device *dev, struct comedi_devconfig *it) · line 134

Quoted 10 lines
drivers/staging/comedi/drivers/fl512.c +3 -1
  	dev->iobase = iobase;
  	dev->board_name = "fl512";
-	if (alloc_private(dev, sizeof(struct fl512_private)) < 0)
+	if (alloc_private(dev, sizeof(struct fl512_private)) < 0) {
+		release_region(iobase, FL512_SIZE);
  		return -ENOMEM;
+	}
 
  #if DEBUG
  	printk(KERN_DEBUG "malloc ok\n");

No. The I/O region will be deallocated in fl512_detach() because dev->iobase has been set non-zero. fl512_detach() will be called by the comedi core if fl512_attach() returns an error. This is an unusual aspect of the comedi drivers.

Signature · 2 lines
-=( Ian Abbott @ MEV Ltd.    E-mail: <[email protected]>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-
author
#2 lore raw
Quoted 7 lines · Ian Abbott
Quoted 6 lines · Devendra Naga

if alloc_private fail, we wont' release the I/O region, which was request_region 'ed.

release the allocated I/O region if alloc_private fails.

Quoted 9 lines
Quoted 2 lines
Signed-off-by Devendra Naga <[email protected]>

---

No. The I/O region will be deallocated in fl512_detach() because dev->iobase has been set non-zero. fl512_detach() will be called by the comedi core if fl512_attach() returns an error. This is an unusual aspect of the comedi drivers.

Oh, i didn't see the comedi core, i went and saw that its not freed up, so actaully driver unload wont' be called if driver attach , probe fail. so i started sending this patch.

Sorry.

Quoted 3 lines

--
-=( Ian Abbott @ MEV Ltd. E-mail: <[email protected]> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-

Devendra.

#3 lore raw
In reply to Ian Abbott
Quoted 4 lines · Ian Abbott

No. The I/O region will be deallocated in fl512_detach() because dev->iobase has been set non-zero. fl512_detach() will be called by the comedi core if fl512_attach() returns an error. This is an unusual aspect of the comedi drivers.

I have been wondering if that aspect should be "fixed".

It's more typical for kernel drivers to clean up after themselves if the probe/attach/init/etc. fails. And the release/detach/exit/etc. is only called if the driver has successfully loaded.

With the comedi drivers, the "detach" is always called if the "attach" failed. And, of course the "detach" is called when the device is removed.

Because of this the "detach" routines need to do all the checks to see what needs to be cleaned up. Not a big deal but it does create some confusion as this patch shows.

Ian, what's your opinion on this? Do you think we should refactor all the driver "attach" routines so they clean up on failure and fix the core so the "detach" is only called after a successful "attach"?

This would be a pretty big patch since it affects every driver as well as the core.

We could break it up by introducing a temporary flag in the comedi_driver struct that indicates if the driver has been "fixed". The core could then work as-is for non-updated drivers. Once all the drivers have been updated we then fix the core and remove the flag from all the drivers.

Regards,
Hartley

#4 lore raw
Quoted 12 lines · H Hartley Sweeten
Quoted 4 lines · Ian Abbott

No. The I/O region will be deallocated in fl512_detach() because dev->iobase has been set non-zero. fl512_detach() will be called by the comedi core if fl512_attach() returns an error. This is an unusual aspect of the comedi drivers.

I have been wondering if that aspect should be "fixed".

It's more typical for kernel drivers to clean up after themselves if the probe/attach/init/etc. fails. And the release/detach/exit/etc. is only called if the driver has successfully loaded.

Quoted 6 lines

With the comedi drivers, the "detach" is always called if the "attach" failed. And, of course the "detach" is called when the device is removed.

Because of this the "detach" routines need to do all the checks to see what needs to be cleaned up. Not a big deal but it does create some confusion as this patch shows.

At the very least, this behaviour should be documented with a comment at the appropriate place in comedidev.h.

I think Comedi's current clean-up model is based on the open/close model of TTY driver operations, where the 'close' tty_operation is called when the 'open' tty_operation fails (although typically these operations don't do much allocation or deallocation).

Quoted 3 lines

Ian, what's your opinion on this? Do you think we should refactor all the driver "attach" routines so they clean up on failure and fix the core so the "detach" is only called after a successful "attach"?

It would be nicer, although the existing mechanism does have the slight advantage of using less code.

If this mechanism is adopted, drivers using the new mechanism should also be responsible for freeing their subdevices (which will need a new function comedi_free_subdevices()) and their comedi_device private data.
(They're already responsible for freeing comedi_subdevice private data
in their 'detach' routines.)

Quoted 7 lines

This would be a pretty big patch since it affects every driver as well as the core.

We could break it up by introducing a temporary flag in the comedi_driver struct that indicates if the driver has been "fixed". The core could then work as-is for non-updated drivers. Once all the drivers have been updated we then fix the core and remove the flag from all the drivers.

That sounds like a reasonable plan.

Signature · 2 lines
-=( Ian Abbott @ MEV Ltd.    E-mail: <[email protected]>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-