lkml.org 
[lkml]   [2012]   [Jan]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/1][RESEND] Staging: hv: storvsc: Move the storage driver out of the staging area
> +#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.


\
 
 \ /
  Last update: 2012-01-11 11:39    [W:0.074 / U:0.108 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site