[ewg] RE: [ofa-general] [PATCH 5/5] nes: napi interface fix

[Glenn Grundstrom]

> 
> >--- a/drivers/infiniband/hw/nes/nes_hw.c
> >+++ b/drivers/infiniband/hw/nes/nes_hw.c
> >@@ -1232,6 +1232,12 @@ static void 
> nes_replenish_nic_rq(struct nes_vnic
> >*nesvnic)
> > 	nesnic = &nesvnic->nic;
> > 	nesdev = nesvnic->nesdev;
> > 	spin_lock_irqsave(&nesnic->rq_lock, flags);
> >+	if (nesnic->replenishing_rq !=0) {
> >+		spin_unlock_irqrestore(&nesnic->rq_lock, flags);
> >+		return;
> >+	}
> >+	nesnic->replenishing_rq = 1;
> >+	spin_unlock_irqrestore(&nesnic->rq_lock, flags);
> > 	do {
> > 		skb = dev_alloc_skb(nesvnic->max_frame_size);
> > 		if (skb) {
> >@@ -1275,7 +1281,7 @@ static void 
> nes_replenish_nic_rq(struct nes_vnic
> >*nesvnic)
> > 	if (rx_wqes_posted) {
> > 		nes_write32(nesdev->regs+NES_WQE_ALLOC, 
> (rx_wqes_posted << 24) |
> >nesnic->qp_id);
> > 	}
> >-	spin_unlock_irqrestore(&nesnic->rq_lock, flags);
> >+	nesnic->replenishing_rq = 0;
> 
> It seems racy that this is set under lock, but cleared 
> without the lock held.
> How do you ensure that the nic_rq will always be replenished?
> 

nes_replenish_nic_rq() gets called only when an atomic(rx_skbs_needed)
flag is set.  Skbs are alloc'd until no more are needed,
rx_skbs_needed flag is cleared, and then replenishing_rq is
cleared.  Therefore, the lock is really only needed around
the set.  It's much more clear by viewing more of the code
rather than just this patch.


> >@@ -2314,11 +2313,11 @@ void nes_nic_ce_handler(struct 
> nes_device *nesdev,
> >struct nes_hw_nic_cq *cq)
> > 				cqe_count = 0;
> > 			}
> > #ifdef NES_NAPI
> 
> Is #ifdef napi sprinkled throughout the code common for most 
> drivers?  Is there
> a better way to handle this?  (Is this OFED only for backports, or for
> upstream?)

I'll need to think of a better way to handle this one.


> > struct nes_cqp_request {
> > 	wait_queue_head_t     waitq;
> > 	struct nes_hw_cqp_wqe cqp_wqe;
> >@@ -857,6 +859,8 @@ struct nes_hw_nic {
> > 	u16 rq_head;
> > 	u16 rq_tail;
> > 	u16 rq_size;
> >+	u8 replenishing_rq;
> >+	u8 reserved;
> 
> Why is reserved added?

For structure padding.

Glenn.

> 
> - Sean