Re: [PATCH 2/3] cnic: Add CNIC driver.

["Michael Chan" <mchan@broadcom.com>]

On Thu, 2008-05-22 at 00:44 -0700, Paul E. McKenney wrote:

> > +
> > +int cnic_register_driver(int ulp_type, struct cnic_ulp_ops *ulp_ops)
> > +{
> > +	struct cnic_dev *dev;
> > +
> > +	if (ulp_type >= MAX_CNIC_ULP_TYPE) {
> > +		printk(KERN_ERR PFX "cnic_register_driver: Bad type %d\n",
> > +		       ulp_type);
> > +		return -EINVAL;
> > +	}
> > +	mutex_lock(&cnic_lock);
> > +	if (cnic_ulp_tbl[ulp_type]) {
> > +		printk(KERN_ERR PFX "cnic_register_driver: Type %d has already "
> > +				    "been registered\n", ulp_type);
> > +		mutex_unlock(&cnic_lock);
> > +		return -EBUSY;
> > +	}
> > +
> > +	read_lock(&cnic_dev_lock);
> > +	list_for_each_entry(dev, &cnic_dev_list, list) {
> > +		struct cnic_local *cp = dev->cnic_priv;
> > +
> > +		clear_bit(ULP_F_INIT, &cp->ulp_flags[ulp_type]);
> > +	}
> > +	read_unlock(&cnic_dev_lock);
> > +
> > +	rcu_assign_pointer(cnic_ulp_tbl[ulp_type], ulp_ops);
> 
> OK, protected by cnic_lock.
> 
> > +	mutex_unlock(&cnic_lock);
> > +
> > +	/* Prevent race conditions with netdev_event */
> > +	rtnl_lock();
> > +	read_lock(&cnic_dev_lock);
> > +	list_for_each_entry(dev, &cnic_dev_list, list) {
> > +		struct cnic_local *cp = dev->cnic_priv;
> > +
> > +		if (!test_and_set_bit(ULP_F_INIT, &cp->ulp_flags[ulp_type]))
> > +			ulp_ops->cnic_init(dev);
> > +	}
> > +	read_unlock(&cnic_dev_lock);
> > +	rtnl_unlock();
> > +
> > +	return 0;
> > +}
> > +
> > +int cnic_unregister_driver(int ulp_type)
> > +{
> > +	struct cnic_dev *dev;
> > +
> > +	if (ulp_type >= MAX_CNIC_ULP_TYPE) {
> > +		printk(KERN_ERR PFX "cnic_unregister_driver: Bad type %d\n",
> > +		       ulp_type);
> > +		return -EINVAL;
> > +	}
> > +	mutex_lock(&cnic_lock);
> > +	if (!cnic_ulp_tbl[ulp_type]) {
> > +		printk(KERN_ERR PFX "cnic_unregister_driver: Type %d has not "
> > +				    "been registered\n", ulp_type);
> > +		goto out_unlock;
> > +	}
> > +	read_lock(&cnic_dev_lock);
> > +	list_for_each_entry(dev, &cnic_dev_list, list) {
> > +		struct cnic_local *cp = dev->cnic_priv;
> > +		
> > +		if (rcu_dereference(cp->ulp_ops[ulp_type])) {
> 
> The rcu_dereference() is redundant because we hold cnic_lock.
> (Which is OK, just wanting to make sure I understand the code.)

Yes, I wanted to access these RCU protected pointers in a uniform way.

> 
> > +			printk(KERN_ERR PFX "cnic_unregister_driver: Type %d "
> > +			       "still has devices registered\n", ulp_type);
> > +			read_unlock(&cnic_dev_lock);
> > +			goto out_unlock;
> > +		}
> > +	}
> > +	read_unlock(&cnic_dev_lock);
> > +
> > +	rcu_assign_pointer(cnic_ulp_tbl[ulp_type], NULL);
> 
> OK, protected by cnic_lock.
> 
> > +
> > +	mutex_unlock(&cnic_lock);
> > +	synchronize_rcu();
> 
> The caller is responsible for freeing up cnic_ulp_tbl[ulp_type]?  If so,
> the caller had better have kept a pointer to it...
> 
> But the caller would need to snapshot the pointer before the cnic_lock
> was acquired, which means that some other pointer might in fact be
> in place by the time this function returns.
> 
> So, is this data element statically allocated?  Or is there some other
> trick being used?
> 
> Or is the whole point of this code simply to ensure that any calls to
> the old cnic_ulp_tbl[ulp_type] functions have completed before this
> function returns?  If so, please add a comment to this effect.

Yes, once again to ensure that any calls have completed before
continuing.  I will document the use of RCU more in the next version.

> 
> > +	return 0;
> > +
> > +out_unlock:
> > +	mutex_unlock(&cnic_lock);
> > +	return -EINVAL;
> > +}
> > +
> > +EXPORT_SYMBOL(cnic_register_driver);
> > +EXPORT_SYMBOL(cnic_unregister_driver);
> > +
> > +static int cnic_start_hw(struct cnic_dev *);
> > +static void cnic_stop_hw(struct cnic_dev *);
> > +
> > +static int cnic_register_device(struct cnic_dev *dev, int ulp_type,
> > +				void *ulp_ctx)
> > +{
> > +	struct cnic_local *cp = dev->cnic_priv;
> > +	struct cnic_ulp_ops *ulp_ops;
> > +
> > +	if (ulp_type >= MAX_CNIC_ULP_TYPE) {
> > +		printk(KERN_ERR PFX "cnic_register_device: Bad type %d\n",
> > +		       ulp_type);
> > +		return -EINVAL;
> > +	}
> > +	mutex_lock(&cnic_lock);
> > +	if (cnic_ulp_tbl[ulp_type] == NULL) {
> > +		printk(KERN_ERR PFX "cnic_register_device: Driver with type %d "
> > +				    "has not been registered\n", ulp_type);
> > +		mutex_unlock(&cnic_lock);
> > +		return -EAGAIN;
> > +	}
> > +	if (rcu_dereference(cp->ulp_ops[ulp_type])) {
> 
> Again, the rcu_dereference() is redundant due to the cnic_lock being
> held, and again, this is OK, just checking to make sure I understand it.
> 
> > +		printk(KERN_ERR PFX "cnic_register_device: Type %d has already "
> > +		       "been registered to this device\n", ulp_type);
> > +		mutex_unlock(&cnic_lock);
> > +		return -EBUSY;
> > +	}
> > +	if (!try_module_get(cnic_ulp_tbl[ulp_type]->owner)) {
> > +		mutex_unlock(&cnic_lock);
> > +		return -EBUSY;
> > +	}
> > +
> > +	clear_bit(ULP_F_START, &cp->ulp_flags[ulp_type]);
> > +	cp->ulp_handle[ulp_type] = ulp_ctx;
> > +	ulp_ops = cnic_ulp_tbl[ulp_type];
> > +	rcu_assign_pointer(cp->ulp_ops[ulp_type], ulp_ops);
> 
> Good, protected by cnic_lock.
> 
> > +	cnic_hold(dev);
> > +	if (!dev->use_count) {
> > +		if (!test_bit(CNIC_F_IF_GOING_DOWN, &dev->flags)) {
> > +			if (dev->netdev->flags & IFF_UP)
> > +				set_bit(CNIC_F_IF_UP, &dev->flags);
> > +		}
> > +	}
> > +	dev->use_count++;
> > +
> > +	if (dev->use_count == 1) {
> > +		if (test_bit(CNIC_F_IF_UP, &dev->flags))
> > +			cnic_start_hw(dev);
> > +	}
> > +
> > +	if (test_bit(CNIC_F_CNIC_UP, &dev->flags))
> > +		if (!test_and_set_bit(ULP_F_START, &cp->ulp_flags[ulp_type]))
> > +			ulp_ops->cnic_start(cp->ulp_handle[ulp_type]);
> > +
> > +	mutex_unlock(&cnic_lock);
> > +
> > +	return 0;
> > +
> > +}
> > +
> > +static int cnic_unregister_device(struct cnic_dev *dev, int ulp_type)
> > +{
> > +	struct cnic_local *cp = dev->cnic_priv;
> > +
> > +	if (ulp_type >= MAX_CNIC_ULP_TYPE) {
> > +		printk(KERN_ERR PFX "cnic_unregister_device: Bad type %d\n",
> > +		       ulp_type);
> > +		return -EINVAL;
> > +	}
> > +	mutex_lock(&cnic_lock);
> > +	if (rcu_dereference(cp->ulp_ops[ulp_type])) {
> 
> Ditto...
> 
> > +		dev->use_count--;
> > +		module_put(cp->ulp_ops[ulp_type]->owner);
> > +		rcu_assign_pointer(cp->ulp_ops[ulp_type], NULL);
> 
> OK, cnic_lock held...
> 
> > +		if (dev->use_count == 0)
> > +			cnic_stop_hw(dev);
> > +		cnic_put(dev);
> > +	} else {
> > +		printk(KERN_ERR PFX "cnic_unregister_device: device not "
> > +		       "registered to this ulp type %d\n", ulp_type);
> > +		mutex_unlock(&cnic_lock);
> > +		return -EINVAL;
> > +	}
> > +	mutex_unlock(&cnic_lock);
> > +
> > +	synchronize_rcu();
> 
> Caller is again responsible for freeing up cp->ulp_ops[ulp_type]?
> If so, the caller had better have obtained a reference to it beforehand.
> But it might have changed in the meantime.  So, how is this freed?
> 
> Or is this statically allocated with the only purpose of the
> synchronize_rcu() being to ensure that calls though the old ops vector
> have completed before this function returns?  If so, please add a
> comment to this effect.

Yes same as above.

> > +
> > +static int cnic_cm_open(struct cnic_dev *dev)
> > +{
> > +	struct cnic_local *cp = dev->cnic_priv;
> > +	int err;
> > +
> > +	err = cnic_cm_alloc_mem(dev);
> > +	if (err)
> > +		return err;
> > +
> > +	err = cp->start_cm(dev);
> > +
> > +	if (err)
> > +		goto err_out;
> > +
> > +	spin_lock_init(&cp->wr_lock);
> > +
> > +	tasklet_init(&cp->cnic_task, &cnic_task, (unsigned long) cp);
> > +
> > +	cp->cm_nb.notifier_call = cnic_net_callback;
> > +	register_netevent_notifier(&cp->cm_nb);
> > +
> > +	dev->cm_create = cnic_cm_create;
> > +	dev->cm_destroy = cnic_cm_destroy;
> > +	dev->cm_connect = cnic_cm_connect;
> > +	dev->cm_abort = cnic_cm_abort;
> > +	dev->cm_close = cnic_cm_close;
> > +	dev->cm_select_dev = cnic_cm_select_dev;
> > +
> > +	cp->ulp_handle[CNIC_ULP_L4] = dev;
> > +	rcu_assign_pointer(cp->ulp_ops[CNIC_ULP_L4], &cm_ulp_ops);
> 
> The cnic_lock is not held due to this being initialization time, and
> that no one else can be messing with this until initialization is
> complete?

Yes, this is actually the CNIC driver registering with itself.  Only the
CNIC driver will be using the CNIC_ULP_L4 ID.

> 
> > +	return 0;
> > +
> > +err_out:
> > +	cnic_cm_free_mem(dev);
> > +	return err;
> > +}
> > +

> > +static void cnic_stop_bnx2_hw(struct cnic_dev *dev)
> > +{
> > +	struct cnic_local *cp = dev->cnic_priv;
> > +	struct cnic_eth_dev *ethdev = cp->ethdev;
> > +
> > +	cnic_disable_bnx2_int_sync(dev);
> > +
> > +	cnic_bnx2_reg_wr_ind(dev, BNX2_CP_SCRATCH + 0x20, 0);
> > +	cnic_bnx2_reg_wr_ind(dev, BNX2_COM_SCRATCH + 0x20, 0);
> > +
> > +	cnic_init_context(dev, KWQ_CID);
> > +	cnic_init_context(dev, KCQ_CID);
> > +
> > +	cnic_setup_5709_context(dev, 0);
> > +	cnic_free_irq(dev);
> > +
> > +	ethdev->drv_unregister_cnic(dev->netdev);
> > +
> > +	cnic_free_resc(dev);
> > +}
> > +
> > +static void cnic_stop_hw(struct cnic_dev *dev)
> > +{
> > +	if (test_bit(CNIC_F_CNIC_UP, &dev->flags)) {
> > +		struct cnic_local *cp = dev->cnic_priv;
> > +
> > +		clear_bit(CNIC_F_CNIC_UP, &dev->flags);
> > +		rcu_assign_pointer(cp->ulp_ops[CNIC_ULP_L4], NULL);
> 
> Given that the cnic_lock does not appear to be held, what prevents
> other CPUs from manipulating cp->ulp_ops[CNIC_ULP_L4] concurrently
> with this function?
> 

This is again the CNIC driver unregistering with itself.  Only the CNIC
driver will be using the CNIC_ULP_L4 ID.

> > +		synchronize_rcu();
> > +		cnic_cm_shutdown(dev);
> > +		cp->stop_hw(dev);
> > +		pci_dev_put(dev->pcidev);
> > +	}
> > +}
> > +