RE: [PATCH v2 03/10] qla2xxx: Add send, receive and accept for auth_els

[Quinn Tran <qutran@marvell.com>]

If it shouldn't, why not add a WARN_ON(!list_empty()) or something here?

> +
> +	if (!node) {
> +		ql_dbg(ql_dbg_edif, vha, 0x09122,
> +		    "%s error - no valid node passed\n", __func__);
> +		return;
> +	}
QT:   noted.  Will fix in v3.

> +		spin_unlock_irqrestore(&vha->pur_cinfo.pur_lock, flags);
> +		qla_enode_free(vha, node);
> +		spin_lock_irqsave(&vha->pur_cinfo.pur_lock, flags);

The whole point of 'list_for_each_safe()' is that you don't need to protect against deletion of the entries.
Having to drop the lock here will (slightly) defeat it's purpose.
Also there's nothing in qla_enode_free() which would require the lock to be dropped.
So please consider either not dropping the lock here or (maybe) move to implicit list unrolling like

while (node = list_first_entry()) {
  list_del_init();
  ...
}

> +	list_for_each_safe(pos, q, &vha->pur_cinfo.head) {
> +		list_node = list_entry(pos, struct enode, list);

list_for_each_entry()


QT:  Noted. We're still harden the code.  Will clean this up as part of next phase.

> +
> +		if (node_rtn) {

Why isn't the part of the 'else' branch above?

QT: will fix it in v3.

> +	/*
> +	 * ql_print_bsg_sglist(ql_dbg_user, vha, 0x7008,
> +	 *     "SG bsg->request", &bsg_job->request_payload);
> +	 */

??? Debug code?
Please move to 'dev_dbg' if you need it, otherwise delete it.

QT: ack to various debug code comments.  will delete.


> +#define	LSTATE_OFF	1	// node not on list
> +#define	LSTATE_ON	2	// node on list
> +#define	LSTATE_DEST	3	// node destoyed

Why do you need this?
If the node is on a list it's 'list_head' structure should be pointing to a list, ie it should be possible to do a 'list_empty()' to get this information, no?

QT: I miss this as part of pruning. 
-----

> +	// no need for endiance conversion.

No C++ comments, please.

QT:  ack. Will fix in v3.


Regards,
Quinn Tran