There are quite a few commented out debugging statements and deprecated
C++ style comments dotted throughout this driver ... could these be
corrected? (move to C style comments and either implement a debugging
strategy or pull out the statements).
Another thing that strikes me is on the ARM side, it looks like this is
effectively vectoring up to a user space target ... is that true? And
if so, would the existing in-kernel target infrastructure be a better
way of doing this?
On Wed, 2007-01-24 at 11:24 -0700, Dan Williams wrote:
> +static int imu_queuecommand(struct scsi_cmnd *scp,
[...]
> + if (!imu_scsi->connection_handle) {
> + scp->result = (DID_RESET << 16) | (SUGGEST_RETRY << 24);
> + done(scp);
> + return SCSI_MLQUEUE_HOST_BUSY;
> + }
Absolutely wrong. Either you execute done() on the command and return
zero, or you *don't* execute done on the command and return a queueing
condition. Doing both will lead to double completions and really
horrible things.
> + switch (scp->cmnd[0]) {
> + case TEST_UNIT_READY:
> + break;
> + case READ_10:
> + case READ_6:
> + case WRITE_10:
> + case WRITE_6:
> + case READ_CAPACITY:
> + default:
The predicate for this switch looks wrong. You're not really
interpreting the commands, you appear to be interested in whether they
have data buffers attached. Might I suggest this might be better done
by
if (scp->sc_data_direction == DMA_NONE)
break;
else if (scp->use_sg == 0)
/* request buffer case; hopefully disappearing soon */
else
/* sg case */
> + if (scp->use_sg == 0) {
> + dma_addr_t buff = 0;
> + if (NULL == scp->request_buffer)
> + imu_warn("got cmd with NULL request_buffer\n");
> + else
> + buff =
> + dma_map_single(NULL, scp->request_buffer,
> + scp->request_bufflen,
> + DMA_BIDIRECTIONAL);
> + if (buff == 0) {
> + imu_warn("null return from dma_map_single\n");
> + msg->flags = IMU_FCODE_STATUS;
> + msg->status = 0; //todo: what is error status?
> + iop_queue_postmsg(Q_NUM, msg);
> + return 1;
> + }
> + sge = &(msg->cdb_cmd.sge);
> + sge->addr_l = buff;
> + sge->addr_h = 0;
> + sge->flen = scp->request_bufflen | FLAG_LAST_ELEMENT;
> +
> + // This shouldn't be necessary
> + //dma_sync_single(NULL, buff, scp->request_bufflen, DMA_BIDIRECTIONAL);
> + imu_debug("cmd msg with 1 sgl entry, addr=0x%x\n",
> + buff);
Why BIDIRECTIONAL? This can be expensive on some architectures
(although I don't know about ARM). However, for these commands, the
scp->sc_data_direction should contain the exact direction you need to
program into the DMA engines. (Consider this the same comment for the
use_sg > 0 portion as well).
> + iop_queue_postmsg(Q_NUM, msg);
This has error returns ... should you be handling them?
> +static int imu_eh_abort(struct scsi_cmnd *cmd)
> +{
> + imu_warn("Received eh command abort\n");
> + //dbg_print_cmd(cmd);
> + return SUCCESS;
> +}
> +
> +static int imu_eh_device_reset(struct scsi_cmnd *cmd)
> +{
> + int target = cmd->device->id;
> + imu_warn("Received eh device reset for target %d\n", target);
> + return SUCCESS;
> +}
> +
> +static int imu_eh_host_reset(struct scsi_cmnd *cmd)
> +{
> + imu_warn("Received eh host reset\n");
> + return SUCCESS;
> +}
> +
> +static int imu_eh_bus_reset(struct scsi_cmnd *cmd)
> +{
> + imu_warn("Received eh bus reset\n");
> + return SUCCESS;
> +}
This lot all look really wrong. All you're doing is printing a message
and then telling the mid-layer the device is recovered, when in fact
you've done nothing to alleviate the error condition. The mid-layer's
next action will be to send down a TUR to check the device condition,
which will likely fail and continue on to offline the device.
James
-
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
