> +#define STORVSC_MIN_BUF_NR 64
What about a comment explaining what this is for?
> +#define STORVSC_RING_BUFFER_SIZE (20*PAGE_SIZE)
> +static int storvsc_ringbuffer_size = STORVSC_RING_BUFFER_SIZE;
I don't think you need the #define here.
> +/* to alert the user that structure sizes may be mismatched even though the */
> +/* protocol versions match. */
> +
> +
> +#define REVISION_STRING(REVISION_) #REVISION_
> +#define FILL_VMSTOR_REVISION(RESULT_LVALUE_) \
> + do { \
> + char *revision_string \
> + = REVISION_STRING($Rev : 6 $) + 6; \
> + RESULT_LVALUE_ = 0; \
> + while (*revision_string >= '0' \
> + && *revision_string <= '9') { \
> + RESULT_LVALUE_ *= 10; \
> + RESULT_LVALUE_ += *revision_string - '0'; \
> + revision_string++; \
> + } \
> + } while (0)
> +
How can these macros work? I don't think git ever expans $Rev, and in
either case I really don't get how this is supposed to work.
> +/* Major/minor macros. Minor version is in LSB, meaning that earlier flat */
> +/* version numbers will be interpreted as "0.x" (i.e., 1 becomes 0.1). */
Please use comment formats like this:
/*
* Blah ....
* .....
*/
(this also applies to a few other places).
> +#define VMSTOR_PROTOCOL_MAJOR(VERSION_) (((VERSION_) >> 8) & 0xff)
> +#define VMSTOR_PROTOCOL_MINOR(VERSION_) (((VERSION_)) & 0xff)
> +#define VMSTOR_PROTOCOL_VERSION(MAJOR_, MINOR_) ((((MAJOR_) & 0xff) << 8) | \
> + (((MINOR_) & 0xff)))
All these should be inline functions.
> +#define VMSTOR_INVALID_PROTOCOL_VERSION (-1)
Not used.
> + * Platform neutral description of a scsi request -
> + * this remains the same across the write regardless of 32/64 bit
> + * note: it's patterned off the SCSI_PASS_THROUGH structure
> + */
> +#define CDB16GENERIC_LENGTH 0x10
> +
> +#ifndef SENSE_BUFFER_SIZE
> +#define SENSE_BUFFER_SIZE 0x12
> +#endif
> +
> +#define MAX_DATA_BUF_LEN_WITH_PADDING 0x14
Please don't conditionally re-use the SCSI-layer SENSE_BUFFER_SIZE for
your on the wire packets, but define your own. Please also give all
theses constants a VMSCSI_ or similar prefix.
> +struct vmscsi_request {
> + unsigned short length;
> + unsigned char srb_status;
> + unsigned char scsi_status;
> +
> + unsigned char port_number;
> + unsigned char path_id;
> + unsigned char target_id;
> + unsigned char lun;
> +
> + unsigned char cdb_length;
> + unsigned char sense_info_length;
> + unsigned char data_in;
> + unsigned char reserved;
> +
> + unsigned int data_transfer_length;
> +
> + union {
> + unsigned char cdb[CDB16GENERIC_LENGTH];
> + unsigned char sense_data[SENSE_BUFFER_SIZE];
> + unsigned char reserved_array[MAX_DATA_BUF_LEN_WITH_PADDING];
> + };
> +} __attribute((packed));
Please use fixed size u8/16/32/etc types for all the on-wire defintions.
I'd also really recommend splitting the actual protocol defintion in
a header separate from the driver implementation to make it clear what
is part of the protocol and what's internal to the driver.
> +/* This is the set of flags that the vsc can set in any packets it sends */
> +#define VSC_LEGAL_FLAGS (REQUEST_COMPLETION_FLAG)
unused.
> +#define STORVSC_MAX_CMD_LEN 16
This seems to duplicate the CDB16GENERIC_LENGTH define used above,
please make sure you only have one #define for this constant.
> +
> +/* Matches Windows-end */
> +enum storvsc_request_type {
> + WRITE_TYPE,
> + READ_TYPE,
> + UNKNOWN_TYPE,
> +};
For enums specifying the protocol please always use explicit values.
> +struct hv_storvsc_request {
> + struct hv_device *device;
> +
> + /* Synchronize the request/response if needed */
> + struct completion wait_event;
> +
> + unsigned char *sense_buffer;
> + void *context;
This always is a struct storvsc_cmd_request, and should be typed as
such.
> + void (*on_io_completion)(struct hv_storvsc_request *request);
This always points to storvsc_command_completion, so please remove the
indirect call and use it directly.
> + struct hv_multipage_buffer data_buffer;
> +
> + struct vstor_packet vstor_packet;
> +};
Btw, what is the reason that storvsc_cmd_request and hv_storvsc_request
are separate structures and not merged into one? This wastes a tiny
amount of memory for the init/reset requests, but keeps the code
a lot simpler.
> +static inline struct storvsc_device *get_out_stor_device(
> + struct hv_device *device)
> +static inline struct storvsc_device *get_in_stor_device(
> + struct hv_device *device)
I'm pretty sure you defended this odd reference counting scheme last
time the discussion came up, but please write up a long comment in
the code explaning it so that the question doesn't come up again
all the time.
> + /*
> + * The current SCSI handling on the host side does
> + * not correctly handle:
> + * INQUIRY command with page code parameter set to 0x80
> + * MODE_SENSE command with cmd[2] == 0x1c
> + *
> + * Setup srb and scsi status so this won't be fatal.
> + * We do this so we can distinguish truly fatal failues
> + * (srb status == 0x4) and off-line the device in that case.
> + */
> +
> + if ((stor_pkt->vm_srb.cdb[0] == INQUIRY) ||
> + (stor_pkt->vm_srb.cdb[0] == MODE_SENSE)) {
This should be:
if (stor_pkt->vm_srb.cdb[0] == INQUIRY ||
stor_pkt->vm_srb.cdb[0] == MODE_SENSE) {
or even better use a switch statement to clarify what's going on.
> +static int storvsc_remove(struct hv_device *dev)
> +{
> + struct storvsc_device *stor_device = hv_get_drvdata(dev);
> + struct Scsi_Host *host = stor_device->host;
> +
> + scsi_remove_host(host);
> +
> + scsi_host_put(host);
> +
> + storvsc_dev_remove(dev);
> +
> + return 0;
> +}
Why isn't this near storvcs_probe at the end of the file given that
they logically belong together? Also please only do the scsi_host_put
once you are fully done with it, that is after storvsc_dev_remove.
> +/*
> + * storvsc_host_reset_handler - Reset the scsi HBA
> + */
> +static int storvsc_host_reset_handler(struct scsi_cmnd *scmnd)
> +{
> + struct hv_host_device *host_dev = shost_priv(scmnd->device->host);
> + struct hv_device *dev = host_dev->dev;
> +
> + return storvsc_host_reset(dev);
> +}
Why is storvsc_host_reset split from this function? Also the comment
really doesn't tell us anything, I'd rather leave it away.
> +/*
> + * storvsc_command_completion - Command completion processing
> + */
> +static void storvsc_command_completion(struct hv_storvsc_request *request)
I really don't see how this comment adds any value over the pure
function name.
> + if (vm_srb->srb_status == 0x4)
> + if (vm_srb->srb_status == 0x20) {
Please add defines for the vm_srb->srb_status values.
> +static bool storvsc_check_scsi_cmd(struct scsi_cmnd *scmnd)
Please call this storvsc_ignore_command or similar to make the use more
obvious.
> + /* If retrying, no need to prep the cmd */
> + if (scmnd->host_scribble) {
> +
> + cmd_request =
> + (struct storvsc_cmd_request *)scmnd->host_scribble;
> +
> + goto retry_request;
> + }
I don't think this can ever happen given that you make sure to always
clear ->host_scribble when returning SCSI_MLQUEUE_*_BUSY.
> + request_size = sizeof(struct storvsc_cmd_request);
> +
> + cmd_request = mempool_alloc(memp->request_mempool,
> + GFP_ATOMIC);
> + if (!cmd_request)
> + return SCSI_MLQUEUE_DEVICE_BUSY;
The point of the mempool allocator is that it will never return NULL.
> +
> + memset(cmd_request, 0, sizeof(struct storvsc_cmd_request));
> +
> + /* Setup the cmd request */
> + cmd_request->bounce_sgl_count = 0;
> + cmd_request->bounce_sgl = NULL;
> + cmd_request->cmd = scmnd;
Either use memset for the whole structure, or set the fields that need
it to 0 explicitly, but not both. I doubt it's more efficient to only
initialise the fields that need it.
> + if (!cmd_request->bounce_sgl) {
> + scmnd->host_scribble = NULL;
> + mempool_free(cmd_request,
> + memp->request_mempool);
> +
> + return SCSI_MLQUEUE_HOST_BUSY;
> + }
> + if (cmd_request->bounce_sgl_count)
> + destroy_bounce_buffer(cmd_request->bounce_sgl,
> + cmd_request->bounce_sgl_count);
> +
> + mempool_free(cmd_request, memp->request_mempool);
> +
> + scmnd->host_scribble = NULL;
> +
> + ret = SCSI_MLQUEUE_DEVICE_BUSY;
Please use goto labels and a single error exit for unwindining on
failure.
> +/* Scsi driver */
Not a useful comment, just remove it.
> +/*
> + * storvsc_probe - Add a new device for this driver
> + */
Not a very useful comment, just remove it.
> + if (dev_is_ide)
> + storvsc_get_ide_info(device, &target, &path);
path is never used, so drop that argument from storvsc_get_ide_info
please. Target is only used in the next dev_is_ide block, so please
move this call to it.
> + /* max # of devices per target */
> + host->max_lun = STORVSC_MAX_LUNS_PER_TARGET;
> + /* max # of targets per channel */
> + host->max_id = STORVSC_MAX_TARGETS;
> + /* max # of channels */
> + host->max_channel = STORVSC_MAX_CHANNELS - 1;
> + /* max cmd length */
> + host->max_cmd_len = STORVSC_MAX_CMD_LEN;
Any reason these aren't set directly in the host template?
> + if (!dev_is_ide) {
> + scsi_scan_host(host);
> + return 0;
> + }
> + ret = scsi_add_device(host, 0, target, 0);
> + if (ret) {
> + scsi_remove_host(host);
> + goto err_out2;
> + }
> + return 0;
I'd write this as an if/else block to be be more clear.
> +/* The one and only one */
this isn't an overly useful comment.
--
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
