On Wed, 2011-11-02 at 12:19 -0400, Christoph Hellwig wrote:
> > + struct work_struct work_free;
>
> free_work would be the more conventional name.
>
Changed to free_work
> > out:
> > if (tcm_qla2xxx_fabric_configfs != NULL)
> > target_fabric_configfs_deregister(tcm_qla2xxx_fabric_configfs);
> > + if (tcm_qla2xxx_npiv_fabric_configfs != NULL)
> > + target_fabric_configfs_deregister(tcm_qla2xxx_npiv_fabric_configfs);
>
> please use separate labels for each ressource that needs to be unwinded,
> and avoid the NULL checks.
>
Fixed
> > +static void tcm_qla2xxx_complete_free(struct work_struct *work)
> > +{
> > + struct qla_tgt_cmd *cmd = container_of(work,
> > + struct qla_tgt_cmd, work_free);
> > +
> > + transport_generic_free_cmd(&cmd->se_cmd, 0);
> > +}
>
> whitespace damage.
>
Fixed
> > /*
> > * Called from qla_target_template->free_cmd(), and will call
> > * tcm_qla2xxx_release_cmd via normal struct target_core_fabric_ops
> > @@ -411,7 +421,8 @@ void tcm_qla2xxx_free_cmd(struct qla_tgt_cmd *cmd)
> > atomic_set(&cmd->cmd_free, 1);
> > smp_mb__after_atomic_dec();
> >
> > - transport_generic_free_cmd_intr(&cmd->se_cmd);
> > + INIT_WORK(&cmd->work_free, tcm_qla2xxx_complete_free);
> > + queue_work(tcm_qla2xxx_free_wq, &cmd->work_free);
>
> Now that there is a global workqueue is there any reason to keep the
> !cmd->se_cmd.se_dev special case?
Yep, this should be able to go away as well in tcm_qla2xxx_free_cmd().
>
> Also how do we guantee all workqueue items have been flushed when
> a hba goes away (e.g. is hot-unplugged)?
Yes, because all active sessions and associated active I/O descriptors
are explicitly shutdown for each endpoint via tcm_qla2xxx_drop_tpg() ->
qla_tgt_stop_phase1(), and implicitly protected by configfs references
before tcm_qla2xxx can be unloaded.
--nab
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
