Christoph, Let me begin by first thanking you for your detailed review. I have addressed all the issues you have identified - the code looks much better now; thank you! I am currently testing this code and I will post the patches soon - individual patches against the current code in the staging tree as well as a patch to move the driver out of staging (as I have done in the past). Please find my responses to your comments in-line. Regards, K. Y > -----Original Message----- > From: Christoph Hellwig [mailto:hch@xxxxxxxxxxxxx] > Sent: Wednesday, January 11, 2012 5:37 AM > To: KY Srinivasan > Cc: gregkh@xxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > devel@xxxxxxxxxxxxxxxxxxxxxx; virtualization@xxxxxxxxxxxxxx; ohering@xxxxxxxx; > James.Bottomley@xxxxxxxxxxxxxxxxxxxxx; hch@xxxxxxxxxxxxx; linux- > scsi@xxxxxxxxxxxxxxx > Subject: Re: [PATCH 1/1][RESEND] Staging: hv: storvsc: Move the storage driver > out of the staging area > > > 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. I have consolidated all of the protocol defines at the start of the file and these are properly commented. The decision to not have a separate header file was based on the comments I got from the community a while ago. Hopefully, this consolidation will address all the concerns. > > > +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. I don't have any reference counting anymore. I have added a comment explaining the protocol for managing the life-cycle. > > > + 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. With GFP_ATOMIC flag, the allocation I think can fail. At least that is what mempool_alloc() comment says. > > > + /* 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? This state is not in the scsi_host_template. What am I missing here. -- 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
