[ofa-general] Re: [PATCH 01/13] QLogic VNIC: Driver - netdev implementation

[Ramachandra K]

Roland,

Thanks for the review. Your comments make sense and we will fix the
things you pointed out. Please see some clarifications in-line.

On Fri, May 2, 2008 at 11:45 PM, Roland Dreier <rdreier at cisco.com> wrote:
>  > From: Ramachandra K <ramachandra.kuchimanchi at qlogic.com>
>   >
>   > Signed-off-by: Poornima Kamath <poornima.kamath at qlogic.com>
>   > Signed-off-by: Amar Mudrankit <amar.mudrankit at qlogic.com>
>
>  For the next submission please clean up the From and Signed-off-by
>  lines.  As it stands now you are saying that you (Ramachandra K) are the
>  author of the patch, and that Poornima and Amar signed off on it (ie
>  forwarded it), but you as the person sending the email did not sign off
>  on it.
>

I will make sure to sign off on all patches. Should I also drop the From line
for the patches which I developed, since I am mailing them myself ?

I am using the Signed-off-by line to indicate the people who were
involved in the development of the patches at some stage.

>
>
>   > +void vnic_stop_xmit(struct vnic *vnic, struct netpath *netpath)
>   > +{
>   > +    VNIC_FUNCTION("vnic_stop_xmit()\n");
>   > +    if (netpath == vnic->current_path) {
>   > +            if (vnic->xmit_started) {
>   > +                    netif_stop_queue(vnic->netdevice);
>   > +                    vnic->xmit_started = 0;
>   > +            }
>   > +
>   > +            vnic_stop_xmit_stats(vnic);
>   > +    }
>   > +}
>
>  Do you have sufficient locking here?  Could vnic->current_path or
>  vnic->xmit_started change after they are tested, leading to bad results?
>  Also do you get anything from having a xmit_started flag that you
>  couldn't get just by testing with netif_queue_stopped()?
>

You are right, xmit_started might not be required and we will look at
the locking
issue too.

>
>
>   > +extern cycles_t recv_ref;
>
>  seems like too generic a name to make global.  What the heck are you
>  using cycle_t to keep track of anyway?
>

This is being used as part of the driver internal statistics
collection to keep track of the time
elapsed between a message arriving from the EVIC indicating that it
has done an RDMA write of
an Ethernet packet to the driver memory and the driver giving the packet
to the network stack.  Will fix the variable name.

Regards,
Ram