Re: [PATCH 3/12] qla2xxx: Collapse ISP2xxx queuecommand implementations.

[Andrew Vasquez <andrew.vasquez@qlogic.com>]

On Fri, 14 Oct 2005, Christoph Hellwig wrote:

> On Thu, Oct 13, 2005 at 04:16:04PM -0700, Andrew Vasquez wrote:
> > Signed-off-by: Andrew Vasquez <andrew.vasquez@qlogic.com>
> 
> I don't like this at all.  Even the first variant with the indirect
> function call seems better than this.

The indirect function call is still there -- the resultant code hasn't
changed with this patch.  This patch simply insures that common
queueing code gets updated in one spot rather than two
(qla2x00_queuecommand() qla24xx_queuecommand()).  The *only*
difference between the two was the call to
[qla2x00|qla24xx]_start_scsi().  So, beyond style, are there any other
objections.

> > +#define QLA_QUEUE_COMMAND(isp)  \
> > +static int \
> > +isp##_queuecommand(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *)) \
> > +{ \
> > +	scsi_qla_host_t *ha = to_qla_host(cmd->device->host); \
> > +	fc_port_t *fcport = (struct fc_port *) cmd->device->hostdata; \
> > +	srb_t *sp; \
> > +	int rval; \
> > +	if (!fcport) { \
> > +		cmd->result = DID_NO_CONNECT << 16; \
> > +		goto fail_command; \
> > +	} \
> 
> While we're at it, I don't think cmd->device->hostdata could ever be
> NULL.

Yes, with the target_parent checks in scsi_alloc_target():

        if (shost->transportt->target_parent) {
                spin_lock_irqsave(shost->host_lock, flags);
                parent = shost->transportt->target_parent(shost, channel, id);
                spin_unlock_irqrestore(shost->host_lock, flags);
                if (!parent)
                        return NULL;
        }

you are correct.  Yet another reason to collapse both -- change the
code in one place.

-- 
Andrew Vasquez