[PATCH] staging/sbe-2t3e3: error path cleanup in t3e3_init_channel

[PATCH] staging/sbe-2t3e3: error path cleanup in t3e3_init_channel

[Devendra Naga]

    a) if alloc_hdlcdev fails, we are going into the free_regions,
and returning out the err (which is 0 by the prev call),
       return -ENOMEM if this function fail.

    b) setup_device also can fail, as it calls around the register_hdlc_dev which
is again a macro of the register_netdev.

       take the error from the setup_device and return it out in error condition

    c) request_irq when fails, we are freeing requested mem regions and disabling
the pci device(?) and returning err which is agian 0 here.

      take the error from request_irq and err path will take care of returning it.

as if we return 0 , at the init function, t3e3_init_card, we have a success case
and if there are two channels we call this function again, having the result of
it completely unknown.

This result in having the probe return 0, unloading the driver may (not) cause
ambigous result.

Signed-off-by: Devendra Naga <devendra.aaru@gmail.com>
---
 drivers/staging/sbe-2t3e3/module.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/sbe-2t3e3/module.c b/drivers/staging/sbe-2t3e3/module.c
index cd778b3..1a1a9be 100644
--- a/drivers/staging/sbe-2t3e3/module.c
+++ b/drivers/staging/sbe-2t3e3/module.c
@@ -66,6 +66,7 @@ static int __devinit t3e3_init_channel(struct channel *channel, struct pci_dev *
 
 	dev = alloc_hdlcdev(channel);
 	if (!dev) {
+		err = -ENOMEM;
 		printk(KERN_ERR "SBE 2T3E3" ": Out of memory\n");
 		goto free_regions;
 	}
@@ -82,7 +83,8 @@ static int __devinit t3e3_init_channel(struct channel *channel, struct pci_dev *
 	else
 		channel->h.slot = 0;
 
-	if (setup_device(dev, channel))
+	err = setup_device(dev, channel);
+	if (err)
 		goto free_regions;
 
 	pci_read_config_dword(channel->pdev, 0x40, &val); /* mask sleep mode */
@@ -92,7 +94,8 @@ static int __devinit t3e3_init_channel(struct channel *channel, struct pci_dev *
 	pci_read_config_dword(channel->pdev, PCI_COMMAND, &channel->h.command);
 	t3e3_init(channel);
 
-	if (request_irq(dev->irq, &t3e3_intr, IRQF_SHARED, dev->name, dev)) {
+	err = request_irq(dev->irq, &t3e3_intr, IRQF_SHARED, dev->name, dev);
+	if (err) {
 		printk(KERN_WARNING "%s: could not get irq: %d\n", dev->name, dev->irq);
 		goto free_regions;
 	}
-- 
1.7.0.4

Re: [PATCH] staging/sbe-2t3e3: error path cleanup in t3e3_init_channel

[Dan Carpenter]

Cleanup means there are no behavior changes.  This is a bug fix.

On Thu, Jul 19, 2012 at 06:00:01PM +0530, Devendra Naga wrote:
>     a) if alloc_hdlcdev fails, we are going into the free_regions,
> and returning out the err (which is 0 by the prev call),
>        return -ENOMEM if this function fail.
> 
>     b) setup_device also can fail, as it calls around the register_hdlc_dev which
> is again a macro of the register_netdev.
> 
>        take the error from the setup_device and return it out in error condition
> 
>     c) request_irq when fails, we are freeing requested mem regions and disabling
> the pci device(?) and returning err which is agian 0 here.
> 
>       take the error from request_irq and err path will take care of returning it.
> 
> as if we return 0 , at the init function, t3e3_init_card, we have a success case
> and if there are two channels we call this function again, having the result of
> it completely unknown.
> 
> This result in having the probe return 0, unloading the driver may (not) cause
> ambigous result.

These bugs were there before your patch, but we should also be doing
an unregister_hdlc_device() and a free_netdev().

regards,
dan carpenter

Re: [PATCH] staging/sbe-2t3e3: error path cleanup in t3e3_init_channel

[devendra.aaru]

Hi Dan,

On Thu, Jul 19, 2012 at 6:20 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Cleanup means there are no behavior changes.  This is a bug fix.
>
Thanks Dan, i will put a proper subject line in commit.
> On Thu, Jul 19, 2012 at 06:00:01PM +0530, Devendra Naga wrote:
>>     a) if alloc_hdlcdev fails, we are going into the free_regions,
>> and returning out the err (which is 0 by the prev call),
>>        return -ENOMEM if this function fail.
>>
>>     b) setup_device also can fail, as it calls around the register_hdlc_dev which
>> is again a macro of the register_netdev.
>>
>>        take the error from the setup_device and return it out in error condition
>>
>>     c) request_irq when fails, we are freeing requested mem regions and disabling
>> the pci device(?) and returning err which is agian 0 here.
>>
>>       take the error from request_irq and err path will take care of returning it.
>>
>> as if we return 0 , at the init function, t3e3_init_card, we have a success case
>> and if there are two channels we call this function again, having the result of
>> it completely unknown.
>>
>> This result in having the probe return 0, unloading the driver may (not) cause
>> ambigous result.
>
> These bugs were there before your patch, but we should also be doing
> an unregister_hdlc_device() and a free_netdev().
>
t3e3_remove_channel is called when channel init of channel2 fails, and
i think this cause the double
call to pci_release_region and pci_disable_device,

so i think its better to follow your approach and remove the call to
t3e3_remove_channel and clean all
the things we allocated at the init path itself, rather cleaning them
up when call and a subsequent call to the
t3e3_init_channel fails?

> regards,
> dan carpenter
>

Thanks,
Devendra