powerpc - EHEA fixes
Subsystems (2)
cover powerpc - EHEA fixes
Hi,
here are 2 fixes for the ehea driver (nothing urgent here):
- fix a typo which prevents building when DEBUG is #defined
- fix mutex and spinlock usage in ehea_main
and the diffstat for the patchset:
drivers/net/ehea/ehea_main.c | 26 ++++---------------------- drivers/net/ehea/ehea_phyp.c | 2 +- 2 files changed, 5 insertions(+), 23 deletions(-)
Thanks,
Sebastien.
PATCH 1/2 ehea: fix phyp debugging typo
Fix typo in ehea_h_query_ehea() which prevents building when DEBUG is on.
--- drivers/net/ehea/ehea_phyp.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/net/ehea/ehea_phyp.c +1 -1
@@ -535,7 +535,7 @@ u64 ehea_h_query_ehea(const u64 adapter_handle, void *cb_addr) cb_logaddr,/* R5 */ 0, 0, 0, 0, 0); /* R6-R10 */#ifdef DEBUG -ehea_dmp (cb_addr,sizeof (struct hcp_query_ehea ),"hcp_query_ehea" ); +ehea_dump (cb_addr,sizeof (struct hcp_query_ehea ),"hcp_query_ehea" );#endif return hret; }
Signature · 1 line
1.6.0.1.308.gede4c
Jeff Garzik
AppliedRe: [PATCH 1/2] ehea: fix phyp debugging typo
Quoted 12 lines · Sebastien Dugue
Fix typo in
ehea_h_query_ehea()which prevents building when DEBUG is on.Signed-off-by Sebastien Dugue <[email protected]>--- drivers/net/ehea/ehea_phyp.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)drivers/net/ehea/ehea_phyp.c +0
-0
drivers/net/ehea/ehea_phyp.c u64 ehea_h_query_ehea(const u64 adapter_handle, void *cb_addr) · line 540
Quoted 7 lines
drivers/net/ehea/ehea_phyp.c +1
-1@@ -535,7 +535,7 @@ u64 ehea_h_query_ehea(const u64 adapter_handle, void *cb_addr) cb_logaddr,/* R5 */ 0, 0, 0, 0, 0); /* R6-R10 */#ifdef DEBUG -ehea_dmp (cb_addr,sizeof (struct hcp_query_ehea ),"hcp_query_ehea" ); +ehea_dump (cb_addr,sizeof (struct hcp_query_ehea ),"hcp_query_ehea" );#endif return hret;
applied
PATCH 2/2 ehea: fix mutex and spinlock use
In reply to Sebastien Dugue Looks like to me that the ehea_fw_handles.lock mutex and the
ehea_bcmc_regs.lock spinlock are taken much longer than necessary and could as well be pushed inside the functions that need them (ehea_update_firmware_handles() and ehea_update_bcmc_registrations()) rather than at each callsite.
--- drivers/net/ehea/ehea_main.c | 26 ++++---------------------- 1 files changed, 4 insertions(+), 22 deletions(-)
drivers/net/ehea/ehea_main.c +4 -22
@@ -219,9 +219,11 @@ static void ehea_update_firmware_handles(void) } out_update: +mutex_lock (&ehea_fw_handles.lock);kfree (ehea_fw_handles.arr); ehea_fw_handles.arr = arr; ehea_fw_handles.num_entries = i; +mutex_unlock (&ehea_fw_handles.lock); }static void ehea_update_bcmc_registrations (void )
@@ -293,9 +295,11 @@ static void ehea_update_bcmc_registrations(void) } out_update: +spin_lock (&ehea_bcmc_regs.lock);kfree (ehea_bcmc_regs.arr); ehea_bcmc_regs.arr = arr; ehea_bcmc_regs.num_entries = i; +spin_unlock (&ehea_bcmc_regs.lock); }static struct net_device_stats *ehea_get_stats (struct net_device *dev)
@@ -1770,8 +1774,6 @@ static int ehea_set_mac_addr(struct net_device *dev, void *sa)memcpy (dev->dev_addr, mac_addr->sa_data, dev->addr_len); -spin_lock (&ehea_bcmc_regs.lock); -/* Deregister old MAC in pHYP */ if (port->state ==EHEA_PORT_UP ) { ret =ehea_broadcast_reg_helper (port,H_DEREG_BCMC );
@@ -1792,7 +1794,6 @@ static int ehea_set_mac_addr(struct net_device *dev, void *sa) out_upregs:ehea_update_bcmc_registrations (); -spin_unlock (&ehea_bcmc_regs.lock); out_free:kfree (cb0); out:
@@ -1954,8 +1955,6 @@ static void ehea_set_multicast_list(struct net_device *dev) }ehea_promiscuous (dev,0 ); -spin_lock (&ehea_bcmc_regs.lock); -if (dev->flags &IFF_ALLMULTI ) {ehea_allmulti (dev,1 ); goto out;
@@ -1985,7 +1984,6 @@ static void ehea_set_multicast_list(struct net_device *dev) } out:ehea_update_bcmc_registrations (); -spin_unlock (&ehea_bcmc_regs.lock);return ; }
@@ -2466,8 +2464,6 @@ static int ehea_up(struct net_device *dev)if (port->state ==EHEA_PORT_UP )return 0 ; -mutex_lock (&ehea_fw_handles.lock); - ret =ehea_port_res_setup (port, port->num_def_qps, port->num_add_tx_qps);if (ret) {
@@ -2504,8 +2500,6 @@ static int ehea_up(struct net_device *dev) } } -spin_lock (&ehea_bcmc_regs.lock); - ret =ehea_broadcast_reg_helper (port,H_REG_BCMC );if (ret) { ret = -EIO ;
@@ -2527,10 +2521,8 @@ out:ehea_info ("Failed starting %s. ret=%i" , dev->name, ret);ehea_update_bcmc_registrations (); -spin_unlock (&ehea_bcmc_regs.lock);ehea_update_firmware_handles (); -mutex_unlock (&ehea_fw_handles.lock);return ret; }
@@ -2580,9 +2572,6 @@ static int ehea_down(struct net_device *dev)if (port->state ==EHEA_PORT_DOWN )return 0 ; -mutex_lock (&ehea_fw_handles.lock); - -spin_lock (&ehea_bcmc_regs.lock);ehea_drop_multicast_list (dev);ehea_broadcast_reg_helper (port,H_DEREG_BCMC );
@@ -2591,7 +2580,6 @@ static int ehea_down(struct net_device *dev) port->state =EHEA_PORT_DOWN ;ehea_update_bcmc_registrations (); -spin_unlock (&ehea_bcmc_regs.lock); ret =ehea_clean_all_portres (port);if (ret)
@@ -2599,7 +2587,6 @@ static int ehea_down(struct net_device *dev) dev->name, ret);ehea_update_firmware_handles (); -mutex_unlock (&ehea_fw_handles.lock);return ret; }
@@ -3378,7 +3365,6 @@ static int __devinit ehea_probe_adapter(struct of_device *dev,ehea_error ("Invalid ibmebus device probed" );return -EINVAL ; } -mutex_lock (&ehea_fw_handles.lock); adapter =kzalloc (sizeof (*adapter),GFP_KERNEL );if (!adapter) {
@@ -3462,7 +3448,6 @@ out_free_ad: out:ehea_update_firmware_handles (); -mutex_unlock (&ehea_fw_handles.lock);return ret; }
@@ -3481,8 +3466,6 @@ static int __devexit ehea_remove(struct of_device *dev)flush_scheduled_work (); -mutex_lock (&ehea_fw_handles.lock); -ibmebus_free_irq (adapter->neq->attr.ist1, adapter);tasklet_kill (&adapter->neq_tasklet);
@@ -3492,7 +3475,6 @@ static int __devexit ehea_remove(struct of_device *dev)kfree (adapter);ehea_update_firmware_handles (); -mutex_unlock (&ehea_fw_handles.lock);return 0 ; }
Signature · 1 line
1.6.0.1.308.gede4c
Jeff Garzik
AppliedRe: [PATCH 2/2] ehea: fix mutex and spinlock use
Quoted 145 lines · Sebastien Dugue
Looks like to me that the ehea_fw_handles.lock mutex and the
ehea_bcmc_regs.lock spinlock are taken much longer than necessary and could as well be pushed inside the functions that need them (ehea_update_firmware_handles()andehea_update_bcmc_registrations()) rather than at each callsite.Signed-off-by Sebastien Dugue <[email protected]>--- drivers/net/ehea/ehea_main.c | 26 ++++---------------------- 1 files changed, 4 insertions(+), 22 deletions(-)drivers/net/ehea/ehea_main.c +4
-19@@ -219,9 +219,11 @@ static void ehea_update_firmware_handles(void) } out_update: +mutex_lock (&ehea_fw_handles.lock);kfree (ehea_fw_handles.arr); ehea_fw_handles.arr = arr; ehea_fw_handles.num_entries = i; +mutex_unlock (&ehea_fw_handles.lock); }static void ehea_update_bcmc_registrations (void )@@ -293,9 +295,11 @@ static void ehea_update_bcmc_registrations(void) } out_update: +spin_lock (&ehea_bcmc_regs.lock);kfree (ehea_bcmc_regs.arr); ehea_bcmc_regs.arr = arr; ehea_bcmc_regs.num_entries = i; +spin_unlock (&ehea_bcmc_regs.lock); }static struct net_device_stats *ehea_get_stats (struct net_device *dev)@@ -1770,8 +1774,6 @@ static int ehea_set_mac_addr(struct net_device *dev, void *sa)memcpy (dev->dev_addr, mac_addr->sa_data, dev->addr_len); -spin_lock (&ehea_bcmc_regs.lock); -/* Deregister old MAC in pHYP */ if (port->state ==EHEA_PORT_UP ) { ret =ehea_broadcast_reg_helper (port,H_DEREG_BCMC );@@ -1792,7 +1794,6 @@ static int ehea_set_mac_addr(struct net_device *dev, void *sa) out_upregs:ehea_update_bcmc_registrations (); -spin_unlock (&ehea_bcmc_regs.lock); out_free:kfree (cb0); out:@@ -1954,8 +1955,6 @@ static void ehea_set_multicast_list(struct net_device *dev) }ehea_promiscuous (dev,0 ); -spin_lock (&ehea_bcmc_regs.lock); -if (dev->flags &IFF_ALLMULTI ) {ehea_allmulti (dev,1 ); goto out;@@ -1985,7 +1984,6 @@ static void ehea_set_multicast_list(struct net_device *dev) } out:ehea_update_bcmc_registrations (); -spin_unlock (&ehea_bcmc_regs.lock);return ; }@@ -2466,8 +2464,6 @@ static int ehea_up(struct net_device *dev)if (port->state ==EHEA_PORT_UP )return 0 ; -mutex_lock (&ehea_fw_handles.lock); - ret =ehea_port_res_setup (port, port->num_def_qps, port->num_add_tx_qps);if (ret) {@@ -2504,8 +2500,6 @@ static int ehea_up(struct net_device *dev) } } -spin_lock (&ehea_bcmc_regs.lock); - ret =ehea_broadcast_reg_helper (port,H_REG_BCMC );if (ret) { ret = -EIO ;@@ -2527,10 +2521,8 @@ out:ehea_info ("Failed starting %s. ret=%i" , dev->name, ret);ehea_update_bcmc_registrations (); -spin_unlock (&ehea_bcmc_regs.lock);ehea_update_firmware_handles (); -mutex_unlock (&ehea_fw_handles.lock);return ret; }@@ -2580,9 +2572,6 @@ static int ehea_down(struct net_device *dev)if (port->state ==EHEA_PORT_DOWN )return 0 ; -mutex_lock (&ehea_fw_handles.lock); - -spin_lock (&ehea_bcmc_regs.lock);ehea_drop_multicast_list (dev);ehea_broadcast_reg_helper (port,H_DEREG_BCMC );@@ -2591,7 +2580,6 @@ static int ehea_down(struct net_device *dev) port->state =EHEA_PORT_DOWN ;ehea_update_bcmc_registrations (); -spin_unlock (&ehea_bcmc_regs.lock); ret =ehea_clean_all_portres (port);if (ret)@@ -2599,7 +2587,6 @@ static int ehea_down(struct net_device *dev) dev->name, ret);ehea_update_firmware_handles (); -mutex_unlock (&ehea_fw_handles.lock);return ret; }@@ -3378,7 +3365,6 @@ static int __devinit ehea_probe_adapter(struct of_device *dev,ehea_error ("Invalid ibmebus device probed" );return -EINVAL ; } -mutex_lock (&ehea_fw_handles.lock); adapter =kzalloc (sizeof (*adapter),GFP_KERNEL );if (!adapter) {@@ -3462,7 +3448,6 @@ out_free_ad: out:ehea_update_firmware_handles (); -mutex_unlock (&ehea_fw_handles.lock);return ret; }
drivers/net/ehea/ehea_main.c static int __devexit ehea_remove(struct of_device *dev) · line 3470
Quoted 7 lines
drivers/net/ehea/ehea_main.c +0
-2@@ -3481,8 +3466,6 @@ static int __devexit ehea_remove(struct of_device *dev)flush_scheduled_work (); -mutex_lock (&ehea_fw_handles.lock); -ibmebus_free_irq (adapter->neq->attr.ist1, adapter);tasklet_kill (&adapter->neq_tasklet);
applied
Thomas Klein
NAKRe: [PATCH 2/2] ehea: fix mutex and spinlock use
In reply to Sebastien DugueNACK!
I regret but this patch is wrong. It is not sufficient to only lock the replacement of an old list with a new list. Building up those lists is a 3-step process:
1. Count the number of entries a list will contain and allocate mem
2. Fill the list
3. Replace old list with updated list
It's obvious that the contents of the list may not change during this procedure. That means that not only the list build-up procedure must be locked. It must be assured that no function that modifies the list's content can be called while another list update is in progress.
Jeff, please revert this patch.
Thanks
Thomas
Quoted 161 lines · Sebastien Dugue
Looks like to me that the ehea_fw_handles.lock mutex and the
ehea_bcmc_regs.lock spinlock are taken much longer than necessary and could as well be pushed inside the functions that need them (ehea_update_firmware_handles()andehea_update_bcmc_registrations()) rather than at each callsite.Signed-off-by Sebastien Dugue <[email protected]>--- drivers/net/ehea/ehea_main.c | 26 ++++---------------------- 1 files changed, 4 insertions(+), 22 deletions(-)drivers/net/ehea/ehea_main.c +4
-22@@ -219,9 +219,11 @@ static void ehea_update_firmware_handles(void) } out_update: +mutex_lock (&ehea_fw_handles.lock);kfree (ehea_fw_handles.arr); ehea_fw_handles.arr = arr; ehea_fw_handles.num_entries = i; +mutex_unlock (&ehea_fw_handles.lock); }static void ehea_update_bcmc_registrations (void )@@ -293,9 +295,11 @@ static void ehea_update_bcmc_registrations(void) } out_update: +spin_lock (&ehea_bcmc_regs.lock);kfree (ehea_bcmc_regs.arr); ehea_bcmc_regs.arr = arr; ehea_bcmc_regs.num_entries = i; +spin_unlock (&ehea_bcmc_regs.lock); }static struct net_device_stats *ehea_get_stats (struct net_device *dev)@@ -1770,8 +1774,6 @@ static int ehea_set_mac_addr(struct net_device *dev, void *sa)memcpy (dev->dev_addr, mac_addr->sa_data, dev->addr_len); -spin_lock (&ehea_bcmc_regs.lock); -/* Deregister old MAC in pHYP */ if (port->state ==EHEA_PORT_UP ) { ret =ehea_broadcast_reg_helper (port,H_DEREG_BCMC );@@ -1792,7 +1794,6 @@ static int ehea_set_mac_addr(struct net_device *dev, void *sa) out_upregs:ehea_update_bcmc_registrations (); -spin_unlock (&ehea_bcmc_regs.lock); out_free:kfree (cb0); out:@@ -1954,8 +1955,6 @@ static void ehea_set_multicast_list(struct net_device *dev) }ehea_promiscuous (dev,0 ); -spin_lock (&ehea_bcmc_regs.lock); -if (dev->flags &IFF_ALLMULTI ) {ehea_allmulti (dev,1 ); goto out;@@ -1985,7 +1984,6 @@ static void ehea_set_multicast_list(struct net_device *dev) } out:ehea_update_bcmc_registrations (); -spin_unlock (&ehea_bcmc_regs.lock);return ; }@@ -2466,8 +2464,6 @@ static int ehea_up(struct net_device *dev)if (port->state ==EHEA_PORT_UP )return 0 ; -mutex_lock (&ehea_fw_handles.lock); - ret =ehea_port_res_setup (port, port->num_def_qps, port->num_add_tx_qps);if (ret) {@@ -2504,8 +2500,6 @@ static int ehea_up(struct net_device *dev) } } -spin_lock (&ehea_bcmc_regs.lock); - ret =ehea_broadcast_reg_helper (port,H_REG_BCMC );if (ret) { ret = -EIO ;@@ -2527,10 +2521,8 @@ out:ehea_info ("Failed starting %s. ret=%i" , dev->name, ret);ehea_update_bcmc_registrations (); -spin_unlock (&ehea_bcmc_regs.lock);ehea_update_firmware_handles (); -mutex_unlock (&ehea_fw_handles.lock);return ret; }@@ -2580,9 +2572,6 @@ static int ehea_down(struct net_device *dev)if (port->state ==EHEA_PORT_DOWN )return 0 ; -mutex_lock (&ehea_fw_handles.lock); - -spin_lock (&ehea_bcmc_regs.lock);ehea_drop_multicast_list (dev);ehea_broadcast_reg_helper (port,H_DEREG_BCMC );@@ -2591,7 +2580,6 @@ static int ehea_down(struct net_device *dev) port->state =EHEA_PORT_DOWN ;ehea_update_bcmc_registrations (); -spin_unlock (&ehea_bcmc_regs.lock); ret =ehea_clean_all_portres (port);if (ret)@@ -2599,7 +2587,6 @@ static int ehea_down(struct net_device *dev) dev->name, ret);ehea_update_firmware_handles (); -mutex_unlock (&ehea_fw_handles.lock);return ret; }@@ -3378,7 +3365,6 @@ static int __devinit ehea_probe_adapter(struct of_device *dev,ehea_error ("Invalid ibmebus device probed" );return -EINVAL ; } -mutex_lock (&ehea_fw_handles.lock); adapter =kzalloc (sizeof (*adapter),GFP_KERNEL );if (!adapter) {@@ -3462,7 +3448,6 @@ out_free_ad: out:ehea_update_firmware_handles (); -mutex_unlock (&ehea_fw_handles.lock);return ret; }@@ -3481,8 +3466,6 @@ static int __devexit ehea_remove(struct of_device *dev)flush_scheduled_work (); -mutex_lock (&ehea_fw_handles.lock); -ibmebus_free_irq (adapter->neq->attr.ist1, adapter);tasklet_kill (&adapter->neq_tasklet);@@ -3492,7 +3475,6 @@ static int __devexit ehea_remove(struct of_device *dev)kfree (adapter);ehea_update_firmware_handles (); -mutex_unlock (&ehea_fw_handles.lock);return 0 ; }
Re: [PATCH 2/2] ehea: fix mutex and spinlock use
Quoted 10 lines · On Mon, 15 Sep 2008 17:18:27 +0200 Thomas Klein <[email protected]> wrote:
NACK!
I regret but this patch is wrong. It is not sufficient to only lock the replacement of an old list with a new list. Building up those lists is a 3-step process:
1. Count the number of entries a list will contain and allocate mem
2. Fill the list
3. Replace old list with updated list
Quoted 6 lines
It's obvious that the contents of the list may not change during this procedure. That means that not only the list build-up procedure must be locked. It must be assured that no function that modifies the list's content can be called while another list update is in progress.
Jeff, please revert this patch.
OK, your call, you know the code better than I do.
But the locking could at least be pushed into ehea_update_firmware_handles()
and ehea_update_bcmc_registrations() instead of being at each call site.
Thanks,
Sebastien.
Quoted 175 lines
Thanks
ThomasQuoted 154 lines · Sebastien Dugue
Looks like to me that the ehea_fw_handles.lock mutex and the
ehea_bcmc_regs.lock spinlock are taken much longer than necessary and could as well be pushed inside the functions that need them (ehea_update_firmware_handles()andehea_update_bcmc_registrations()) rather than at each callsite.Signed-off-by Sebastien Dugue <[email protected]>--- drivers/net/ehea/ehea_main.c | 26 ++++---------------------- 1 files changed, 4 insertions(+), 22 deletions(-)drivers/net/ehea/ehea_main.c +4
-21@@ -219,9 +219,11 @@ static void ehea_update_firmware_handles(void) } out_update: +mutex_lock (&ehea_fw_handles.lock);kfree (ehea_fw_handles.arr); ehea_fw_handles.arr = arr; ehea_fw_handles.num_entries = i; +mutex_unlock (&ehea_fw_handles.lock); }static void ehea_update_bcmc_registrations (void )@@ -293,9 +295,11 @@ static void ehea_update_bcmc_registrations(void) } out_update: +spin_lock (&ehea_bcmc_regs.lock);kfree (ehea_bcmc_regs.arr); ehea_bcmc_regs.arr = arr; ehea_bcmc_regs.num_entries = i; +spin_unlock (&ehea_bcmc_regs.lock); }static struct net_device_stats *ehea_get_stats (struct net_device *dev)@@ -1770,8 +1774,6 @@ static int ehea_set_mac_addr(struct net_device *dev, void *sa)memcpy (dev->dev_addr, mac_addr->sa_data, dev->addr_len); -spin_lock (&ehea_bcmc_regs.lock); -/* Deregister old MAC in pHYP */ if (port->state ==EHEA_PORT_UP ) { ret =ehea_broadcast_reg_helper (port,H_DEREG_BCMC );@@ -1792,7 +1794,6 @@ static int ehea_set_mac_addr(struct net_device *dev, void *sa) out_upregs:ehea_update_bcmc_registrations (); -spin_unlock (&ehea_bcmc_regs.lock); out_free:kfree (cb0); out:@@ -1954,8 +1955,6 @@ static void ehea_set_multicast_list(struct net_device *dev) }ehea_promiscuous (dev,0 ); -spin_lock (&ehea_bcmc_regs.lock); -if (dev->flags &IFF_ALLMULTI ) {ehea_allmulti (dev,1 ); goto out;@@ -1985,7 +1984,6 @@ static void ehea_set_multicast_list(struct net_device *dev) } out:ehea_update_bcmc_registrations (); -spin_unlock (&ehea_bcmc_regs.lock);return ; }@@ -2466,8 +2464,6 @@ static int ehea_up(struct net_device *dev)if (port->state ==EHEA_PORT_UP )return 0 ; -mutex_lock (&ehea_fw_handles.lock); - ret =ehea_port_res_setup (port, port->num_def_qps, port->num_add_tx_qps);if (ret) {@@ -2504,8 +2500,6 @@ static int ehea_up(struct net_device *dev) } } -spin_lock (&ehea_bcmc_regs.lock); - ret =ehea_broadcast_reg_helper (port,H_REG_BCMC );if (ret) { ret = -EIO ;@@ -2527,10 +2521,8 @@ out:ehea_info ("Failed starting %s. ret=%i" , dev->name, ret);ehea_update_bcmc_registrations (); -spin_unlock (&ehea_bcmc_regs.lock);ehea_update_firmware_handles (); -mutex_unlock (&ehea_fw_handles.lock);return ret; }@@ -2580,9 +2572,6 @@ static int ehea_down(struct net_device *dev)if (port->state ==EHEA_PORT_DOWN )return 0 ; -mutex_lock (&ehea_fw_handles.lock); - -spin_lock (&ehea_bcmc_regs.lock);ehea_drop_multicast_list (dev);ehea_broadcast_reg_helper (port,H_DEREG_BCMC );@@ -2591,7 +2580,6 @@ static int ehea_down(struct net_device *dev) port->state =EHEA_PORT_DOWN ;ehea_update_bcmc_registrations (); -spin_unlock (&ehea_bcmc_regs.lock); ret =ehea_clean_all_portres (port);if (ret)@@ -2599,7 +2587,6 @@ static int ehea_down(struct net_device *dev) dev->name, ret);ehea_update_firmware_handles (); -mutex_unlock (&ehea_fw_handles.lock);return ret; }@@ -3378,7 +3365,6 @@ static int __devinit ehea_probe_adapter(struct of_device *dev,ehea_error ("Invalid ibmebus device probed" );return -EINVAL ; } -mutex_lock (&ehea_fw_handles.lock); adapter =kzalloc (sizeof (*adapter),GFP_KERNEL );if (!adapter) {@@ -3462,7 +3448,6 @@ out_free_ad: out:ehea_update_firmware_handles (); -mutex_unlock (&ehea_fw_handles.lock);return ret; }@@ -3481,8 +3466,6 @@ static int __devexit ehea_remove(struct of_device *dev)flush_scheduled_work (); -mutex_lock (&ehea_fw_handles.lock); -ibmebus_free_irq (adapter->neq->attr.ist1, adapter);tasklet_kill (&adapter->neq_tasklet);drivers/net/ehea/ehea_main.c static int __devexit ehea_remove(struct of_device *dev) · line 3480
Quoted 7 lines
drivers/net/ehea/ehea_main.c +0
-1@@ -3492,7 +3475,6 @@ static int __devexit ehea_remove(struct of_device *dev)kfree (adapter);ehea_update_firmware_handles (); -mutex_unlock (&ehea_fw_handles.lock);return 0 ; }_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/2] ehea: fix mutex and spinlock use
Quoted 19 lines · Sebastien Dugue
Quoted 10 lines · On Mon, 15 Sep 2008 17:18:27 +0200 Thomas Klein <[email protected]> wrote:
NACK!
I regret but this patch is wrong. It is not sufficient to only lock the replacement of an old list with a new list. Building up those lists is a 3-step process:
1. Count the number of entries a list will contain and allocate mem
2. Fill the list
3. Replace old list with updated listQuoted 6 lines
It's obvious that the contents of the list may not change during this procedure. That means that not only the list build-up procedure must be locked. It must be assured that no function that modifies the list's content can be called while another list update is in progress.
Jeff, please revert this patch.
Quoted 9 lines
OK, your call, you know the code better than I do.
But the locking could at least be pushed into
ehea_update_firmware_handles()
andehea_update_bcmc_registrations()instead of being at each call site.Thanks,
Sebastien.
It unfortunately can't. As I already mentioned "it must be assured that no function that modifies the list's content can be called while another list update is in progress". This means that for example ehea_broadcast_reg_helper() may not run during a list update. That's why the locks surround these function calls as well.
Thomas
Quoted 173 lines
Quoted 173 lines
Thanks
ThomasQuoted 154 lines · Sebastien Dugue
Looks like to me that the ehea_fw_handles.lock mutex and the
ehea_bcmc_regs.lock spinlock are taken much longer than necessary and could as well be pushed inside the functions that need them (ehea_update_firmware_handles()andehea_update_bcmc_registrations()) rather than at each callsite.Signed-off-by Sebastien Dugue <[email protected]>--- drivers/net/ehea/ehea_main.c | 26 ++++---------------------- 1 files changed, 4 insertions(+), 22 deletions(-)drivers/net/ehea/ehea_main.c +4
-21@@ -219,9 +219,11 @@ static void ehea_update_firmware_handles(void) } out_update: +mutex_lock (&ehea_fw_handles.lock);kfree (ehea_fw_handles.arr); ehea_fw_handles.arr = arr; ehea_fw_handles.num_entries = i; +mutex_unlock (&ehea_fw_handles.lock); }static void ehea_update_bcmc_registrations (void )@@ -293,9 +295,11 @@ static void ehea_update_bcmc_registrations(void) } out_update: +spin_lock (&ehea_bcmc_regs.lock);kfree (ehea_bcmc_regs.arr); ehea_bcmc_regs.arr = arr; ehea_bcmc_regs.num_entries = i; +spin_unlock (&ehea_bcmc_regs.lock); }static struct net_device_stats *ehea_get_stats (struct net_device *dev)@@ -1770,8 +1774,6 @@ static int ehea_set_mac_addr(struct net_device *dev, void *sa)memcpy (dev->dev_addr, mac_addr->sa_data, dev->addr_len); -spin_lock (&ehea_bcmc_regs.lock); -/* Deregister old MAC in pHYP */ if (port->state ==EHEA_PORT_UP ) { ret =ehea_broadcast_reg_helper (port,H_DEREG_BCMC );@@ -1792,7 +1794,6 @@ static int ehea_set_mac_addr(struct net_device *dev, void *sa) out_upregs:ehea_update_bcmc_registrations (); -spin_unlock (&ehea_bcmc_regs.lock); out_free:kfree (cb0); out:@@ -1954,8 +1955,6 @@ static void ehea_set_multicast_list(struct net_device *dev) }ehea_promiscuous (dev,0 ); -spin_lock (&ehea_bcmc_regs.lock); -if (dev->flags &IFF_ALLMULTI ) {ehea_allmulti (dev,1 ); goto out;@@ -1985,7 +1984,6 @@ static void ehea_set_multicast_list(struct net_device *dev) } out:ehea_update_bcmc_registrations (); -spin_unlock (&ehea_bcmc_regs.lock);return ; }@@ -2466,8 +2464,6 @@ static int ehea_up(struct net_device *dev)if (port->state ==EHEA_PORT_UP )return 0 ; -mutex_lock (&ehea_fw_handles.lock); - ret =ehea_port_res_setup (port, port->num_def_qps, port->num_add_tx_qps);if (ret) {@@ -2504,8 +2500,6 @@ static int ehea_up(struct net_device *dev) } } -spin_lock (&ehea_bcmc_regs.lock); - ret =ehea_broadcast_reg_helper (port,H_REG_BCMC );if (ret) { ret = -EIO ;@@ -2527,10 +2521,8 @@ out:ehea_info ("Failed starting %s. ret=%i" , dev->name, ret);ehea_update_bcmc_registrations (); -spin_unlock (&ehea_bcmc_regs.lock);ehea_update_firmware_handles (); -mutex_unlock (&ehea_fw_handles.lock);return ret; }@@ -2580,9 +2572,6 @@ static int ehea_down(struct net_device *dev)if (port->state ==EHEA_PORT_DOWN )return 0 ; -mutex_lock (&ehea_fw_handles.lock); - -spin_lock (&ehea_bcmc_regs.lock);ehea_drop_multicast_list (dev);ehea_broadcast_reg_helper (port,H_DEREG_BCMC );@@ -2591,7 +2580,6 @@ static int ehea_down(struct net_device *dev) port->state =EHEA_PORT_DOWN ;ehea_update_bcmc_registrations (); -spin_unlock (&ehea_bcmc_regs.lock); ret =ehea_clean_all_portres (port);if (ret)@@ -2599,7 +2587,6 @@ static int ehea_down(struct net_device *dev) dev->name, ret);ehea_update_firmware_handles (); -mutex_unlock (&ehea_fw_handles.lock);return ret; }@@ -3378,7 +3365,6 @@ static int __devinit ehea_probe_adapter(struct of_device *dev,ehea_error ("Invalid ibmebus device probed" );return -EINVAL ; } -mutex_lock (&ehea_fw_handles.lock); adapter =kzalloc (sizeof (*adapter),GFP_KERNEL );if (!adapter) {@@ -3462,7 +3448,6 @@ out_free_ad: out:ehea_update_firmware_handles (); -mutex_unlock (&ehea_fw_handles.lock);return ret; }@@ -3481,8 +3466,6 @@ static int __devexit ehea_remove(struct of_device *dev)flush_scheduled_work (); -mutex_lock (&ehea_fw_handles.lock); -ibmebus_free_irq (adapter->neq->attr.ist1, adapter);tasklet_kill (&adapter->neq_tasklet);drivers/net/ehea/ehea_main.c static int __devexit ehea_remove(struct of_device *dev) · line 3480
Quoted 7 lines
drivers/net/ehea/ehea_main.c +0
-1@@ -3492,7 +3475,6 @@ static int __devexit ehea_remove(struct of_device *dev)kfree (adapter);ehea_update_firmware_handles (); -mutex_unlock (&ehea_fw_handles.lock);return 0 ; }_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/2] ehea: fix mutex and spinlock use
Quoted 27 lines · On Tue, 16 Sep 2008 11:13:13 +0200 Thomas Klein <[email protected]> wrote:
Quoted 26 lines · Sebastien Dugue
Quoted 10 lines · On Mon, 15 Sep 2008 17:18:27 +0200 Thomas Klein <[email protected]> wrote:
NACK!
I regret but this patch is wrong. It is not sufficient to only lock the replacement of an old list with a new list. Building up those lists is a 3-step process:
1. Count the number of entries a list will contain and allocate mem
2. Fill the list
3. Replace old list with updated listQuoted 6 lines
It's obvious that the contents of the list may not change during this procedure. That means that not only the list build-up procedure must be locked. It must be assured that no function that modifies the list's content can be called while another list update is in progress.
Jeff, please revert this patch.
OK, your call, you know the code better than I do.
But the locking could at least be pushed into
ehea_update_firmware_handles()
andehea_update_bcmc_registrations()instead of being at each call site.Thanks,
Quoted 8 lines
Quoted 2 lines
Sebastien.
It unfortunately can't. As I already mentioned "it must be assured that no function that modifies the list's content can be called while another list update is in progress". This means that for example
ehea_broadcast_reg_helper()may not run during a list update. That's why the locks surround these function calls as well.
OK, I see.
Thanks,
Sebastien.
Quoted 183 lines
Thomas
Quoted 165 lines
Quoted 165 lines
Thanks
ThomasQuoted 159 lines · Sebastien Dugue
Looks like to me that the ehea_fw_handles.lock mutex and the
ehea_bcmc_regs.lock spinlock are taken much longer than necessary and could as well be pushed inside the functions that need them (ehea_update_firmware_handles()andehea_update_bcmc_registrations()) rather than at each callsite.Signed-off-by Sebastien Dugue <[email protected]>--- drivers/net/ehea/ehea_main.c | 26 ++++---------------------- 1 files changed, 4 insertions(+), 22 deletions(-)drivers/net/ehea/ehea_main.c +4
-22@@ -219,9 +219,11 @@ static void ehea_update_firmware_handles(void) } out_update: +mutex_lock (&ehea_fw_handles.lock);kfree (ehea_fw_handles.arr); ehea_fw_handles.arr = arr; ehea_fw_handles.num_entries = i; +mutex_unlock (&ehea_fw_handles.lock); }static void ehea_update_bcmc_registrations (void )@@ -293,9 +295,11 @@ static void ehea_update_bcmc_registrations(void) } out_update: +spin_lock (&ehea_bcmc_regs.lock);kfree (ehea_bcmc_regs.arr); ehea_bcmc_regs.arr = arr; ehea_bcmc_regs.num_entries = i; +spin_unlock (&ehea_bcmc_regs.lock); }static struct net_device_stats *ehea_get_stats (struct net_device *dev)@@ -1770,8 +1774,6 @@ static int ehea_set_mac_addr(struct net_device *dev, void *sa)memcpy (dev->dev_addr, mac_addr->sa_data, dev->addr_len); -spin_lock (&ehea_bcmc_regs.lock); -/* Deregister old MAC in pHYP */ if (port->state ==EHEA_PORT_UP ) { ret =ehea_broadcast_reg_helper (port,H_DEREG_BCMC );@@ -1792,7 +1794,6 @@ static int ehea_set_mac_addr(struct net_device *dev, void *sa) out_upregs:ehea_update_bcmc_registrations (); -spin_unlock (&ehea_bcmc_regs.lock); out_free:kfree (cb0); out:@@ -1954,8 +1955,6 @@ static void ehea_set_multicast_list(struct net_device *dev) }ehea_promiscuous (dev,0 ); -spin_lock (&ehea_bcmc_regs.lock); -if (dev->flags &IFF_ALLMULTI ) {ehea_allmulti (dev,1 ); goto out;@@ -1985,7 +1984,6 @@ static void ehea_set_multicast_list(struct net_device *dev) } out:ehea_update_bcmc_registrations (); -spin_unlock (&ehea_bcmc_regs.lock);return ; }@@ -2466,8 +2464,6 @@ static int ehea_up(struct net_device *dev)if (port->state ==EHEA_PORT_UP )return 0 ; -mutex_lock (&ehea_fw_handles.lock); - ret =ehea_port_res_setup (port, port->num_def_qps, port->num_add_tx_qps);if (ret) {@@ -2504,8 +2500,6 @@ static int ehea_up(struct net_device *dev) } } -spin_lock (&ehea_bcmc_regs.lock); - ret =ehea_broadcast_reg_helper (port,H_REG_BCMC );if (ret) { ret = -EIO ;@@ -2527,10 +2521,8 @@ out:ehea_info ("Failed starting %s. ret=%i" , dev->name, ret);ehea_update_bcmc_registrations (); -spin_unlock (&ehea_bcmc_regs.lock);ehea_update_firmware_handles (); -mutex_unlock (&ehea_fw_handles.lock);return ret; }@@ -2580,9 +2572,6 @@ static int ehea_down(struct net_device *dev)if (port->state ==EHEA_PORT_DOWN )return 0 ; -mutex_lock (&ehea_fw_handles.lock); - -spin_lock (&ehea_bcmc_regs.lock);ehea_drop_multicast_list (dev);ehea_broadcast_reg_helper (port,H_DEREG_BCMC );@@ -2591,7 +2580,6 @@ static int ehea_down(struct net_device *dev) port->state =EHEA_PORT_DOWN ;ehea_update_bcmc_registrations (); -spin_unlock (&ehea_bcmc_regs.lock); ret =ehea_clean_all_portres (port);if (ret)@@ -2599,7 +2587,6 @@ static int ehea_down(struct net_device *dev) dev->name, ret);ehea_update_firmware_handles (); -mutex_unlock (&ehea_fw_handles.lock);return ret; }@@ -3378,7 +3365,6 @@ static int __devinit ehea_probe_adapter(struct of_device *dev,ehea_error ("Invalid ibmebus device probed" );return -EINVAL ; } -mutex_lock (&ehea_fw_handles.lock); adapter =kzalloc (sizeof (*adapter),GFP_KERNEL );if (!adapter) {@@ -3462,7 +3448,6 @@ out_free_ad: out:ehea_update_firmware_handles (); -mutex_unlock (&ehea_fw_handles.lock);return ret; }@@ -3481,8 +3466,6 @@ static int __devexit ehea_remove(struct of_device *dev)flush_scheduled_work (); -mutex_lock (&ehea_fw_handles.lock); -ibmebus_free_irq (adapter->neq->attr.ist1, adapter);tasklet_kill (&adapter->neq_tasklet);@@ -3492,7 +3475,6 @@ static int __devexit ehea_remove(struct of_device *dev)kfree (adapter);ehea_update_firmware_handles (); -mutex_unlock (&ehea_fw_handles.lock);Quoted 8 lines
Quoted 8 lines
drivers/net/ehea/ehea_main.c static int __devexit ehea_remove(struct of_device *dev) · line 3480
Quoted 2 lines
drivers/net/ehea/ehea_main.c +0
-0return 0 ; }_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev
PATCH 2/2 ehea: fix possible DLPAR/mem deadlock
In reply to Sebastien DugueForce serialization of userspace-triggered DLPAR/mem operations
---
Patch created against 2.6.34-rc4
/drivers/net/ehea/ehea.h +1 -1
@@ -40,7 +40,7 @@#include <asm/io.h> #define DRV_NAME "ehea" -#define DRV_VERSION "EHEA_0102" +#define DRV_VERSION "EHEA_0103"/* eHEA capability flags */ #define DLPAR_PORT_ADD_REM 1
/drivers/net/ehea/ehea_main.c +14 -5
@@ -2889,7 +2889,6 @@ static void ehea_rereg_mrs(struct work_sint ret, i;struct ehea_adapter *adapter; -mutex_lock (&dlpar_mem_lock);ehea_info ("LPAR memory changed - re-initializing driver" );list_for_each_entry (adapter, &adapter_list, list)
@@ -2959,7 +2958,6 @@ static void ehea_rereg_mrs(struct work_s }ehea_info ("re-initializing driver complete" ); out: -mutex_unlock (&dlpar_mem_lock);return ; }
@@ -3542,7 +3540,14 @@ void ehea_crash_handler(void)static int ehea_mem_notifier (struct notifier_block *nb,unsigned long action,void *data) { +int ret =NOTIFY_BAD ;struct memory_notify *arg = data; + +if (!mutex_trylock (&dlpar_mem_lock)) { +ehea_info ("ehea_mem_notifier must not be called parallelized" ); + goto out; + } +switch (action) {case MEM_CANCEL_OFFLINE :ehea_info ("memory offlining canceled" );
@@ -3551,14 +3556,14 @@ static int ehea_mem_notifier(struct notiehea_info ("memory is going online" );set_bit (__EHEA_STOP_XFER, &ehea_driver_flags);if (ehea_add_sect_bmap (arg->start_pfn, arg->nr_pages)) -return NOTIFY_BAD ; + goto out_unlock;ehea_rereg_mrs (NULL );break ;case MEM_GOING_OFFLINE :ehea_info ("memory is going offline" );set_bit (__EHEA_STOP_XFER, &ehea_driver_flags);if (ehea_rem_sect_bmap (arg->start_pfn, arg->nr_pages)) -return NOTIFY_BAD ; + goto out_unlock;ehea_rereg_mrs (NULL );break ;default :
@@ -3566,8 +3571,12 @@ static int ehea_mem_notifier(struct noti }ehea_update_firmware_handles (); + ret =NOTIFY_OK ; -return NOTIFY_OK ; +out_unlock: +mutex_unlock (&dlpar_mem_lock); +out: +return ret; }static struct notifier_block ehea_mem_nb = {