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);
> > + }
> > +}
> > +
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
