[PATCH v2 0/3] staging: unisys: visorbus fixes

[PATCH v2 0/3] staging: unisys: visorbus fixes

[David Kershner]

This series contains fixes for problems found while testing visorbus.

v2 Updates: 
	patch 1: made phys_info __packed coming into iochannel.h
	patch 2: Fixed up extra line and comments 
	patch 3: Incorporated comments from gregkh. 

David Kershner (3):
  staging: unisys: Move phys_info to iochannel.h
  staging: unisys: convert pack pragma to __packed
  staging: unisys: Don't hold device responses until driver loads

 drivers/staging/unisys/include/iochannel.h        | 49 +++++++++++------------
 drivers/staging/unisys/visorbus/visorbus_main.c   | 28 ++++---------
 drivers/staging/unisys/visorbus/vmcallinterface.h | 12 ------
 3 files changed, 30 insertions(+), 59 deletions(-)

-- 
1.9.1

[PATCH v2 1/3] staging: unisys: Move phys_info to iochannel.h

[David Kershner]

This moves phys_info to iochannel.h. It is only
used by iochannel.h and is not needed in
vmcallinterfaces.h.

Signed-off-by: David Kershner <david.kershner@unisys.com>
---
 drivers/staging/unisys/include/iochannel.h        |  6 ++++++
 drivers/staging/unisys/visorbus/vmcallinterface.h | 12 ------------
 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/unisys/include/iochannel.h b/drivers/staging/unisys/include/iochannel.h
index cbb5875..ee28cdc 100644
--- a/drivers/staging/unisys/include/iochannel.h
+++ b/drivers/staging/unisys/include/iochannel.h
@@ -149,6 +149,12 @@ enum vdisk_mgmt_types {
 
 /* structs with pragma pack  */
 
+struct phys_info {
+	u64 pi_pfn;
+	u16 pi_off;
+	u16 pi_len;
+} __packed;
+
 /* ///////////// BEGIN PRAGMA PACK PUSH 1 ///////////////////////// */
 /* ///////////// ONLY STRUCT TYPE SHOULD BE BELOW */
 
diff --git a/drivers/staging/unisys/visorbus/vmcallinterface.h b/drivers/staging/unisys/visorbus/vmcallinterface.h
index dc09caf..7a53df0 100644
--- a/drivers/staging/unisys/visorbus/vmcallinterface.h
+++ b/drivers/staging/unisys/visorbus/vmcallinterface.h
@@ -94,18 +94,6 @@ enum vmcall_monitor_interface_method_tuple { /* VMCALL identification tuples  */
 /* ///////////// BEGIN PRAGMA PACK PUSH 1 ///////////////////////// */
 /* ///////////// ONLY STRUCT TYPE SHOULD BE BELOW */
 #pragma pack(push, 1)
-struct phys_info {
-	u64 pi_pfn;
-	u16 pi_off;
-	u16 pi_len;
-};
-
-#pragma pack(pop)
-/* ///////////// END PRAGMA PACK PUSH 1 /////////////////////////// */
-
-/* ///////////// BEGIN PRAGMA PACK PUSH 1 ///////////////////////// */
-/* ///////////// ONLY STRUCT TYPE SHOULD BE BELOW */
-#pragma pack(push, 1)
 /* Parameters to VMCALL_IO_CONTROLVM_ADDR interface */
 struct vmcall_io_controlvm_addr_params {
 	    /* The Guest-relative physical address of the ControlVm channel.
-- 
1.9.1

[PATCH v2 2/3] staging: unisys: convert pack pragma to __packed

[David Kershner]

It was noticed that iochannel.h was still using pragmas to
pack the datastructures, should be using __packed instead.

Signed-off-by: David Kershner <david.kershner@unisys.com>
---
 drivers/staging/unisys/include/iochannel.h | 45 ++++++++++++------------------
 1 file changed, 18 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/unisys/include/iochannel.h b/drivers/staging/unisys/include/iochannel.h
index ee28cdc..538036d 100644
--- a/drivers/staging/unisys/include/iochannel.h
+++ b/drivers/staging/unisys/include/iochannel.h
@@ -147,23 +147,16 @@ enum vdisk_mgmt_types {
 	VDISK_MGMT_RELEASE,
 };
 
-/* structs with pragma pack  */
-
 struct phys_info {
 	u64 pi_pfn;
 	u16 pi_off;
 	u16 pi_len;
 } __packed;
 
-/* ///////////// BEGIN PRAGMA PACK PUSH 1 ///////////////////////// */
-/* ///////////// ONLY STRUCT TYPE SHOULD BE BELOW */
-
-#pragma pack(push, 1)
-
 struct guest_phys_info {
 	u64 address;
 	u64 length;
-};
+} __packed;
 
 #define GPI_ENTRIES_PER_PAGE (PAGE_SIZE / sizeof(struct guest_phys_info))
 
@@ -171,12 +164,12 @@ struct uisscsi_dest {
 	u32 channel;		/* channel == bus number */
 	u32 id;			/* id == target number */
 	u32 lun;		/* lun == logical unit number */
-};
+} __packed;
 
 struct vhba_wwnn {
 	u32 wwnn1;
 	u32 wwnn2;
-};
+} __packed;
 
 /* WARNING: Values stired in this structure must contain maximum counts (not
  * maximum values). */
@@ -193,7 +186,7 @@ struct vhba_config_max {	/* 20 bytes */
 				 * bus */
 	/* max io size is often determined by the resource of the hba. e.g */
 	/* max scatter gather list length * page size / sector size */
-};
+} __packed;
 
 struct uiscmdrsp_scsi {
 	void *scsicmd;		/* the handle to the cmd that was received -
@@ -232,7 +225,7 @@ struct uiscmdrsp_scsi {
 	 * scsi.linuxstat is SAM_STAT_GOOD
 	 * That is, there is NO error.
 	 */
-};
+} __packed;
 
 /* Defines to support sending correct inquiry result when no disk is
  * configured.
@@ -346,7 +339,7 @@ struct sense_data {
 	u8 additional_sense_code_qualifier;
 	u8 fru_code;
 	u8 sense_key_specific[3];
-};
+} __packed;
 
 struct net_pkt_xmt {
 	int len;	/* full length of data in the packet */
@@ -374,11 +367,11 @@ struct net_pkt_xmt {
 	     * guest memory to get to the header. uisnic needs ethhdr to
 	     * determine how to route the packet.
 	     */
-};
+} __packed;
 
 struct net_pkt_xmtdone {
 	u32 xmt_done_result;	/* result of NET_XMIT */
-};
+} __packed;
 
 /* RCVPOST_BUF_SIZe must be at most page_size(4096) - cache_line_size (64) The
  * reason is because dev_skb_alloc which is used to generate RCV_POST skbs in
@@ -400,7 +393,7 @@ struct net_pkt_rcvpost {
 	    u64 unique_num;		/* This is used to make sure that
 					 * receive posts are returned to  */
 	    /* the Adapter which we sent them originally. */
-};
+} __packed;
 
 struct net_pkt_rcv {
 	/* the number of receive buffers that can be chained  */
@@ -414,17 +407,17 @@ struct net_pkt_rcv {
 	/* NOTE: first rcvbuf in the chain will also be provided in net.buf. */
 	u64 unique_num;
 	u32 rcvs_dropped_delta;
-};
+} __packed;
 
 struct net_pkt_enbdis {
 	void *context;
 	u16 enable;		/* 1 = enable, 0 = disable */
-};
+} __packed;
 
 struct net_pkt_macaddr {
 	void *context;
 	u8 macaddr[MAX_MACADDR_LEN];	/* 6 bytes */
-};
+} __packed;
 
 /* cmd rsp packet used for VNIC network traffic  */
 struct uiscmdrsp_net {
@@ -441,7 +434,7 @@ struct uiscmdrsp_net {
 						/* and NET_CONNECT_STATUS */
 		struct net_pkt_macaddr macaddr;
 	};
-};
+} __packed;
 
 struct uiscmdrsp_scsitaskmgmt {
 	enum task_mgmt_types tasktype;
@@ -478,7 +471,7 @@ struct uiscmdrsp_scsitaskmgmt {
 
 	    /* result of taskmgmt command - set by IOPart - values are: */
 #define TASK_MGMT_FAILED  0
-};
+} __packed;
 
 /* The following is used by uissd to send disk add/remove notifications to
  * Guest */
@@ -488,7 +481,7 @@ struct uiscmdrsp_disknotify {
 	void *v_hba;		/* Pointer to vhba_info for channel info to
 				 * route msg */
 	u32 channel, id, lun;	/* SCSI Path of Disk to added or removed */
-};
+} __packed;
 
 /* The following is used by virthba/vSCSI to send the Acquire/Release commands
  * to the IOVM. */
@@ -527,7 +520,7 @@ struct uiscmdrsp_vdiskmgmt {
 
 	    /* result of taskmgmt command - set by IOPart - values are: */
 #define VDISK_MGMT_FAILED  0
-};
+} __packed;
 
 /* keeping cmd & rsp info in one structure for now cmd rsp packet for scsi */
 struct uiscmdrsp {
@@ -551,7 +544,7 @@ struct uiscmdrsp {
 	struct uiscmdrsp *next;	/* General Purpose Queue Link */
 	struct uiscmdrsp *activeQ_next;	/* Used to track active commands */
 	struct uiscmdrsp *activeQ_prev;	/* Used to track active commands */
-};
+} __packed;
 
 /* This is just the header of the IO channel.  It is assumed that directly after
  * this header there is a large region of memory which contains the command and
@@ -577,10 +570,8 @@ struct spar_io_channel_protocol {
 #define MAX_CLIENTSTRING_LEN 1024
 	 u8 client_string[MAX_CLIENTSTRING_LEN];/* NULL terminated - so holds
 						 * max - 1 bytes */
-};
+} __packed;
 
-#pragma pack(pop)
-/* ///////////// END PRAGMA PACK PUSH 1 /////////////////////////// */
 
 /*
  * INLINE functions for initializing and accessing I/O data channels
-- 
1.9.1

Re: [PATCH v2 2/3] staging: unisys: convert pack pragma to __packed

[Greg KH]

On Fri, Jun 12, 2015 at 04:46:07PM -0400, David Kershner wrote:
> It was noticed that iochannel.h was still using pragmas to
> pack the datastructures, should be using __packed instead.
> 
> Signed-off-by: David Kershner <david.kershner@unisys.com>
> ---
>  drivers/staging/unisys/include/iochannel.h | 45 ++++++++++++------------------
>  1 file changed, 18 insertions(+), 27 deletions(-)

Other .h files also are like this in the driver, you should fix them up
as well in future patches.

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 2/3] staging: unisys: convert pack pragma to __packed

[Dan Carpenter]

On Fri, Jun 12, 2015 at 04:46:07PM -0400, David Kershner wrote:
> It was noticed that iochannel.h was still using pragmas to
> pack the datastructures, should be using __packed instead.
> 
> Signed-off-by: David Kershner <david.kershner@unisys.com>

Could you take some time and figure out which structs should actually
be packed and which should not?  For example, in net_pkt_rcv the
pointers are not aligned so it will cause a slow down on x86 and crash
on other arches (which we don't care about).

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH v2 2/3] staging: unisys: convert pack pragma to __packed

[Kershner, David A]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Friday, June 19, 2015 8:55 AM
> To: Kershner, David A
> Cc: gregkh@linuxfoundation.org; Jes.Sorensen@redhat.com; *S-Par-
> Maintainer; driverdev-devel@linuxdriverproject.org; Romer, Benjamin M
> Subject: Re: [PATCH v2 2/3] staging: unisys: convert pack pragma to
> __packed
> 
> On Fri, Jun 12, 2015 at 04:46:07PM -0400, David Kershner wrote:
> > It was noticed that iochannel.h was still using pragmas to
> > pack the datastructures, should be using __packed instead.
> >
> > Signed-off-by: David Kershner <david.kershner@unisys.com>
> 
> Could you take some time and figure out which structs should actually
> be packed and which should not?  For example, in net_pkt_rcv the
> pointers are not aligned so it will cause a slow down on x86 and crash
> on other arches (which we don't care about).
> 
> regards,
> dan carpenter

Dan, 

Thanks for the find. Unfortunately, all the structs defined in iochannel.h
need to be packed since they are shared across different OS and code
instances. I'll look into changing the s-Par firmware and will update the
iochannels structure as well.  

Your reference to x86, does that include x86_64? 

Thanks, 
David

Re: [PATCH v2 2/3] staging: unisys: convert pack pragma to __packed

[Jes Sorensen]

"Kershner, David A" <David.Kershner@unisys.com> writes:
>> -----Original Message-----
>> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
>> Sent: Friday, June 19, 2015 8:55 AM
>> To: Kershner, David A
>> Cc: gregkh@linuxfoundation.org; Jes.Sorensen@redhat.com; *S-Par-
>> Maintainer; driverdev-devel@linuxdriverproject.org; Romer, Benjamin M
>> Subject: Re: [PATCH v2 2/3] staging: unisys: convert pack pragma to
>> __packed
>> 
>> On Fri, Jun 12, 2015 at 04:46:07PM -0400, David Kershner wrote:
>> > It was noticed that iochannel.h was still using pragmas to
>> > pack the datastructures, should be using __packed instead.
>> >
>> > Signed-off-by: David Kershner <david.kershner@unisys.com>
>> 
>> Could you take some time and figure out which structs should actually
>> be packed and which should not?  For example, in net_pkt_rcv the
>> pointers are not aligned so it will cause a slow down on x86 and crash
>> on other arches (which we don't care about).
>> 
>> regards,
>> dan carpenter
>
> Dan, 
>
> Thanks for the find. Unfortunately, all the structs defined in iochannel.h
> need to be packed since they are shared across different OS and code
> instances. I'll look into changing the s-Par firmware and will update the
> iochannels structure as well.  
>
> Your reference to x86, does that include x86_64? 

Not sure how the firmware interface is, but I hope you can version tag
it so the OS can know when to do what?

Cheers,
Jes

Re: [PATCH v2 2/3] staging: unisys: convert pack pragma to __packed

[Dan Carpenter]

On Fri, Jun 19, 2015 at 03:02:32PM +0000, Kershner, David A wrote:
> Thanks for the find. Unfortunately, all the structs defined in iochannel.h
> need to be packed since they are shared across different OS and code
> instances. I'll look into changing the s-Par firmware and will update the
> iochannels structure as well.  
> 
> Your reference to x86, does that include x86_64? 

Yes.  I just it up on google so I don't know if it makes a difference
here.

http://stackoverflow.com/questions/3057452/misaligned-pointer-performance

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 3/3] staging: unisys: Don't hold device responses until driver loads

[David Kershner]

Currently if a driver is not loaded for a device, we will not
respond to the device create until it is done. This causes
s-Par to not mark the partition running if the driver for the
device is not loaded. Since there are several devices that
could be assigned to a guest that don't have an actual
driver this will cause us to never go running.

If the device driver is loaded, we WILL continue to only
respond to the device PAUSE message when the device driver
has responded that it is done with the device.

Signed-off-by: David Kershner <david.kershner@unisys.com>
---
 drivers/staging/unisys/visorbus/visorbus_main.c | 28 +++++++------------------
 1 file changed, 7 insertions(+), 21 deletions(-)

diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index dcce1f0..b22af16 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -70,7 +70,6 @@ static const struct attribute_group *visorbus_bus_groups[] = {
 	NULL,
 };
 
-
 /** This describes the TYPE of bus.
  *  (Don't confuse this with an INSTANCE of the bus.)
  */
@@ -745,19 +744,6 @@ visordriver_probe_device(struct device *xdev)
 away:
 	if (rc != 0)
 		put_device(&dev->device);
-	/*  We could get here more than once if the child driver module is
-	 *  unloaded and re-loaded while devices are present.  That's why we
-	 *  need a flag to be sure that we only respond to the device_create
-	 *  once.  We cannot respond to the device_create prior to here,
-	 *  because until we call drv->probe() above, the channel has not been
-	 *  initialized.
-	 */
-	if (!dev->responded_to_device_create) {
-
-		dev->responded_to_device_create = true;
-		if (chipset_responders.device_create)
-			(*chipset_responders.device_create)(dev, rc);
-	}
 	return rc;
 }
 
@@ -1306,15 +1292,15 @@ chipset_device_create(struct visor_device *dev_info)
 			 POSTCODE_SEVERITY_INFO);
 
 	rc = create_visor_device(dev_info);
-	if (rc < 0) {
+	if (chipset_responders.device_create)
+		chipset_responders.device_create(dev_info, rc);
+
+	if (rc < 0)
 		POSTCODE_LINUX_4(DEVICE_CREATE_FAILURE_PC, dev_no, bus_no,
 				 POSTCODE_SEVERITY_ERR);
-		if (chipset_responders.device_create)
-			(*chipset_responders.device_create)(dev_info, rc);
-	}
-
-	POSTCODE_LINUX_4(DEVICE_CREATE_SUCCESS_PC, dev_no, bus_no,
-			 POSTCODE_SEVERITY_INFO);
+	else
+		POSTCODE_LINUX_4(DEVICE_CREATE_SUCCESS_PC, dev_no, bus_no,
+				 POSTCODE_SEVERITY_INFO);
 }
 
 static void
-- 
1.9.1