Messages in this thread | | | Date | Thu, 14 Dec 2006 15:48:12 -0500 | From | Kristian Høgsberg <> | Subject | Re: [PATCH 3/3] Import fw-sbp2 driver. |
| |
Jeff Garzik wrote:
Again, thanks for your comments, I've added patches to my git repo, will send out a new set on LKML before the end of this week.
>> +/* I don't know why the SCSI stack doesn't define something like >> this... */ >> +typedef void (*scsi_done_fn_t) (struct scsi_cmnd *); > > submit a patch?
Hehe, yeah, that might work.
>> +struct sbp2_status { >> + unsigned int orb_high:16; > > unsigned short? probably generates better code than a bitfield:16
This and all other bit fields are now just u32s that I access by shifting and masking.
>> +struct sbp2_login_response { >> + u16 login_id; >> + u16 length; >> + u32 command_block_agent_high; >> + u32 command_block_agent_low; >> + u32 reconnect_hold; >> +}; > > __le16 and __le32?
This struct is filled in using fw_memcpy_from_be32() to copy and byteswap the incoming payload, so the fields here are always cpu endian. The two u16 fields assume little endian ordering though, I fixed that.
>> +sbp2_send_management_orb(struct fw_unit *unit, int node_id, int >> generation, >> + int function, int lun, void *response) >> +{ >> + struct fw_device *device = fw_device(unit->device.parent); >> + struct sbp2_device *sd = unit->device.driver_data; >> + struct sbp2_management_orb *orb; >> + unsigned long timeout; >> + int retval = -EIO; >> + >> + orb = kzalloc(sizeof *orb, GFP_ATOMIC); >> + if (orb == NULL) >> + return -ENOMEM; >> + >> + /* The sbp2 device is going to send a block read request to >> + * read out the request from host memory, so map it for >> + * dma. */ >> + orb->base.request_bus = >> + dma_map_single(device->card->device, &orb->request, >> + sizeof orb->request, DMA_TO_DEVICE); >> + >> + orb->response_bus = >> + dma_map_single(device->card->device, &orb->response, >> + sizeof orb->response, DMA_FROM_DEVICE); > > check for DMA mapping error
Oops, fixed.
>> + if (sd->workarounds) >> + fw_notify("Workarounds for node %s: 0x%x " >> + "(firmware_revision 0x%06x, model_id 0x%06x)\n", >> + unit->device.bus_id, >> + sd->workarounds, firmware_revision, model); >> + >> + /* FIXME: Make this work for multi-lun devices. */ >> + lun = 0; > > doesn't allowing the stack to issue REPORT LUNS take care of this?
Possibly, I don't have firewire multi-LUN devices to test with here. The LUNs are also discoverable from the firewire config rom, which is why I put the comment there. This doesn't mean that the SCSI commands for discovering LUNs doesn't also work.
>> +/* SCSI stack integration */ >> + >> +static int sbp2_scsi_queuecommand(struct scsi_cmnd *cmd, >> scsi_done_fn_t done) >> +{ >> + if (cmd->cmnd[0] == REQUEST_SENSE) { >> + fw_notify("request_sense"); >> + memcpy(cmd->request_buffer, cmd->sense_buffer, >> cmd->request_bufflen); >> + memset(cmd->sense_buffer, 0, sizeof(cmd->sense_buffer)); >> + cmd->result = DID_OK << 16; >> + done(cmd); >> + return 0; >> + } > > this is a broken emulation. this command is specified to not repeatedly > return the same sense data.
I copied it over from the old stack under the assumption that it fixed something for some device. I took it out and tested with the 10 or so storage devices I have here and it makes no difference. I've never seen the fw_notify() that I put in there trigger. I'm taking out this workaround for now, unless someone can tell me why it should stay there.
Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |