[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
|
