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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1503934884.2841.22.camel@wdc.com>
Date:   Mon, 28 Aug 2017 15:41:28 +0000
From:   Bart Van Assche <Bart.VanAssche@....com>
To:     "sfr@...b.auug.org.au" <sfr@...b.auug.org.au>,
        "greg@...ah.com" <greg@...ah.com>
CC:     "James.Bottomley@...senPartnership.com" 
        <James.Bottomley@...senPartnership.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "hare@...e.de" <hare@...e.de>,
        "linux-next@...r.kernel.org" <linux-next@...r.kernel.org>,
        "martin.petersen@...cle.com" <martin.petersen@...cle.com>,
        "sameer.wadgaonkar@...sys.com" <sameer.wadgaonkar@...sys.com>
Subject: Re: linux-next: manual merge of the scsi tree with the staging tree

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.