Re: [PATCH 1/3] bnx2: Add support for CNIC driver.

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Thu, May 22, 2008 at 12:23:28PM -0700, Michael Chan wrote:
> On Wed, 2008-05-21 at 23:45 -0700, Paul E. McKenney wrote:
> 
> > Some RCU-related questions interspersed below, for example, how are
> > updates protected?
> 
> Only one CNIC driver will ever register with the BNX2 driver.  The main
> purpose of using RCU is so that the fast path can avoid locking when
> handling interrupt events from the hardware.

So if a second CNIC driver attempts to register, it gets -EBUSY or
something, right?

> > > +
> > > +static void bnx2_setup_cnic_irq_info(struct bnx2 *bp)
> > > +{
> > > +	struct cnic_ops *c_ops;
> > > +	struct cnic_eth_dev *cp = &bp->cnic_eth_dev;
> > > +	struct bnx2_napi *bnapi = &bp->bnx2_napi[0];
> > > +	int sb_id;
> > > +
> > > +	rcu_read_lock();
> > > +	c_ops = rcu_dereference(bp->cnic_ops);
> > > +	if (!c_ops)
> > > +		goto done;
> > 
> > Given that c_ops is unused below, what is happening here?  What prevents
> > bp->cnic_ops from becoming NULL immediately after we test it?  And if
> > it is OK for bp->cnic_ops to become NULL immediately after we test it,
> > why are we bothering to test it?
> 
> You are right.  We should just unconditionally set up the IRQ
> information without checking for c_ops.  The data structures we set up
> below are owned by us.

OK.  Hmmm....  You cannot even get away with sarcasm these days!  ;-)

> > > +
> > > +	if (bp->flags & BNX2_FLAG_USING_MSIX) {
> > > +		cp->drv_state |= CNIC_DRV_STATE_USING_MSIX;
> > > +		bnapi->cnic_present = 0;
> > > +		sb_id = BNX2_CNIC_VEC;
> > > +		cp->irq_arr[0].irq_flags |= CNIC_IRQ_FL_MSIX;
> > > +	} else {
> > > +		cp->drv_state &= ~CNIC_DRV_STATE_USING_MSIX;
> > > +		bnapi->cnic_tag = bnapi->last_status_idx;
> > > +		bnapi->cnic_present = 1;
> > > +		sb_id = 0;
> > > +		cp->irq_arr[0].irq_flags &= !CNIC_IRQ_FL_MSIX;
> > > +	}
> > > +
> > > +	cp->irq_arr[0].vector = bp->irq_tbl[sb_id].vector;
> > > +	cp->irq_arr[0].status_blk = (void *) ((unsigned long) bp->status_blk +
> > > +		(BNX2_SBLK_MSIX_ALIGN_SIZE * sb_id));
> > > +	cp->irq_arr[0].status_blk_num = sb_id;
> > > +	cp->num_irq = 1;
> > > +
> > > +done:
> > > +	rcu_read_unlock();
> > > +}
> > > +
> > > +static int bnx2_register_cnic(struct net_device *dev, struct cnic_ops *ops,
> > > +			      void *data)
> > > +{
> > > +	struct bnx2 *bp = netdev_priv(dev);
> > > +	struct cnic_eth_dev *cp = &bp->cnic_eth_dev;
> > > +
> > > +	if (ops == NULL)
> > > +		return -EINVAL;
> > > +
> > > +	if (!try_module_get(ops->cnic_owner))
> > > +		return -EBUSY;
> > > +
> > > +	bp->cnic_data = data;
> > > +	rcu_assign_pointer(bp->cnic_ops, ops);
> > 
> > What prevents multiple tasks from doing this assignment concurrently?
> 
> As I said above, only one CNIC driver will ever register.

OK.

> > > +
> > > +	cp->num_irq = 0;
> > > +	cp->drv_state = CNIC_DRV_STATE_REGD;
> > > +
> > > +	bnx2_setup_cnic_irq_info(bp);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int bnx2_unregister_cnic(struct net_device *dev)
> > > +{
> > > +	struct bnx2 *bp = netdev_priv(dev);
> > > +	struct bnx2_napi *bnapi = &bp->bnx2_napi[0];
> > > +	struct cnic_eth_dev *cp = &bp->cnic_eth_dev;
> > > +
> > > +	cp->drv_state = 0;
> > > +	module_put(bp->cnic_ops->cnic_owner);
> > > +	bnapi->cnic_present = 0;
> > > +	rcu_assign_pointer(bp->cnic_ops, NULL);
> > 
> > What prevents this from running concurrently with bnx2_register_cnic()?
> 
> We expect the CNIC driver to be well behaved and will only unregister
> after successfully registered.

Trusting, but fair enough.  ;-)

> > > +	synchronize_rcu();
> > 
> > The caller is responsible for freeing up the old bp->cnic_ops structure?
> > Or is this structure statically allocated?
> > 
> > If so, is the idea to delay return until all prior calls through the
> > old ops structure have completed?
> 
> The cnic_ops contain call back function pointers to the CNIC driver.  We
> want to make sure that those calls have completed (in the fast path)
> before continuing.

Very good.  Could you please add a comment to that effect?  Otherwise
people search for what data structure is being freed up.

							Thanx, Paul

> > > +	return 0;
> > > +}
> > > +
> 
>