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

["Bryant G. Ly" <bryantly@linux.vnet.ibm.com>]

On 6/23/16, 9:22 AM, "Christoph Hellwig" <target-devel-owner@vger.kernel.org on behalf of hch@infradead.org> 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 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?
>

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

No reason, ill remove the copyright. 

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

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.

<SNIP>

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

I wasn’t aware there was existing infrastructure.

<SNIP>

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

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 = Medium Error (0x03)
                 * Additonal Length = 0xa
                 * Logical Unit Communication Time-out asc/ascq = 0x801
                 */
                scsi_build_sense_buffer(0, se_cmd->sense_buffer, MEDIUM_ERROR,
                                        0x08, 0x01);
        }

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


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