From: Bart Van Assche <Bart.VanAssche@wdc.com>
To: "sfr@canb.auug.org.au" <sfr@canb.auug.org.au>,
        "greg@kroah.com" <greg@kroah.com>
CC: "James.Bottomley@HansenPartnership.com" 
        <James.Bottomley@HansenPartnership.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "hare@suse.de" <hare@suse.de>,
        "linux-next@vger.kernel.org" <linux-next@vger.kernel.org>,
        "martin.petersen@oracle.com" <martin.petersen@oracle.com>,
        "sameer.wadgaonkar@unisys.com" <sameer.wadgaonkar@unisys.com>
Subject: Re: linux-next: manual merge of the scsi tree with the staging tree
Thread-Topic: linux-next: manual merge of the scsi tree with the staging tree
Thread-Index: AQHTIA5V5cYCCR1KWkWcyt7ODzVJyaKZ6BgA
Date: Mon, 28 Aug 2017 15:41:28 +0000
Message-ID: <1503934884.2841.22.camel@wdc.com>
References: <20170828164127.15902025@canb.auug.org.au>
         <20170828064921.GA24696@kroah.com>
In-Reply-To: <20170828064921.GA24696@kroah.com>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="utf-8"
Content-ID: <369FDC46C03CEA4B80304C5E39A13EFF@namprd04.prod.outlook.com>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Aug 2017 15:41:28.5472
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY1PR0401MB1353
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by nfs id v7SFgOPi002489
Content-Length: 2910
Lines: 64

On Mon, 2017-08-28 at 08:49 +0200, Greg KH wrote:
> On Mon, Aug 28, 2017 at 04:41:27PM +1000, Stephen Rothwell wrote:
> > Hi James,
> > 
> > Today's linux-next merge of the scsi tree got a conflict in:
> > 
> >   drivers/staging/unisys/visorhba/visorhba_main.c
> > 
> > between commits:
> > 
> >   781facd05eb9 ("staging: unisys: visorhba: visorhba_main.c: fixed comment formatting issues")
> > 
> > from the staging tree and commit:
> > 
> >   7bc4e528d9f6 ("scsi: visorhba: sanitze private device data allocation")
> > 
> > from the scsi tree.
> > 
> > I fixed it up (see below) and can carry the fix as necessary. This
> > is now fixed as far as linux-next is concerned, but any non trivial
> > conflicts should be mentioned to your upstream maintainer when your tree
> > is submitted for merging.  You may also want to consider cooperating
> > with the maintainer of the conflicting tree to minimise any particularly
> > complex conflicts.
> 
> Ick, messy merge, thanks for doing this.

Hello Greg,

If you agree with the following, please communicate this to the visorhba
authors:
* Most SCSI drivers exist under drivers/scsi, including the virtio-scsi and
  xen-scsifront drivers. So why has the visorhba driver been added under
  unisys/visorhba?
* Since the SCSI core already keeps track of which commands are pending, the
  visorhba driver should not do this. I'm referring here to the scsipending
  data structure and the pending[] array in struct visorhb_devdata.
* The pending[] array should be removed and should be converted into
  driver-private command data by setting the .cmd_size member of the SCSI host
  template.
* scsi_host_find_tag() should be used when a command completion is received
  to look up the pointer of the SCSI command that has been completed.
* All code that calls for_each_vdisk_match() looks buggy to me in some way.
  E.g. visorhba_abort_handler() should only affect the command passed as an
  argument to that function and no other SCSI commands.
* The device and bus reset handlers should call scsi_done() for all affected
  commands if forward_taskmgmt_command() succeeds instead of only the command
  passed as an argument.
* The for_each_vdisk_match() macro does not protect against concurrent device
  removal and hence is unsafe. This macro should be removed and in contexts
  where it is *really* needed to iterate over SCSI devices
  shost_for_each_device() should be used instead.
* Instead of creating a kernel thread to poll for responses the visorhba driver
  should only process responses after it has received an interrupt that tells it
  to do so (see also the visor_thread_start() calls).
* SCSI drivers should not have special-case code for host shutdown.
* The 'dev_info_list' member in struct visorhba_data is not used and hence
  should be removed.

Please note that I have not attempted to do a full review of the visorhba driver.

Thanks,

Bart.