[PATCH net-next 0/3] s390: qeth patches for net-next

[PATCH net-next 0/3] s390: qeth patches for net-next

[Ursula Braun]

From: Ursula Braun <ursula.braun@de.ibm.com>

Hi Dave,

here are some s390 related qeth patches for net-next

shortlog:

Eugene Crosser (1):
  qeth: remove extraneous length from %pM format

Vaishali Thakkar (1):
  qeth: Convert use of __constant_htons to htons

Lakhvich Dmitriy (1):
  qeth: no write permission for readonly sysattr

Thanks,
       Ursula

[PATCH net-next 1/3] qeth: remove extraneous length from %pM format

[Ursula Braun]

From: Eugene Crosser <Eugene.Crosser@ru.ibm.com>

Length specifier in the %pM format is not supported (at least, not
documented). Remove it, and also an extraneous '&' for the array.

Signed-off-by: Eugene Crosser <Eugene.Crosser@ru.ibm.com>
Signed-off-by: Ursula Braun <ursula.braun@de.ibmn.com>
Suggested-by: Joe Perches <joe@perches.com>
---
 drivers/s390/net/qeth_l2_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/net/qeth_l2_main.c b/drivers/s390/net/qeth_l2_main.c
index 2e65b98..e0f0188 100644
--- a/drivers/s390/net/qeth_l2_main.c
+++ b/drivers/s390/net/qeth_l2_main.c
@@ -1455,8 +1455,8 @@ static void qeth_bridge_emit_host_event(struct qeth_card *card,
 			env[i] = str[i]; i++;
 		}
 		if (code & IPA_ADDR_CHANGE_CODE_MACADDR) {
-			snprintf(str[i], sizeof(str[i]), "MAC=%pM6",
-				&addr_lnid->mac);
+			snprintf(str[i], sizeof(str[i]), "MAC=%pM",
+				addr_lnid->mac);
 			env[i] = str[i]; i++;
 		}
 		snprintf(str[i], sizeof(str[i]), "NTOK_BUSID=%x.%x.%04x",
-- 
2.3.8

[PATCH net-next 2/3] qeth: Convert use of __constant_htons to htons

[Ursula Braun]

From: Vaishali Thakkar <vthakkar1994@gmail.com>

In little endian cases, the macro htons unfolds to __swab16 which
provides special case for constants. In big endian cases,
__constant_htons and htons expand directly to the same expression.
So, replace __constant_htons with htons with the goal of getting
rid of the definition of __constant_htons completely.

The semantic patch that performs this transformation is as follows:

@@expression x;@@

- __constant_htons(x)
+ htons(x)

Signed-off-by: Vaishali Thakkar <vthakkar1994@gmail.com>
Signed-off-by: Ursula Braun <ursula.braun@de.ibm.com>
---
 drivers/s390/net/qeth_l2_main.c | 2 +-
 drivers/s390/net/qeth_l3_main.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/net/qeth_l2_main.c b/drivers/s390/net/qeth_l2_main.c
index e0f0188..dce1538 100644
--- a/drivers/s390/net/qeth_l2_main.c
+++ b/drivers/s390/net/qeth_l2_main.c
@@ -272,7 +272,7 @@ static void qeth_l2_fill_header(struct qeth_card *card, struct qeth_hdr *hdr,
 	/* VSWITCH relies on the VLAN
 	 * information to be present in
 	 * the QDIO header */
-	if (veth->h_vlan_proto == __constant_htons(ETH_P_8021Q)) {
+	if (veth->h_vlan_proto == htons(ETH_P_8021Q)) {
 		hdr->hdr.l2.flags[2] |= QETH_LAYER2_FLAG_VLAN;
 		hdr->hdr.l2.vlan_id = ntohs(veth->h_vlan_TCI);
 	}
diff --git a/drivers/s390/net/qeth_l3_main.c b/drivers/s390/net/qeth_l3_main.c
index 70eb2f6..ecfe622 100644
--- a/drivers/s390/net/qeth_l3_main.c
+++ b/drivers/s390/net/qeth_l3_main.c
@@ -1887,13 +1887,13 @@ static inline int qeth_l3_rebuild_skb(struct qeth_card *card,
 		case QETH_CAST_MULTICAST:
 			switch (prot) {
 #ifdef CONFIG_QETH_IPV6
-			case __constant_htons(ETH_P_IPV6):
+			case htons(ETH_P_IPV6):
 				ndisc_mc_map((struct in6_addr *)
 				     skb->data + 24,
 				     tg_addr, card->dev, 0);
 				break;
 #endif
-			case __constant_htons(ETH_P_IP):
+			case htons(ETH_P_IP):
 				ip_hdr = (struct iphdr *)skb->data;
 				ip_eth_mc_map(ip_hdr->daddr, tg_addr);
 				break;
-- 
2.3.8

RE: [PATCH net-next 2/3] qeth: Convert use of __constant_htons to htons

[David Laight]

From: Ursula Braun
> Sent: 19 August 2015 09:21
> In little endian cases, the macro htons unfolds to __swab16 which
> provides special case for constants. In big endian cases,
> __constant_htons and htons expand directly to the same expression.
> So, replace __constant_htons with htons with the goal of getting
> rid of the definition of __constant_htons completely.
...
> diff --git a/drivers/s390/net/qeth_l3_main.c b/drivers/s390/net/qeth_l3_main.c
> index 70eb2f6..ecfe622 100644
> --- a/drivers/s390/net/qeth_l3_main.c
> +++ b/drivers/s390/net/qeth_l3_main.c
> @@ -1887,13 +1887,13 @@ static inline int qeth_l3_rebuild_skb(struct qeth_card *card,
>  		case QETH_CAST_MULTICAST:
>  			switch (prot) {
>  #ifdef CONFIG_QETH_IPV6
> -			case __constant_htons(ETH_P_IPV6):
> +			case htons(ETH_P_IPV6):

I didn't think htons() was 'constant enough' to be used as a case label.

Using byteswapped constants in a case statement can change it from being
implemented as a jump table to a branch tree.
This might be more expensive than byteswapping the value (even on systems
that don't have cheap byteswap instructions).

	David

RE: [PATCH net-next 2/3] qeth: Convert use of __constant_htons to htons

[Ursula Braun]

On Thu, 2015-08-20 at 10:46 +0000, David Laight wrote:
> From: Ursula Braun
> > Sent: 19 August 2015 09:21
> > In little endian cases, the macro htons unfolds to __swab16 which
> > provides special case for constants. In big endian cases,
> > __constant_htons and htons expand directly to the same expression.
> > So, replace __constant_htons with htons with the goal of getting
> > rid of the definition of __constant_htons completely.
> ...
> > diff --git a/drivers/s390/net/qeth_l3_main.c b/drivers/s390/net/qeth_l3_main.c
> > index 70eb2f6..ecfe622 100644
> > --- a/drivers/s390/net/qeth_l3_main.c
> > +++ b/drivers/s390/net/qeth_l3_main.c
> > @@ -1887,13 +1887,13 @@ static inline int qeth_l3_rebuild_skb(struct qeth_card *card,
> >  		case QETH_CAST_MULTICAST:
> >  			switch (prot) {
> >  #ifdef CONFIG_QETH_IPV6
> > -			case __constant_htons(ETH_P_IPV6):
> > +			case htons(ETH_P_IPV6):
> 
> I didn't think htons() was 'constant enough' to be used as a case label.
> 
> Using byteswapped constants in a case statement can change it from being
> implemented as a jump table to a branch tree.
> This might be more expensive than byteswapping the value (even on systems
> that don't have cheap byteswap instructions).
> 
> 	David
> 
For big endian systems both __constant_htons(x) and htons(x) are
resolved to ((__force __be16)(__u16)(x)). Thus I do not see a reason to
reject the patch proposal from Vaishali Thakkar.

Ursula

RE: [PATCH net-next 2/3] qeth: Convert use of __constant_htons to htons

[David Laight]

From: Ursula Braun [mailto:ubraun@linux.vnet.ibm.com]
> Sent: 20 August 2015 12:44
> On Thu, 2015-08-20 at 10:46 +0000, David Laight wrote:
> > From: Ursula Braun
> > > Sent: 19 August 2015 09:21
> > > In little endian cases, the macro htons unfolds to __swab16 which
> > > provides special case for constants. In big endian cases,
> > > __constant_htons and htons expand directly to the same expression.
> > > So, replace __constant_htons with htons with the goal of getting
> > > rid of the definition of __constant_htons completely.
> > ...
> > > diff --git a/drivers/s390/net/qeth_l3_main.c b/drivers/s390/net/qeth_l3_main.c
> > > index 70eb2f6..ecfe622 100644
> > > --- a/drivers/s390/net/qeth_l3_main.c
> > > +++ b/drivers/s390/net/qeth_l3_main.c
> > > @@ -1887,13 +1887,13 @@ static inline int qeth_l3_rebuild_skb(struct qeth_card *card,
> > >  		case QETH_CAST_MULTICAST:
> > >  			switch (prot) {
> > >  #ifdef CONFIG_QETH_IPV6
> > > -			case __constant_htons(ETH_P_IPV6):
> > > +			case htons(ETH_P_IPV6):
> >
> > I didn't think htons() was 'constant enough' to be used as a case label.
> >
> > Using byteswapped constants in a case statement can change it from being
> > implemented as a jump table to a branch tree.
> > This might be more expensive than byteswapping the value (even on systems
> > that don't have cheap byteswap instructions).
> >
> > 	David
> >
> For big endian systems both __constant_htons(x) and htons(x) are
> resolved to ((__force __be16)(__u16)(x)). Thus I do not see a reason to
> reject the patch proposal from Vaishali Thakkar.

Look at a little-endian one (eg amd64).
I think you'll find a C ?: expression that uses __builtin_constant() to
select between an expression the compiler can evaluate and a call to an
inline function that uses some appropriate asm.
The latter isn't a compile-time constant so can't be used as a case
label or as an initialiser.

	David

RE: [PATCH net-next 2/3] qeth: Convert use of __constant_htons to htons

[Ursula Braun]

On Thu, 2015-08-20 at 11:51 +0000, David Laight wrote:
> From: Ursula Braun [mailto:ubraun@linux.vnet.ibm.com]
> > Sent: 20 August 2015 12:44
> > On Thu, 2015-08-20 at 10:46 +0000, David Laight wrote:
> > > From: Ursula Braun
> > > > Sent: 19 August 2015 09:21
> > > > In little endian cases, the macro htons unfolds to __swab16 which
> > > > provides special case for constants. In big endian cases,
> > > > __constant_htons and htons expand directly to the same expression.
> > > > So, replace __constant_htons with htons with the goal of getting
> > > > rid of the definition of __constant_htons completely.
> > > ...
> > > > diff --git a/drivers/s390/net/qeth_l3_main.c b/drivers/s390/net/qeth_l3_main.c
> > > > index 70eb2f6..ecfe622 100644
> > > > --- a/drivers/s390/net/qeth_l3_main.c
> > > > +++ b/drivers/s390/net/qeth_l3_main.c
> > > > @@ -1887,13 +1887,13 @@ static inline int qeth_l3_rebuild_skb(struct qeth_card *card,
> > > >  		case QETH_CAST_MULTICAST:
> > > >  			switch (prot) {
> > > >  #ifdef CONFIG_QETH_IPV6
> > > > -			case __constant_htons(ETH_P_IPV6):
> > > > +			case htons(ETH_P_IPV6):
> > >
> > > I didn't think htons() was 'constant enough' to be used as a case label.
> > >
> > > Using byteswapped constants in a case statement can change it from being
> > > implemented as a jump table to a branch tree.
> > > This might be more expensive than byteswapping the value (even on systems
> > > that don't have cheap byteswap instructions).
> > >
> > > 	David
> > >
> > For big endian systems both __constant_htons(x) and htons(x) are
> > resolved to ((__force __be16)(__u16)(x)). Thus I do not see a reason to
> > reject the patch proposal from Vaishali Thakkar.
> 
> Look at a little-endian one (eg amd64).
> I think you'll find a C ?: expression that uses __builtin_constant() to
> select between an expression the compiler can evaluate and a call to an
> inline function that uses some appropriate asm.
> The latter isn't a compile-time constant so can't be used as a case
> label or as an initialiser.
> 
> 	David
> 
> 
qeth is an s390-driver, and s390 is a big-endian architecture. Thus
arguments valid for little-endian do not apply to qeth-code.

Ursula

Re: [PATCH net-next 2/3] qeth: Convert use of __constant_htons to htons

[David Miller]

From: Ursula Braun <ubraun@linux.vnet.ibm.com>
Date: Thu, 20 Aug 2015 15:53:42 +0200

> qeth is an s390-driver, and s390 is a big-endian architecture. Thus
> arguments valid for little-endian do not apply to qeth-code.

You can not throw out generally good tree-wide conventions just because
it happens to work on your platform.

In fact, I would be really thrilled if some of this code could be compiled
on other platforms via COMPILE_TEST or similar, especially the socket code
and drivers.

Re: [PATCH net-next 2/3] qeth: Convert use of __constant_htons to htons

[Vaishali Thakkar]

On Thu, Aug 20, 2015 at 11:28 PM, David Miller <davem@davemloft.net> wrote:
> From: Ursula Braun <ubraun@linux.vnet.ibm.com>
> Date: Thu, 20 Aug 2015 15:53:42 +0200
>
>> qeth is an s390-driver, and s390 is a big-endian architecture. Thus
>> arguments valid for little-endian do not apply to qeth-code.
>
> You can not throw out generally good tree-wide conventions just because
> it happens to work on your platform.
>
> In fact, I would be really thrilled if some of this code could be compiled
> on other platforms via COMPILE_TEST or similar, especially the socket code
> and drivers.

I have test-compiled this patch on little-endian architecture. I used
this script for cross compilation:
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross

And it worked fine for me.

-- 
Vaishali

Re: [PATCH net-next 2/3] qeth: Convert use of __constant_htons to htons

[David Miller]

From: Vaishali Thakkar <vthakkar1994@gmail.com>
Date: Fri, 21 Aug 2015 00:28:59 +0530

> On Thu, Aug 20, 2015 at 11:28 PM, David Miller <davem@davemloft.net> wrote:
>> From: Ursula Braun <ubraun@linux.vnet.ibm.com>
>> Date: Thu, 20 Aug 2015 15:53:42 +0200
>>
>>> qeth is an s390-driver, and s390 is a big-endian architecture. Thus
>>> arguments valid for little-endian do not apply to qeth-code.
>>
>> You can not throw out generally good tree-wide conventions just because
>> it happens to work on your platform.
>>
>> In fact, I would be really thrilled if some of this code could be compiled
>> on other platforms via COMPILE_TEST or similar, especially the socket code
>> and drivers.
> 
> I have test-compiled this patch on little-endian architecture. I used
> this script for cross compilation:
> https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross

If you cross compiled "from" a little endian architecture, that doesn't
mean anything.

What matters is if you compiled this code "targetting" a little-endian
architecture which currently is not possible.

[PATCH net-next 3/3] qeth: no write permission for readonly sysattr

[Ursula Braun]

From: Lakhvich Dmitriy <ldmitriy@ru.ibm.com>

User is not allowed to write into bridge_state sysfs file.
Fixed attribute not mislead the user

Signed-off-by: Lakhvich Dmitriy <ldmitriy@ru.ibm.com>
Signed-off-by: Ursula Braun <ursula.braun@de.ibm.com>
Reported-by: Peter Oberparleiter <oberpar@linux.vnet.ibm.com>
Reviewed-by: Eugene Crosser <Eugene.Crosser@ru.ibm.com>
Reviewed-by: Thomas Richter <tmricht@de.ibm.com>
---
 drivers/s390/net/qeth_l2_sys.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/s390/net/qeth_l2_sys.c b/drivers/s390/net/qeth_l2_sys.c
index 52673cd..692db49 100644
--- a/drivers/s390/net/qeth_l2_sys.c
+++ b/drivers/s390/net/qeth_l2_sys.c
@@ -109,7 +109,7 @@ static ssize_t qeth_bridge_port_state_show(struct device *dev,
 	return qeth_bridge_port_role_state_show(dev, attr, buf, 1);
 }
 
-static DEVICE_ATTR(bridge_state, 0644, qeth_bridge_port_state_show,
+static DEVICE_ATTR(bridge_state, 0444, qeth_bridge_port_state_show,
 		   NULL);
 
 static ssize_t qeth_bridgeport_hostnotification_show(struct device *dev,
-- 
2.3.8