Re: linux-next: manual merge of the scsi tree with the staging tree

[Bart Van Assche]

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.