[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH V5 3/5] Introduce xen-scsifront module



On 08/23/2014 12:25 AM, Konrad Rzeszutek Wilk wrote:
--- /dev/null
+++ b/drivers/scsi/xen-scsifront.c
@@ -0,0 +1,1017 @@
+/*
+ * Xen SCSI frontend driver
+ *
+ * Copyright (c) 2008, FUJITSU Limited
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation; or, when distributed
+ * separately from the Linux kernel or incorporated into other
+ * software packages, subject to the following license:
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this source file (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use, copy, modify,
+ * merge, publish, distribute, sublicense, and/or sell copies of the Software,
+ * and to permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#define DEBUG

:-) I think you don't want that in the driver.

Correct. :-)


+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/wait.h>
+#include <linux/interrupt.h>
+#include <linux/spinlock.h>
+#include <linux/sched.h>
+#include <linux/blkdev.h>
+#include <linux/pfn.h>
+#include <linux/slab.h>
+
+#include <scsi/scsi_cmnd.h>
+#include <scsi/scsi_device.h>
+#include <scsi/scsi.h>
+#include <scsi/scsi_host.h>
+
+#include <xen/xen.h>
+#include <xen/xenbus.h>
+#include <xen/grant_table.h>
+#include <xen/events.h>
+#include <xen/page.h>
+
+#include <xen/interface/grant_table.h>
+#include <xen/interface/io/vscsiif.h>
+#include <xen/interface/io/protocols.h>
+
+#include <asm/xen/hypervisor.h>
+
+
+#define GRANT_INVALID_REF      0
+
+#define VSCSIFRONT_OP_ADD_LUN  1
+#define VSCSIFRONT_OP_DEL_LUN  2

Shouldn't those be defined in the vscsiff.h file?

No, they are private for the frontend.


+
+#define DEFAULT_TASK_COMM_LEN  TASK_COMM_LEN

Not sure why you need the DEFAULT_ ? Could you use TASK_COMM_LEN?

Sure.


+
+/* tuning point*/

Missing stop and a space after the 't':

/* Tuning point. */

Okay.


+#define VSCSIIF_DEFAULT_CMD_PER_LUN 10
+#define VSCSIIF_MAX_TARGET          64
+#define VSCSIIF_MAX_LUN             255
+
+#define VSCSIIF_RING_SIZE      __CONST_RING_SIZE(vscsiif, PAGE_SIZE)
+#define VSCSIIF_MAX_REQS       VSCSIIF_RING_SIZE
+
+#define vscsiif_grants_sg(_sg) (PFN_UP((_sg) *         \
+                               sizeof(struct scsiif_request_segment)))
+
+struct vscsifrnt_shadow {
+       /* command between backend and frontend */
+       unsigned char act;

act? How about 'op' ? Or 'cmd_op'?

I wanted to use the same name as the related element in vscsiif.h


+       uint16_t rqid;

rqid? Could you have a comment explaining what that acronym is?
Oh wait - request id? How about just req_id?

Same again. It's called rqid in vscsiiif.h


+
+       /* Number of pieces of scatter-gather */
+       unsigned int nr_grants;

s/nr_grants/nr_sg/ ?

I'll update the comment, as this is really the number of grants.


+       struct scsiif_request_segment *sg;
+
+       /* do reset or abort function */

Full stop missing.
+       wait_queue_head_t wq_reset;     /* reset work queue           */
+       int wait_reset;                 /* reset work queue condition */
+       int32_t rslt_reset;             /* reset response status      */
+                                       /* (SUCESS or FAILED)         */

Full stop missing. s/SUCESS/SUCCESS/

Okay.

+
+       /* requested struct scsi_cmnd is stored from kernel */

Full stop missing.

Okay.

+       struct scsi_cmnd *sc;
+       int gref[vscsiif_grants_sg(SG_ALL) + SG_ALL];
+};
+
+struct vscsifrnt_info {
+       struct xenbus_device *dev;
+
+       struct Scsi_Host *host;
+       int host_active;

This 'host_active' seems to be a guard to call 'scsi_host_remove'
through either the disconnect (so backend told us to disconnect)
or the .remove (so XenStore keys are moved). Either way I think
it is possible to run _both_ of them and this being just
an 'int' is not sufficient.

I would recommend you change this to an ref_count.

Hmm, I think a lock is required here.

+
+       spinlock_t shadow_lock;

Could you move this spinlock below - to where
you have tons of of 'shadow' values please?

Okay.


+       unsigned int evtchn;
+       unsigned int irq;
+
+       grant_ref_t ring_ref;
+       struct vscsiif_front_ring ring;
+       struct vscsiif_response ring_res;
+
+       unsigned long shadow_free;

A comment would help in explainig this. 'shadow_free' sounds
a bit cryptic. Oh, and xen-blkfront has it as 'shadow_free'
too! Argh, 'shadow_free_bitmask' could be a better name?
Oh, what if we converted to an bitmask type?

Either way - if you think the existing one should be this way
then lets leave it as is. But if you think a better name
is suited I can change in xen-blkfront too.

I'll use the bitmap operations and call it shadow_free_bitmap.


+       struct vscsifrnt_shadow *shadow[VSCSIIF_MAX_REQS];
+
+       wait_queue_head_t wq_sync;
+       unsigned int waiting_sync:1;

That sounds like it needs a spinlock to protect it?

It is protected by info->host->host_lock.

Could you also explain a bit of what its purpose is? What
is it suppose to wait for? It looks like it is tied to the
error handler, perhaps then it ought to be called:

err_handler_done

It is used by the error handler, yes. But this is only due to the
fact that the error handler can't return with SCSI_MLQUEUE_HOST_BUSY.

I think I rename it to wait_ring_available, because this is the real
meaning: someone is waiting until a request can be allocated in the
ring.



+
+       char dev_state_path[64];
+       struct task_struct *curr;
+};
+
+#define DPRINTK(_f, _a...)                             \
+       pr_debug("(file=%s, line=%d) " _f, __FILE__ , __LINE__ , ## _a)

Could all the DPRINTK be converted to pr_debug right away?

Yes, I'll do it.

+
+#define PREFIX(lvl) KERN_##lvl "scsifront: "

Perhaps you want?

#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

Not quite, but I'll remove the PREFIX define.

+
+static void scsifront_wake_up(struct vscsifrnt_info *info)
+{
+       info->waiting_sync = 0;
+       wake_up(&info->wq_sync);
+}
+
+static int scsifront_get_rqid(struct vscsifrnt_info *info)
+{
+       unsigned long flags;
+       int free;
+
+       spin_lock_irqsave(&info->shadow_lock, flags);
+
+       free = find_first_bit(&info->shadow_free, VSCSIIF_MAX_REQS);
+       info->shadow_free &= ~(1UL << free);
+
+       spin_unlock_irqrestore(&info->shadow_lock, flags);
+
+       return free;
+}
+
+static int _scsifront_put_rqid(struct vscsifrnt_info *info, uint32_t id)
+{
+       info->shadow_free |= 1UL << id;
+       info->shadow[id] = NULL;
+
+       return (info->shadow_free == 1UL << id || info->waiting_sync);
+}
+
+static void scsifront_put_rqid(struct vscsifrnt_info *info, uint32_t id)
+{
+       unsigned long flags;
+       int was_empty;
+
+       spin_lock_irqsave(&info->shadow_lock, flags);
+       was_empty = _scsifront_put_rqid(info, id);

I wouldn't call this 'was_empty' as it will also be true if the 'waiting_sync'
is turned on. Perhaps this should be called: 'kick' or 'wake_wq' ?

kick is okay, I think.


+       spin_unlock_irqrestore(&info->shadow_lock, flags);
+
+       if (was_empty)
+               scsifront_wake_up(info);
+}
+
+static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info)
+{
+       struct vscsiif_front_ring *ring = &(info->ring);
+       struct vscsiif_request *ring_req;
+       uint32_t id;
+
+       id = scsifront_get_rqid(info);  /* use id by response */

s/by/in/

Okay.


+       if (id >= VSCSIIF_MAX_REQS)
+               return NULL;
+
+       ring_req = RING_GET_REQUEST(&(info->ring), ring->req_prod_pvt);
+
+       ring->req_prod_pvt++;
+
+       ring_req->rqid = (uint16_t)id;
+
+       return ring_req;
+}
+
+static void scsifront_do_request(struct vscsifrnt_info *info)
+{
+       struct vscsiif_front_ring *ring = &(info->ring);
+       int notify;
+
+       RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(ring, notify);
+       if (notify)
+               notify_remote_via_irq(info->irq);
+}
+
+static void scsifront_gnttab_done(struct vscsifrnt_info *info, uint32_t id)
+{
+       struct vscsifrnt_shadow *s = info->shadow[id];
+       int i;
+
+       if (s->sc->sc_data_direction == DMA_NONE)
+               return;
+
+       for (i = 0; i < s->nr_grants; i++) {
+               if (unlikely(gnttab_query_foreign_access(s->gref[i]) != 0)) {
+                       shost_printk(PREFIX(ALERT), info->host,
+                                    "grant still in use by backend\n");
+                       BUG();
+               }
+               gnttab_end_foreign_access(s->gref[i], 0, 0UL);
+       }
+
+       kfree(s->sg);
+}
+
+static void scsifront_cdb_cmd_done(struct vscsifrnt_info *info,
+                                  struct vscsiif_response *ring_res)

s/ring_res/ring_rsp/ ?

Yes, that's better.


+{
+       struct scsi_cmnd *sc;
+       uint32_t id;
+       uint8_t sense_len;
+
+       id = ring_res->rqid;
+       sc = info->shadow[id]->sc;
+
+       BUG_ON(sc == NULL);
+
+       scsifront_gnttab_done(info, id);
+       scsifront_put_rqid(info, id);
+
+       sc->result = ring_res->rslt;
+       scsi_set_resid(sc, ring_res->residual_len);
+
+       sense_len = min_t(uint8_t, VSCSIIF_SENSE_BUFFERSIZE,
+                         ring_res->sense_len);
+
+       if (sense_len)
+               memcpy(sc->sense_buffer, ring_res->sense_buffer, sense_len);
+
+       sc->scsi_done(sc);
+}
+
+static void scsifront_sync_cmd_done(struct vscsifrnt_info *info,
+                                   struct vscsiif_response *ring_res)
+{
+       uint16_t id = ring_res->rqid;
+       unsigned long flags;
+       struct vscsifrnt_shadow *shadow = info->shadow[id];
+       int was_empty;
+
+       spin_lock_irqsave(&info->shadow_lock, flags);
+       shadow->wait_reset = 1;
+       switch (shadow->rslt_reset) {
+       case 0:
+               shadow->rslt_reset = ring_res->rslt;
+               break;
+       case -1:

Is there an #define for this? The comment at the top mentioned
SUCCESS and FAILED ?

Adding defines.


+               was_empty = _scsifront_put_rqid(info, id);

Perhaps call it 'kick' or 'wake_wq' ?

kick, again.


+               spin_unlock_irqrestore(&info->shadow_lock, flags);
+               kfree(shadow);
+               if (was_empty)
+                       scsifront_wake_up(info);
+               return;
+       default:
+               shost_printk(PREFIX(ERR), info->host,
+                            "bad reset state %d, possibly leaking %u\n",
+                            shadow->rslt_reset, id);
+               break;
+       }
+       spin_unlock_irqrestore(&info->shadow_lock, flags);
+
+       wake_up(&shadow->wq_reset);
+}
+
+static int scsifront_cmd_done(struct vscsifrnt_info *info)
+{
+       struct vscsiif_response *ring_res;
+       RING_IDX i, rp;
+       int more_to_do = 0;
+       unsigned long flags;
+
+       spin_lock_irqsave(info->host->host_lock, flags);
+
+       rp = info->ring.sring->rsp_prod;
+       rmb();  /* ordering required respective to dom0 */
+       for (i = info->ring.rsp_cons; i != rp; i++) {
+
+               ring_res = RING_GET_RESPONSE(&info->ring, i);

I would recommend you look at git commit 
6878c32e5cc0e40980abe51d1f02fb453e27493e

As the 'rqid' value - might be corrupted by the backend. Which means
that the 'info->shadow[rubbish_val]->act' could blow up.

I would just add a check to make sure that the 'rqid' value is
within the expected values.

I'll add a check (the rqid should be in use as well).


+
+               if (info->shadow[ring_res->rqid]->act == VSCSIIF_ACT_SCSI_CDB)
+                       scsifront_cdb_cmd_done(info, ring_res);
+               else
+                       scsifront_sync_cmd_done(info, ring_res);
+       }
+
+       info->ring.rsp_cons = i;
+
+       if (i != info->ring.req_prod_pvt)
+               RING_FINAL_CHECK_FOR_RESPONSES(&info->ring, more_to_do);
+       else
+               info->ring.sring->rsp_event = i + 1;
+
+       info->waiting_sync = 0;
+
+       spin_unlock_irqrestore(info->host->host_lock, flags);
+
+       wake_up(&info->wq_sync);
+
+       return more_to_do;
+}
+
+static irqreturn_t scsifront_irq_fn(int irq, void *dev_id)
+{
+       struct vscsifrnt_info *info = dev_id;
+
+       while (scsifront_cmd_done(info))
+               /* Yield point for this unbounded loop. */
+               cond_resched();
+
+       return IRQ_HANDLED;
+}
+
+static int map_data_for_request(struct vscsifrnt_info *info,
+                               struct scsi_cmnd *sc,
+                               struct vscsiif_request *ring_req,
+                               struct vscsifrnt_shadow *shadow)
+{
+       grant_ref_t gref_head;
+       struct page *page;
+       int err, ref, ref_cnt = 0;
+       int write = (sc->sc_data_direction == DMA_TO_DEVICE);

What if it is DMA_BIDIRECTIONAL ?

That's okay. DMA_TO_DEVICE is the only case where a grant is flagged as
read only. Perhaps I should rename 'write' to 'grant_ro'.


+       unsigned int i, off, len, bytes;
+       unsigned int data_len = scsi_bufflen(sc);
+       unsigned int data_grants = 0, seg_grants = 0;
+       struct scatterlist *sg;
+       unsigned long mfn;
+       struct scsiif_request_segment *seg;
+
+       ring_req->nr_segments = 0;
+       if (sc->sc_data_direction == DMA_NONE || !data_len)
+               return 0;
+
+       scsi_for_each_sg(sc, sg, scsi_sg_count(sc), i)
+               data_grants += PFN_UP(sg->offset + sg->length);
+
+       if (data_grants > VSCSIIF_SG_TABLESIZE) {
+               if (data_grants > info->host->sg_tablesize) {
+                       shost_printk(PREFIX(ERR), info->host,
+                            "Unable to map request_buffer for command!\n");
+                       return -E2BIG;
+               }
+               seg_grants = vscsiif_grants_sg(data_grants);
+               shadow->sg = kcalloc(data_grants,
+                       sizeof(struct scsiif_request_segment), GFP_NOIO);
+               if (!shadow->sg)
+                       return -ENOMEM;
+       }
+       seg = shadow->sg ? : ring_req->seg;
+
+       err = gnttab_alloc_grant_references(seg_grants + data_grants,
+                                           &gref_head);
+       if (err) {
+               kfree(shadow->sg);
+               shost_printk(PREFIX(ERR), info->host,
+                            "gnttab_alloc_grant_references() error\n");
+               return -ENOMEM;
+       }
+
+       if (seg_grants) {
+               page = virt_to_page(seg);
+               off = (unsigned long)seg & ~PAGE_MASK;
+               len = sizeof(struct scsiif_request_segment) * data_grants;
+               while (len > 0) {
+                       bytes = min_t(unsigned int, len, PAGE_SIZE - off);
+
+                       ref = gnttab_claim_grant_reference(&gref_head);
+                       BUG_ON(ref == -ENOSPC);
+
+                       mfn = pfn_to_mfn(page_to_pfn(page));
+                       gnttab_grant_foreign_access_ref(ref,
+                               info->dev->otherend_id, mfn, 1);
+                       shadow->gref[ref_cnt] = ref;
+                       ring_req->seg[ref_cnt].gref   = ref;
+                       ring_req->seg[ref_cnt].offset = (uint16_t)off;
+                       ring_req->seg[ref_cnt].length = (uint16_t)bytes;
+
+                       page++;
+                       len -= bytes;
+                       off = 0;
+                       ref_cnt++;
+               }
+               BUG_ON(seg_grants < ref_cnt);
+               seg_grants = ref_cnt;
+       }
+
+       scsi_for_each_sg(sc, sg, scsi_sg_count(sc), i) {
+               page = sg_page(sg);
+               off = sg->offset;
+               len = sg->length;
+
+               while (len > 0 && data_len > 0) {
+                       /*
+                        * sg sends a scatterlist that is larger than
+                        * the data_len it wants transferred for certain
+                        * IO sizes

Full stop missing.

Yep.

+                        */
+                       bytes = min_t(unsigned int, len, PAGE_SIZE - off);
+                       bytes = min(bytes, data_len);
+
+                       ref = gnttab_claim_grant_reference(&gref_head);
+                       BUG_ON(ref == -ENOSPC);
+
+                       mfn = pfn_to_mfn(page_to_pfn(page));
+                       gnttab_grant_foreign_access_ref(ref,
+                               info->dev->otherend_id, mfn, write);
+
+                       shadow->gref[ref_cnt] = ref;
+                       seg->gref   = ref;
+                       seg->offset = (uint16_t)off;
+                       seg->length = (uint16_t)bytes;
+
+                       page++;
+                       seg++;
+                       len -= bytes;
+                       data_len -= bytes;
+                       off = 0;
+                       ref_cnt++;
+               }
+       }
+
+       if (seg_grants)
+               ring_req->nr_segments = VSCSIIF_SG_GRANT | seg_grants;
+       else
+               ring_req->nr_segments = (uint8_t)ref_cnt;
+       shadow->nr_grants = ref_cnt;
+
+       return 0;
+}
+
+static struct vscsiif_request *scsifront_command2ring(
+               struct vscsifrnt_info *info, struct scsi_cmnd *sc,
+               struct vscsifrnt_shadow *shadow)
+{
+       struct vscsiif_request *ring_req;
+
+       memset(shadow, 0, sizeof(*shadow));
+
+       ring_req = scsifront_pre_req(info);
+       if (!ring_req)
+               return NULL;
+
+       info->shadow[ring_req->rqid] = shadow;
+       shadow->rqid = ring_req->rqid;
+
+       ring_req->id      = sc->device->id;
+       ring_req->lun     = sc->device->lun;
+       ring_req->channel = sc->device->channel;
+       ring_req->cmd_len = sc->cmd_len;
+
+       BUG_ON(sc->cmd_len > VSCSIIF_MAX_COMMAND_SIZE);
+
+       memcpy(ring_req->cmnd, sc->cmnd, sc->cmd_len);
+
+       ring_req->sc_data_direction   = (uint8_t)sc->sc_data_direction;
+       ring_req->timeout_per_command = sc->request->timeout / HZ;
+
+       return ring_req;
+}
+
+static int scsifront_queuecommand(struct Scsi_Host *shost,
+                                 struct scsi_cmnd *sc)
+{
+       struct vscsifrnt_info *info = shost_priv(shost);
+       struct vscsiif_request *ring_req;
+       struct vscsifrnt_shadow *shadow = scsi_cmd_priv(sc);
+       unsigned long flags;
+       int err;
+       uint16_t rqid;
+

BUG_ON(sc->sc_data_direction == DMA_BIDIRECTIONAL); ?

No.



+       spin_lock_irqsave(shost->host_lock, flags);
+       if (RING_FULL(&info->ring))
+               goto busy;
+
+       ring_req = scsifront_command2ring(info, sc, shadow);
+       if (!ring_req)
+               goto busy;
+
+       sc->result    = 0;

This has some odd spacing? Is it suppose to be at the same
aligment as the ones below? Maybe it is my editor having
issues.

Historical reasons. :-)
I'll change it.


+
+       rqid              = ring_req->rqid;
+       ring_req->act     = VSCSIIF_ACT_SCSI_CDB;
+
+       shadow->sc  = sc;
+       shadow->act = VSCSIIF_ACT_SCSI_CDB;
+
+       err = map_data_for_request(info, sc, ring_req, shadow);
+       if (err < 0) {
+               DPRINTK("%s: err %d\n", __func__, err);
+               scsifront_put_rqid(info, rqid);
+               spin_unlock_irqrestore(shost->host_lock, flags);
+               if (err == -ENOMEM)
+                       return SCSI_MLQUEUE_HOST_BUSY;
+               sc->result = DID_ERROR << 16;
+               sc->scsi_done(sc);
+               return 0;
+       }
+
+       scsifront_do_request(info);
+       spin_unlock_irqrestore(shost->host_lock, flags);
+
+       return 0;
+
+busy:
+       spin_unlock_irqrestore(shost->host_lock, flags);
+       DPRINTK("%s: busy\n", __func__);
+       return SCSI_MLQUEUE_HOST_BUSY;
+}
+
+/*
+ * Any exception handling (reset or abort) must be forwarded to the backend.
+ * We have to wait until an answer is returned. This answer contains the
+ * result to be returned to the requestor.
+ */
+static int scsifront_action_handler(struct scsi_cmnd *sc, uint8_t act)
+{
+       struct Scsi_Host *host = sc->device->host;
+       struct vscsifrnt_info *info = shost_priv(host);
+       struct vscsifrnt_shadow *shadow, *s = scsi_cmd_priv(sc);
+       struct vscsiif_request *ring_req;
+       int err = 0;
+
+       shadow = kmalloc(sizeof(*shadow), GFP_NOIO);
+       if (!shadow)
+               return FAILED;
+
+       for (;;) {
+               spin_lock_irq(host->host_lock);
+               if (!RING_FULL(&info->ring)) {
+                       ring_req = scsifront_command2ring(info, sc, shadow);
+                       if (ring_req)
+                               break;
+               }
+               if (err) {
+                       spin_unlock_irq(host->host_lock);
+                       kfree(shadow);
+                       return FAILED;
+               }
+               info->waiting_sync = 1;
+               spin_unlock_irq(host->host_lock);
+               err = wait_event_interruptible(info->wq_sync,
+                                              !info->waiting_sync);
+               spin_lock_irq(host->host_lock);
+       }
+
+       ring_req->act = act;
+       ring_req->ref_rqid = s->rqid;
+
+       shadow->act = act;
+       shadow->rslt_reset = 0;
+       init_waitqueue_head(&shadow->wq_reset);
+
+       ring_req->nr_segments         = 0;

Something odd with the spaces here.

Okay.


+
+       scsifront_do_request(info);
+
+       spin_unlock_irq(host->host_lock);
+       err = wait_event_interruptible(shadow->wq_reset, shadow->wait_reset);
+       spin_lock_irq(host->host_lock);
+
+       if (!err) {
+               err = shadow->rslt_reset;
+               scsifront_put_rqid(info, shadow->rqid);
+               kfree(shadow);
+       } else {
+               spin_lock(&info->shadow_lock);
+               shadow->rslt_reset = -1;

#define for -1?

Done.


+               spin_unlock(&info->shadow_lock);
+               err = FAILED;
+       }
+
+       spin_unlock_irq(host->host_lock);
+       return err;
+}
+
+static int scsifront_eh_abort_handler(struct scsi_cmnd *sc)
+{
+       DPRINTK("%s\n", __func__);
+       return scsifront_action_handler(sc, VSCSIIF_ACT_SCSI_ABORT);
+}
+
+static int scsifront_dev_reset_handler(struct scsi_cmnd *sc)
+{
+       DPRINTK("%s\n", __func__);
+       return scsifront_action_handler(sc, VSCSIIF_ACT_SCSI_RESET);
+}
+
+static int scsifront_sdev_configure(struct scsi_device *sdev)
+{
+       struct vscsifrnt_info *info = shost_priv(sdev->host);
+
+       if (info && current == info->curr)
+               xenbus_printf(XBT_NIL, info->dev->nodename,
+                             info->dev_state_path, "%d", XenbusStateConnected);
+
+       return 0;
+}
+
+static void scsifront_sdev_destroy(struct scsi_device *sdev)
+{
+       struct vscsifrnt_info *info = shost_priv(sdev->host);
+
+       if (info && current == info->curr)
+               xenbus_printf(XBT_NIL, info->dev->nodename,
+                             info->dev_state_path, "%d", XenbusStateClosed);
+}
+
+static struct scsi_host_template scsifront_sht = {
+       .module                 = THIS_MODULE,
+       .name                   = "Xen SCSI frontend driver",
+       .queuecommand           = scsifront_queuecommand,
+       .eh_abort_handler       = scsifront_eh_abort_handler,
+       .eh_device_reset_handler = scsifront_dev_reset_handler,
+       .slave_configure        = scsifront_sdev_configure,
+       .slave_destroy          = scsifront_sdev_destroy,
+       .cmd_per_lun            = VSCSIIF_DEFAULT_CMD_PER_LUN,
+       .can_queue              = VSCSIIF_MAX_REQS,
+       .this_id                = -1,
+       .cmd_size               = sizeof(struct vscsifrnt_shadow),
+       .sg_tablesize           = VSCSIIF_SG_TABLESIZE,
+       .use_clustering         = DISABLE_CLUSTERING,
+       .proc_name              = "scsifront",
+};
+
+static int scsifront_alloc_ring(struct vscsifrnt_info *info)
+{
+       struct xenbus_device *dev = info->dev;
+       struct vscsiif_sring *sring;
+       int err = -ENOMEM;
+
+       /***** Frontend to Backend ring start *****/
+       sring = (struct vscsiif_sring *) __get_free_page(GFP_KERNEL);
+       if (!sring) {
+               xenbus_dev_fatal(dev, err,
+                       "fail to allocate shared ring (Front to Back)");
+               return err;
+       }
+       SHARED_RING_INIT(sring);
+       FRONT_RING_INIT(&info->ring, sring, PAGE_SIZE);
+
+       err = xenbus_grant_ring(dev, virt_to_mfn(sring));
+       if (err < 0) {
+               free_page((unsigned long) sring);

You can remove the space there.

Indeed.


+               xenbus_dev_fatal(dev, err,
+                       "fail to grant shared ring (Front to Back)");
+               return err;
+       }
+       info->ring_ref = err;
+
+       err = xenbus_alloc_evtchn(dev, &info->evtchn);
+       if (err) {
+               xenbus_dev_fatal(dev, err, "xenbus_alloc_evtchn");
+               goto free_gnttab;
+       }
+
+       err = bind_evtchn_to_irq(info->evtchn);
+       if (err <= 0) {
+               xenbus_dev_fatal(dev, err, "bind_evtchn_to_irq");
+               goto free_gnttab;
+       }
+
+       info->irq = err;
+
+       err = request_threaded_irq(info->irq, NULL, scsifront_irq_fn,
+                                  IRQF_ONESHOT, "scsifront", info);
+       if (err) {
+               xenbus_dev_fatal(dev, err, "request_threaded_irq");
+               goto free_irq;
+       }
+
+       return 0;
+
+/* free resource */
+free_irq:
+       unbind_from_irqhandler(info->irq, info);
+free_gnttab:
+       gnttab_end_foreign_access(info->ring_ref, 0,
+                                 (unsigned long)info->ring.sring);
+

free_page((unsigned long)sring);

No, gnttab_end_foreign_access() is doing it.


+       return err;
+}
+
+static int scsifront_init_ring(struct vscsifrnt_info *info)
+{
+       struct xenbus_device *dev = info->dev;
+       struct xenbus_transaction xbt;
+       int err;
+
+       DPRINTK("%s\n", __func__);
+
+       err = scsifront_alloc_ring(info);
+       if (err)
+               return err;
+       DPRINTK("%u %u\n", info->ring_ref, info->evtchn);
+
+again:
+       err = xenbus_transaction_start(&xbt);
+       if (err)
+               xenbus_dev_fatal(dev, err, "starting transaction");
+
+       err = xenbus_printf(xbt, dev->nodename, "ring-ref", "%u",
+                           info->ring_ref);
+       if (err) {
+               xenbus_dev_fatal(dev, err, "%s", "writing ring-ref");
+               goto fail;
+       }
+
+       err = xenbus_printf(xbt, dev->nodename, "event-channel", "%u",
+                           info->evtchn);
+
+       if (err) {
+               xenbus_dev_fatal(dev, err, "%s", "writing event-channel");
+               goto fail;
+       }
+
+       err = xenbus_transaction_end(xbt, 0);
+       if (err) {
+               if (err == -EAGAIN)
+                       goto again;
+               xenbus_dev_fatal(dev, err, "completing transaction");
+               goto free_sring;
+       }
+
+       return 0;
+
+fail:
+       xenbus_transaction_end(xbt, 1);
+free_sring:
+       unbind_from_irqhandler(info->irq, info);
+       gnttab_end_foreign_access(info->ring_ref, 0,
+                                 (unsigned long)info->ring.sring);
+

The label says 'free_sring' but I am not seeing it being freed?

gnttab_end_foreign_access() is doing magic things. :-)


+       return err;
+}
+
+
+static int scsifront_probe(struct xenbus_device *dev,
+                          const struct xenbus_device_id *id)
+{
+       struct vscsifrnt_info *info;
+       struct Scsi_Host *host;
+       int err = -ENOMEM;
+       char name[DEFAULT_TASK_COMM_LEN];
+
+       host = scsi_host_alloc(&scsifront_sht, sizeof(*info));
+       if (!host) {
+               xenbus_dev_fatal(dev, err, "fail to allocate scsi host");
+               return err;
+       }
+       info = (struct vscsifrnt_info *)host->hostdata;
+
+       dev_set_drvdata(&dev->dev, info);
+       info->dev  = dev;

Extra space.

They pile up already. :-)


+
+       info->shadow_free = (1UL << VSCSIIF_MAX_REQS) - 1;
+
+       err = scsifront_init_ring(info);
+       if (err) {
+               scsi_host_put(host);
+               return err;
+       }
+
+       init_waitqueue_head(&info->wq_sync);
+       spin_lock_init(&info->shadow_lock);
+
+       snprintf(name, DEFAULT_TASK_COMM_LEN, "vscsiif.%d", host->host_no);
+
+       host->max_id      = VSCSIIF_MAX_TARGET;
+       host->max_channel = 0;
+       host->max_lun     = VSCSIIF_MAX_LUN;
+       host->max_sectors = (host->sg_tablesize - 1) * PAGE_SIZE / 512;
+       host->max_cmd_len = VSCSIIF_MAX_COMMAND_SIZE;
+
+       err = scsi_add_host(host, &dev->dev);
+       if (err) {
+               dev_err(&dev->dev, "fail to add scsi host %d\n", err);
+               goto free_sring;
+       }
+       info->host = host;
+       info->host_active = 1;
+
+       xenbus_switch_state(dev, XenbusStateInitialised);
+
+       return 0;
+
+free_sring:
+       unbind_from_irqhandler(info->irq, info);
+       gnttab_end_foreign_access(info->ring_ref, 0,
+                                 (unsigned long)info->ring.sring);
+       scsi_host_put(host);

The label says 'free_sring' but I am not seeing it being freed?

As above.


Hm, could those operations - unbind_from_irqhandler, gnttab_end_foreign_access
and free_page be moved to a seperate function to call?

I don't think it's necessary, those are only _two_ function calls.


+       return err;
+}
+
+static int scsifront_remove(struct xenbus_device *dev)
+{
+       struct vscsifrnt_info *info = dev_get_drvdata(&dev->dev);
+
+       DPRINTK("%s: %s removed\n", __func__, dev->nodename);
+
+       if (info->host_active) {
+               /* Scsi_host not yet removed */
+               scsi_remove_host(info->host);
+               info->host_active = 0;
+       }
+
+       gnttab_end_foreign_access(info->ring_ref, 0,
+                                 (unsigned long)info->ring.sring);
+       unbind_from_irqhandler(info->irq, info);
+

Should you free the ring?

I did.


+       scsi_host_put(info->host);
+
+       return 0;
+}
+
+static void scsifront_disconnect(struct vscsifrnt_info *info)
+{
+       struct xenbus_device *dev = info->dev;
+       struct Scsi_Host *host = info->host;
+
+       DPRINTK("%s: %s disconnect\n", __func__, dev->nodename);
+
+       /*
+        * When this function is executed, all devices of
+        * Frontend have been deleted.
+        * Therefore, it need not block I/O before remove_host.
+        */
+
+       if (info->host_active)
+               scsi_remove_host(host);
+       info->host_active = 0;
+
+       xenbus_frontend_closed(dev);
+}
+
+static void scsifront_do_lun_hotplug(struct vscsifrnt_info *info, int op)
+{
+       struct xenbus_device *dev = info->dev;
+       int i, err = 0;
+       char str[64];
+       char **dir;
+       unsigned int dir_n = 0;
+       unsigned int device_state;
+       unsigned int hst, chn, tgt, lun;
+       struct scsi_device *sdev;
+
+       dir = xenbus_directory(XBT_NIL, dev->otherend, "vscsi-devs", &dir_n);
+       if (IS_ERR(dir))
+               return;
+
+       /* mark current task as the one allowed to modify device states */
+       BUG_ON(info->curr);
+       info->curr = current;
+
+       for (i = 0; i < dir_n; i++) {
+               /* read status */
+               snprintf(str, sizeof(str), "vscsi-devs/%s/state", dir[i]);
+               err = xenbus_scanf(XBT_NIL, dev->otherend, str, "%u",
+                                  &device_state);
+               if (XENBUS_EXIST_ERR(err))
+                       continue;
+
+               /* virtual SCSI device */
+               snprintf(str, sizeof(str), "vscsi-devs/%s/v-dev", dir[i]);
+               err = xenbus_scanf(XBT_NIL, dev->otherend, str,
+                                  "%u:%u:%u:%u", &hst, &chn, &tgt, &lun);
+               if (XENBUS_EXIST_ERR(err))
+                       continue;
+
+               /*
+                * Front device state path, used in slave_configure called
+                * on successfull scsi_add_device, and in slave_destroy called
+                * on remove of a device.
+                */
+               snprintf(info->dev_state_path, sizeof(info->dev_state_path),
+                        "vscsi-devs/%s/state", dir[i]);
+
+               switch (op) {
+               case VSCSIFRONT_OP_ADD_LUN:
+                       if (device_state == XenbusStateInitialised) {

You could convert to make this a bit easier to read to do:

        if (device_state != XenbusStateInitialised)
                break;
        .. and then with the rest of the code.

Yes, that's better.


+                               err = scsi_add_device(info->host, chn, tgt,
+                                                     lun);

You can make that more than 80 characters long.

It's shorter now.

+
+                               if (err) {
+                                       dev_err(&dev->dev, "scsi_add_device\n");
+                                       xenbus_printf(XBT_NIL, dev->nodename,
+                                                     info->dev_state_path,
+                                                     "%d", XenbusStateClosed);
+                               }
+                       }
+                       break;
+               case VSCSIFRONT_OP_DEL_LUN:
+                       if (device_state == XenbusStateClosing) {

Ditto.

Yep.

+                               sdev = scsi_device_lookup(info->host, chn, tgt,
+                                                         lun);
+                               if (sdev) {
+                                       scsi_remove_device(sdev);
+                                       scsi_device_put(sdev);
+                               }
+                       }
+                       break;
+               default:
+                       break;
+               }
+       }
+
+       info->curr = NULL;
+
+       kfree(dir);
+}
+
+static void scsifront_read_backend_params(struct xenbus_device *dev,
+                                         struct vscsifrnt_info *info)
+{
+       unsigned int sg_grant;
+       int ret;
+       struct Scsi_Host *host = info->host;
+
+       ret = xenbus_scanf(XBT_NIL, dev->otherend, "feature-sg-grant", "%u",
+                          &sg_grant);
+       if (ret == 1 && sg_grant) {
+               sg_grant = min_t(unsigned int, sg_grant, SG_ALL);
+               host->sg_tablesize = min_t(unsigned int, sg_grant,
+                       VSCSIIF_SG_TABLESIZE * PAGE_SIZE /
+                       sizeof(struct scsiif_request_segment));
+               dev_info(&dev->dev, "using up to %d SG entries\n",
+                        host->sg_tablesize);

Perhaps this dev_info can be printed regardless of whether
the backend has advertised an value? Isn't this information also
visible in the SysFS? If so, should it be just removed?

I don't know any entry in SysFS covering this information.

Always printing the information is possible, yes.


+               host->max_sectors = (host->sg_tablesize - 1) * PAGE_SIZE / 512;

If the backend decides to give us an value less than optimal - say '2'
should we just ignore it? Perhaps have

if (sg_grant < VSCSIIF_SG_TABLESIZE)
        /* Pfffff. */
        return;

Something like that, yes.

+       }
+}
+
+static void scsifront_backend_changed(struct xenbus_device *dev,
+                                     enum xenbus_state backend_state)
+{
+       struct vscsifrnt_info *info = dev_get_drvdata(&dev->dev);
+
+       DPRINTK("%p %u %u\n", dev, dev->state, backend_state);
+
+       switch (backend_state) {
+       case XenbusStateUnknown:
+       case XenbusStateInitialising:
+       case XenbusStateInitWait:
+       case XenbusStateInitialised:
+               break;
+
+       case XenbusStateConnected:
+               scsifront_read_backend_params(dev, info);
+               if (xenbus_read_driver_state(dev->nodename) ==
+                       XenbusStateInitialised) {

You can make this more than 80 characters. And also remove the '{' and '}'

Okay.



+                       scsifront_do_lun_hotplug(info, VSCSIFRONT_OP_ADD_LUN);
+               }
+
+               if (dev->state != XenbusStateConnected)
+                       xenbus_switch_state(dev, XenbusStateConnected);
+               break;
+
+       case XenbusStateClosed:
+               if (dev->state == XenbusStateClosed)
+                       break;
+               /* Missed the backend's Closing state -- fallthrough */
+       case XenbusStateClosing:
+               scsifront_disconnect(info);
+               break;
+
+       case XenbusStateReconfiguring:
+               scsifront_do_lun_hotplug(info, VSCSIFRONT_OP_DEL_LUN);
+               xenbus_switch_state(dev, XenbusStateReconfiguring);
+               break;
+
+       case XenbusStateReconfigured:
+               scsifront_do_lun_hotplug(info, VSCSIFRONT_OP_ADD_LUN);
+               xenbus_switch_state(dev, XenbusStateConnected);
+               break;
+       }
+}
+
+static const struct xenbus_device_id scsifront_ids[] = {
+       { "vscsi" },
+       { "" }
+};
+
+static DEFINE_XENBUS_DRIVER(scsifront, ,
+       .probe                  = scsifront_probe,
+       .remove                 = scsifront_remove,
+       .otherend_changed       = scsifront_backend_changed,
+);
+
+static int __init scsifront_init(void)
+{
+       if (!xen_domain())
+               return -ENODEV;
+
+       return xenbus_register_frontend(&scsifront_driver);
+}
+module_init(scsifront_init);
+
+static void __exit scsifront_exit(void)
+{
+       xenbus_unregister_driver(&scsifront_driver);
+}
+module_exit(scsifront_exit);
+
+MODULE_DESCRIPTION("Xen SCSI frontend driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("xen:vscsi");

MODULE_AUTHOR?

I don't mind putting myself in there, but I'm not the original author...


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.