On 07/29/2014 03:53 PM, Christoph Hellwig wrote:
+ * Patched to support >2TB drives
+ * 2010, Samuel Kvasnica, IMS Nanofabrication AG
+ */
This doesn't really belong into the top of the file comment and should
be moved to the patch description.
Okay.
+
+#include <linux/version.h>
not needed.
Okay.
+static int get_id_from_freelist(struct vscsifrnt_info *info)
+{
+ unsigned long flags;
+ uint32_t free;
+
+ spin_lock_irqsave(&info->shadow_lock, flags);
+
+ free = info->shadow_free;
+ BUG_ON(free >= VSCSIIF_MAX_REQS);
+ info->shadow_free = info->shadow[free].next_free;
+ info->shadow[free].next_free = VSCSIIF_MAX_REQS;
+ info->shadow[free].wait_reset = 0;
+
+ spin_unlock_irqrestore(&info->shadow_lock, flags);
+
+ return free;
Is the shadow array exposed to the hypervisor? If not it might be a
better idea to just allocate the driver private data with the command
by setting the cmd_size field in the host template. Take a look
at the virtio_scsi driver in recent kernels for an example.
Ah, okay. I'll still need an array for managing the request ids, but
this will be much smaller...
+static irqreturn_t scsifront_intr(int irq, void *dev_id)
+{
+ scsifront_notify_work((struct vscsifrnt_info *)dev_id);
+ return IRQ_HANDLED;
Seems like you should simply use threaded interrupt handlers. If that
doesn't work for you at least this should use a workqueue instead of
manually reimplementing it using a kthread.
A threaded interrupt handler seems to be a good idea. I'll check if it
is working.
+/* debug printk to identify more missing scsi commands
+ shost_printk(KERN_INFO "scsicmd: ", sc->device->host,
+ "len=%u %#x,%#x,%#x,%#x,%#x,%#x,%#x,%#x,%#x,%#x\n",
+ sc->cmd_len, sc->cmnd[0], sc->cmnd[1],
+ sc->cmnd[2], sc->cmnd[3], sc->cmnd[4], sc->cmnd[5],
+ sc->cmnd[6], sc->cmnd[7], sc->cmnd[8], sc->cmnd[9]);
+*/
We already have pretty good ways to debug this in the midlayer, I don't
think this is needed.
Okay, I'll remove it.
+ spin_lock_irqsave(shost->host_lock, flags);
What's actually protected by the host lock in this driver? It has
ample of driver-local locking, so avoiding to touch the host lock
would be a clear win.
The host_lock protects the ring buffers used to communicate with Dom0.
I think this is the correct lock for this operation, as there is exactly
one such ring buffer pair for each host structure allocated in
scsifront_probe().
+ scsi_cmd_get_serial(shost, sc);
What do you need the serial number for? Generally only legacy drivers
use it.
Historical reasons :-)
I'll remove it.
+static int scsifront_eh_abort_handler(struct scsi_cmnd *sc)
+{
+ return scsifront_action_handler(sc, VSCSIIF_ACT_SCSI_ABORT);
+}
The amount of waiting that your command abort handler does inside
scsifront_action_handler looks wrong to me. Can you explain the
theory of operation for error handling in this driver? How much
of error recovery is supposed to be handled by the hypervisor,
and much is supposed to be handle in the intiator side driver?
The initiator side just forwards the abort/reset request to Dom0
and waits for an answer (okay or error).
+static void scsifront_free(struct vscsifrnt_info *info)
+{
+ struct Scsi_Host *host = info->host;
+
+ if (host->shost_state != SHOST_DEL)
+ scsi_remove_host(info->host);
A driver really shouldn't look at shost_state like this. Is there some
SCM history explaining where this comes from?
I'll investigate it. It was added soon after the first version was
available, but I haven't found a reason yet.
+ switch (op) {
+ case VSCSIFRONT_OP_ADD_LUN:
+ if (device_state == XenbusStateInitialised) {
+ sdev = scsi_device_lookup(info->host, chn, tgt,
+ lun);
+ if (sdev) {
+ dev_err(&dev->dev,
+ "Device already in use.\n");
+ scsi_device_put(sdev);
+ xenbus_printf(XBT_NIL, dev->nodename,
+ state_str, "%d",
+ XenbusStateClosed);
+ } else {
+ scsi_add_device(info->host, chn, tgt,
+ lun);
+ xenbus_printf(XBT_NIL, dev->nodename,
+ state_str, "%d",
+ XenbusStateConnected);
+ }
+ }
+ break;
scsi_add_device handles an already existing device just fine, and unlike
this construct isn't racy.
Okay. I'll change it.
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
