Re: [PATCH] ibmvscsi driver - fourth version

James Bottomley
2004-03-11T17:45:22Z

+static int map_sg_data(struct scsi_cmnd *cmd,
+ struct SRP_CMD *srp_cmd, struct device *dev)
+{
[...]
+ data->virtual_address = sg[0].dma_address;
+ data->length = sg[0].dma_length;

This is wrong, you must use the data accessors sg_dma_addres() and
sg_dma_len().

+static int map_single_data(struct scsi_cmnd *cmd,
+ struct SRP_CMD *srp_cmd, struct device *dev)
+{
[...]
+ data->virtual_address =
+ (u64) (unsigned long)dma_map_single(dev, cmd->request_buffer,
+ cmd->request_bufflen,
+ DMA_BIDIRECTIONAL);
+ if (data->virtual_address == 0xFFFFFFFF) {
+ printk(KERN_ERR
+ "ibmvscsi: Unable to map request_buffer for command!\n");

dma_map_single() has no error return currently, what is this trying to
do?

+
+ /* Block requests until we get the SRP login back */
+ scsi_block_requests(host);
+
+ if (!scsi_add_host(hostdata->host, hostdata->dev)) {
+
+ scsi_scan_host(host);
+ return hostdata;
+ }

Er, have you actually tried this ... the block requests will block all
requests including the attempt to scan. I assume the unblock is coming
from the schedule_work, but you're still hanging the rest of the system
waiting for this on a bootup.

+static int ibmvscsi_send_srp_event(struct srp_event_struct *evt_struct,
+ struct ibmvscsi_host_data *hostdata)
+{
[...]
+ if (ibmvscsi_send_crq(hostdata, crq_as_u64[0], crq_as_u64[1]) != 0) {
+ list_del(&evt_struct->list);
+
+ cmnd = evt_struct->cmnd;
+ printk(KERN_ERR "ibmvscsi: failed to send event struct\n");
+ unmap_cmd_data(&evt_struct->evt->srp.cmd, hostdata->dev);
+ ibmvscsi_free_event_struct(&hostdata->pool, evt_struct);
+ cmnd->result = DID_ERROR << 16;
+ evt_struct->cmnd_done(cmnd);
+ }

This is still in the queuecommand path. Are you sure you want to end
the command with DID_ERROR here, which will decrement the retry count in
error handling rather than returning SCSI_MQUEUE_DEVICE_BUSY?

There seem to be other places in the driver where the return status of
ibmvscsi_send_crq isn't being checked, should it be?

James