Re: [Xen-devel] [PATCH V2 2/4] Introduce xen-scsifront module
On Fri, Aug 01, 2014 at 03:06:04PM +0200, Juergen Gross wrote: > >That's right. If you need that I still think it's better to add a > >variant of scsi_add_device helping you with that. > > I'm open to that solution. > > Do you have preferences how to do it (IOW: can you give me a hint)? I thought about it a bit more and came to the conclusion that we shouldn't bother. Why do you care if the scsi_add_device actually added the device? Any per-device setup should be done in ->slave_configure anyway. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Xen-devel] [PATCH V2 2/4] Introduce xen-scsifront module
On 08/01/2014 02:08 PM, Christoph Hellwig wrote: On Wed, Jul 30, 2014 at 06:53:59AM +0200, Juergen Gross wrote: Hmm, I looked into scsi_add_device(). It seems as if the caller can't distinguish between a new created and an already existing device. Am I missing something? That's right. If you need that I still think it's better to add a variant of scsi_add_device helping you with that. I'm open to that solution. Do you have preferences how to do it (IOW: can you give me a hint)? The race is not existing: scsi_add_device() (and scsi_remove_device() as well) for this scsi_host is called in scsifront_do_lun_hotplug() only, and this function is always called in the same thread (xenbus watch). A comment seems to be a good idea. Do you disable scanning through procfs and sysfs as well? No, I don't. OTOH I don't think I see the problem. What could go wrong? Either scsi_device_lookup() does find an existing device, then I refuse to add it again. If I don't find it, it will be added. I can't see how any harm would be done in either case when the device is added/removed between the check and the action. What am I missing? Juergen -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Xen-devel] [PATCH V2 2/4] Introduce xen-scsifront module
On Wed, Jul 30, 2014 at 06:53:59AM +0200, Juergen Gross wrote: > Hmm, I looked into scsi_add_device(). It seems as if the caller can't > distinguish between a new created and an already existing device. Am I > missing something? That's right. If you need that I still think it's better to add a variant of scsi_add_device helping you with that. > The race is not existing: scsi_add_device() (and scsi_remove_device() > as well) for this scsi_host is called in scsifront_do_lun_hotplug() > only, and this function is always called in the same thread (xenbus > watch). A comment seems to be a good idea. Do you disable scanning through procfs and sysfs as well? -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Xen-devel] [PATCH V2 2/4] Introduce xen-scsifront module
On 07/29/2014 04:57 PM, Juergen Gross wrote:
On 07/29/2014 03:53 PM, Christoph Hellwig wrote:
+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.
Hmm, I looked into scsi_add_device(). It seems as if the caller can't
distinguish between a new created and an already existing device. Am I
missing something?
The race is not existing: scsi_add_device() (and scsi_remove_device()
as well) for this scsi_host is called in scsifront_do_lun_hotplug()
only, and this function is always called in the same thread (xenbus
watch). A comment seems to be a good idea.
Juergen
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Xen-devel] [PATCH V2 2/4] Introduce xen-scsifront module
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
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 alrea
