Re: [PATCH RFC/RFT 2/4] add scsi helpers

[Mike Christie <michaelc@cs.wisc.edu>]

Christoph Hellwig wrote:
>>+struct __scsi_request {
>>+	void *data;
>>+	void (*done)(void *data, char *sense, int result, int resid);
>>+	char sense[SCSI_SENSE_BUFFERSIZE];
>>+};
> 
> 
> I don't think this is a good structure name.  Just something like
> iocontext  or similar?

ok.

> 
> 
>>+int scsi_execute_async_iov_req(struct scsi_device *sdev,
>>+			       const unsigned char *cmd, int data_direction,
>>+			       struct kvec *vec, int vec_count, int timeout,
>>+			       int retries, void *privdata,
>>+			       void (*done)(void *, char *, int, int))
> 
> 
> If you passed an request_queue_t instead of th scsi_device this function
> would not have any knowledge about scsi internals and could be moved up
> to the block layer.  not sure that's actually a good idea.

Did you want to move scsi_execute too?


> 
> If we stick to putting it into the scsi layer the done callback could
> normalize the sense data, though.
> 
> 
>>+	struct request *req;
>>+	struct __scsi_request *sreq;
>>+	int write = (data_direction == DMA_TO_DEVICE);
> 
> 
> What about just passing in a write parameter directly?

I thought most users would just have the DMA_* value so it would be 
nicer for them to pass that down. It also then works like 
scsi_execute_req/scsi_execute. I guess if we move one of those functions 
to the block layer it would be better to pass in write/read for the 
block layer versions.


> 
> 
>>+
>>+	sreq = kzalloc(sizeof(*sreq), GFP_ATOMIC);
>>+	if (!sreq) {
>>+		return DRIVER_ERROR << 24;
>>+	}
>>+
>>+	req = blk_get_request(sdev->request_queue, write, GFP_ATOMIC);
> 
> 
> can you add a gfp_mask argument instead of hardcoding GFP_ATOMIC?
> 

yeah will do. I think I got lazy :)