* [PATCH][ATM] iphase: BUG: sleeping function called from invalid context @ 2008-06-05 16:12 Jorge Boncompte [DTI2] 2008-06-14 21:05 ` Chas Williams (CONTRACTOR) 0 siblings, 1 reply; 4+ messages in thread From: Jorge Boncompte [DTI2] @ 2008-06-05 16:12 UTC (permalink / raw) To: netdev; +Cc: chas williams iphase driver calls ioremap under spinlock_irqsave in his initialization function. The spinlock seems to be there just for the sole purpose of preventing initializing multiple cards at once. So I think that removing it should be fine. Signed-off-by: Jorge Boncompte [DTI2] <jorge@dti2.net> --- drivers/atm/iphase.c | 6 ------ drivers/atm/iphase.h | 2 +- 2 files changed, 1 insertions(+), 7 deletions(-) diff --git a/drivers/atm/iphase.c b/drivers/atm/iphase.c index 5c28ca7..a51b7cb 100644 --- a/drivers/atm/iphase.c +++ b/drivers/atm/iphase.c @@ -3171,7 +3171,6 @@ static int __devinit ia_init_one(struct pci_dev *pdev, { struct atm_dev *dev; IADEV *iadev; - unsigned long flags; int ret; iadev = kzalloc(sizeof(*iadev), GFP_KERNEL); @@ -3201,19 +3200,14 @@ static int __devinit ia_init_one(struct pci_dev *pdev, ia_dev[iadev_count] = iadev; _ia_dev[iadev_count] = dev; iadev_count++; - spin_lock_init(&iadev->misc_lock); - /* First fixes first. I don't want to think about this now. */ - spin_lock_irqsave(&iadev->misc_lock, flags); if (ia_init(dev) || ia_start(dev)) { IF_INIT(printk("IA register failed!\n");) iadev_count--; ia_dev[iadev_count] = NULL; _ia_dev[iadev_count] = NULL; - spin_unlock_irqrestore(&iadev->misc_lock, flags); ret = -EINVAL; goto err_out_deregister_dev; } - spin_unlock_irqrestore(&iadev->misc_lock, flags); IF_EVENT(printk("iadev_count = %d\n", iadev_count);) iadev->next_board = ia_boards; diff --git a/drivers/atm/iphase.h b/drivers/atm/iphase.h index b2cd20f..f3457a9 100644 --- a/drivers/atm/iphase.h +++ b/drivers/atm/iphase.h @@ -1022,7 +1022,7 @@ typedef struct iadev_t { struct dle_q rx_dle_q; struct free_desc_q *rx_free_desc_qhead; struct sk_buff_head rx_dma_q; - spinlock_t rx_lock, misc_lock; + spinlock_t rx_lock; struct atm_vcc **rx_open; /* list of all open VCs */ u16 num_rx_desc, rx_buf_sz, rxing; u32 rx_pkt_ram, rx_tmp_cnt; -- ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH][ATM] iphase: BUG: sleeping function called from invalid context 2008-06-05 16:12 [PATCH][ATM] iphase: BUG: sleeping function called from invalid context Jorge Boncompte [DTI2] @ 2008-06-14 21:05 ` Chas Williams (CONTRACTOR) 2008-06-15 13:00 ` Jorge Boncompte [DTI2] 0 siblings, 1 reply; 4+ messages in thread From: Chas Williams (CONTRACTOR) @ 2008-06-14 21:05 UTC (permalink / raw) To: jorge; +Cc: netdev In message <4848106F.30503@dti2.net>,"Jorge Boncompte [DTI2]" writes: > iphase driver calls ioremap under spinlock_irqsave in his >initialization function. The spinlock seems to be there just for the >sole purpose of preventing initializing multiple cards at once. So I >think that removing it should be fine. the lock is necessary to prevent multiple cards from modifying iadev_count at the wrong time since iadev_count is incremented and decremented. it might be simpler to just make a local copy of iadev_count and just always increment iadev_count. if you fail to init a particular card, you will just leave a hole in ia_dev[]. something like the attached. also, ia_remove_one() is broken since it assumes that the cards will be removed in the same order that they were installed. even better would be to rewrite this to avoid the iadev[] array entirely and use a linked list. diff --git a/drivers/atm/iphase.c b/drivers/atm/iphase.c index 5c28ca7..64d1c97 100644 --- a/drivers/atm/iphase.c +++ b/drivers/atm/iphase.c @@ -3171,7 +3171,6 @@ static int __devinit ia_init_one(struct pci_dev *pdev, { struct atm_dev *dev; IADEV *iadev; - unsigned long flags; int ret; iadev = kzalloc(sizeof(*iadev), GFP_KERNEL); @@ -3180,6 +3179,7 @@ static int __devinit ia_init_one(struct pci_dev *pdev, goto err_out; } + iadev->iadev_count = iadev_count++; iadev->pci = pdev; IF_INIT(printk("ia detected at bus:%d dev: %d function:%d\n", @@ -3198,23 +3198,17 @@ static int __devinit ia_init_one(struct pci_dev *pdev, IF_INIT(printk("dev_id = 0x%x iadev->LineRate = %d \n", (u32)dev, iadev->LineRate);) - ia_dev[iadev_count] = iadev; - _ia_dev[iadev_count] = dev; - iadev_count++; - spin_lock_init(&iadev->misc_lock); - /* First fixes first. I don't want to think about this now. */ - spin_lock_irqsave(&iadev->misc_lock, flags); + ia_dev[iadev->iadev_count] = iadev; + _ia_dev[iadev->iadev_count] = dev; if (ia_init(dev) || ia_start(dev)) { IF_INIT(printk("IA register failed!\n");) iadev_count--; - ia_dev[iadev_count] = NULL; - _ia_dev[iadev_count] = NULL; - spin_unlock_irqrestore(&iadev->misc_lock, flags); + ia_dev[iadev->iadev_count] = NULL; + _ia_dev[iadev->iadev_count] = NULL; ret = -EINVAL; goto err_out_deregister_dev; } - spin_unlock_irqrestore(&iadev->misc_lock, flags); - IF_EVENT(printk("iadev_count = %d\n", iadev_count);) + IF_EVENT(printk("iadev_count = %d\n", iadev->iadev_count);) iadev->next_board = ia_boards; ia_boards = dev; @@ -3243,9 +3237,8 @@ static void __devexit ia_remove_one(struct pci_dev *pdev) /* De-register device */ free_irq(iadev->irq, dev); - iadev_count--; - ia_dev[iadev_count] = NULL; - _ia_dev[iadev_count] = NULL; + ia_dev[iadev->iadev_count] = NULL; + _ia_dev[iadev->iadev_count] = NULL; IF_EVENT(printk("deregistering iav at (itf:%d)\n", dev->number);) atm_dev_deregister(dev); diff --git a/drivers/atm/iphase.h b/drivers/atm/iphase.h index b2cd20f..a0b1982 100644 --- a/drivers/atm/iphase.h +++ b/drivers/atm/iphase.h @@ -1022,7 +1022,7 @@ typedef struct iadev_t { struct dle_q rx_dle_q; struct free_desc_q *rx_free_desc_qhead; struct sk_buff_head rx_dma_q; - spinlock_t rx_lock, misc_lock; + spinlock_t rx_lock; struct atm_vcc **rx_open; /* list of all open VCs */ u16 num_rx_desc, rx_buf_sz, rxing; u32 rx_pkt_ram, rx_tmp_cnt; @@ -1064,6 +1064,7 @@ typedef struct iadev_t { struct testTable_t **testTable; dma_addr_t tx_dle_dma; dma_addr_t rx_dle_dma; + int iadev_count; } IADEV; ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH][ATM] iphase: BUG: sleeping function called from invalid context 2008-06-14 21:05 ` Chas Williams (CONTRACTOR) @ 2008-06-15 13:00 ` Jorge Boncompte [DTI2] 2008-06-15 14:47 ` Chas Williams (CONTRACTOR) 0 siblings, 1 reply; 4+ messages in thread From: Jorge Boncompte [DTI2] @ 2008-06-15 13:00 UTC (permalink / raw) To: chas3; +Cc: netdev Chas Williams (CONTRACTOR) escribió: > In message <4848106F.30503@dti2.net>,"Jorge Boncompte [DTI2]" writes: >> iphase driver calls ioremap under spinlock_irqsave in his >> initialization function. The spinlock seems to be there just for the >> sole purpose of preventing initializing multiple cards at once. So I >> think that removing it should be fine. > > the lock is necessary to prevent multiple cards from modifying iadev_count > at the wrong time since iadev_count is incremented and decremented. > it might be simpler to just make a local copy of iadev_count and just > always increment iadev_count. if you fail to init a particular card, > you will just leave a hole in ia_dev[]. something like the attached. I thought that device initialization inside a driver is serialized. isn't it? > also, ia_remove_one() is broken since it assumes that the cards will be > removed in the same order that they were installed. > > even better would be to rewrite this to avoid the iadev[] array entirely > and use a linked list. > I have a patch that removes iadev and iadev_count enterely that i did not sent because i thought it wouldn't be considered a bug fix. I'll sent it this week. Thanks for your review, -Jorge -- ============================================================== Jorge Boncompte - Ingenieria y Gestion de RED DTI2 - Desarrollo de la Tecnologia de las Comunicaciones -------------------------------------------------------------- C/ Abogado Enriquez Barrios, 5 14004 CORDOBA (SPAIN) Tlf: +34 957 761395 / FAX: +34 957 450380 ============================================================== - Sin pistachos no hay Rock & Roll... - Without wicker a basket cannot be made. ============================================================== ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH][ATM] iphase: BUG: sleeping function called from invalid context 2008-06-15 13:00 ` Jorge Boncompte [DTI2] @ 2008-06-15 14:47 ` Chas Williams (CONTRACTOR) 0 siblings, 0 replies; 4+ messages in thread From: Chas Williams (CONTRACTOR) @ 2008-06-15 14:47 UTC (permalink / raw) To: jorge; +Cc: netdev In message <48551280.1090706@dti2.net>,"Jorge Boncompte [DTI2]" writes: >> the lock is necessary to prevent multiple cards from modifying iadev_count >> at the wrong time since iadev_count is incremented and decremented. >> it might be simpler to just make a local copy of iadev_count and just >> always increment iadev_count. if you fail to init a particular card, >> you will just leave a hole in ia_dev[]. something like the attached. > > I thought that device initialization inside a driver is serialized. isn't it? i honestly dont know. even if it is serialized, that code is just hideous. if you had a system with hotplug support, it wouldnt work right. > I have a patch that removes iadev and iadev_count enterely that i did not sent because i thought it wouldn't be >considered a bug fix. I'll sent it this week. i would certainly prefer that version. no matter, how you fix the problem it is a bug fix. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2008-06-15 14:47 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-06-05 16:12 [PATCH][ATM] iphase: BUG: sleeping function called from invalid context Jorge Boncompte [DTI2] 2008-06-14 21:05 ` Chas Williams (CONTRACTOR) 2008-06-15 13:00 ` Jorge Boncompte [DTI2] 2008-06-15 14:47 ` Chas Williams (CONTRACTOR)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).