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
