linux-next.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* linux-next: manual merge of the scsi tree with the staging tree
@ 2017-08-28  6:41 Stephen Rothwell
  2017-08-28  6:49 ` Greg KH
  2017-09-05  0:18 ` Stephen Rothwell
  0 siblings, 2 replies; 10+ messages in thread
From: Stephen Rothwell @ 2017-08-28  6:41 UTC (permalink / raw)
  To: James Bottomley, Greg KH
  Cc: Linux-Next Mailing List, Linux Kernel Mailing List,
	Hannes Reinecke, Martin K. Petersen, Sameer Wadgaonkar

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.

-- 
Cheers,
Stephen Rothwell

diff --cc drivers/staging/unisys/visorhba/visorhba_main.c
index 8567e447891e,ddce92552ff5..000000000000
--- a/drivers/staging/unisys/visorhba/visorhba_main.c
+++ b/drivers/staging/unisys/visorhba/visorhba_main.c
@@@ -44,12 -44,11 +44,11 @@@ static struct visor_channeltype_descrip
  };
  
  MODULE_DEVICE_TABLE(visorbus, visorhba_channel_types);
 -MODULE_ALIAS("visorbus:" VISOR_VHBA_CHANNEL_UUID_STR);
 +MODULE_ALIAS("visorbus:" VISOR_VHBA_CHANNEL_GUID_STR);
  
  struct visordisk_info {
+ 	struct scsi_device *sdev;
  	u32 valid;
- 	/* Disk Path */
- 	u32 channel, id, lun;
  	atomic_t ios_threshold;
  	atomic_t error_count;
  	struct visordisk_info *next;
@@@ -105,25 -101,19 +104,19 @@@ struct visorhba_devices_open 
  	struct visorhba_devdata *devdata;
  };
  
- #define for_each_vdisk_match(iter, list, match) \
- 	for (iter = &list->head; iter->next; iter = iter->next) \
- 		if ((iter->channel == match->channel) && \
- 		    (iter->id == match->id) && \
- 		    (iter->lun == match->lun))
- 
  /*
 - *	visor_thread_start - starts a thread for the device
 - *	@threadfn: Function the thread starts
 - *	@thrcontext: Context to pass to the thread, i.e. devdata
 - *	@name: string describing name of thread
 + * visor_thread_start - Starts a thread for the device
 + * @threadfn:   Function the thread starts
 + * @thrcontext: Context to pass to the thread, i.e. devdata
 + * @name:	String describing name of thread
   *
 - *	Starts a thread for the device.
 + * Starts a thread for the device.
   *
 - *	Return the task_struct * denoting the thread on success,
 - *             or NULL on failure
 + * Return: The task_struct * denoting the thread on success,
 + *	   or NULL on failure
   */
 -static struct task_struct *visor_thread_start
 -(int (*threadfn)(void *), void *thrcontext, char *name)
 +static struct task_struct *visor_thread_start(int (*threadfn)(void *),
 +					      void *thrcontext, char *name)
  {
  	struct task_struct *task;
  
@@@ -302,21 -280,19 +295,20 @@@ static void cleanup_scsitaskmgmt_handle
  }
  
  /*
 - *	forward_taskmgmt_command - send taskmegmt command to the Service
 - *				   Partition
 - *	@tasktype: Type of taskmgmt command
 - *	@scsidev: Scsidev that issued command
 + * forward_taskmgmt_command - Send taskmegmt command to the Service
 + *			      Partition
 + * @tasktype: Type of taskmgmt command
 + * @scsidev:  Scsidev that issued command
   *
 - *	Create a cmdrsp packet and send it to the Serivce Partition
 - *	that will service this request.
 - *	Returns whether the command was queued successfully or not.
 + * Create a cmdrsp packet and send it to the Serivce Partition
 + * that will service this request.
 + *
 + * Return: Int representing whether command was queued successfully or not
   */
  static int forward_taskmgmt_command(enum task_mgmt_types tasktype,
- 				    struct scsi_cmnd *scsicmd)
+ 				    struct scsi_device *scsidev)
  {
  	struct uiscmdrsp *cmdrsp;
- 	struct scsi_device *scsidev = scsicmd->device;
  	struct visorhba_devdata *devdata =
  		(struct visorhba_devdata *)scsidev->host->hostdata;
  	int notifyresult = 0xffff;
@@@ -607,24 -570,19 +604,21 @@@ static int visorhba_slave_alloc(struct 
  	struct visorhba_devdata *devdata;
  	struct Scsi_Host *scsihost = (struct Scsi_Host *)scsidev->host;
  
++	/* already allocated return success */
+ 	if (scsidev->hostdata)
 -		return 0; /* already allocated return success */
++		return 0;
+ 
 +	/* even though we errored, treat as success */
  	devdata = (struct visorhba_devdata *)scsihost->hostdata;
  	if (!devdata)
 -		return 0; /* even though we errored, treat as success */
 +		return 0;
  
- 	/* already allocated return success */
- 	for_each_vdisk_match(vdisk, devdata, scsidev)
- 		return 0;
- 
- 	tmpvdisk = kzalloc(sizeof(*tmpvdisk), GFP_ATOMIC);
- 	if (!tmpvdisk)
+ 	vdisk = kzalloc(sizeof(*vdisk), GFP_ATOMIC);
+ 	if (!vdisk)
  		return -ENOMEM;
  
- 	tmpvdisk->channel = scsidev->channel;
- 	tmpvdisk->id = scsidev->id;
- 	tmpvdisk->lun = scsidev->lun;
- 	vdisk->next = tmpvdisk;
+ 	vdisk->sdev = scsidev;
+ 	scsidev->hostdata = vdisk;
  	return 0;
  }
  
@@@ -814,16 -766,16 +802,15 @@@ static int visorhba_serverdown(struct v
  }
  
  /*
 - *	do_scsi_linuxstat - scsi command returned linuxstat
 - *	@cmdrsp: response from IOVM
 - *	@scsicmd: Command issued.
 + * do_scsi_linuxstat - Scsi command returned linuxstat
 + * @cmdrsp:  Response from IOVM
 + * @scsicmd: Command issued
   *
 - *	Don't log errors for disk-not-present inquiries
 - *	Returns void
 + * Don't log errors for disk-not-present inquiries.
   */
 -static void
 -do_scsi_linuxstat(struct uiscmdrsp *cmdrsp, struct scsi_cmnd *scsicmd)
 +static void do_scsi_linuxstat(struct uiscmdrsp *cmdrsp,
 +			      struct scsi_cmnd *scsicmd)
  {
- 	struct visorhba_devdata *devdata;
  	struct visordisk_info *vdisk;
  	struct scsi_device *scsidev;
  

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: linux-next: manual merge of the scsi tree with the staging tree
  2017-08-28  6:41 linux-next: manual merge of the scsi tree with the staging tree Stephen Rothwell
@ 2017-08-28  6:49 ` Greg KH
  2017-08-28 15:41   ` Bart Van Assche
  2017-09-05  0:18 ` Stephen Rothwell
  1 sibling, 1 reply; 10+ messages in thread
From: Greg KH @ 2017-08-28  6:49 UTC (permalink / raw)
  To: Stephen Rothwell
  Cc: James Bottomley, Linux-Next Mailing List,
	Linux Kernel Mailing List, Hannes Reinecke, Martin K. Petersen,
	Sameer Wadgaonkar

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.

greg k-h

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: linux-next: manual merge of the scsi tree with the staging tree
  2017-08-28  6:49 ` Greg KH
@ 2017-08-28 15:41   ` Bart Van Assche
  2017-08-28 16:05     ` greg
  0 siblings, 1 reply; 10+ messages in thread
From: Bart Van Assche @ 2017-08-28 15:41 UTC (permalink / raw)
  To: sfr, greg
  Cc: James.Bottomley, linux-kernel, hare, linux-next, martin.petersen,
	sameer.wadgaonkar

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: linux-next: manual merge of the scsi tree with the staging tree
  2017-08-28 15:41   ` Bart Van Assche
@ 2017-08-28 16:05     ` greg
  2017-08-28 16:36       ` Bart Van Assche
  0 siblings, 1 reply; 10+ messages in thread
From: greg @ 2017-08-28 16:05 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: sfr, James.Bottomley, linux-kernel, hare, linux-next,
	martin.petersen, sameer.wadgaonkar

On Mon, Aug 28, 2017 at 03:41:28PM +0000, Bart Van Assche wrote:
> 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:

<snip>

No reason you can't tell them this yourself, right?  :)

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

That's because right now it's still a staging driver.  Also, there are
other scsi drivers in other portions of the kernel tree (like the USB
driver), so there's no hard rule that all scsi drivers have to be under
drivers/scsi/

<snip>

Please provide this review to them, on the properly mailing list, I'm
sure they would be glad to get it.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: linux-next: manual merge of the scsi tree with the staging tree
  2017-08-28 16:05     ` greg
@ 2017-08-28 16:36       ` Bart Van Assche
  2017-08-28 16:44         ` greg
  0 siblings, 1 reply; 10+ messages in thread
From: Bart Van Assche @ 2017-08-28 16:36 UTC (permalink / raw)
  To: greg
  Cc: sfr, linux-kernel, jejb, hare, linux-next, martin.petersen,
	sameer.wadgaonkar

On Mon, 2017-08-28 at 18:05 +0200, greg@kroah.com wrote:
> On Mon, Aug 28, 2017 at 03:41:28PM +0000, Bart Van Assche wrote:
> > * 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?
> 
> That's because right now it's still a staging driver.  Also, there are
> other scsi drivers in other portions of the kernel tree (like the USB
> driver), so there's no hard rule that all scsi drivers have to be under
> drivers/scsi/
> 
> <snip>
> 
> Please provide this review to them, on the properly mailing list, I'm
> sure they would be glad to get it.

OK, I will do that. BTW, is there a written down version of the rules for
adding a driver under drivers/staging available somewhere? As far as I can
see the visorhba driver went in without the linux-scsi mailing list having
been CC-ed. See also Benjamin Romer, [PATCH] staging: unisys: Add s-Par
visorhba, linux-driver-devel mailing list, July 2015
(https://marc.info/?l=linux-driver-devel&m=143681271902628).

Thanks,

Bart.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: linux-next: manual merge of the scsi tree with the staging tree
  2017-08-28 16:36       ` Bart Van Assche
@ 2017-08-28 16:44         ` greg
  2017-08-28 17:06           ` Wadgaonkar, Sameer Laxmikant
  2017-08-28 17:51           ` James Bottomley
  0 siblings, 2 replies; 10+ messages in thread
From: greg @ 2017-08-28 16:44 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: sfr, linux-kernel, jejb, hare, linux-next, martin.petersen,
	sameer.wadgaonkar

On Mon, Aug 28, 2017 at 04:36:06PM +0000, Bart Van Assche wrote:
> On Mon, 2017-08-28 at 18:05 +0200, greg@kroah.com wrote:
> > On Mon, Aug 28, 2017 at 03:41:28PM +0000, Bart Van Assche wrote:
> > > * 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?
> > 
> > That's because right now it's still a staging driver.  Also, there are
> > other scsi drivers in other portions of the kernel tree (like the USB
> > driver), so there's no hard rule that all scsi drivers have to be under
> > drivers/scsi/
> > 
> > <snip>
> > 
> > Please provide this review to them, on the properly mailing list, I'm
> > sure they would be glad to get it.
> 
> OK, I will do that. BTW, is there a written down version of the rules for
> adding a driver under drivers/staging available somewhere? 

The only 2 rules for adding a new drivers/staging driver is:
	- has to compile
	- correct license
and sometimes we let code in if the first one isn't true :)

> As far as I can
> see the visorhba driver went in without the linux-scsi mailing list having
> been CC-ed. See also Benjamin Romer, [PATCH] staging: unisys: Add s-Par
> visorhba, linux-driver-devel mailing list, July 2015
> (https://marc.info/?l=linux-driver-devel&m=143681271902628).

That's totally normal, why would the scsi developers care about a
staging driver in such a rough state.  Only when it looks "good enough"
would we ask for a scsi developer review to move it out of staging.

hope this helps,

greg k-h

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: linux-next: manual merge of the scsi tree with the staging tree
  2017-08-28 16:44         ` greg
@ 2017-08-28 17:06           ` Wadgaonkar, Sameer Laxmikant
  2017-08-28 17:51           ` James Bottomley
  1 sibling, 0 replies; 10+ messages in thread
From: Wadgaonkar, Sameer Laxmikant @ 2017-08-28 17:06 UTC (permalink / raw)
  To: greg, Bart Van Assche
  Cc: sfr, linux-kernel, jejb, hare, linux-next, martin.petersen,
	*S-Par-Maintainer

[-- Attachment #1: Type: text/plain, Size: 2085 bytes --]

Adding SParMaintainer@unisys.com.

-Sameer Wadgaonkar

-----Original Message-----
From: greg@kroah.com [mailto:greg@kroah.com] 
Sent: Monday, August 28, 2017 12:44 PM
To: Bart Van Assche <Bart.VanAssche@wdc.com>
Cc: sfr@canb.auug.org.au; linux-kernel@vger.kernel.org;
jejb@linux.vnet.ibm.com; hare@suse.de; linux-next@vger.kernel.org;
martin.petersen@oracle.com; Wadgaonkar, Sameer Laxmikant
<Sameer.Wadgaonkar@unisys.com>
Subject: Re: linux-next: manual merge of the scsi tree with the staging tree

On Mon, Aug 28, 2017 at 04:36:06PM +0000, Bart Van Assche wrote:
> On Mon, 2017-08-28 at 18:05 +0200, greg@kroah.com wrote:
> > On Mon, Aug 28, 2017 at 03:41:28PM +0000, Bart Van Assche wrote:
> > > * 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?
> > 
> > That's because right now it's still a staging driver.  Also, there are
> > other scsi drivers in other portions of the kernel tree (like the USB
> > driver), so there's no hard rule that all scsi drivers have to be under
> > drivers/scsi/
> > 
> > <snip>
> > 
> > Please provide this review to them, on the properly mailing list, I'm
> > sure they would be glad to get it.
> 
> OK, I will do that. BTW, is there a written down version of the rules for
> adding a driver under drivers/staging available somewhere? 

The only 2 rules for adding a new drivers/staging driver is:
	- has to compile
	- correct license
and sometimes we let code in if the first one isn't true :)

> As far as I can
> see the visorhba driver went in without the linux-scsi mailing list having
> been CC-ed. See also Benjamin Romer, [PATCH] staging: unisys: Add s-Par
> visorhba, linux-driver-devel mailing list, July 2015
> (https://marc.info/?l=linux-driver-devel&m=143681271902628).

That's totally normal, why would the scsi developers care about a
staging driver in such a rough state.  Only when it looks "good enough"
would we ask for a scsi developer review to move it out of staging.

hope this helps,

greg k-h

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 7876 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: linux-next: manual merge of the scsi tree with the staging tree
  2017-08-28 16:44         ` greg
  2017-08-28 17:06           ` Wadgaonkar, Sameer Laxmikant
@ 2017-08-28 17:51           ` James Bottomley
  2017-08-28 18:47             ` greg
  1 sibling, 1 reply; 10+ messages in thread
From: James Bottomley @ 2017-08-28 17:51 UTC (permalink / raw)
  To: greg, Bart Van Assche
  Cc: sfr, linux-kernel, hare, linux-next, martin.petersen,
	sameer.wadgaonkar, linux-scsi

On Mon, 2017-08-28 at 18:44 +0200, greg@kroah.com wrote:
> On Mon, Aug 28, 2017 at 04:36:06PM +0000, Bart Van Assche wrote:
> > 
> > On Mon, 2017-08-28 at 18:05 +0200, greg@kroah.com wrote:
> > > 
> > > On Mon, Aug 28, 2017 at 03:41:28PM +0000, Bart Van Assche wrote:
> > > > 
> > > > * 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?
> > > 
> > > That's because right now it's still a staging driver.  Also,
> > > there are other scsi drivers in other portions of the kernel tree
> > > (like the USB driver), so there's no hard rule that all scsi
> > > drivers have to be under drivers/scsi/
> > > 
> > > <snip>
> > > 
> > > Please provide this review to them, on the properly mailing list,
> > > I'm sure they would be glad to get it.
> > 
> > OK, I will do that. BTW, is there a written down version of the
> > rules for adding a driver under drivers/staging available
> > somewhere? 
> 
> The only 2 rules for adding a new drivers/staging driver is:
> 	- has to compile
> 	- correct license
> and sometimes we let code in if the first one isn't true :)
> 
> > 
> > As far as I can see the visorhba driver went in without the linux-
> > scsi mailing list having been CC-ed. See also Benjamin Romer,
> > [PATCH] staging: unisys: Add s-Par visorhba, linux-driver-devel
> > mailing list, July 2015
> > (https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-
> > 3Fl-3Dlinux-2Ddriver-2Ddevel-26m-
> > 3D143681271902628&d=DwIBAg&c=jf_iaSHvJObTbx-
> > siA1ZOg&r=lxuMXTRCffN3cvb-
> > aRKL97jBhWZUIH_7zc3AgXUz9Mw&m=muMI5n73yuzpaRBjZnIrYLfhcO8lIP0JZSjBG
> > D1LC5I&s=76_tO8QEuhUmhJPijDkweBOfh6DLPRILWj9Rhphb8j0&e= ).
> 
> That's totally normal, why would the scsi developers care about a
> staging driver in such a rough state.  Only when it looks "good
> enough" would we ask for a scsi developer review to move it out of
> staging.

I think I've said before, we'd really rather a SCSI driver went via the
SCSI tree because most of what's wrong with it will be the mechanics of
the driver and its interaction with the SCSI and block subsystems which
simply don't get looked at in the staging tree.  The aesthetics and the
formatting issues are mostly fixed by a quick trip through lindent or a
brief education of the submitters on Linux style.

James

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: linux-next: manual merge of the scsi tree with the staging tree
  2017-08-28 17:51           ` James Bottomley
@ 2017-08-28 18:47             ` greg
  0 siblings, 0 replies; 10+ messages in thread
From: greg @ 2017-08-28 18:47 UTC (permalink / raw)
  To: James Bottomley
  Cc: Bart Van Assche, sfr, linux-kernel, hare, linux-next,
	martin.petersen, sameer.wadgaonkar, linux-scsi

On Mon, Aug 28, 2017 at 06:51:59PM +0100, James Bottomley wrote:
> On Mon, 2017-08-28 at 18:44 +0200, greg@kroah.com wrote:
> > On Mon, Aug 28, 2017 at 04:36:06PM +0000, Bart Van Assche wrote:
> > > 
> > > On Mon, 2017-08-28 at 18:05 +0200, greg@kroah.com wrote:
> > > > 
> > > > On Mon, Aug 28, 2017 at 03:41:28PM +0000, Bart Van Assche wrote:
> > > > > 
> > > > > * 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?
> > > > 
> > > > That's because right now it's still a staging driver.  Also,
> > > > there are other scsi drivers in other portions of the kernel tree
> > > > (like the USB driver), so there's no hard rule that all scsi
> > > > drivers have to be under drivers/scsi/
> > > > 
> > > > <snip>
> > > > 
> > > > Please provide this review to them, on the properly mailing list,
> > > > I'm sure they would be glad to get it.
> > > 
> > > OK, I will do that. BTW, is there a written down version of the
> > > rules for adding a driver under drivers/staging available
> > > somewhere? 
> > 
> > The only 2 rules for adding a new drivers/staging driver is:
> > 	- has to compile
> > 	- correct license
> > and sometimes we let code in if the first one isn't true :)
> > 
> > > 
> > > As far as I can see the visorhba driver went in without the linux-
> > > scsi mailing list having been CC-ed. See also Benjamin Romer,
> > > [PATCH] staging: unisys: Add s-Par visorhba, linux-driver-devel
> > > mailing list, July 2015
> > > (https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-
> > > 3Fl-3Dlinux-2Ddriver-2Ddevel-26m-
> > > 3D143681271902628&d=DwIBAg&c=jf_iaSHvJObTbx-
> > > siA1ZOg&r=lxuMXTRCffN3cvb-
> > > aRKL97jBhWZUIH_7zc3AgXUz9Mw&m=muMI5n73yuzpaRBjZnIrYLfhcO8lIP0JZSjBG
> > > D1LC5I&s=76_tO8QEuhUmhJPijDkweBOfh6DLPRILWj9Rhphb8j0&e= ).
> > 
> > That's totally normal, why would the scsi developers care about a
> > staging driver in such a rough state.  Only when it looks "good
> > enough" would we ask for a scsi developer review to move it out of
> > staging.
> 
> I think I've said before, we'd really rather a SCSI driver went via the
> SCSI tree because most of what's wrong with it will be the mechanics of
> the driver and its interaction with the SCSI and block subsystems which
> simply don't get looked at in the staging tree.  The aesthetics and the
> formatting issues are mostly fixed by a quick trip through lindent or a
> brief education of the submitters on Linux style.

This driver has been in the tree for almost 2 years now, with no
problems in this area before.  It, and the infrastructure around it, has
needed hundreds, if not thousands, of patches and changes and reworks to
get it to the almost readable state it is in today.

It was anything but a "quick trip through lindent".

And we don't have code in the "real" part of the kernel depend on code
in drivers/staging/, as that would not work.

So let's just keep things as they are here, there's been no issues with
this for 2 years, and for times when there is, great, let me know and we
can work to fix things up.  You are always free to break staging drivers
with api changes, it's up to the owner of the staging driver to fix the
issues in that case.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: linux-next: manual merge of the scsi tree with the staging tree
  2017-08-28  6:41 linux-next: manual merge of the scsi tree with the staging tree Stephen Rothwell
  2017-08-28  6:49 ` Greg KH
@ 2017-09-05  0:18 ` Stephen Rothwell
  1 sibling, 0 replies; 10+ messages in thread
From: Stephen Rothwell @ 2017-09-05  0:18 UTC (permalink / raw)
  To: James Bottomley, Greg KH
  Cc: Linux-Next Mailing List, Linux Kernel Mailing List,
	Hannes Reinecke, Martin K. Petersen, Sameer Wadgaonkar

Hi all,

On Mon, 28 Aug 2017 16:41:27 +1000 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> 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.
> 
> -- 
> Cheers,
> Stephen Rothwell
> 
> diff --cc drivers/staging/unisys/visorhba/visorhba_main.c
> index 8567e447891e,ddce92552ff5..000000000000
> --- a/drivers/staging/unisys/visorhba/visorhba_main.c
> +++ b/drivers/staging/unisys/visorhba/visorhba_main.c
> @@@ -44,12 -44,11 +44,11 @@@ static struct visor_channeltype_descrip
>   };
>   
>   MODULE_DEVICE_TABLE(visorbus, visorhba_channel_types);
>  -MODULE_ALIAS("visorbus:" VISOR_VHBA_CHANNEL_UUID_STR);
>  +MODULE_ALIAS("visorbus:" VISOR_VHBA_CHANNEL_GUID_STR);
>   
>   struct visordisk_info {
> + 	struct scsi_device *sdev;
>   	u32 valid;
> - 	/* Disk Path */
> - 	u32 channel, id, lun;
>   	atomic_t ios_threshold;
>   	atomic_t error_count;
>   	struct visordisk_info *next;
> @@@ -105,25 -101,19 +104,19 @@@ struct visorhba_devices_open 
>   	struct visorhba_devdata *devdata;
>   };
>   
> - #define for_each_vdisk_match(iter, list, match) \
> - 	for (iter = &list->head; iter->next; iter = iter->next) \
> - 		if ((iter->channel == match->channel) && \
> - 		    (iter->id == match->id) && \
> - 		    (iter->lun == match->lun))
> - 
>   /*
>  - *	visor_thread_start - starts a thread for the device
>  - *	@threadfn: Function the thread starts
>  - *	@thrcontext: Context to pass to the thread, i.e. devdata
>  - *	@name: string describing name of thread
>  + * visor_thread_start - Starts a thread for the device
>  + * @threadfn:   Function the thread starts
>  + * @thrcontext: Context to pass to the thread, i.e. devdata
>  + * @name:	String describing name of thread
>    *
>  - *	Starts a thread for the device.
>  + * Starts a thread for the device.
>    *
>  - *	Return the task_struct * denoting the thread on success,
>  - *             or NULL on failure
>  + * Return: The task_struct * denoting the thread on success,
>  + *	   or NULL on failure
>    */
>  -static struct task_struct *visor_thread_start
>  -(int (*threadfn)(void *), void *thrcontext, char *name)
>  +static struct task_struct *visor_thread_start(int (*threadfn)(void *),
>  +					      void *thrcontext, char *name)
>   {
>   	struct task_struct *task;
>   
> @@@ -302,21 -280,19 +295,20 @@@ static void cleanup_scsitaskmgmt_handle
>   }
>   
>   /*
>  - *	forward_taskmgmt_command - send taskmegmt command to the Service
>  - *				   Partition
>  - *	@tasktype: Type of taskmgmt command
>  - *	@scsidev: Scsidev that issued command
>  + * forward_taskmgmt_command - Send taskmegmt command to the Service
>  + *			      Partition
>  + * @tasktype: Type of taskmgmt command
>  + * @scsidev:  Scsidev that issued command
>    *
>  - *	Create a cmdrsp packet and send it to the Serivce Partition
>  - *	that will service this request.
>  - *	Returns whether the command was queued successfully or not.
>  + * Create a cmdrsp packet and send it to the Serivce Partition
>  + * that will service this request.
>  + *
>  + * Return: Int representing whether command was queued successfully or not
>    */
>   static int forward_taskmgmt_command(enum task_mgmt_types tasktype,
> - 				    struct scsi_cmnd *scsicmd)
> + 				    struct scsi_device *scsidev)
>   {
>   	struct uiscmdrsp *cmdrsp;
> - 	struct scsi_device *scsidev = scsicmd->device;
>   	struct visorhba_devdata *devdata =
>   		(struct visorhba_devdata *)scsidev->host->hostdata;
>   	int notifyresult = 0xffff;
> @@@ -607,24 -570,19 +604,21 @@@ static int visorhba_slave_alloc(struct 
>   	struct visorhba_devdata *devdata;
>   	struct Scsi_Host *scsihost = (struct Scsi_Host *)scsidev->host;
>   
> ++	/* already allocated return success */
> + 	if (scsidev->hostdata)
>  -		return 0; /* already allocated return success */
> ++		return 0;
> + 
>  +	/* even though we errored, treat as success */
>   	devdata = (struct visorhba_devdata *)scsihost->hostdata;
>   	if (!devdata)
>  -		return 0; /* even though we errored, treat as success */
>  +		return 0;
>   
> - 	/* already allocated return success */
> - 	for_each_vdisk_match(vdisk, devdata, scsidev)
> - 		return 0;
> - 
> - 	tmpvdisk = kzalloc(sizeof(*tmpvdisk), GFP_ATOMIC);
> - 	if (!tmpvdisk)
> + 	vdisk = kzalloc(sizeof(*vdisk), GFP_ATOMIC);
> + 	if (!vdisk)
>   		return -ENOMEM;
>   
> - 	tmpvdisk->channel = scsidev->channel;
> - 	tmpvdisk->id = scsidev->id;
> - 	tmpvdisk->lun = scsidev->lun;
> - 	vdisk->next = tmpvdisk;
> + 	vdisk->sdev = scsidev;
> + 	scsidev->hostdata = vdisk;
>   	return 0;
>   }
>   
> @@@ -814,16 -766,16 +802,15 @@@ static int visorhba_serverdown(struct v
>   }
>   
>   /*
>  - *	do_scsi_linuxstat - scsi command returned linuxstat
>  - *	@cmdrsp: response from IOVM
>  - *	@scsicmd: Command issued.
>  + * do_scsi_linuxstat - Scsi command returned linuxstat
>  + * @cmdrsp:  Response from IOVM
>  + * @scsicmd: Command issued
>    *
>  - *	Don't log errors for disk-not-present inquiries
>  - *	Returns void
>  + * Don't log errors for disk-not-present inquiries.
>    */
>  -static void
>  -do_scsi_linuxstat(struct uiscmdrsp *cmdrsp, struct scsi_cmnd *scsicmd)
>  +static void do_scsi_linuxstat(struct uiscmdrsp *cmdrsp,
>  +			      struct scsi_cmnd *scsicmd)
>   {
> - 	struct visorhba_devdata *devdata;
>   	struct visordisk_info *vdisk;
>   	struct scsi_device *scsidev;
>   

Just a reminder that the above conflict still exists.

-- 
Cheers,
Stephen Rothwell

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2017-09-05  0:18 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-28  6:41 linux-next: manual merge of the scsi tree with the staging tree Stephen Rothwell
2017-08-28  6:49 ` Greg KH
2017-08-28 15:41   ` Bart Van Assche
2017-08-28 16:05     ` greg
2017-08-28 16:36       ` Bart Van Assche
2017-08-28 16:44         ` greg
2017-08-28 17:06           ` Wadgaonkar, Sameer Laxmikant
2017-08-28 17:51           ` James Bottomley
2017-08-28 18:47             ` greg
2017-09-05  0:18 ` Stephen Rothwell

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).