Re: [PATCH v7] ibmvscsis: Initial commit of IBM VSCSI Tgt Driver

[Christoph Hellwig <hch@infradead.org>]

> -config SCSI_SRP
> -	tristate "SCSI RDMA Protocol helper library"
> -	depends on SCSI && PCI
> -	select SCSI_TGT
> -	help
> -	  If you wish to use SRP target drivers, say Y.
> -
> -	  To compile this driver as a module, choose M here: the
> -	  module will be called libsrp.
> -

Please split that removal of libsrp into a separate patch before
adding the new driver.

> new file mode 100644
> index 0000000..887574d
> --- /dev/null
> +++ b/drivers/scsi/ibmvscsi_tgt/Makefile
> @@ -0,0 +1,4 @@
> +obj-$(CONFIG_SCSI_IBMVSCSIS)	+= ibmvscsis.o
> +
> +ibmvscsis-objs := libsrp.o ibmvscsi_tgt.o

please use module-y for adding objects.  Also what's the reason
for splitting these files?

> +/*******************************************************************************
> + * IBM Virtual SCSI Target Driver
> + * Copyright (C) 2003-2005 Dave Boutcher (boutcher@us.ibm.com) IBM Corp.
> + *			   Santiago Leon (santil@us.ibm.com) IBM Corp.
> + *			   Linda Xie (lxie@us.ibm.com) IBM Corp.
> + *
> + * Copyright (C) 2005-2011 FUJITA Tomonori <tomof@acm.org>
> + * Copyright (C) 2010 Nicholas A. Bellinger <nab@kernel.org>
> + * Copyright (C) 2016 Bryant G. Ly <bryantly@linux.vnet.ibm.com> IBM Corp.
> + *
> + * Authors: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
> + * Authors: Michael Cyr <mikecyr@linux.vnet.ibm.com>

What's the reational for the copyright vs Authors lines?

> +#include "ibmvscsi_tgt.h"
> +
> +#ifndef H_GET_PARTNER_INFO
> +#define H_GET_PARTNER_INFO      0x0000000000000008LL
> +#endif

Should this be in a header with the other hcalls?

> +static const char ibmvscsis_driver_name[] = "ibmvscsis";

I think you can just assign the name directly in the driver ops
structure.

> +static const char ibmvscsis_workq_name[] = "ibmvscsis";

This one seems unused.

> +		vscsi->flags &= (~PROCESSING_MAD);

No need for the braces here. (ditto for many other places later on)

> +static long ibmvscsis_parse_command(struct scsi_info *vscsi,
> +				    struct viosrp_crq *crq);

Can you avoid forward declarations where easily possible, and if not
keep them all at the beginning of the file?

> +static struct ibmvscsis_tport *ibmvscsis_lookup_port(const char *name)
> +{
> +	struct ibmvscsis_tport *tport = NULL;
> +	struct vio_dev *vdev;
> +	struct scsi_info *vscsi;
> +
> +	spin_lock_bh(&ibmvscsis_dev_lock);
> +	list_for_each_entry(vscsi, &ibmvscsis_dev_list, list) {
> +		vdev = vscsi->dma_dev;
> +		if (!strcmp(dev_name(&vdev->dev), name)) {
> +			tport = &vscsi->tport;
> +			break;
> +		}
> +	}
> +	spin_unlock_bh(&ibmvscsis_dev_lock);

Without grabbing a reference this looks inherently unsafe.

> +static void ibmvscsis_scheduler(struct work_struct *work)

Odd name for a work item.

> +{
> +	struct ibmvscsis_cmd *cmd = container_of(work, struct ibmvscsis_cmd,
> +						 work);
> +	struct scsi_info *vscsi = cmd->adapter;
> +
> +	spin_lock_bh(&vscsi->intr_lock);
> +
> +	/* Remove from schedule_q */
> +	list_del(&cmd->list);

What do you need the schedule_q for as the workqueue already tracks
the commands?

> +static int ibmvscsis_alloc_cmds(struct scsi_info *vscsi, int num)
> +{
> +	struct ibmvscsis_cmd *cmd;
> +	int i;
> +
> +	INIT_LIST_HEAD(&vscsi->free_cmd);
> +	vscsi->cmd_pool = kcalloc(num, sizeof(struct ibmvscsis_cmd),
> +				  GFP_KERNEL);
> +	if (!vscsi->cmd_pool)
> +		return -ENOMEM;
> +
> +	for (i = 0, cmd = (struct ibmvscsis_cmd *)vscsi->cmd_pool; i < num;
> +	     i++, cmd++) {
> +		cmd->adapter = vscsi;
> +		INIT_WORK(&cmd->work, ibmvscsis_scheduler);
> +		list_add_tail(&cmd->list, &vscsi->free_cmd);
> +	}
> +
> +	return 0;
> +}

Why can't you use the existing infrastructure for cmd pools in the
target core?

> +static void ibmvscsis_alloc_common_locks(struct scsi_info *vscsi)
> +{
> +	spin_lock_init(&vscsi->intr_lock);
> +}
> +
> +static void ibmvscsis_free_common_locks(struct scsi_info *vscsi)
> +{
> +	/* Nothing to do here */
> +}

No need for these wrapers.

> +static irqreturn_t ibmvscsis_interrupt(int dummy, void *data)
> +{
> +	struct scsi_info *vscsi = data;
> +
> +	vio_disable_interrupts(vscsi->dma_dev);
> +	tasklet_schedule(&vscsi->work_task);
> +
> +	return IRQ_HANDLED;
> +}

Can you explain the need for the tasklet?  There shouldn't be a whole
lot of working before passing the command off to the workqueue anyway.

> +	rc = srp_transfer_data(cmd, &vio_iu(iue)->srp.cmd, ibmvscsis_rdma, 1,
> +			       1);
> +	if (rc) {
> +		pr_err("srp_transfer_data failed: %d\n", rc);
> +		sd = se_cmd->sense_buffer;
> +		se_cmd->scsi_sense_length = 18;
> +		memset(se_cmd->sense_buffer, 0, se_cmd->scsi_sense_length);
> +		/* Current error */
> +		sd[0] = 0x70;
> +		/* sense key = Medium Error */
> +		sd[2] = 3;
> +		/* additional length (length - 8) */
> +		sd[7] = 10;
> +		/* asc/ascq 0x801 = Logical Unit Communication time-out */
> +		sd[12] = 8;
> +		sd[13] = 1;

The Fabrics driver shouldn't generate it's own sense codes.  This
would for example break for a lun using descriptor format sense data.