On 08/11/2014 11:54 AM, Christoph Hellwig wrote:
+ BUG_ON(sc->cmd_len > VSCSIIF_MAX_COMMAND_SIZE);
+
+ if (sc->cmd_len)
I can't see how you can get a zero cmd_len here.
Ahh, thanks for spotting this. In a previous version it could be zero
in case of reset.
+static int scsifront_action_handler(struct scsi_cmnd *sc, uint8_t act)
Please add a comment explaining your unusual EH strategy here.
What do you mean with "unusual"? You mean transferring the EH action to
Dom0?
+static void scsifront_free(struct vscsifrnt_info *info)
+{
+ if (info->host && info->host_active) {
+ /* Scsi_host not yet removed */
+ scsi_remove_host(info->host);
+ info->host_active = 0;
+ }
+
+ if (info->ring_ref != GRANT_INVALID_REF) {
+ gnttab_end_foreign_access(info->ring_ref, 0,
+ (unsigned long)info->ring.sring);
+ info->ring_ref = GRANT_INVALID_REF;
+ info->ring.sring = NULL;
+ }
+
+ if (info->irq)
+ unbind_from_irqhandler(info->irq, info);
+ info->irq = 0;
+ info->evtchn = 0;
+
+ if (info->host)
+ scsi_host_put(info->host);
+}
I don't think most of the ifs should be here, just use proper symmetric
goto unwinding in the initialization error path instead.
The way this function can be called from different levels of the
callstack on init failure is very confusing.
Okay, I'll look into making it easier to understand.
+ switch (op) {
+ case VSCSIFRONT_OP_ADD_LUN:
+ if (device_state == XenbusStateInitialised) {
+ sdev = __scsi_add_device(info->host, chn, tgt,
+ lun, NULL);
+ err = (IS_ERR(sdev) || !sdev->hostdata);
+ if (!IS_ERR(sdev)) {
+ sdev->hostdata = NULL;
+ scsi_device_put(sdev);
+ }
Given that you put the device immediatly you should be using
scsi_add_device instead of __scsi_add_device. Also all the messing
with ->hostdata from ->slave_alloc looks wrong. For one thing every
setup done ->slave_alloc should be paired with teardown in
->slave_destroy. Second I don't see any need for that.
The problem is I have to take different actions depending on the device
being new or not.
+ } else {
+ xenbus_printf(XBT_NIL, dev->nodename,
+ state_str, "%d",
+ XenbusStateConnected);
+ }
Just print this message in ->slave_configure.
This is calling for problems, I think. xenbus_printf() is not just a
printing function, but it changes an entry in the xenstore. And this
requires locking, switching threads, ...
I doubt doing this while holding SCSI-internal locks is a good idea.
Juergen
--
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
