Thanks for the quick reply and analysis Hannes! My apologies in advance, I'm stuck on
Outlook here at work - I'll try to format this to be readable (hopefully it doesn't get
mangled).
On 02/27/2018 06:01 AM, Hannes Reinecke wrote:
> Hmm. Isn't is rather the case that the labels and gotos are mixed up?
> We have this order of labels:
> probe_init_failed
> probe_failed
> probe_hw_failed
> iospace_config_failed
> disable_device
> but this potential calling order:
> disable_device
> iospace_config_failed
> probe_hw_failed
> probe_hw_failed
> probe_init_failed
> probe_failed
> probe_failed
> probe_failed
> So it looks to me as if the 'probe_init_failed' and 'probe_failed'
> labels should be exchanged...
> But yeah, we need to fix this up.
> I've run into this issue myself recently :-(
So, that is partly the problem. But.....if we switch the order, we still have problems.
Consider the code with order switched:
probe_failed:
....
qla2x00_free_device(base_vha);
scsi_host_put(base_vha->host);
probe_init_failed:
qla2x00_free_req_que(ha, req);
ha->req_q_map[0] = NULL; alloc_queues succeeds
clear_bit(0, ha->req_qid_map);
qla2x00_free_rsp_que(ha, rsp);
ha->rsp_q_map[0] = NULL;
clear_bit(0, ha->rsp_qid_map);
ha->max_req_queues = ha->max_rsp_queues = 0;
probe_hw_failed:
qla2x00_mem_free(ha);
qla2x00_free_req_que(ha, req);
qla2x00_free_rsp_que(ha, rsp);
qla2x00_clear_drv_active(ha);
1) We call probe_init_failed only if qla2x00_alloc_queues fails. But if this routine fails,
ha->req_q_map = NULL (as well as rsp_q_map), so we crash doing
ha->req_q_map[0] = NULL. So, I can remove these assignments. But the
qla2x00_free_req_que calls are redundant (they are done below), so we're left
with clearing out max_req_queues and max_rsp_queues for this label. Since
we're freeing ha anyways, I'm not sure if this is worth having another label for.
2) We still have the problem where in probe_failed, qla2x00_free_device does
frees which are repeated later on in probe_hw_failed.
qla2x00_free_device -> qla2x00_mem_free
qla2x00_free_device -> qla2x00_free_queues -> qla2x00_free_req_que
3) Also, I still need another label for when qla2x00_mem_alloc fails. We can't just
jump to probe_hw_failed.
I suppose we could do something like this:
probe_failed:
probe_failed = 1;
....
qla2x00_free_device(base_vha);
scsi_host_put(base_vha->host);
probe_init_failed:
ha->max_req_queues = ha->max_rsp_queues = 0;
probe_hw_failed:
if (!probe_failed) {
qla2x00_mem_free(ha);
qla2x00_free_req_que(ha, req);
qla2x00_free_rsp_que(ha, rsp);
}
probe_hw_failed_no_mem:
qla2x00_clear_drv_active(ha);
This is essentially what I have, except my version does not have the probe_init_failed
label. Thoughts??
Thanks again and regards.
---
