From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751496AbcFNGYA (ORCPT ); Tue, 14 Jun 2016 02:24:00 -0400 Received: from mail.linux-iscsi.org ([67.23.28.174]:47798 "EHLO linux-iscsi.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751242AbcFNGX4 (ORCPT ); Tue, 14 Jun 2016 02:23:56 -0400 Message-ID: <1465885432.5365.214.camel@haakon3.risingtidesystems.com> Subject: Re: [PATCH] ibmvscsis: Initial commit of IBM VSCSI Tgt Driver From: "Nicholas A. Bellinger" To: Bart Van Assche Cc: Bryant G Ly , James Bottomley , martin.petersen@oracle.com, tyreld@linux.vnet.ibm.com, akpm@linux-foundation.org, kvalo@codeaurora.org, davem@davemloft.net, gregkh@linuxfoundation.org, mchehab@osg.samsung.com, jslaby@suse.com, joe@perches.com, bp@suse.de, linux-kernel@vger.kernel.org, linux-scsi@vger.kernel.org, target-devel@vger.kernel.org, bgly Date: Mon, 13 Jun 2016 23:23:52 -0700 In-Reply-To: <12c6229a-3188-b88c-523f-99219ed60c05@sandisk.com> References: <1464097978-88457-1-git-send-email-bryantly@linux.vnet.ibm.com> <3303bc98-ade7-62f0-71c7-11e7ef42b42d@sandisk.com> <20160524160047.Horde.M1bK0S-HUnY41nXa9ZoJ6w1@ltc.linux.ibm.com> <12c6229a-3188-b88c-523f-99219ed60c05@sandisk.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.4.4-1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 2016-06-10 at 12:03 -0700, Bart Van Assche wrote: > On 05/24/2016 01:00 PM, Bryant G Ly wrote: > > Quoting Bart Van Assche : > >> On 05/24/2016 06:52 AM, Bryant G. Ly wrote: > >>> +static uint64_t ibmvscsis_unpack_lun(const uint8_t *lun, int len) > >>> +{ > >>> + uint64_t res = NO_SUCH_LUN; > >>> + int addressing_method; > >>> + > >>> + if (unlikely(len < 2)) { > >>> + pr_err("Illegal LUN length %d, expected 2 bytes or more\n", > >>> + len); > >>> + goto out; > >>> + } > >>> + > >>> + switch (len) { > >>> + case 8: > >>> + if ((*((__be64 *)lun) & cpu_to_be64(0x0000FFFFFFFFFFFFLL)) > >>> != 0) > >>> + goto out_err; > >>> + break; > >>> + case 4: > >>> + if (*((__be16 *)&lun[2]) != 0) > >>> + goto out_err; > >>> + break; > >>> + case 6: > >>> + if (*((__be32 *)&lun[2]) != 0) > >>> + goto out_err; > >>> + break; > >>> + case 2: > >>> + break; > >>> + default: > >>> + goto out_err; > >>> + } > >>> + > >>> + addressing_method = (*lun) >> 6; /* highest two bits of byte 0 */ > >>> + switch (addressing_method) { > >>> + case SCSI_LUN_ADDR_METHOD_PERIPHERAL: > >>> + case SCSI_LUN_ADDR_METHOD_FLAT: > >>> + case SCSI_LUN_ADDR_METHOD_LUN: > >>> + res = *(lun + 1) | (((*lun) & 0x3f) << 8); > >>> + break; > >>> + > >>> + case SCSI_LUN_ADDR_METHOD_EXTENDED_LUN: > >>> + default: > >>> + pr_err("Unimplemented LUN addressing method %u\n", > >>> + addressing_method); > >>> + break; > >>> + } > >>> + > >>> +out: > >>> + return res; > >>> +out_err: > >>> + pr_err("Support for multi-level LUNs has not yet been > >>> implemented\n"); > >>> + goto out; > >>> +} > >> > >> In the above function there is nothing that is specific to the VIO > >> mechanism. Please consider to merge this function with > >> scsilun_to_int(), e.g. by introducing a new function that accepts a > >> SCSI LUN and the addressing method as arguments and by making > >> scsilun_to_int() call that function with > >> SCSI_LUN_ADDR_METHOD_PERIPHERAL as one of its arguments. > >> > > > > We are going to keep this here atm, until we can make sure canonical can > > pick up > > these changes, since we have a deliverable for a 3rd party vendor who is > > going to > > fully utilize this driver. Once they can pick it up in 4.4 kernel I will > > make the > > change you recommended / move the scsi_lun_addr_method addressed below. > > I will also > > address the changes that ib_srpt needs to utilize this new function too. > > A much better approach would be to move this function into the SCSI core > and to add a copy of that function to the backported driver that will be > provided to Canonical. AFAICT, the problem with this improvement is that it's not stable material, which means that Canonical would have to carry this separately beyond linux-4.4.y, in order to support ibmvscsi-tgt. As-is, it looks like we'll have two target-core fixes to support ibmvscsi-tgt, both of which are considered stable material at this point and will eventually make it to linux-4.4.y. That said Bryant, I still don't have an objection for the initial commit to use it's own internal ibmvscsis_unpack_lun(), but it would be nice in the patch series containing the initial ibmvscsi-tgt commit to include an incremental patch to make this common code, as Bart has highlighted.