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

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

 



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




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]
  Powered by Linux