Linux kernel development

Devendra Naga | 1 Jul 2012 15:36

Picon

[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 <devendra.aaru <at> gmail.com>
---
 drivers/staging/comedi/drivers/fl512.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/fl512.c b/drivers/staging/comedi/drivers/fl512.c
index d1da809..52e6d14 100644
--- a/drivers/staging/comedi/drivers/fl512.c
+++ b/drivers/staging/comedi/drivers/fl512.c
 <at>  <at>  -125,8 +125,10  <at>  <at>  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");
--

-- 
1.7.9.5

(Continue reading)

Ian Abbott | 2 Jul 2012 10:40

Picon

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

On 2012-07-01 14:36, Devendra Naga wrote:
> 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 <devendra.aaru <at> gmail.com>
> ---
>   drivers/staging/comedi/drivers/fl512.c |    4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/comedi/drivers/fl512.c b/drivers/staging/comedi/drivers/fl512.c
> index d1da809..52e6d14 100644
> --- a/drivers/staging/comedi/drivers/fl512.c
> +++ b/drivers/staging/comedi/drivers/fl512.c
>  <at>  <at>  -125,8 +125,10  <at>  <at>  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");

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

(Continue reading)

devendra.aaru | 2 Jul 2012 10:43

Picon

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

On Mon, Jul 2, 2012 at 2:10 PM, Ian Abbott <abbotti <at> mev.co.uk> wrote:
> On 2012-07-01 14:36, Devendra Naga wrote:
>>
>> 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 <devendra.aaru <at> gmail.com>
>> ---
>
>
> 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.
> --
> -=( Ian Abbott  <at>  MEV Ltd.    E-mail: <abbotti <at> mev.co.uk>        )=-
> -=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Devendra.

devendra.aaru | 2 Jul 2012 10:43

Picon

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

On Mon, Jul 2, 2012 at 2:10 PM, Ian Abbott <abbotti <at> mev.co.uk> wrote:
> On 2012-07-01 14:36, Devendra Naga wrote:
>>
>> 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 <devendra.aaru <at> gmail.com>
>> ---
>
>
> 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.
> --
> -=( Ian Abbott  <at>  MEV Ltd.    E-mail: <abbotti <at> mev.co.uk>        )=-
> -=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Devendra.

H Hartley Sweeten | 2 Jul 2012 17:57

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

On Monday, July 02, 2012 1:41 AM, Ian Abbott wrote:
> 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.

(Continue reading)

Ian Abbott | 3 Jul 2012 12:18

Picon

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

On 2012-07-02 16:57, H Hartley Sweeten wrote:
> On Monday, July 02, 2012 1:41 AM, Ian Abbott wrote:
>> 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.

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).

> 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

(Continue reading)

Ian Abbott | 3 Jul 2012 12:18

Picon

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

On 2012-07-02 16:57, H Hartley Sweeten wrote:
> On Monday, July 02, 2012 1:41 AM, Ian Abbott wrote:
>> 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.

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).

> 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

(Continue reading)

H Hartley Sweeten | 2 Jul 2012 17:57

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

On Monday, July 02, 2012 1:41 AM, Ian Abbott wrote:
> 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.

(Continue reading)

Ian Abbott | 2 Jul 2012 10:40

Picon

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

On 2012-07-01 14:36, Devendra Naga wrote:
> 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 <devendra.aaru <at> gmail.com>
> ---
>   drivers/staging/comedi/drivers/fl512.c |    4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/comedi/drivers/fl512.c b/drivers/staging/comedi/drivers/fl512.c
> index d1da809..52e6d14 100644
> --- a/drivers/staging/comedi/drivers/fl512.c
> +++ b/drivers/staging/comedi/drivers/fl512.c
>  <at>  <at>  -125,8 +125,10  <at>  <at>  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");

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

(Continue reading)

Return

Return to gmane.linux.kernel.

Search Archive

Language

Change language

Options

Current view: Threads only / Showing only 20 lines / Not hiding cited text.
Change to All messages, whole messages, or hide cited text.

Post a message
NNTP Newsgroup
Classic Gmane web interface
XML

XML

RSS Feed
List Information

About Gmane