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
