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

[Michael Cyr]

Responding to those issues not already addressed by Bryant.

On 6/23/16 9:22 AM, Christoph Hellwig wrote:



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

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

Fixed.



+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(_dev_lock);
+   list_for_each_entry(vscsi, _dev_list, list) {
+   vdev = vscsi->dma_dev;
+   if (!strcmp(dev_name(>dev), name)) {
+   tport = >tport;
+   break;
+   }
+   }
+   spin_unlock_bh(_dev_lock);

Without grabbing a reference this looks inherently unsafe.

I don't understand your concern.  Could you please provide more detail?

+static void ibmvscsis_scheduler(struct work_struct *work)

Odd name for a work item.

Not sure why it seems odd.  It schedules work to TCM.

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

What do you need the schedule_q for as the workqueue already tracks
the commands?
There are a number of places where we need to see if there is anything 
on the work queue, and I am not aware of an existing function to do 
this, so we keep our own list of commands which have been queued.




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

No need for these wrapers.

Removed.

+static irqreturn_t ibmvscsis_interrupt(int dummy, void *data)
+{
+   struct scsi_info *vscsi = data;
+
+   vio_disable_interrupts(vscsi->dma_dev);
+   tasklet_schedule(>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.
There are some requests, like SRP Login and the various MADs, which can 
be handled at the interrupt level, and so we do so.


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Bryant G. Ly]

On 6/23/16, 9:22 AM, "Christoph Hellwig"  wrote:

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

Why do you suggest splitting the path up for the libsrp stuff? The current 
upstream
does not contain libsrp. The only reason the patch shows that there is a 
removal of
libsrp is that James Bottomley suggested doing a revert of: 
libsrp: removal - commit f6667938cfceefd8afe6355ceb6497dce4883ae9
to make it clear that I’m bringing back what had existed a year ago within the 
upstream.

>
>> new file mode 100644
>> index 000..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?
>

I will change to module-y. The reason is we are possibly going to write
another target fabric in the future with the IBMVFC driver so just incase
we want to leave it separated for now. 

>> +/***
>> + * IBM Virtual SCSI Target Driver
>> + * Copyright (C) 2003-2005 Dave Boutcher (boutc...@us.ibm.com) IBM Corp.
>> + * Santiago Leon (san...@us.ibm.com) IBM Corp.
>> + * Linda Xie (l...@us.ibm.com) IBM Corp.
>> + *
>> + * Copyright (C) 2005-2011 FUJITA Tomonori 
>> + * Copyright (C) 2010 Nicholas A. Bellinger 
>> + * Copyright (C) 2016 Bryant G. Ly  IBM Corp.
>> + *
>> + * Authors: Bryant G. Ly 
>> + * Authors: Michael Cyr 
>
>What's the reational for the copyright vs Authors lines?

No reason, ill remove the copyright. 

>
>> +#include "ibmvscsi_tgt.h"
>> +
>> +#ifndef H_GET_PARTNER_INFO
>> +#define H_GET_PARTNER_INFO  0x0008LL
>> +#endif
>
>Should this be in a header with the other hcalls?

Yes, Moved.

>
>> +static const char ibmvscsis_driver_name[] = "ibmvscsis";
>
>I think you can just assign the name directly in the driver ops
>structure.

Fixed.

>
>> +static const char ibmvscsis_workq_name[] = "ibmvscsis";
>
>This one seems unused.

Removed

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

Will do.



>> +static int ibmvscsis_alloc_cmds(struct scsi_info *vscsi, int num)
>> +{
>> +struct ibmvscsis_cmd *cmd;
>> +int i;
>> +
>> +INIT_LIST_HEAD(>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(>work, ibmvscsis_scheduler);
>> +list_add_tail(>list, >free_cmd);
>> +}
>> +
>> +return 0;
>> +}
>
>Why can't you use the existing infrastructure for cmd pools in the
>target core?
>

I wasn’t aware there was existing infrastructure.



>> +rc = srp_transfer_data(cmd, _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.

Changed to:

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);
/* Fixed/Current Error = 0
 * Sense Key 

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

[Christoph Hellwig]

> -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 000..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 (boutc...@us.ibm.com) IBM Corp.
> + *  Santiago Leon (san...@us.ibm.com) IBM Corp.
> + *  Linda Xie (l...@us.ibm.com) IBM Corp.
> + *
> + * Copyright (C) 2005-2011 FUJITA Tomonori 
> + * Copyright (C) 2010 Nicholas A. Bellinger 
> + * Copyright (C) 2016 Bryant G. Ly  IBM Corp.
> + *
> + * Authors: Bryant G. Ly 
> + * Authors: Michael Cyr 

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

> +#include "ibmvscsi_tgt.h"
> +
> +#ifndef H_GET_PARTNER_INFO
> +#define H_GET_PARTNER_INFO  0x0008LL
> +#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(_dev_lock);
> + list_for_each_entry(vscsi, _dev_list, list) {
> + vdev = vscsi->dma_dev;
> + if (!strcmp(dev_name(>dev), name)) {
> + tport = >tport;
> + break;
> + }
> + }
> + spin_unlock_bh(_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(>intr_lock);
> +
> + /* Remove from schedule_q */
> + list_del(>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(>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(>work, ibmvscsis_scheduler);
> + list_add_tail(>list, >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(>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(>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, _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;