Chris Leech wrote:
> Rather than rely on the hostlist_lock to be held while creating exchange
> managers, serialize fcoe instance creation and destruction with a mutex.
> This will allow the hostlist addition to be moved out of fcoe_if_create(),
> which will simplify NPIV support.
>
> RTNL mutex was used because it needs to be held in parts of both the create
> and destroy path anyway, and because the netdevice notifier will be called
> with RTNL held. If a new mutex was added and fine-grained use of RTNL was
> maintained, then destroying the interface when the netdevice was unregistered
> would deadlock on RTNL unless it was deferred using a workqueue or similar.
>
> Signed-off-by: Chris Leech <[email protected]>
> ---
>
> drivers/scsi/fcoe/fcoe.c | 68
> ++++++++++++++++++++++++++++------------------
> 1 files changed, 41 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> index 49a9d5b..a4b8991 100644
> --- a/drivers/scsi/fcoe/fcoe.c
> +++ b/drivers/scsi/fcoe/fcoe.c
> @@ -49,9 +49,9 @@ MODULE_AUTHOR("Open-FCoE.org");
> MODULE_DESCRIPTION("FCoE");
> MODULE_LICENSE("GPL v2");
>
> -/* fcoe host list */
> +/* fcoe host list
> + * must be accessed under rtnl_lock */
> LIST_HEAD(fcoe_hostlist);
> -DEFINE_RWLOCK(fcoe_hostlist_lock);
> DEFINE_PER_CPU(struct fcoe_percpu_s, fcoe_percpu);
>
> /* Function Prototypes */
> @@ -186,13 +186,11 @@ static int fcoe_interface_setup(struct fcoe_interface
> *fcoe,
> * or enter promiscuous mode if not capable of listening
> * for multiple unicast MACs.
> */
> - rtnl_lock();
> memcpy(flogi_maddr, (u8[6]) FC_FCOE_FLOGI_MAC, ETH_ALEN);
> dev_unicast_add(netdev, flogi_maddr);
> if (fip->spma)
> dev_unicast_add(netdev, fip->ctl_src_addr);
> dev_mc_add(netdev, FIP_ALL_ENODE_MACS, ETH_ALEN, 0);
> - rtnl_unlock();
>
> /*
> * setup the receive function from ethernet driver
> @@ -259,7 +257,6 @@ void fcoe_interface_cleanup(struct fcoe_interface *fcoe)
> dev_remove_pack(&fcoe->fip_packet_type);
>
> /* Delete secondary MAC addresses */
> - rtnl_lock();
> memcpy(flogi_maddr, (u8[6]) FC_FCOE_FLOGI_MAC, ETH_ALEN);
> dev_unicast_delete(netdev, flogi_maddr);
> if (!is_zero_ether_addr(fip->data_src_addr))
> @@ -267,7 +264,6 @@ void fcoe_interface_cleanup(struct fcoe_interface *fcoe)
> if (fip->spma)
> dev_unicast_delete(netdev, fip->ctl_src_addr);
> dev_mc_delete(netdev, FIP_ALL_ENODE_MACS, ETH_ALEN, 0);
> - rtnl_unlock();
> }
>
> /**
> @@ -516,8 +512,6 @@ bool fcoe_oem_match(struct fc_frame *fp)
> * fcoe_em_config() - allocates em for this lport
> * @lp: the fcoe that em is to allocated for
> *
> - * Called with write fcoe_hostlist_lock held.
> - *
> * Returns : 0 on success
> */
> static inline int fcoe_em_config(struct fc_lport *lp)
> @@ -739,11 +733,14 @@ static struct fc_lport *fcoe_if_create(struct
> fcoe_interface *fcoe,
>
> /*
> * fcoe_em_alloc() and fcoe_hostlist_add() both
> - * need to be atomic under fcoe_hostlist_lock
> + * need to be atomic with respect to other changes to the hostlist
> * since fcoe_em_alloc() looks for an existing EM
> * instance on host list updated by fcoe_hostlist_add().
> + *
> + * This is OK because the entire create and destroy paths are called
> + * with the RTNL mutex held.
> */
> - write_lock(&fcoe_hostlist_lock);
> +
> /* lport exch manager allocation */
> rc = fcoe_em_config(lport);
> if (rc) {
> @@ -754,7 +751,6 @@ static struct fc_lport *fcoe_if_create(struct
> fcoe_interface *fcoe,
>
> /* add to lports list */
> fcoe_hostlist_add(lport);
> - write_unlock(&fcoe_hostlist_lock);
>
> dev_hold(netdev);
> fcoe_interface_get(fcoe);
> @@ -795,6 +791,7 @@ static int __init fcoe_if_init(void)
> int __exit fcoe_if_exit(void)
> {
> fc_release_transport(scsi_transport_fcoe_sw);
> + scsi_transport_fcoe_sw = NULL;
> return 0;
> }
>
> @@ -1524,14 +1521,14 @@ static int fcoe_device_notification(struct
> notifier_block *notifier,
> u32 mfs;
> int rc = NOTIFY_OK;
>
> - read_lock(&fcoe_hostlist_lock);
> + ASSERT_RTNL();
> +
> list_for_each_entry(fcoe, &fcoe_hostlist, list) {
> if (fcoe->real_dev == real_dev) {
> lp = fcoe->ctlr.lp;
> break;
> }
> }
> - read_unlock(&fcoe_hostlist_lock);
> if (lp == NULL) {
> rc = NOTIFY_DONE;
> goto out;
> @@ -1674,6 +1671,18 @@ static int fcoe_destroy(const char *buffer, struct
> kernel_param *kp)
> struct fc_lport *lport;
> int rc;
>
> + rtnl_lock();
> +
> + /*
> + * Make sure the module has been initialized, and is not about to be
> + * removed. Module paramter sysfs files are writable before the
> + * module_init function is called and after module_exit.
> + */
> + if (!scsi_transport_fcoe_sw) {
> + rc = -ENODEV;
> + goto out_nodev;
> + }
> +
> netdev = fcoe_if_to_netdev(buffer);
> if (!netdev) {
> rc = -ENODEV;
> @@ -1693,6 +1702,7 @@ static int fcoe_destroy(const char *buffer, struct
> kernel_param *kp)
> out_putdev:
> dev_put(netdev);
> out_nodev:
> + rtnl_unlock();
> return rc;
> }
>
> @@ -1710,6 +1720,18 @@ static int fcoe_create(const char *buffer, struct
> kernel_param *kp)
> struct fc_lport *lport;
> struct net_device *netdev;
>
> + rtnl_lock();
> +
> + /*
> + * Make sure the module has been initialized, and is not about to be
> + * removed. Module paramter sysfs files are writable before the
> + * module_init function is called and after module_exit.
> + */
> + if (!scsi_transport_fcoe_sw) {
> + rc = -ENODEV;
> + goto out_nodev;
> + }
> +
> netdev = fcoe_if_to_netdev(buffer);
> if (!netdev) {
> rc = -ENODEV;
> @@ -1746,14 +1768,15 @@ static int fcoe_create(const char *buffer, struct
> kernel_param *kp)
> if (!fcoe_link_ok(lport))
> fcoe_ctlr_link_up(&fcoe->ctlr);
>
> - dev_put(netdev);
> - return 0;
> + rc = 0;
> + goto out_putdev;
>
> out_free:
> fcoe_interface_put(fcoe);
> out_putdev:
> dev_put(netdev);
> out_nodev:
> + rtnl_unlock();
> return rc;
> }
>
> @@ -1872,8 +1895,6 @@ int fcoe_reset(struct Scsi_Host *shost)
> * fcoe_hostlist_lookup_port() - find the corresponding lport by a given
> device
> * @dev: this is currently ptr to net_device
> *
> - * Called with fcoe_hostlist_lock held.
> - *
> * Returns: NULL or the located fcoe_port
> */
> static struct fcoe_interface *
> @@ -1898,10 +1919,7 @@ struct fc_lport *fcoe_hostlist_lookup(const struct
> net_device *netdev)
> {
> struct fcoe_interface *fcoe;
>
> - read_lock(&fcoe_hostlist_lock);
> fcoe = fcoe_hostlist_lookup_port(netdev);
> - read_unlock(&fcoe_hostlist_lock);
> -
> return (fcoe) ? fcoe->ctlr.lp : NULL;
> }
>
> @@ -1909,8 +1927,6 @@ struct fc_lport *fcoe_hostlist_lookup(const struct
> net_device *netdev)
> * fcoe_hostlist_add() - Add a lport to lports list
> * @lp: ptr to the fc_lport to be added
> *
> - * Called with write fcoe_hostlist_lock held.
> - *
> * Returns: 0 for success
> */
> int fcoe_hostlist_add(const struct fc_lport *lport)
> @@ -1937,11 +1953,9 @@ int fcoe_hostlist_remove(const struct fc_lport *lport)
> {
> struct fcoe_interface *fcoe;
>
> - write_lock_bh(&fcoe_hostlist_lock);
> fcoe = fcoe_hostlist_lookup_port(fcoe_netdev(lport));
> BUG_ON(!fcoe);
> list_del(&fcoe->list);
> - write_unlock_bh(&fcoe_hostlist_lock);
>
> return 0;
> }
> @@ -1957,9 +1971,6 @@ static int __init fcoe_init(void)
> int rc = 0;
> struct fcoe_percpu_s *p;
>
> - INIT_LIST_HEAD(&fcoe_hostlist);
> - rwlock_init(&fcoe_hostlist_lock);
> -
> for_each_possible_cpu(cpu) {
> p = &per_cpu(fcoe_percpu, cpu);
> skb_queue_head_init(&p->fcoe_rx_list);
> @@ -2000,6 +2011,7 @@ static void __exit fcoe_exit(void)
> struct fcoe_interface *fcoe, *tmp;
>
> fcoe_dev_cleanup();
> + rtnl_lock();
>
> /* releases the associated fcoe hosts */
> list_for_each_entry_safe(fcoe, tmp, &fcoe_hostlist, list)
> @@ -2012,5 +2024,7 @@ static void __exit fcoe_exit(void)
>
> /* detach from scsi transport */
> fcoe_if_exit();
> +
> + rtnl_unlock();
> }
> module_exit(fcoe_exit);
>
> _______________________________________________
> devel mailing list
> [email protected]
> http://www.open-fcoe.org/mailman/listinfo/devel
This looks good to me. Thanks for doing it.
Joe
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
