[PATCH net-next 0/4] s390/net updates, part 2

[PATCH net-next 0/4] s390/net updates, part 2

[Julian Wiedmann]

Hi Dave,

some additional qeth patches for net-next, please apply.

Note: there's one out-of-topic modification to arch/s390 that
adds a new hypervisor call, for use in qeth. Heiko acked to upstream
this via your tree.

Thanks,
Julian


Julian Wiedmann (3):
  s390/qeth: fix packing buffer statistics
  s390/diag: add diag26c support
  s390/qeth: use diag26c to get MAC address on L2

Kittipon Meesompop (1):
  s390/qeth: add ipa return codes for bridgeport

 arch/s390/include/asm/diag.h      | 26 +++++++++++++
 arch/s390/kernel/diag.c           | 29 +++++++++++++++
 drivers/s390/net/qeth_core.h      |  1 +
 drivers/s390/net/qeth_core_main.c | 78 +++++++++++++++++++++++++++++++++++----
 drivers/s390/net/qeth_core_mpc.c  | 14 +++++++
 drivers/s390/net/qeth_core_mpc.h  | 18 +++++++++
 drivers/s390/net/qeth_l2_main.c   | 50 +++++++++++++++----------
 7 files changed, 190 insertions(+), 26 deletions(-)

-- 
2.11.2

[PATCH net-next 1/4] s390/qeth: add ipa return codes for bridgeport

[Julian Wiedmann]

From: Kittipon Meesompop <kmeesomp@linux.vnet.ibm.com>

add ipa return codes for Bridgeport (HiperSockets and OSA) according to
system level design.

Signed-off-by: Kittipon Meesompop <kmeesomp@linux.vnet.ibm.com>
Reviewed-by: Julian Wiedmann <jwi@linux.vnet.ibm.com>
Reviewed-by: Ursula Braun <ubraun@linux.vnet.ibm.com>
Signed-off-by: Julian Wiedmann <jwi@linux.vnet.ibm.com>
---
 drivers/s390/net/qeth_core_mpc.c | 14 ++++++++++++++
 drivers/s390/net/qeth_core_mpc.h | 18 ++++++++++++++++++
 drivers/s390/net/qeth_l2_main.c  | 34 +++++++++++++++++-----------------
 3 files changed, 49 insertions(+), 17 deletions(-)

diff --git a/drivers/s390/net/qeth_core_mpc.c b/drivers/s390/net/qeth_core_mpc.c
index ab9b1376467f..6dd7d05e5693 100644
--- a/drivers/s390/net/qeth_core_mpc.c
+++ b/drivers/s390/net/qeth_core_mpc.c
@@ -170,12 +170,18 @@ static struct ipa_rc_msg qeth_ipa_rc_msg[] = {
 	{IPA_RC_TRACE_ALREADY_ACTIVE,	"trace already active"},
 	{IPA_RC_INVALID_FORMAT,		"invalid format or length"},
 	{IPA_RC_DUP_IPV6_REMOTE, "ipv6 address already registered remote"},
+	{IPA_RC_SBP_IQD_NOT_CONFIGURED,	"Not configured for bridgeport"},
 	{IPA_RC_DUP_IPV6_HOME,		"ipv6 address already registered"},
 	{IPA_RC_UNREGISTERED_ADDR,	"Address not registered"},
 	{IPA_RC_NO_ID_AVAILABLE,	"No identifiers available"},
 	{IPA_RC_ID_NOT_FOUND,		"Identifier not found"},
+	{IPA_RC_SBP_IQD_ANO_DEV_PRIMARY, "Primary bridgeport exists already"},
+	{IPA_RC_SBP_IQD_CURRENT_SECOND,	"Bridgeport is currently secondary"},
+	{IPA_RC_SBP_IQD_LIMIT_SECOND, "Limit of secondary bridgeports reached"},
 	{IPA_RC_INVALID_IP_VERSION,	"IP version incorrect"},
+	{IPA_RC_SBP_IQD_CURRENT_PRIMARY, "Bridgeport is currently primary"},
 	{IPA_RC_LAN_FRAME_MISMATCH,	"LAN and frame mismatch"},
+	{IPA_RC_SBP_IQD_NO_QDIO_QUEUES,	"QDIO queues not established"},
 	{IPA_RC_L2_UNSUPPORTED_CMD,	"Unsupported layer 2 command"},
 	{IPA_RC_L2_DUP_MAC,		"Duplicate MAC address"},
 	{IPA_RC_L2_ADDR_TABLE_FULL,	"Layer2 address table full"},
@@ -187,6 +193,14 @@ static struct ipa_rc_msg qeth_ipa_rc_msg[] = {
 	{IPA_RC_L2_INVALID_VLAN_ID,	"L2 invalid vlan id"},
 	{IPA_RC_L2_DUP_VLAN_ID,		"L2 duplicate vlan id"},
 	{IPA_RC_L2_VLAN_ID_NOT_FOUND,	"L2 vlan id not found"},
+	{IPA_RC_SBP_OSA_NOT_CONFIGURED,	"Not configured for bridgeport"},
+	{IPA_RC_SBP_OSA_OS_MISMATCH,	"OS mismatch"},
+	{IPA_RC_SBP_OSA_ANO_DEV_PRIMARY, "Primary bridgeport exists already"},
+	{IPA_RC_SBP_OSA_CURRENT_SECOND,	"Bridgeport is currently secondary"},
+	{IPA_RC_SBP_OSA_LIMIT_SECOND, "Limit of secondary bridgeports reached"},
+	{IPA_RC_SBP_OSA_NOT_AUTHD_BY_ZMAN, "Not authorized by zManager"},
+	{IPA_RC_SBP_OSA_CURRENT_PRIMARY, "Bridgeport is currently primary"},
+	{IPA_RC_SBP_OSA_NO_QDIO_QUEUES,	"QDIO queues not established"},
 	{IPA_RC_DATA_MISMATCH,		"Data field mismatch (v4/v6 mixed)"},
 	{IPA_RC_INVALID_MTU_SIZE,	"Invalid MTU size"},
 	{IPA_RC_INVALID_LANTYPE,	"Invalid LAN type"},
diff --git a/drivers/s390/net/qeth_core_mpc.h b/drivers/s390/net/qeth_core_mpc.h
index 45bbea2843bf..912e0107de8f 100644
--- a/drivers/s390/net/qeth_core_mpc.h
+++ b/drivers/s390/net/qeth_core_mpc.h
@@ -142,12 +142,18 @@ enum qeth_ipa_return_codes {
 	IPA_RC_TRACE_ALREADY_ACTIVE	= 0x0005,
 	IPA_RC_INVALID_FORMAT		= 0x0006,
 	IPA_RC_DUP_IPV6_REMOTE		= 0x0008,
+	IPA_RC_SBP_IQD_NOT_CONFIGURED	= 0x000C,
 	IPA_RC_DUP_IPV6_HOME		= 0x0010,
 	IPA_RC_UNREGISTERED_ADDR	= 0x0011,
 	IPA_RC_NO_ID_AVAILABLE		= 0x0012,
 	IPA_RC_ID_NOT_FOUND		= 0x0013,
+	IPA_RC_SBP_IQD_ANO_DEV_PRIMARY	= 0x0014,
+	IPA_RC_SBP_IQD_CURRENT_SECOND	= 0x0018,
+	IPA_RC_SBP_IQD_LIMIT_SECOND	= 0x001C,
 	IPA_RC_INVALID_IP_VERSION	= 0x0020,
+	IPA_RC_SBP_IQD_CURRENT_PRIMARY	= 0x0024,
 	IPA_RC_LAN_FRAME_MISMATCH	= 0x0040,
+	IPA_RC_SBP_IQD_NO_QDIO_QUEUES	= 0x00EB,
 	IPA_RC_L2_UNSUPPORTED_CMD	= 0x2003,
 	IPA_RC_L2_DUP_MAC		= 0x2005,
 	IPA_RC_L2_ADDR_TABLE_FULL	= 0x2006,
@@ -159,6 +165,14 @@ enum qeth_ipa_return_codes {
 	IPA_RC_L2_INVALID_VLAN_ID	= 0x2015,
 	IPA_RC_L2_DUP_VLAN_ID		= 0x2016,
 	IPA_RC_L2_VLAN_ID_NOT_FOUND	= 0x2017,
+	IPA_RC_SBP_OSA_NOT_CONFIGURED	= 0x2B0C,
+	IPA_RC_SBP_OSA_OS_MISMATCH	= 0x2B10,
+	IPA_RC_SBP_OSA_ANO_DEV_PRIMARY	= 0x2B14,
+	IPA_RC_SBP_OSA_CURRENT_SECOND	= 0x2B18,
+	IPA_RC_SBP_OSA_LIMIT_SECOND	= 0x2B1C,
+	IPA_RC_SBP_OSA_NOT_AUTHD_BY_ZMAN = 0x2B20,
+	IPA_RC_SBP_OSA_CURRENT_PRIMARY	= 0x2B24,
+	IPA_RC_SBP_OSA_NO_QDIO_QUEUES	= 0x2BEB,
 	IPA_RC_DATA_MISMATCH		= 0xe001,
 	IPA_RC_INVALID_MTU_SIZE		= 0xe002,
 	IPA_RC_INVALID_LANTYPE		= 0xe003,
@@ -187,6 +201,10 @@ enum qeth_ipa_return_codes {
 #define IPA_RC_INVALID_SUBCMD		IPA_RC_IP_TABLE_FULL
 #define IPA_RC_HARDWARE_AUTH_ERROR	IPA_RC_UNKNOWN_ERROR
 
+/* for SETBRIDGEPORT (double occupancies) */
+#define IPA_RC_SBP_IQD_OS_MISMATCH	 IPA_RC_DUP_IPV6_HOME
+#define IPA_RC_SBP_IQD_NOT_AUTHD_BY_ZMAN IPA_RC_INVALID_IP_VERSION
+
 /* IPA function flags; each flag marks availability of respective function */
 enum qeth_ipa_funcs {
 	IPA_ARP_PROCESSING      = 0x00000001L,
diff --git a/drivers/s390/net/qeth_l2_main.c b/drivers/s390/net/qeth_l2_main.c
index c6bc63b8b295..9e7f12aba55e 100644
--- a/drivers/s390/net/qeth_l2_main.c
+++ b/drivers/s390/net/qeth_l2_main.c
@@ -1650,27 +1650,27 @@ static int qeth_bridgeport_makerc(struct qeth_card *card,
 	if ((is_iqd && (cbctl->ipa_rc == IPA_RC_SUCCESS)) ||
 	    (!is_iqd && (cbctl->ipa_rc == cbctl->cmd_rc)))
 		switch (cbctl->cmd_rc) {
-		case 0x0000:
+		case IPA_RC_SUCCESS:
 			rc = 0;
 			break;
-		case 0x2B04:
-		case 0x0004:
+		case IPA_RC_L2_UNSUPPORTED_CMD:
+		case IPA_RC_UNSUPPORTED_COMMAND:
 			rc = -EOPNOTSUPP;
 			break;
-		case 0x2B0C:
-		case 0x000C: /* Not configured as bridge Port */
+		case IPA_RC_SBP_OSA_NOT_CONFIGURED:
+		case IPA_RC_SBP_IQD_NOT_CONFIGURED:
 			rc = -ENODEV; /* maybe not the best code here? */
 			dev_err(&card->gdev->dev,
 	"The device is not configured as a Bridge Port\n");
 			break;
-		case 0x2B10:
-		case 0x0010: /* OS mismatch */
+		case IPA_RC_SBP_OSA_OS_MISMATCH:
+		case IPA_RC_SBP_IQD_OS_MISMATCH:
 			rc = -EPERM;
 			dev_err(&card->gdev->dev,
 	"A Bridge Port is already configured by a different operating system\n");
 			break;
-		case 0x2B14:
-		case 0x0014: /* Another device is Primary */
+		case IPA_RC_SBP_OSA_ANO_DEV_PRIMARY:
+		case IPA_RC_SBP_IQD_ANO_DEV_PRIMARY:
 			switch (setcmd) {
 			case IPA_SBP_SET_PRIMARY_BRIDGE_PORT:
 				rc = -EEXIST;
@@ -1686,26 +1686,26 @@ static int qeth_bridgeport_makerc(struct qeth_card *card,
 				rc = -EIO;
 			}
 			break;
-		case 0x2B18:
-		case 0x0018: /* This device is currently Secondary */
+		case IPA_RC_SBP_OSA_CURRENT_SECOND:
+		case IPA_RC_SBP_IQD_CURRENT_SECOND:
 			rc = -EBUSY;
 			dev_err(&card->gdev->dev,
 	"The device is already a secondary Bridge Port\n");
 			break;
-		case 0x2B1C:
-		case 0x001C: /* Limit for Secondary devices reached */
+		case IPA_RC_SBP_OSA_LIMIT_SECOND:
+		case IPA_RC_SBP_IQD_LIMIT_SECOND:
 			rc = -EEXIST;
 			dev_err(&card->gdev->dev,
 	"The LAN cannot have more secondary Bridge Ports\n");
 			break;
-		case 0x2B24:
-		case 0x0024: /* This device is currently Primary */
+		case IPA_RC_SBP_OSA_CURRENT_PRIMARY:
+		case IPA_RC_SBP_IQD_CURRENT_PRIMARY:
 			rc = -EBUSY;
 			dev_err(&card->gdev->dev,
 	"The device is already a primary Bridge Port\n");
 			break;
-		case 0x2B20:
-		case 0x0020: /* Not authorized by zManager */
+		case IPA_RC_SBP_OSA_NOT_AUTHD_BY_ZMAN:
+		case IPA_RC_SBP_IQD_NOT_AUTHD_BY_ZMAN:
 			rc = -EACCES;
 			dev_err(&card->gdev->dev,
 	"The device is not authorized to be a Bridge Port\n");
-- 
2.11.2

[PATCH net-next 2/4] s390/qeth: fix packing buffer statistics

[Julian Wiedmann]

There's two spots in qeth_send_packet() where we don't accurately
account for transmitted packing buffers in qeth's performance
statistics:

1) when flushing the current buffer due to insufficient size,
   and the next buffer is not EMPTY, we need to account for that
   flushed buffer.
2) when synchronizing with the TX completion code, we reset
   flush_count and thus forget to account for any previously
   flushed buffers.

Reported-by: Nils Hoppmann <niho@de.ibm.com>
Signed-off-by: Julian Wiedmann <jwi@linux.vnet.ibm.com>
---
 drivers/s390/net/qeth_core_main.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c
index 08338f27c82c..13a55f1dee7a 100644
--- a/drivers/s390/net/qeth_core_main.c
+++ b/drivers/s390/net/qeth_core_main.c
@@ -4103,7 +4103,8 @@ int qeth_do_send_packet(struct qeth_card *card, struct qeth_qdio_out_q *queue,
 							   flush_count);
 				atomic_set(&queue->state,
 						QETH_OUT_Q_UNLOCKED);
-				return -EBUSY;
+				rc = -EBUSY;
+				goto out;
 			}
 		}
 	}
@@ -4122,19 +4123,21 @@ int qeth_do_send_packet(struct qeth_card *card, struct qeth_qdio_out_q *queue,
 	 * In that case we will enter this loop
 	 */
 	while (atomic_dec_return(&queue->state)) {
-		flush_count = 0;
 		start_index = queue->next_buf_to_fill;
 		/* check if we can go back to non-packing state */
-		flush_count += qeth_switch_to_nonpacking_if_needed(queue);
+		tmp = qeth_switch_to_nonpacking_if_needed(queue);
 		/*
 		 * check if we need to flush a packing buffer to get a pci
 		 * flag out on the queue
 		 */
-		if (!flush_count && !atomic_read(&queue->set_pci_flags_count))
-			flush_count += qeth_prep_flush_pack_buffer(queue);
-		if (flush_count)
-			qeth_flush_buffers(queue, start_index, flush_count);
+		if (!tmp && !atomic_read(&queue->set_pci_flags_count))
+			tmp = qeth_prep_flush_pack_buffer(queue);
+		if (tmp) {
+			qeth_flush_buffers(queue, start_index, tmp);
+			flush_count += tmp;
+		}
 	}
+out:
 	/* at this point the queue is UNLOCKED again */
 	if (queue->card->options.performance_stats && do_pack)
 		queue->card->perf_stats.bufs_sent_pack += flush_count;
-- 
2.11.2

[PATCH net-next 3/4] s390/diag: add diag26c support

[Julian Wiedmann]

Implement support for the hypervisor diagnose 0x26c
('Access Certain System Information').
It passes a request buffer and a subfunction code, and receives
a response buffer and a return code.

Also add the scaffolding for the 'MAC Services' subfunction.
It may be used by network devices to obtain a hypervisor-managed
MAC address.

Signed-off-by: Julian Wiedmann <jwi@linux.vnet.ibm.com>
Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 arch/s390/include/asm/diag.h | 26 ++++++++++++++++++++++++++
 arch/s390/kernel/diag.c      | 29 +++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+)

diff --git a/arch/s390/include/asm/diag.h b/arch/s390/include/asm/diag.h
index 8acf482162ed..96abd80528fc 100644
--- a/arch/s390/include/asm/diag.h
+++ b/arch/s390/include/asm/diag.h
@@ -8,6 +8,7 @@
 #ifndef _ASM_S390_DIAG_H
 #define _ASM_S390_DIAG_H
 
+#include <linux/if_ether.h>
 #include <linux/percpu.h>
 
 enum diag_stat_enum {
@@ -24,6 +25,7 @@ enum diag_stat_enum {
 	DIAG_STAT_X224,
 	DIAG_STAT_X250,
 	DIAG_STAT_X258,
+	DIAG_STAT_X26C,
 	DIAG_STAT_X288,
 	DIAG_STAT_X2C4,
 	DIAG_STAT_X2FC,
@@ -225,6 +227,30 @@ struct diag204_x_phys_block {
 	struct diag204_x_phys_cpu cpus[];
 } __packed;
 
+enum diag26c_sc {
+	DIAG26C_MAC_SERVICES = 0x00000030
+};
+
+enum diag26c_version {
+	DIAG26C_VERSION2 = 0x00000002	/* z/VM 5.4.0 */
+};
+
+#define DIAG26C_GET_MAC	0x0000
+struct diag26c_mac_req {
+	u32	resp_buf_len;
+	u32	resp_version;
+	u16	op_code;
+	u16	devno;
+	u8	res[4];
+} __packed;
+
+struct diag26c_mac_resp {
+	u32	version;
+	u8	mac[ETH_ALEN];
+	u16	res;
+} __packed __aligned(8);
+
 int diag204(unsigned long subcode, unsigned long size, void *addr);
 int diag224(void *ptr);
+int diag26c(void *req, void *resp, enum diag26c_sc subcode);
 #endif /* _ASM_S390_DIAG_H */
diff --git a/arch/s390/kernel/diag.c b/arch/s390/kernel/diag.c
index ac6abcd3fe6a..349914571772 100644
--- a/arch/s390/kernel/diag.c
+++ b/arch/s390/kernel/diag.c
@@ -38,6 +38,7 @@ static const struct diag_desc diag_map[NR_DIAG_STAT] = {
 	[DIAG_STAT_X224] = { .code = 0x224, .name = "EBCDIC-Name Table" },
 	[DIAG_STAT_X250] = { .code = 0x250, .name = "Block I/O" },
 	[DIAG_STAT_X258] = { .code = 0x258, .name = "Page-Reference Services" },
+	[DIAG_STAT_X26C] = { .code = 0x26c, .name = "Certain System Information" },
 	[DIAG_STAT_X288] = { .code = 0x288, .name = "Time Bomb" },
 	[DIAG_STAT_X2C4] = { .code = 0x2c4, .name = "FTP Services" },
 	[DIAG_STAT_X2FC] = { .code = 0x2fc, .name = "Guest Performance Data" },
@@ -236,3 +237,31 @@ int diag224(void *ptr)
 	return rc;
 }
 EXPORT_SYMBOL(diag224);
+
+/*
+ * Diagnose 26C: Access Certain System Information
+ */
+static inline int __diag26c(void *req, void *resp, enum diag26c_sc subcode)
+{
+	register unsigned long _req asm("2") = (addr_t) req;
+	register unsigned long _resp asm("3") = (addr_t) resp;
+	register unsigned long _subcode asm("4") = subcode;
+	register unsigned long _rc asm("5") = -EOPNOTSUPP;
+
+	asm volatile(
+		"	sam31\n"
+		"	diag	%[rx],%[ry],0x26c\n"
+		"0:	sam64\n"
+		EX_TABLE(0b,0b)
+		: "+d" (_rc)
+		: [rx] "d" (_req), "d" (_resp), [ry] "d" (_subcode)
+		: "cc", "memory");
+	return _rc;
+}
+
+int diag26c(void *req, void *resp, enum diag26c_sc subcode)
+{
+	diag_stat_inc(DIAG_STAT_X26C);
+	return __diag26c(req, resp, subcode);
+}
+EXPORT_SYMBOL(diag26c);
-- 
2.11.2

Re: [PATCH net-next 3/4] s390/diag: add diag26c support

[David Miller]

From: Julian Wiedmann <jwi@linux.vnet.ibm.com>
Date: Mon, 19 Jun 2017 13:22:24 +0200

> +#define DIAG26C_GET_MAC	0x0000
> +struct diag26c_mac_req {
> +	u32	resp_buf_len;
> +	u32	resp_version;
> +	u16	op_code;
> +	u16	devno;
> +	u8	res[4];
> +} __packed;

The packed attribute is not necessary here, the structure will be
perfectly packed together because of the types used and the order of
the members.

__packed is to be used only in the last possible resort for
correctness and every effort whatsoever should be used to avoid using
it.

> +
> +struct diag26c_mac_resp {
> +	u32	version;
> +	u8	mac[ETH_ALEN];
> +	u16	res;
> +} __packed __aligned(8);

Using packed with an 8 byte alignment is even more unnecessary.

Again, it is not needed, so please don't use it.

> + */
> +static inline int __diag26c(void *req, void *resp, enum diag26c_sc subcode)

Do not mark functions inline in *.c files, let the compiler decide.

Re: [PATCH net-next 3/4] s390/diag: add diag26c support

[Martin Schwidefsky]

Hi Dave,

On Mon, 19 Jun 2017 10:47:26 -0400 (EDT)
David Miller <davem@davemloft.net> wrote:

> From: Julian Wiedmann <jwi@linux.vnet.ibm.com>
> Date: Mon, 19 Jun 2017 13:22:24 +0200
> 
> > +#define DIAG26C_GET_MAC	0x0000
> > +struct diag26c_mac_req {
> > +	u32	resp_buf_len;
> > +	u32	resp_version;
> > +	u16	op_code;
> > +	u16	devno;
> > +	u8	res[4];
> > +} __packed;  
> 
> The packed attribute is not necessary here, the structure will be
> perfectly packed together because of the types used and the order of
> the members.

We (as in the s390 guys) tend to add __packed to hardware and hypervisor
structures even if the attribute is not strictly necessary. Most of the
diagnose related structures look that way. Dunno if it is worth to change
them.

I agree that __packed should be avoided for software defined structures.

> __packed is to be used only in the last possible resort for
> correctness and every effort whatsoever should be used to avoid using
> it.
> 
> > +
> > +struct diag26c_mac_resp {
> > +	u32	version;
> > +	u8	mac[ETH_ALEN];
> > +	u16	res;
> > +} __packed __aligned(8);  
> 
> Using packed with an 8 byte alignment is even more unnecessary.
> 
> Again, it is not needed, so please don't use it.

The diag26c struct needs to be aligned on a doubleword boundary, the
__aligned(8) is necessary. The __packed attribute is again superfluous but
follows along the lines of the other diag structures.

I do not mind the extra __packed attributes, but if you care about them
we could remove them from the structures in diag.h.

> > + */
> > +static inline int __diag26c(void *req, void *resp, enum diag26c_sc subcode)  
> 
> Do not mark functions inline in *.c files, let the compiler decide.
> 

Here I disagree. Basically all of our functions with assembly code are
static inline, it is a common pattern even in C files. Sometimes the compiler
*is* stupid and won't inline a function. And on s390 function calls do not
come for free.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [PATCH net-next 3/4] s390/diag: add diag26c support

[David Miller]

From: Martin Schwidefsky <schwidefsky@de.ibm.com>
Date: Mon, 19 Jun 2017 17:34:25 +0200

> We (as in the s390 guys) tend to add __packed to hardware and hypervisor
> structures even if the attribute is not strictly necessary. Most of the
> diagnose related structures look that way. Dunno if it is worth to change
> them.

It causes gcc to generate bad code on certain platforms (yes, probably not
yours) and is in general something to avoid.

Please do not use __packed unless absolutely necessary.

> The diag26c struct needs to be aligned on a doubleword boundary, the
> __aligned(8) is necessary.

That's fine.

> The __packed attribute is again superfluous but follows along the
> lines of the other diag structures.

Please remove it.

Re: [PATCH net-next 3/4] s390/diag: add diag26c support

[Martin Schwidefsky]

Hi Dave,

On Mon, 19 Jun 2017 13:37:43 -0400 (EDT)
David Miller <davem@davemloft.net> wrote:

> From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Date: Mon, 19 Jun 2017 17:34:25 +0200
> 
> > We (as in the s390 guys) tend to add __packed to hardware and hypervisor
> > structures even if the attribute is not strictly necessary. Most of the
> > diagnose related structures look that way. Dunno if it is worth to change
> > them.  
> 
> It causes gcc to generate bad code on certain platforms (yes, probably not
> yours) and is in general something to avoid.
> 
> Please do not use __packed unless absolutely necessary.
> 
> > The diag26c struct needs to be aligned on a doubleword boundary, the
> > __aligned(8) is necessary.  
> 
> That's fine.
> 
> > The __packed attribute is again superfluous but follows along the
> > lines of the other diag structures.  
> 
> Please remove it.

I looked at the various structures with the packed attribute in arch/s390
and found that we could remove a lot of them. As __packed also changes the
alignment of the structure removing the attribute from structures defined
in arch/s390/include/uapi/asm may cause trouble in user space. I stayed
away from the uapi headers.

For the rest of the code about 120 packed attributes could be removed, see:

https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/log/?h=packed-cleanup

bloat-o-meter found only a few functions where the removal of packed
makes a difference (gcc 7.1.0):

add/remove: 1/1 grow/shrink: 8/7 up/down: 240/-308 (-68)
function                                     old     new   delta
pin_scb.isra                                   -     200    +200
stp_sync_clock                               982    1000     +18
stp_work_fn                                  416     420      +4
sca_ext_call_pending                         290     294      +4
kvm_arch_vcpu_create                        2008    2012      +4
get_vcpu_asce                               1206    1210      +4
hw_perf_event_update                        1738    1740      +2
clp_add_pci_device                          1400    1402      +2
__clp_rescan                                 162     164      +2
stp_timing_state_show                        138     134      -4
stp_timing_mode_show                         134     130      -4
stp_ctn_type_show                            134     130      -4
stp_time_offset_show                         150     144      -6
ccw_device_accumulate_irb                   1838    1832      -6
clp_misc_ioctl                              1142    1102     -40
timing_alert_interrupt                       158     114     -44
pin_scb                                      200       -    -200
Total: Before=155168810, After=155168742, chg -0.00%

The code gets minimally better. I am not convinced yet that this is worth
the hassle. These structures are architecture specific and the s390 CPUs
are perfectly fine with unaligned accesses.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

[PATCH net-next 4/4] s390/qeth: use diag26c to get MAC address on L2

[Julian Wiedmann]

When a s390 guest runs on a z/VM host that's part of a SSI cluster,
it can be migrated to a different host. In this case, the MAC address
it originally obtained on the old host may be re-assigned to a new
guest. This would result in address conflicts between the two guests.

When running as z/VM guest, use the diag26c MAC Service to obtain
a hypervisor-managed MAC address. The MAC Service is SSI-aware, and
won't re-assign the address after the guest is migrated to a new host.

This patch adds support for the z/VM MAC Service on L2 devices.

Signed-off-by: Julian Wiedmann <jwi@linux.vnet.ibm.com>
Acked-by: Ursula Braun <ubraun@linux.vnet.ibm.com>
---
 drivers/s390/net/qeth_core.h      |  1 +
 drivers/s390/net/qeth_core_main.c | 61 +++++++++++++++++++++++++++++++++++++++
 drivers/s390/net/qeth_l2_main.c   | 16 ++++++++--
 3 files changed, 76 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/net/qeth_core.h b/drivers/s390/net/qeth_core.h
index 0efc54a4d82f..7a0ffc71b25d 100644
--- a/drivers/s390/net/qeth_core.h
+++ b/drivers/s390/net/qeth_core.h
@@ -986,6 +986,7 @@ struct qeth_cmd_buffer *qeth_get_setassparms_cmd(struct qeth_card *,
 int qeth_set_features(struct net_device *, netdev_features_t);
 int qeth_recover_features(struct net_device *);
 netdev_features_t qeth_fix_features(struct net_device *, netdev_features_t);
+int qeth_vm_request_mac(struct qeth_card *card);
 
 /* exports for OSN */
 int qeth_osn_assist(struct net_device *, void *, int);
diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c
index 13a55f1dee7a..1d062f086744 100644
--- a/drivers/s390/net/qeth_core_main.c
+++ b/drivers/s390/net/qeth_core_main.c
@@ -27,6 +27,9 @@
 #include <asm/io.h>
 #include <asm/sysinfo.h>
 #include <asm/compat.h>
+#include <asm/diag.h>
+#include <asm/cio.h>
+#include <asm/ccwdev.h>
 
 #include "qeth_core.h"
 
@@ -4773,6 +4776,64 @@ static int qeth_query_card_info(struct qeth_card *card,
 					(void *)carrier_info);
 }
 
+/**
+ * qeth_vm_request_mac() - Request a hypervisor-managed MAC address
+ * @card: pointer to a qeth_card
+ *
+ * Returns
+ *	0, if a MAC address has been set for the card's netdevice
+ *	a return code, for various error conditions
+ */
+int qeth_vm_request_mac(struct qeth_card *card)
+{
+	struct diag26c_mac_req *request;
+	struct diag26c_mac_resp *response;
+	struct ccw_dev_id id;
+	int rc;
+
+	QETH_DBF_TEXT(SETUP, 2, "vmreqmac");
+
+	if (!card->dev)
+		return -ENODEV;
+
+	request = kzalloc(sizeof(*request), GFP_KERNEL | GFP_DMA);
+	response = kzalloc(sizeof(*response), GFP_KERNEL | GFP_DMA);
+	if (!request || !response) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	ccw_device_get_id(CARD_DDEV(card), &id);
+	request->resp_buf_len = sizeof(*response);
+	request->resp_version = DIAG26C_VERSION2;
+	request->op_code = DIAG26C_GET_MAC;
+	request->devno = id.devno;
+
+	rc = diag26c(request, response, DIAG26C_MAC_SERVICES);
+	if (rc)
+		goto out;
+
+	if ((request->resp_buf_len < sizeof(*response)) ||
+	    (response->version != request->resp_version)) {
+		rc = -EIO;
+		QETH_DBF_TEXT(SETUP, 2, "badresp");
+		QETH_DBF_HEX(SETUP, 2, &request->resp_buf_len,
+			     sizeof(request->resp_buf_len));
+	} else if (!is_valid_ether_addr(response->mac)) {
+		rc = -EINVAL;
+		QETH_DBF_TEXT(SETUP, 2, "badmac");
+		QETH_DBF_HEX(SETUP, 2, response->mac, ETH_ALEN);
+	} else {
+		ether_addr_copy(card->dev->dev_addr, response->mac);
+	}
+
+out:
+	kfree(response);
+	kfree(request);
+	return rc;
+}
+EXPORT_SYMBOL_GPL(qeth_vm_request_mac);
+
 static inline int qeth_get_qdio_q_format(struct qeth_card *card)
 {
 	if (card->info.type == QETH_CARD_TYPE_IQD)
diff --git a/drivers/s390/net/qeth_l2_main.c b/drivers/s390/net/qeth_l2_main.c
index 9e7f12aba55e..ad110abfdd47 100644
--- a/drivers/s390/net/qeth_l2_main.c
+++ b/drivers/s390/net/qeth_l2_main.c
@@ -21,6 +21,7 @@
 #include <linux/hash.h>
 #include <linux/hashtable.h>
 #include <linux/string.h>
+#include <asm/setup.h>
 #include "qeth_core.h"
 #include "qeth_l2.h"
 
@@ -505,9 +506,19 @@ static int qeth_l2_request_initial_mac(struct qeth_card *card)
 	int rc = 0;
 	char vendor_pre[] = {0x02, 0x00, 0x00};
 
-	QETH_DBF_TEXT(SETUP, 2, "doL2init");
+	QETH_DBF_TEXT(SETUP, 2, "l2reqmac");
 	QETH_DBF_TEXT_(SETUP, 2, "doL2%s", CARD_BUS_ID(card));
 
+	if (MACHINE_IS_VM) {
+		rc = qeth_vm_request_mac(card);
+		if (!rc)
+			goto out;
+		QETH_DBF_MESSAGE(2, "z/VM MAC Service failed on device %s: x%x\n",
+				 CARD_BUS_ID(card), rc);
+		QETH_DBF_TEXT_(SETUP, 2, "err%04x", rc);
+		/* fall back to alternative mechanism: */
+	}
+
 	if (qeth_is_supported(card, IPA_SETADAPTERPARMS)) {
 		rc = qeth_query_setadapterparms(card);
 		if (rc) {
@@ -528,11 +539,12 @@ static int qeth_l2_request_initial_mac(struct qeth_card *card)
 			QETH_DBF_TEXT_(SETUP, 2, "1err%04x", rc);
 			return rc;
 		}
-		QETH_DBF_HEX(SETUP, 2, card->dev->dev_addr, OSA_ADDR_LEN);
 	} else {
 		eth_random_addr(card->dev->dev_addr);
 		memcpy(card->dev->dev_addr, vendor_pre, 3);
 	}
+out:
+	QETH_DBF_HEX(SETUP, 2, card->dev->dev_addr, card->dev->addr_len);
 	return 0;
 }
 
-- 
2.11.2

Re: [PATCH net-next 4/4] s390/qeth: use diag26c to get MAC address on L2

[David Miller]

From: Julian Wiedmann <jwi@linux.vnet.ibm.com>
Date: Mon, 19 Jun 2017 13:22:25 +0200

> +{
> +	struct diag26c_mac_req *request;
> +	struct diag26c_mac_resp *response;
> +	struct ccw_dev_id id;
> +	int rc;
> +

Please order local variable declarations from longest to shortest
line (reverse christmas tree).

> +	if ((request->resp_buf_len < sizeof(*response)) ||
> +	    (response->version != request->resp_version)) {

Excessive parenthesis here.

Thanks.