On 03/22, James Bottomley wrote:
>
> OK, the fix from the SCSI point of view is to make the mpt teardown path
> actually work. The whole thing looks to be a complete mess because
> there's another place where it will do the wrong thing in
> mptscsih_remove(). You always have to call mpt_detach() otherwise the
> device doesn't get removed from the lists. In theory this patch fixes
> both bugs in the driver.
Yes, I obviously thought about the same change ;)
But note that mpt_detach() doesn't look correct too. This is almost
off-topic, but still...
At least it needs s/cancel_delayed_work/cancel_delayed_work_sync/ to
sync with mpt_fault_reset_work().
And it is not clear if it is safe to simply destroy ->fw_event_q,
perhaps it can't have the pending works if the caller is teardown
path, and otherwise we rely on mptsas_cleanup_fw_event_q(), I do
not know... but mptsas_cleanup_fw_event_q() needs _sync too I guess,
and obviously mptsas_free_fw_event() should be called unconditionally.
OTOH, mpt_attach() doesn't verify that ->fw_event_q was created
succesfully. And, if mpt_do_ioc_recovery() fails it destroys
->reset_work_q but not ->fw_event_q. Looks like this driver needs
some cleanups.
> --- a/drivers/message/fusion/mptscsih.c
> +++ b/drivers/message/fusion/mptscsih.c
> @@ -1176,10 +1176,14 @@ mptscsih_remove(struct pci_dev *pdev)
> MPT_SCSI_HOST *hd;
> int sz1;
>
> + if (!host)
> + /* not brought up far enough to do scsi_host_attach() */
^^^^^^^^^^^^^^^^
scsi_host_alloc ;)
> + goto out;
> +
Yes, I think this is correct. mpt_attach() does kzalloc(MPT_ADAPTER) so
we can probably rely on ->sh == NULL if _alloc failed.
Oleg.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
