[PATCH net-next 0/4] s390/net: updates 2023-02-06

[PATCH net-next 0/4] s390/net: updates 2023-02-06

[Alexandra Winter]

Hi Dave & Jakub,

please apply the following patch series for qeth to netdev's net-next
tree.

Just maintenance patches, no functional changes.

Thanks,
Alexandra

Alexandra Winter (1):
  s390/ctcm: cleanup indenting

Thorsten Winkler (3):
  s390/qeth: Use constant for IP address buffers
  s390/qeth: Convert sysfs sprintf to sysfs_emit
  s390/qeth: Convert sprintf/snprintf to scnprintf

 drivers/s390/net/ctcm_fsms.c      | 32 +++++++--------
 drivers/s390/net/ctcm_main.c      | 16 ++++----
 drivers/s390/net/ctcm_mpc.c       | 15 +++----
 drivers/s390/net/qeth_core_main.c | 14 ++++---
 drivers/s390/net/qeth_core_sys.c  | 66 ++++++++++++++++---------------
 drivers/s390/net/qeth_ethtool.c   |  6 +--
 drivers/s390/net/qeth_l2_main.c   | 53 +++++++++++++------------
 drivers/s390/net/qeth_l2_sys.c    | 28 ++++++-------
 drivers/s390/net/qeth_l3_main.c   |  7 ++--
 drivers/s390/net/qeth_l3_sys.c    | 51 +++++++++++-------------
 10 files changed, 146 insertions(+), 142 deletions(-)

-- 
2.37.2

[PATCH net-next 1/4] s390/ctcm: cleanup indenting

[Alexandra Winter]

Get rid of multiple smatch warnings, like:
warn: inconsistent indenting

Signed-off-by: Alexandra Winter <wintera@linux.ibm.com>
---
 drivers/s390/net/ctcm_fsms.c | 32 ++++++++++++++++----------------
 drivers/s390/net/ctcm_main.c | 16 ++++++++--------
 drivers/s390/net/ctcm_mpc.c  | 15 ++++++++-------
 3 files changed, 32 insertions(+), 31 deletions(-)

diff --git a/drivers/s390/net/ctcm_fsms.c b/drivers/s390/net/ctcm_fsms.c
index dfb84bb03d32..90ec477386a8 100644
--- a/drivers/s390/net/ctcm_fsms.c
+++ b/drivers/s390/net/ctcm_fsms.c
@@ -370,7 +370,7 @@ static void chx_rx(fsm_instance *fi, int event, void *arg)
 					CTCM_FUNTAIL, dev->name, len);
 		priv->stats.rx_dropped++;
 		priv->stats.rx_length_errors++;
-						goto again;
+		goto again;
 	}
 	if (len > ch->max_bufsize) {
 		CTCM_DBF_TEXT_(TRACE, CTC_DBF_NOTICE,
@@ -378,7 +378,7 @@ static void chx_rx(fsm_instance *fi, int event, void *arg)
 				CTCM_FUNTAIL, dev->name, len, ch->max_bufsize);
 		priv->stats.rx_dropped++;
 		priv->stats.rx_length_errors++;
-						goto again;
+		goto again;
 	}
 
 	/*
@@ -403,7 +403,7 @@ static void chx_rx(fsm_instance *fi, int event, void *arg)
 		*((__u16 *)skb->data) = len;
 		priv->stats.rx_dropped++;
 		priv->stats.rx_length_errors++;
-						goto again;
+		goto again;
 	}
 	if (block_len > 2) {
 		*((__u16 *)skb->data) = block_len - 2;
@@ -1006,7 +1006,7 @@ static void ctcm_chx_txretry(fsm_instance *fi, int event, void *arg)
 			use gptr as mpc indicator */
 		if (!(gptr && (fsm_getstate(gptr->fsm) != MPCG_STATE_READY)))
 			ctcm_chx_restart(fi, event, arg);
-				goto done;
+		goto done;
 	}
 
 	CTCM_DBF_TEXT_(TRACE, CTC_DBF_DEBUG,
@@ -1024,7 +1024,7 @@ static void ctcm_chx_txretry(fsm_instance *fi, int event, void *arg)
 						CTCM_FUNTAIL, ch->id);
 			fsm_event(priv->fsm, DEV_EVENT_TXDOWN, dev);
 			ctcm_chx_restart(fi, event, arg);
-				goto done;
+			goto done;
 		}
 		fsm_addtimer(&ch->timer, 1000, CTC_EVENT_TIMER, ch);
 		if (event == CTC_EVENT_TIMER) /* for TIMER not yet locked */
@@ -1251,12 +1251,12 @@ static void ctcmpc_chx_txdone(fsm_instance *fi, int event, void *arg)
 	if ((ch->collect_len <= 0) || (grp->in_sweep != 0)) {
 		spin_unlock(&ch->collect_lock);
 		fsm_newstate(fi, CTC_STATE_TXIDLE);
-				goto done;
+		goto done;
 	}
 
 	if (ctcm_checkalloc_buffer(ch)) {
 		spin_unlock(&ch->collect_lock);
-				goto done;
+		goto done;
 	}
 	ch->trans_skb->data = ch->trans_skb_data;
 	skb_reset_tail_pointer(ch->trans_skb);
@@ -1389,7 +1389,7 @@ static void ctcmpc_chx_rx(fsm_instance *fi, int event, void *arg)
 		CTCM_DBF_TEXT_(MPC_ERROR, CTC_DBF_ERROR,
 			"%s(%s): TRANS_SKB = NULL",
 				CTCM_FUNTAIL, dev->name);
-			goto again;
+		goto again;
 	}
 
 	if (len < TH_HEADER_LENGTH) {
@@ -1409,7 +1409,7 @@ static void ctcmpc_chx_rx(fsm_instance *fi, int event, void *arg)
 				"%s(%s): skb allocation failed",
 						CTCM_FUNTAIL, dev->name);
 			fsm_event(priv->mpcg->fsm, MPCG_EVENT_INOP, dev);
-					goto again;
+			goto again;
 		}
 		switch (fsm_getstate(grp->fsm)) {
 		case MPCG_STATE_RESET:
@@ -1441,9 +1441,9 @@ static void ctcmpc_chx_rx(fsm_instance *fi, int event, void *arg)
 		skb_reset_tail_pointer(ch->trans_skb);
 		ch->trans_skb->len = 0;
 		ch->ccw[1].count = ch->max_bufsize;
-			if (do_debug_ccw)
+		if (do_debug_ccw)
 			ctcmpc_dumpit((char *)&ch->ccw[0],
-					sizeof(struct ccw1) * 3);
+				      sizeof(struct ccw1) * 3);
 		dolock = !in_hardirq();
 		if (dolock)
 			spin_lock_irqsave(
@@ -1562,7 +1562,7 @@ void ctcmpc_chx_rxidle(fsm_instance *fi, int event, void *arg)
 		if (rc != 0) {
 			fsm_newstate(fi, CTC_STATE_RXINIT);
 			ctcm_ccw_check_rc(ch, rc, "initial RX");
-				goto done;
+			goto done;
 		}
 		break;
 	default:
@@ -1677,10 +1677,10 @@ static void ctcmpc_chx_attnbusy(fsm_instance *fsm, int event, void *arg)
 		if (fsm_getstate(ch->fsm) == CH_XID0_INPROGRESS) {
 			fsm_newstate(ch->fsm, CH_XID0_PENDING) ;
 			fsm_deltimer(&grp->timer);
-				goto done;
+			goto done;
 		}
 		fsm_event(grp->fsm, MPCG_EVENT_INOP, dev);
-				goto done;
+		goto done;
 	case MPCG_STATE_XID2INITX:
 		/* XID2 was received before ATTN Busy for second
 		   channel.Send yside xid for second channel.
@@ -1768,7 +1768,7 @@ static void ctcmpc_chx_send_sweep(fsm_instance *fsm, int event, void *arg)
 		/* give the previous IO time to complete */
 		fsm_addtimer(&wch->sweep_timer,
 			200, CTC_EVENT_RSWEEP_TIMER, wch);
-				goto done;
+		goto done;
 	}
 
 	skb = skb_dequeue(&wch->sweep_queue);
@@ -1780,7 +1780,7 @@ static void ctcmpc_chx_send_sweep(fsm_instance *fsm, int event, void *arg)
 		ctcm_clear_busy_do(dev);
 		dev_kfree_skb_any(skb);
 		fsm_event(grp->fsm, MPCG_EVENT_INOP, dev);
-				goto done;
+		goto done;
 	} else {
 		refcount_inc(&skb->users);
 		skb_queue_tail(&wch->io_queue, skb);
diff --git a/drivers/s390/net/ctcm_main.c b/drivers/s390/net/ctcm_main.c
index bdfab9ea0046..28db69d91f17 100644
--- a/drivers/s390/net/ctcm_main.c
+++ b/drivers/s390/net/ctcm_main.c
@@ -494,7 +494,7 @@ static int ctcm_transmit_skb(struct channel *ch, struct sk_buff *skb)
 			ch->collect_len += l;
 		}
 		spin_unlock_irqrestore(&ch->collect_lock, saveflags);
-				goto done;
+		goto done;
 	}
 	spin_unlock_irqrestore(&ch->collect_lock, saveflags);
 	/*
@@ -685,7 +685,7 @@ static int ctcmpc_transmit_skb(struct channel *ch, struct sk_buff *skb)
 		ch->collect_len += skb->len;
 
 		spin_unlock_irqrestore(&ch->collect_lock, saveflags);
-			goto done;
+		goto done;
 	}
 
 	/*
@@ -885,7 +885,7 @@ static netdev_tx_t ctcmpc_tx(struct sk_buff *skb, struct net_device *dev)
 			"%s(%s): NULL sk_buff passed",
 					CTCM_FUNTAIL, dev->name);
 		priv->stats.tx_dropped++;
-					goto done;
+		goto done;
 	}
 	if (skb_headroom(skb) < (TH_HEADER_LENGTH + PDU_HEADER_LENGTH)) {
 		CTCM_DBF_TEXT_(MPC_TRACE, CTC_DBF_ERROR,
@@ -908,7 +908,7 @@ static netdev_tx_t ctcmpc_tx(struct sk_buff *skb, struct net_device *dev)
 			priv->stats.tx_errors++;
 			priv->stats.tx_carrier_errors++;
 			fsm_event(grp->fsm, MPCG_EVENT_INOP, dev);
-					goto done;
+			goto done;
 		}
 		newskb->protocol = skb->protocol;
 		skb_reserve(newskb, TH_HEADER_LENGTH + PDU_HEADER_LENGTH);
@@ -931,7 +931,7 @@ static netdev_tx_t ctcmpc_tx(struct sk_buff *skb, struct net_device *dev)
 		priv->stats.tx_dropped++;
 		priv->stats.tx_errors++;
 		priv->stats.tx_carrier_errors++;
-					goto done;
+		goto done;
 	}
 
 	if (ctcm_test_and_set_busy(dev)) {
@@ -943,7 +943,7 @@ static netdev_tx_t ctcmpc_tx(struct sk_buff *skb, struct net_device *dev)
 		priv->stats.tx_errors++;
 		priv->stats.tx_carrier_errors++;
 		fsm_event(grp->fsm, MPCG_EVENT_INOP, dev);
-					goto done;
+		goto done;
 	}
 
 	netif_trans_update(dev);
@@ -957,7 +957,7 @@ static netdev_tx_t ctcmpc_tx(struct sk_buff *skb, struct net_device *dev)
 		priv->stats.tx_carrier_errors++;
 		ctcm_clear_busy(dev);
 		fsm_event(grp->fsm, MPCG_EVENT_INOP, dev);
-					goto done;
+		goto done;
 	}
 	ctcm_clear_busy(dev);
 done:
@@ -1421,7 +1421,7 @@ static int add_channel(struct ccw_device *cdev, enum ctcm_channel_types type,
 				"%s (%s) already in list, using old entry",
 				__func__, (*c)->id);
 
-				goto free_return;
+		goto free_return;
 	}
 
 	spin_lock_init(&ch->collect_lock);
diff --git a/drivers/s390/net/ctcm_mpc.c b/drivers/s390/net/ctcm_mpc.c
index 8ac213a55141..b8a226c6e1a9 100644
--- a/drivers/s390/net/ctcm_mpc.c
+++ b/drivers/s390/net/ctcm_mpc.c
@@ -481,7 +481,7 @@ void ctc_mpc_establish_connectivity(int port_num,
 				grp->estconnfunc = NULL;
 			}
 			fsm_deltimer(&grp->timer);
-				goto done;
+			goto done;
 		}
 		if ((wch->in_mpcgroup) &&
 				(fsm_getstate(wch->fsm) == CH_XID0_PENDING))
@@ -495,7 +495,7 @@ void ctc_mpc_establish_connectivity(int port_num,
 				grp->estconnfunc = NULL;
 			}
 			fsm_deltimer(&grp->timer);
-				goto done;
+			goto done;
 			}
 		break;
 	case MPCG_STATE_XID0IOWAIT:
@@ -896,8 +896,9 @@ void mpc_group_ready(unsigned long adev)
 		grp->estconnfunc(grp->port_num, 0,
 				    grp->group_max_buflen);
 		grp->estconnfunc = NULL;
-	} else 	if (grp->allochanfunc)
+	} else if (grp->allochanfunc) {
 		grp->allochanfunc(grp->port_num, grp->group_max_buflen);
+	}
 
 	grp->send_qllc_disc = 1;
 	grp->changed_side = 0;
@@ -1109,7 +1110,7 @@ static void ctcmpc_unpack_skb(struct channel *ch, struct sk_buff *pskb)
 
 				priv->stats.rx_dropped++;
 				priv->stats.rx_length_errors++;
-					goto done;
+				goto done;
 			}
 			skb_reset_mac_header(pskb);
 			new_len = curr_pdu->pdu_offset;
@@ -1132,7 +1133,7 @@ static void ctcmpc_unpack_skb(struct channel *ch, struct sk_buff *pskb)
 						CTCM_FUNTAIL, dev->name);
 				priv->stats.rx_dropped++;
 				fsm_event(grp->fsm, MPCG_EVENT_INOP, dev);
-						goto done;
+				goto done;
 			}
 			skb_put_data(skb, pskb->data, new_len);
 
@@ -1543,7 +1544,7 @@ static int mpc_validate_xid(struct mpcg_info *mpcginfo)
 		CTCM_DBF_TEXT_(MPC_ERROR, CTC_DBF_ERROR,
 			"%s(%s): xid = NULL",
 				CTCM_FUNTAIL, ch->id);
-			goto done;
+		goto done;
 	}
 
 	CTCM_D3_DUMP((char *)xid, XID2_LENGTH);
@@ -1556,7 +1557,7 @@ static int mpc_validate_xid(struct mpcg_info *mpcginfo)
 		CTCM_DBF_TEXT_(MPC_ERROR, CTC_DBF_ERROR,
 			"%s(%s): r/w channel pairing mismatch",
 				CTCM_FUNTAIL, ch->id);
-			goto done;
+		goto done;
 	}
 
 	if (xid->xid2_dlc_type == XID2_READ_SIDE) {
-- 
2.37.2

Re: [PATCH net-next 1/4] s390/ctcm: cleanup indenting

[Simon Horman]

On Mon, Feb 06, 2023 at 06:27:51PM +0100, Alexandra Winter wrote:
> Get rid of multiple smatch warnings, like:
> warn: inconsistent indenting
> 
> Signed-off-by: Alexandra Winter <wintera@linux.ibm.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

[PATCH net-next 2/4] s390/qeth: Use constant for IP address buffers

[Alexandra Winter]

From: Thorsten Winkler <twinkler@linux.ibm.com>

Use INET6_ADDRSTRLEN constant with size of 48 which be used for char arrays
storing ip addresses (for IPv4 and IPv6)

Reviewed-by: Alexandra Winkler <wintera@linux.ibm.com>
Signed-off-by: Thorsten Winkler <twinkler@linux.ibm.com>
Signed-off-by: Alexandra Winter <wintera@linux.ibm.com>
---
 drivers/s390/net/qeth_l3_main.c | 3 ++-
 drivers/s390/net/qeth_l3_sys.c  | 6 +++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/net/qeth_l3_main.c b/drivers/s390/net/qeth_l3_main.c
index d8487a10cd55..1cf4e354693f 100644
--- a/drivers/s390/net/qeth_l3_main.c
+++ b/drivers/s390/net/qeth_l3_main.c
@@ -19,6 +19,7 @@
 #include <linux/etherdevice.h>
 #include <linux/ip.h>
 #include <linux/in.h>
+#include <linux/inet.h>
 #include <linux/ipv6.h>
 #include <linux/inetdevice.h>
 #include <linux/igmp.h>
@@ -158,7 +159,7 @@ static int qeth_l3_add_ip(struct qeth_card *card, struct qeth_ipaddr *tmp_addr)
 {
 	int rc = 0;
 	struct qeth_ipaddr *addr;
-	char buf[40];
+	char buf[INET6_ADDRSTRLEN];
 
 	if (tmp_addr->type == QETH_IP_TYPE_RXIP)
 		QETH_CARD_TEXT(card, 2, "addrxip");
diff --git a/drivers/s390/net/qeth_l3_sys.c b/drivers/s390/net/qeth_l3_sys.c
index 1082380b21f8..6143dd485810 100644
--- a/drivers/s390/net/qeth_l3_sys.c
+++ b/drivers/s390/net/qeth_l3_sys.c
@@ -371,7 +371,7 @@ static ssize_t qeth_l3_dev_ipato_add_show(char *buf, struct qeth_card *card,
 
 	mutex_lock(&card->ip_lock);
 	list_for_each_entry(ipatoe, &card->ipato.entries, entry) {
-		char addr_str[40];
+		char addr_str[INET6_ADDRSTRLEN];
 		int entry_len;
 
 		if (ipatoe->proto != proto)
@@ -413,7 +413,7 @@ static int qeth_l3_parse_ipatoe(const char *buf, enum qeth_prot_versions proto,
 	int rc;
 
 	/* Expected input pattern: %addr/%mask */
-	sep = strnchr(buf, 40, '/');
+	sep = strnchr(buf, INET6_ADDRSTRLEN, '/');
 	if (!sep)
 		return -EINVAL;
 
@@ -592,7 +592,7 @@ static ssize_t qeth_l3_dev_ip_add_show(struct device *dev, char *buf,
 
 	mutex_lock(&card->ip_lock);
 	hash_for_each(card->ip_htable, i, ipaddr, hnode) {
-		char addr_str[40];
+		char addr_str[INET6_ADDRSTRLEN];
 		int entry_len;
 
 		if (ipaddr->proto != proto || ipaddr->type != type)
-- 
2.37.2

Re: [PATCH net-next 2/4] s390/qeth: Use constant for IP address buffers

[Simon Horman]

On Mon, Feb 06, 2023 at 06:27:52PM +0100, Alexandra Winter wrote:
> From: Thorsten Winkler <twinkler@linux.ibm.com>
> 
> Use INET6_ADDRSTRLEN constant with size of 48 which be used for char arrays
> storing ip addresses (for IPv4 and IPv6)
> 
> Reviewed-by: Alexandra Winkler <wintera@linux.ibm.com>

s/Winkler/Winter/ ?

> Signed-off-by: Thorsten Winkler <twinkler@linux.ibm.com>
> Signed-off-by: Alexandra Winter <wintera@linux.ibm.com>

Code changes look good to me.

[PATCH net-next 3/4] s390/qeth: Convert sysfs sprintf to sysfs_emit

[Alexandra Winter]

From: Thorsten Winkler <twinkler@linux.ibm.com>

Following the advice of the Documentation/filesystems/sysfs.rst.
All sysfs related show()-functions should only use sysfs_emit() or
sysfs_emit_at() when formatting the value to be returned to user space.

Reported-by: Jules Irenge <jbi.octave@gmail.com>
Reported-by: Joe Perches <joe@perches.com>
Reviewed-by: Alexandra Winkler <wintera@linux.ibm.com>
Signed-off-by: Thorsten Winkler <twinkler@linux.ibm.com>
Signed-off-by: Alexandra Winter <wintera@linux.ibm.com>
---
 drivers/s390/net/qeth_core_sys.c | 66 ++++++++++++++++----------------
 drivers/s390/net/qeth_l2_sys.c   | 28 +++++++-------
 drivers/s390/net/qeth_l3_sys.c   | 41 +++++++++-----------
 3 files changed, 67 insertions(+), 68 deletions(-)

diff --git a/drivers/s390/net/qeth_core_sys.c b/drivers/s390/net/qeth_core_sys.c
index d1adc4b83193..eea93f8f106f 100644
--- a/drivers/s390/net/qeth_core_sys.c
+++ b/drivers/s390/net/qeth_core_sys.c
@@ -23,15 +23,15 @@ static ssize_t qeth_dev_state_show(struct device *dev,
 
 	switch (card->state) {
 	case CARD_STATE_DOWN:
-		return sprintf(buf, "DOWN\n");
+		return sysfs_emit(buf, "DOWN\n");
 	case CARD_STATE_SOFTSETUP:
 		if (card->dev->flags & IFF_UP)
-			return sprintf(buf, "UP (LAN %s)\n",
-				       netif_carrier_ok(card->dev) ? "ONLINE" :
-								     "OFFLINE");
-		return sprintf(buf, "SOFTSETUP\n");
+			return sysfs_emit(buf, "UP (LAN %s)\n",
+					  netif_carrier_ok(card->dev) ?
+					  "ONLINE" : "OFFLINE");
+		return sysfs_emit(buf, "SOFTSETUP\n");
 	default:
-		return sprintf(buf, "UNKNOWN\n");
+		return sysfs_emit(buf, "UNKNOWN\n");
 	}
 }
 
@@ -42,7 +42,7 @@ static ssize_t qeth_dev_chpid_show(struct device *dev,
 {
 	struct qeth_card *card = dev_get_drvdata(dev);
 
-	return sprintf(buf, "%02X\n", card->info.chpid);
+	return sysfs_emit(buf, "%02X\n", card->info.chpid);
 }
 
 static DEVICE_ATTR(chpid, 0444, qeth_dev_chpid_show, NULL);
@@ -52,7 +52,7 @@ static ssize_t qeth_dev_if_name_show(struct device *dev,
 {
 	struct qeth_card *card = dev_get_drvdata(dev);
 
-	return sprintf(buf, "%s\n", netdev_name(card->dev));
+	return sysfs_emit(buf, "%s\n", netdev_name(card->dev));
 }
 
 static DEVICE_ATTR(if_name, 0444, qeth_dev_if_name_show, NULL);
@@ -62,7 +62,7 @@ static ssize_t qeth_dev_card_type_show(struct device *dev,
 {
 	struct qeth_card *card = dev_get_drvdata(dev);
 
-	return sprintf(buf, "%s\n", qeth_get_cardname_short(card));
+	return sysfs_emit(buf, "%s\n", qeth_get_cardname_short(card));
 }
 
 static DEVICE_ATTR(card_type, 0444, qeth_dev_card_type_show, NULL);
@@ -86,7 +86,7 @@ static ssize_t qeth_dev_inbuf_size_show(struct device *dev,
 {
 	struct qeth_card *card = dev_get_drvdata(dev);
 
-	return sprintf(buf, "%s\n", qeth_get_bufsize_str(card));
+	return sysfs_emit(buf, "%s\n", qeth_get_bufsize_str(card));
 }
 
 static DEVICE_ATTR(inbuf_size, 0444, qeth_dev_inbuf_size_show, NULL);
@@ -96,7 +96,7 @@ static ssize_t qeth_dev_portno_show(struct device *dev,
 {
 	struct qeth_card *card = dev_get_drvdata(dev);
 
-	return sprintf(buf, "%i\n", card->dev->dev_port);
+	return sysfs_emit(buf, "%i\n", card->dev->dev_port);
 }
 
 static ssize_t qeth_dev_portno_store(struct device *dev,
@@ -134,7 +134,7 @@ static DEVICE_ATTR(portno, 0644, qeth_dev_portno_show, qeth_dev_portno_store);
 static ssize_t qeth_dev_portname_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
-	return sprintf(buf, "no portname required\n");
+	return sysfs_emit(buf, "no portname required\n");
 }
 
 static ssize_t qeth_dev_portname_store(struct device *dev,
@@ -157,18 +157,18 @@ static ssize_t qeth_dev_prioqing_show(struct device *dev,
 
 	switch (card->qdio.do_prio_queueing) {
 	case QETH_PRIO_Q_ING_PREC:
-		return sprintf(buf, "%s\n", "by precedence");
+		return sysfs_emit(buf, "%s\n", "by precedence");
 	case QETH_PRIO_Q_ING_TOS:
-		return sprintf(buf, "%s\n", "by type of service");
+		return sysfs_emit(buf, "%s\n", "by type of service");
 	case QETH_PRIO_Q_ING_SKB:
-		return sprintf(buf, "%s\n", "by skb-priority");
+		return sysfs_emit(buf, "%s\n", "by skb-priority");
 	case QETH_PRIO_Q_ING_VLAN:
-		return sprintf(buf, "%s\n", "by VLAN headers");
+		return sysfs_emit(buf, "%s\n", "by VLAN headers");
 	case QETH_PRIO_Q_ING_FIXED:
-		return sprintf(buf, "always queue %i\n",
+		return sysfs_emit(buf, "always queue %i\n",
 			       card->qdio.default_out_queue);
 	default:
-		return sprintf(buf, "disabled\n");
+		return sysfs_emit(buf, "disabled\n");
 	}
 }
 
@@ -242,7 +242,7 @@ static ssize_t qeth_dev_bufcnt_show(struct device *dev,
 {
 	struct qeth_card *card = dev_get_drvdata(dev);
 
-	return sprintf(buf, "%i\n", card->qdio.in_buf_pool.buf_count);
+	return sysfs_emit(buf, "%i\n", card->qdio.in_buf_pool.buf_count);
 }
 
 static ssize_t qeth_dev_bufcnt_store(struct device *dev,
@@ -298,7 +298,7 @@ static DEVICE_ATTR(recover, 0200, NULL, qeth_dev_recover_store);
 static ssize_t qeth_dev_performance_stats_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
-	return sprintf(buf, "1\n");
+	return sysfs_emit(buf, "1\n");
 }
 
 static ssize_t qeth_dev_performance_stats_store(struct device *dev,
@@ -335,7 +335,7 @@ static ssize_t qeth_dev_layer2_show(struct device *dev,
 {
 	struct qeth_card *card = dev_get_drvdata(dev);
 
-	return sprintf(buf, "%i\n", card->options.layer);
+	return sysfs_emit(buf, "%i\n", card->options.layer);
 }
 
 static ssize_t qeth_dev_layer2_store(struct device *dev,
@@ -470,23 +470,25 @@ static ssize_t qeth_dev_switch_attrs_show(struct device *dev,
 	int	rc = 0;
 
 	if (!qeth_card_hw_is_reachable(card))
-		return sprintf(buf, "n/a\n");
+		return sysfs_emit(buf, "n/a\n");
 
 	rc = qeth_query_switch_attributes(card, &sw_info);
 	if (rc)
 		return rc;
 
 	if (!sw_info.capabilities)
-		rc = sprintf(buf, "unknown");
+		rc = sysfs_emit(buf, "unknown");
 
 	if (sw_info.capabilities & QETH_SWITCH_FORW_802_1)
-		rc = sprintf(buf, (sw_info.settings & QETH_SWITCH_FORW_802_1 ?
-							"[802.1]" : "802.1"));
+		rc = sysfs_emit(buf,
+				(sw_info.settings & QETH_SWITCH_FORW_802_1 ?
+				"[802.1]" : "802.1"));
 	if (sw_info.capabilities & QETH_SWITCH_FORW_REFL_RELAY)
-		rc += sprintf(buf + rc,
-			(sw_info.settings & QETH_SWITCH_FORW_REFL_RELAY ?
-							" [rr]" : " rr"));
-	rc += sprintf(buf + rc, "\n");
+		rc += sysfs_emit_at(buf, rc,
+				    (sw_info.settings &
+				    QETH_SWITCH_FORW_REFL_RELAY ?
+				    " [rr]" : " rr"));
+	rc += sysfs_emit_at(buf, rc, "\n");
 
 	return rc;
 }
@@ -573,7 +575,7 @@ static ssize_t qeth_dev_blkt_total_show(struct device *dev,
 {
 	struct qeth_card *card = dev_get_drvdata(dev);
 
-	return sprintf(buf, "%i\n", card->info.blkt.time_total);
+	return sysfs_emit(buf, "%i\n", card->info.blkt.time_total);
 }
 
 static ssize_t qeth_dev_blkt_total_store(struct device *dev,
@@ -593,7 +595,7 @@ static ssize_t qeth_dev_blkt_inter_show(struct device *dev,
 {
 	struct qeth_card *card = dev_get_drvdata(dev);
 
-	return sprintf(buf, "%i\n", card->info.blkt.inter_packet);
+	return sysfs_emit(buf, "%i\n", card->info.blkt.inter_packet);
 }
 
 static ssize_t qeth_dev_blkt_inter_store(struct device *dev,
@@ -613,7 +615,7 @@ static ssize_t qeth_dev_blkt_inter_jumbo_show(struct device *dev,
 {
 	struct qeth_card *card = dev_get_drvdata(dev);
 
-	return sprintf(buf, "%i\n", card->info.blkt.inter_packet_jumbo);
+	return sysfs_emit(buf, "%i\n", card->info.blkt.inter_packet_jumbo);
 }
 
 static ssize_t qeth_dev_blkt_inter_jumbo_store(struct device *dev,
diff --git a/drivers/s390/net/qeth_l2_sys.c b/drivers/s390/net/qeth_l2_sys.c
index a617351fff57..7f592f912517 100644
--- a/drivers/s390/net/qeth_l2_sys.c
+++ b/drivers/s390/net/qeth_l2_sys.c
@@ -19,7 +19,7 @@ static ssize_t qeth_bridge_port_role_state_show(struct device *dev,
 	char *word;
 
 	if (!qeth_bridgeport_allowed(card))
-		return sprintf(buf, "n/a (VNIC characteristics)\n");
+		return sysfs_emit(buf, "n/a (VNIC characteristics)\n");
 
 	mutex_lock(&card->sbp_lock);
 	if (qeth_card_hw_is_reachable(card) &&
@@ -53,7 +53,7 @@ static ssize_t qeth_bridge_port_role_state_show(struct device *dev,
 			QETH_CARD_TEXT_(card, 2, "SBP%02x:%02x",
 				card->options.sbp.role, state);
 		else
-			rc = sprintf(buf, "%s\n", word);
+			rc = sysfs_emit(buf, "%s\n", word);
 	}
 	mutex_unlock(&card->sbp_lock);
 
@@ -66,7 +66,7 @@ static ssize_t qeth_bridge_port_role_show(struct device *dev,
 	struct qeth_card *card = dev_get_drvdata(dev);
 
 	if (!qeth_bridgeport_allowed(card))
-		return sprintf(buf, "n/a (VNIC characteristics)\n");
+		return sysfs_emit(buf, "n/a (VNIC characteristics)\n");
 
 	return qeth_bridge_port_role_state_show(dev, attr, buf, 0);
 }
@@ -117,7 +117,7 @@ static ssize_t qeth_bridge_port_state_show(struct device *dev,
 	struct qeth_card *card = dev_get_drvdata(dev);
 
 	if (!qeth_bridgeport_allowed(card))
-		return sprintf(buf, "n/a (VNIC characteristics)\n");
+		return sysfs_emit(buf, "n/a (VNIC characteristics)\n");
 
 	return qeth_bridge_port_role_state_show(dev, attr, buf, 1);
 }
@@ -132,11 +132,11 @@ static ssize_t qeth_bridgeport_hostnotification_show(struct device *dev,
 	int enabled;
 
 	if (!qeth_bridgeport_allowed(card))
-		return sprintf(buf, "n/a (VNIC characteristics)\n");
+		return sysfs_emit(buf, "n/a (VNIC characteristics)\n");
 
 	enabled = card->options.sbp.hostnotification;
 
-	return sprintf(buf, "%d\n", enabled);
+	return sysfs_emit(buf, "%d\n", enabled);
 }
 
 static ssize_t qeth_bridgeport_hostnotification_store(struct device *dev,
@@ -180,7 +180,7 @@ static ssize_t qeth_bridgeport_reflect_show(struct device *dev,
 	char *state;
 
 	if (!qeth_bridgeport_allowed(card))
-		return sprintf(buf, "n/a (VNIC characteristics)\n");
+		return sysfs_emit(buf, "n/a (VNIC characteristics)\n");
 
 	if (card->options.sbp.reflect_promisc) {
 		if (card->options.sbp.reflect_promisc_primary)
@@ -190,7 +190,7 @@ static ssize_t qeth_bridgeport_reflect_show(struct device *dev,
 	} else
 		state = "none";
 
-	return sprintf(buf, "%s\n", state);
+	return sysfs_emit(buf, "%s\n", state);
 }
 
 static ssize_t qeth_bridgeport_reflect_store(struct device *dev,
@@ -280,10 +280,10 @@ static ssize_t qeth_vnicc_timeout_show(struct device *dev,
 
 	rc = qeth_l2_vnicc_get_timeout(card, &timeout);
 	if (rc == -EBUSY)
-		return sprintf(buf, "n/a (BridgePort)\n");
+		return sysfs_emit(buf, "n/a (BridgePort)\n");
 	if (rc == -EOPNOTSUPP)
-		return sprintf(buf, "n/a\n");
-	return rc ? rc : sprintf(buf, "%d\n", timeout);
+		return sysfs_emit(buf, "n/a\n");
+	return rc ? rc : sysfs_emit(buf, "%d\n", timeout);
 }
 
 /* change timeout setting */
@@ -318,10 +318,10 @@ static ssize_t qeth_vnicc_char_show(struct device *dev,
 	rc = qeth_l2_vnicc_get_state(card, vnicc, &state);
 
 	if (rc == -EBUSY)
-		return sprintf(buf, "n/a (BridgePort)\n");
+		return sysfs_emit(buf, "n/a (BridgePort)\n");
 	if (rc == -EOPNOTSUPP)
-		return sprintf(buf, "n/a\n");
-	return rc ? rc : sprintf(buf, "%d\n", state);
+		return sysfs_emit(buf, "n/a\n");
+	return rc ? rc : sysfs_emit(buf, "%d\n", state);
 }
 
 /* change setting of characteristic */
diff --git a/drivers/s390/net/qeth_l3_sys.c b/drivers/s390/net/qeth_l3_sys.c
index 6143dd485810..f0f8adaa2f05 100644
--- a/drivers/s390/net/qeth_l3_sys.c
+++ b/drivers/s390/net/qeth_l3_sys.c
@@ -32,26 +32,26 @@ static ssize_t qeth_l3_dev_route_show(struct qeth_card *card,
 {
 	switch (route->type) {
 	case PRIMARY_ROUTER:
-		return sprintf(buf, "%s\n", "primary router");
+		return sysfs_emit(buf, "%s\n", "primary router");
 	case SECONDARY_ROUTER:
-		return sprintf(buf, "%s\n", "secondary router");
+		return sysfs_emit(buf, "%s\n", "secondary router");
 	case MULTICAST_ROUTER:
 		if (card->info.broadcast_capable == QETH_BROADCAST_WITHOUT_ECHO)
-			return sprintf(buf, "%s\n", "multicast router+");
+			return sysfs_emit(buf, "%s\n", "multicast router+");
 		else
-			return sprintf(buf, "%s\n", "multicast router");
+			return sysfs_emit(buf, "%s\n", "multicast router");
 	case PRIMARY_CONNECTOR:
 		if (card->info.broadcast_capable == QETH_BROADCAST_WITHOUT_ECHO)
-			return sprintf(buf, "%s\n", "primary connector+");
+			return sysfs_emit(buf, "%s\n", "primary connector+");
 		else
-			return sprintf(buf, "%s\n", "primary connector");
+			return sysfs_emit(buf, "%s\n", "primary connector");
 	case SECONDARY_CONNECTOR:
 		if (card->info.broadcast_capable == QETH_BROADCAST_WITHOUT_ECHO)
-			return sprintf(buf, "%s\n", "secondary connector+");
+			return sysfs_emit(buf, "%s\n", "secondary connector+");
 		else
-			return sprintf(buf, "%s\n", "secondary connector");
+			return sysfs_emit(buf, "%s\n", "secondary connector");
 	default:
-		return sprintf(buf, "%s\n", "no");
+		return sysfs_emit(buf, "%s\n", "no");
 	}
 }
 
@@ -138,7 +138,7 @@ static ssize_t qeth_l3_dev_sniffer_show(struct device *dev,
 {
 	struct qeth_card *card = dev_get_drvdata(dev);
 
-	return sprintf(buf, "%i\n", card->options.sniffer ? 1 : 0);
+	return sysfs_emit(buf, "%i\n", card->options.sniffer ? 1 : 0);
 }
 
 static ssize_t qeth_l3_dev_sniffer_store(struct device *dev,
@@ -200,7 +200,7 @@ static ssize_t qeth_l3_dev_hsuid_show(struct device *dev,
 
 	memcpy(tmp_hsuid, card->options.hsuid, sizeof(tmp_hsuid));
 	EBCASC(tmp_hsuid, 8);
-	return sprintf(buf, "%s\n", tmp_hsuid);
+	return sysfs_emit(buf, "%s\n", tmp_hsuid);
 }
 
 static ssize_t qeth_l3_dev_hsuid_store(struct device *dev,
@@ -285,7 +285,7 @@ static ssize_t qeth_l3_dev_ipato_enable_show(struct device *dev,
 {
 	struct qeth_card *card = dev_get_drvdata(dev);
 
-	return sprintf(buf, "%u\n", card->ipato.enabled ? 1 : 0);
+	return sysfs_emit(buf, "%u\n", card->ipato.enabled ? 1 : 0);
 }
 
 static ssize_t qeth_l3_dev_ipato_enable_store(struct device *dev,
@@ -330,7 +330,7 @@ static ssize_t qeth_l3_dev_ipato_invert4_show(struct device *dev,
 {
 	struct qeth_card *card = dev_get_drvdata(dev);
 
-	return sprintf(buf, "%u\n", card->ipato.invert4 ? 1 : 0);
+	return sysfs_emit(buf, "%u\n", card->ipato.invert4 ? 1 : 0);
 }
 
 static ssize_t qeth_l3_dev_ipato_invert4_store(struct device *dev,
@@ -388,14 +388,13 @@ static ssize_t qeth_l3_dev_ipato_add_show(char *buf, struct qeth_card *card,
 		if (entry_len + 1 > PAGE_SIZE - str_len - 1)
 			break;
 
-		entry_len = scnprintf(buf, PAGE_SIZE - str_len,
-				      "%s/%i\n", addr_str, ipatoe->mask_bits);
+		entry_len = sysfs_emit_at(buf, str_len, "%s/%i\n",
+					  addr_str, ipatoe->mask_bits);
 		str_len += entry_len;
-		buf += entry_len;
 	}
 	mutex_unlock(&card->ip_lock);
 
-	return str_len ? str_len : scnprintf(buf, PAGE_SIZE, "\n");
+	return str_len ? str_len : sysfs_emit(buf, "\n");
 }
 
 static ssize_t qeth_l3_dev_ipato_add4_show(struct device *dev,
@@ -501,7 +500,7 @@ static ssize_t qeth_l3_dev_ipato_invert6_show(struct device *dev,
 {
 	struct qeth_card *card = dev_get_drvdata(dev);
 
-	return sprintf(buf, "%u\n", card->ipato.invert6 ? 1 : 0);
+	return sysfs_emit(buf, "%u\n", card->ipato.invert6 ? 1 : 0);
 }
 
 static ssize_t qeth_l3_dev_ipato_invert6_store(struct device *dev,
@@ -607,14 +606,12 @@ static ssize_t qeth_l3_dev_ip_add_show(struct device *dev, char *buf,
 		if (entry_len + 1 > PAGE_SIZE - str_len - 1)
 			break;
 
-		entry_len = scnprintf(buf, PAGE_SIZE - str_len, "%s\n",
-				      addr_str);
+		entry_len = sysfs_emit_at(buf, str_len, "%s\n", addr_str);
 		str_len += entry_len;
-		buf += entry_len;
 	}
 	mutex_unlock(&card->ip_lock);
 
-	return str_len ? str_len : scnprintf(buf, PAGE_SIZE, "\n");
+	return str_len ? str_len : sysfs_emit(buf, "\n");
 }
 
 static ssize_t qeth_l3_dev_vipa_add4_show(struct device *dev,
-- 
2.37.2

Re: [PATCH net-next 3/4] s390/qeth: Convert sysfs sprintf to sysfs_emit

[Simon Horman]

On Mon, Feb 06, 2023 at 06:27:53PM +0100, Alexandra Winter wrote:
> From: Thorsten Winkler <twinkler@linux.ibm.com>
> 
> Following the advice of the Documentation/filesystems/sysfs.rst.
> All sysfs related show()-functions should only use sysfs_emit() or
> sysfs_emit_at() when formatting the value to be returned to user space.
> 
> Reported-by: Jules Irenge <jbi.octave@gmail.com>
> Reported-by: Joe Perches <joe@perches.com>
> Reviewed-by: Alexandra Winkler <wintera@linux.ibm.com>

s/Winkler/Winter/ ?

> Signed-off-by: Thorsten Winkler <twinkler@linux.ibm.com>
> Signed-off-by: Alexandra Winter <wintera@linux.ibm.com>

Code changes look good to me.

Re: [PATCH net-next 3/4] s390/qeth: Convert sysfs sprintf to sysfs_emit

[Joe Perches]

On Mon, 2023-02-06 at 18:27 +0100, Alexandra Winter wrote:
> From: Thorsten Winkler <twinkler@linux.ibm.com>
> 
> Following the advice of the Documentation/filesystems/sysfs.rst.
> All sysfs related show()-functions should only use sysfs_emit() or
> sysfs_emit_at() when formatting the value to be returned to user space.
[]
> diff --git a/drivers/s390/net/qeth_l3_sys.c b/drivers/s390/net/qeth_l3_sys.c
[]
> @@ -607,14 +606,12 @@ static ssize_t qeth_l3_dev_ip_add_show(struct device *dev, char *buf,
>  		if (entry_len + 1 > PAGE_SIZE - str_len - 1)
>  			break;
>  
> -		entry_len = scnprintf(buf, PAGE_SIZE - str_len, "%s\n",
> -				      addr_str);
> +		entry_len = sysfs_emit_at(buf, str_len, "%s\n", addr_str);
>  		str_len += entry_len;
> -		buf += entry_len;
>  	}
>  	mutex_unlock(&card->ip_lock);
>  
> -	return str_len ? str_len : scnprintf(buf, PAGE_SIZE, "\n");
> +	return str_len ? str_len : sysfs_emit(buf, "\n");
>  }
>  
>  static ssize_t qeth_l3_dev_vipa_add4_show(struct device *dev,

One of the intended uses of sysfs_emit is to not require the
knowlege of buf as PAGE_SIZE so it could possibly be
extended/changed.

So perhaps the use of entry_len is useless and the PAGE_SIZE use
above should be removed.

The below though could emit a partial line, dunno if that's a
good thing or not but sysfs is not supposed to emit multiple
lines anyway.
---
diff --git a/drivers/s390/net/qeth_l3_sys.c b/drivers/s390/net/qeth_l3_sys.c
index 1082380b21f85..a2a332f29f5c4 100644
--- a/drivers/s390/net/qeth_l3_sys.c
+++ b/drivers/s390/net/qeth_l3_sys.c
@@ -367,35 +367,24 @@ static ssize_t qeth_l3_dev_ipato_add_show(char *buf, struct qeth_card *card,
 			enum qeth_prot_versions proto)
 {
 	struct qeth_ipato_entry *ipatoe;
-	int str_len = 0;
+	int len = 0;
 
 	mutex_lock(&card->ip_lock);
 	list_for_each_entry(ipatoe, &card->ipato.entries, entry) {
 		char addr_str[40];
-		int entry_len;
 
 		if (ipatoe->proto != proto)
 			continue;
 
-		entry_len = qeth_l3_ipaddr_to_string(proto, ipatoe->addr,
-						     addr_str);
-		if (entry_len < 0)
+		if (qeth_l3_ipaddr_to_string(proto, ipatoe->addr, addr_str) < 0)
 			continue;
 
-		/* Append /%mask to the entry: */
-		entry_len += 1 + ((proto == QETH_PROT_IPV4) ? 2 : 3);
-		/* Enough room to format %entry\n into null terminated page? */
-		if (entry_len + 1 > PAGE_SIZE - str_len - 1)
-			break;
-
-		entry_len = scnprintf(buf, PAGE_SIZE - str_len,
-				      "%s/%i\n", addr_str, ipatoe->mask_bits);
-		str_len += entry_len;
-		buf += entry_len;
+		len += sysfs_emit_at(buf, len, "%s/%i\n",
+				     addr_str, ipatoe->mask_bits);
 	}
 	mutex_unlock(&card->ip_lock);
 
-	return str_len ? str_len : scnprintf(buf, PAGE_SIZE, "\n");
+	return len ?: sysfs_emit(buf, "\n");
 }
 
 static ssize_t qeth_l3_dev_ipato_add4_show(struct device *dev,

Re: [PATCH net-next 3/4] s390/qeth: Convert sysfs sprintf to sysfs_emit

[Alexandra Winter]

On 07.02.23 17:06, Joe Perches wrote:
> On Mon, 2023-02-06 at 18:27 +0100, Alexandra Winter wrote:
>> From: Thorsten Winkler <twinkler@linux.ibm.com>
...
> 
> One of the intended uses of sysfs_emit is to not require the
> knowlege of buf as PAGE_SIZE so it could possibly be
> extended/changed.
> 
> So perhaps the use of entry_len is useless and the PAGE_SIZE use
> above should be removed.
> 

Thanks a lot for pointing that out. Will send a v2 tomorrow.

> The below though could emit a partial line, dunno if that's a
> good thing or not but sysfs is not supposed to emit multiple
> lines anyway.

Agree, this may not be the best usage of sysfs.
But we don't want to change existing behaviour with this patch.

[PATCH net-next 4/4] s390/qeth: Convert sprintf/snprintf to scnprintf

[Alexandra Winter]

From: Thorsten Winkler <twinkler@linux.ibm.com>

This LWN article explains the rationale for this change
https: //lwn.net/Articles/69419/
Ie. snprintf() returns what *would* be the resulting length,
while scnprintf() returns the actual length.

Reported-by: Jules Irenge <jbi.octave@gmail.com>
Reviewed-by: Alexandra Winkler <wintera@linux.ibm.com>
Signed-off-by: Thorsten Winkler <twinkler@linux.ibm.com>
Signed-off-by: Alexandra Winter <wintera@linux.ibm.com>
---
 drivers/s390/net/qeth_core_main.c | 14 ++++----
 drivers/s390/net/qeth_ethtool.c   |  6 ++--
 drivers/s390/net/qeth_l2_main.c   | 53 ++++++++++++++++---------------
 drivers/s390/net/qeth_l3_main.c   |  4 +--
 drivers/s390/net/qeth_l3_sys.c    |  4 +--
 5 files changed, 42 insertions(+), 39 deletions(-)

diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c
index 8bd9fd51208c..1d5b207c2b9e 100644
--- a/drivers/s390/net/qeth_core_main.c
+++ b/drivers/s390/net/qeth_core_main.c
@@ -2801,9 +2801,11 @@ static void qeth_print_status_message(struct qeth_card *card)
 		 * of the level OSA sets the first character to zero
 		 * */
 		if (!card->info.mcl_level[0]) {
-			sprintf(card->info.mcl_level, "%02x%02x",
-				card->info.mcl_level[2],
-				card->info.mcl_level[3]);
+			scnprintf(card->info.mcl_level,
+				  sizeof(card->info.mcl_level),
+				  "%02x%02x",
+				  card->info.mcl_level[2],
+				  card->info.mcl_level[3]);
 			break;
 		}
 		fallthrough;
@@ -6090,7 +6092,7 @@ void qeth_dbf_longtext(debug_info_t *id, int level, char *fmt, ...)
 	if (!debug_level_enabled(id, level))
 		return;
 	va_start(args, fmt);
-	vsnprintf(dbf_txt_buf, sizeof(dbf_txt_buf), fmt, args);
+	vscnprintf(dbf_txt_buf, sizeof(dbf_txt_buf), fmt, args);
 	va_end(args);
 	debug_text_event(id, level, dbf_txt_buf);
 }
@@ -6330,8 +6332,8 @@ static int qeth_core_probe_device(struct ccwgroup_device *gdev)
 		goto err_dev;
 	}
 
-	snprintf(dbf_name, sizeof(dbf_name), "qeth_card_%s",
-		dev_name(&gdev->dev));
+	scnprintf(dbf_name, sizeof(dbf_name), "qeth_card_%s",
+		  dev_name(&gdev->dev));
 	card->debug = qeth_get_dbf_entry(dbf_name);
 	if (!card->debug) {
 		rc = qeth_add_dbf_entry(card, dbf_name);
diff --git a/drivers/s390/net/qeth_ethtool.c b/drivers/s390/net/qeth_ethtool.c
index e250f49535fa..c1caf7734c3e 100644
--- a/drivers/s390/net/qeth_ethtool.c
+++ b/drivers/s390/net/qeth_ethtool.c
@@ -172,7 +172,7 @@ static void qeth_get_strings(struct net_device *dev, u32 stringset, u8 *data)
 		qeth_add_stat_strings(&data, prefix, card_stats,
 				      CARD_STATS_LEN);
 		for (i = 0; i < card->qdio.no_out_queues; i++) {
-			snprintf(prefix, ETH_GSTRING_LEN, "tx%u ", i);
+			scnprintf(prefix, ETH_GSTRING_LEN, "tx%u ", i);
 			qeth_add_stat_strings(&data, prefix, txq_stats,
 					      TXQ_STATS_LEN);
 		}
@@ -192,8 +192,8 @@ static void qeth_get_drvinfo(struct net_device *dev,
 		sizeof(info->driver));
 	strscpy(info->fw_version, card->info.mcl_level,
 		sizeof(info->fw_version));
-	snprintf(info->bus_info, sizeof(info->bus_info), "%s/%s/%s",
-		 CARD_RDEV_ID(card), CARD_WDEV_ID(card), CARD_DDEV_ID(card));
+	scnprintf(info->bus_info, sizeof(info->bus_info), "%s/%s/%s",
+		  CARD_RDEV_ID(card), CARD_WDEV_ID(card), CARD_DDEV_ID(card));
 }
 
 static void qeth_get_channels(struct net_device *dev,
diff --git a/drivers/s390/net/qeth_l2_main.c b/drivers/s390/net/qeth_l2_main.c
index c6ded3fdd715..9f13ed170a43 100644
--- a/drivers/s390/net/qeth_l2_main.c
+++ b/drivers/s390/net/qeth_l2_main.c
@@ -1255,37 +1255,38 @@ static void qeth_bridge_emit_host_event(struct qeth_card *card,
 
 	switch (evtype) {
 	case anev_reg_unreg:
-		snprintf(str[i], sizeof(str[i]), "BRIDGEDHOST=%s",
-				(code & IPA_ADDR_CHANGE_CODE_REMOVAL)
-				? "deregister" : "register");
+		scnprintf(str[i], sizeof(str[i]), "BRIDGEDHOST=%s",
+			  (code & IPA_ADDR_CHANGE_CODE_REMOVAL)
+			  ? "deregister" : "register");
 		env[i] = str[i]; i++;
 		if (code & IPA_ADDR_CHANGE_CODE_VLANID) {
-			snprintf(str[i], sizeof(str[i]), "VLAN=%d",
-				addr_lnid->lnid);
+			scnprintf(str[i], sizeof(str[i]), "VLAN=%d",
+				  addr_lnid->lnid);
 			env[i] = str[i]; i++;
 		}
 		if (code & IPA_ADDR_CHANGE_CODE_MACADDR) {
-			snprintf(str[i], sizeof(str[i]), "MAC=%pM",
-				addr_lnid->mac);
+			scnprintf(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",
-			token->cssid, token->ssid, token->devnum);
+		scnprintf(str[i], sizeof(str[i]), "NTOK_BUSID=%x.%x.%04x",
+			  token->cssid, token->ssid, token->devnum);
 		env[i] = str[i]; i++;
-		snprintf(str[i], sizeof(str[i]), "NTOK_IID=%02x", token->iid);
+		scnprintf(str[i], sizeof(str[i]), "NTOK_IID=%02x", token->iid);
 		env[i] = str[i]; i++;
-		snprintf(str[i], sizeof(str[i]), "NTOK_CHPID=%02x",
-				token->chpid);
+		scnprintf(str[i], sizeof(str[i]), "NTOK_CHPID=%02x",
+			  token->chpid);
 		env[i] = str[i]; i++;
-		snprintf(str[i], sizeof(str[i]), "NTOK_CHID=%04x", token->chid);
+		scnprintf(str[i], sizeof(str[i]), "NTOK_CHID=%04x",
+			  token->chid);
 		env[i] = str[i]; i++;
 		break;
 	case anev_abort:
-		snprintf(str[i], sizeof(str[i]), "BRIDGEDHOST=abort");
+		scnprintf(str[i], sizeof(str[i]), "BRIDGEDHOST=abort");
 		env[i] = str[i]; i++;
 		break;
 	case anev_reset:
-		snprintf(str[i], sizeof(str[i]), "BRIDGEDHOST=reset");
+		scnprintf(str[i], sizeof(str[i]), "BRIDGEDHOST=reset");
 		env[i] = str[i]; i++;
 		break;
 	}
@@ -1314,17 +1315,17 @@ static void qeth_bridge_state_change_worker(struct work_struct *work)
 		NULL
 	};
 
-	snprintf(env_locrem, sizeof(env_locrem), "BRIDGEPORT=statechange");
-	snprintf(env_role, sizeof(env_role), "ROLE=%s",
-		(data->role == QETH_SBP_ROLE_NONE) ? "none" :
-		(data->role == QETH_SBP_ROLE_PRIMARY) ? "primary" :
-		(data->role == QETH_SBP_ROLE_SECONDARY) ? "secondary" :
-		"<INVALID>");
-	snprintf(env_state, sizeof(env_state), "STATE=%s",
-		(data->state == QETH_SBP_STATE_INACTIVE) ? "inactive" :
-		(data->state == QETH_SBP_STATE_STANDBY) ? "standby" :
-		(data->state == QETH_SBP_STATE_ACTIVE) ? "active" :
-		"<INVALID>");
+	scnprintf(env_locrem, sizeof(env_locrem), "BRIDGEPORT=statechange");
+	scnprintf(env_role, sizeof(env_role), "ROLE=%s",
+		  (data->role == QETH_SBP_ROLE_NONE) ? "none" :
+		  (data->role == QETH_SBP_ROLE_PRIMARY) ? "primary" :
+		  (data->role == QETH_SBP_ROLE_SECONDARY) ? "secondary" :
+		  "<INVALID>");
+	scnprintf(env_state, sizeof(env_state), "STATE=%s",
+		  (data->state == QETH_SBP_STATE_INACTIVE) ? "inactive" :
+		  (data->state == QETH_SBP_STATE_STANDBY) ? "standby" :
+		  (data->state == QETH_SBP_STATE_ACTIVE) ? "active" :
+		  "<INVALID>");
 	kobject_uevent_env(&data->card->gdev->dev.kobj,
 				KOBJ_CHANGE, env);
 	kfree(data);
diff --git a/drivers/s390/net/qeth_l3_main.c b/drivers/s390/net/qeth_l3_main.c
index 1cf4e354693f..af4e60d2917e 100644
--- a/drivers/s390/net/qeth_l3_main.c
+++ b/drivers/s390/net/qeth_l3_main.c
@@ -47,9 +47,9 @@ int qeth_l3_ipaddr_to_string(enum qeth_prot_versions proto, const u8 *addr,
 			     char *buf)
 {
 	if (proto == QETH_PROT_IPV4)
-		return sprintf(buf, "%pI4", addr);
+		return scnprintf(buf, INET_ADDRSTRLEN, "%pI4", addr);
 	else
-		return sprintf(buf, "%pI6", addr);
+		return scnprintf(buf, INET6_ADDRSTRLEN, "%pI6", addr);
 }
 
 static struct qeth_ipaddr *qeth_l3_find_addr_by_ip(struct qeth_card *card,
diff --git a/drivers/s390/net/qeth_l3_sys.c b/drivers/s390/net/qeth_l3_sys.c
index f0f8adaa2f05..cce6e621cd88 100644
--- a/drivers/s390/net/qeth_l3_sys.c
+++ b/drivers/s390/net/qeth_l3_sys.c
@@ -252,8 +252,8 @@ static ssize_t qeth_l3_dev_hsuid_store(struct device *dev,
 		goto out;
 	}
 
-	snprintf(card->options.hsuid, sizeof(card->options.hsuid),
-		 "%-8s", tmp);
+	scnprintf(card->options.hsuid, sizeof(card->options.hsuid),
+		  "%-8s", tmp);
 	ASCEBC(card->options.hsuid, 8);
 	memcpy(card->dev->perm_addr, card->options.hsuid, 9);
 
-- 
2.37.2

Re: [PATCH net-next 4/4] s390/qeth: Convert sprintf/snprintf to scnprintf

[Simon Horman]

On Mon, Feb 06, 2023 at 06:27:54PM +0100, Alexandra Winter wrote:
> From: Thorsten Winkler <twinkler@linux.ibm.com>
> 
> This LWN article explains the rationale for this change
> https: //lwn.net/Articles/69419/

https://lwn.net/Articles/69419/

> Ie. snprintf() returns what *would* be the resulting length,
> while scnprintf() returns the actual length.

Ok, but in most cases in this patch the return value is not checked.
Is there any value in this change in those cases?

> Reported-by: Jules Irenge <jbi.octave@gmail.com>
> Reviewed-by: Alexandra Winkler <wintera@linux.ibm.com>

s/Winkler/Winter/ ?

> Signed-off-by: Thorsten Winkler <twinkler@linux.ibm.com>
> Signed-off-by: Alexandra Winter <wintera@linux.ibm.com>

...

> diff --git a/drivers/s390/net/qeth_l3_main.c b/drivers/s390/net/qeth_l3_main.c
> index 1cf4e354693f..af4e60d2917e 100644
> --- a/drivers/s390/net/qeth_l3_main.c
> +++ b/drivers/s390/net/qeth_l3_main.c
> @@ -47,9 +47,9 @@ int qeth_l3_ipaddr_to_string(enum qeth_prot_versions proto, const u8 *addr,
>  			     char *buf)
>  {
>  	if (proto == QETH_PROT_IPV4)
> -		return sprintf(buf, "%pI4", addr);
> +		return scnprintf(buf, INET_ADDRSTRLEN, "%pI4", addr);
>  	else
> -		return sprintf(buf, "%pI6", addr);
> +		return scnprintf(buf, INET6_ADDRSTRLEN, "%pI6", addr);
>  }


This seems to be the once case where the return value is not ignored.

Of the 4 callers of qeth_l3_ipaddr_to_string, two don't ignore the return
value. And I agree in those cases this change seems correct.

However, amongst other usages of the return value,
those callers also check for a return < 0 from this function.
Can that occur, in the sprintf or scnprintf case?

RE: [PATCH net-next 4/4] s390/qeth: Convert sprintf/snprintf to scnprintf

[David Laight]

From: Simon Horman
> Sent: 07 February 2023 15:43
...
> However, amongst other usages of the return value,
> those callers also check for a return < 0 from this function.
> Can that occur, in the sprintf or scnprintf case?

That rather depends on what happens with calls like:
	snprintf(NULL, 0, "*%s%*s", MAX_INT, "", MAX_INT, "");

That is a whole bag of worms you don't want to put your hand into.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH net-next 4/4] s390/qeth: Convert sprintf/snprintf to scnprintf

[Simon Horman]

On Wed, Feb 08, 2023 at 02:37:32PM +0000, David Laight wrote:
> [You don't often get email from david.laight@aculab.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> From: Simon Horman
> > Sent: 07 February 2023 15:43
> ...
> > However, amongst other usages of the return value,
> > those callers also check for a return < 0 from this function.
> > Can that occur, in the sprintf or scnprintf case?
> 
> That rather depends on what happens with calls like:
>         snprintf(NULL, 0, "*%s%*s", MAX_INT, "", MAX_INT, "");
> 
> That is a whole bag of worms you don't want to put your hand into.

Ok :)

Re: [PATCH net-next 4/4] s390/qeth: Convert sprintf/snprintf to scnprintf

[Alexandra Winter]

On 07.02.23 16:42, Simon Horman wrote:
> On Mon, Feb 06, 2023 at 06:27:54PM +0100, Alexandra Winter wrote:
>> From: Thorsten Winkler <twinkler@linux.ibm.com>
>>
>> This LWN article explains the rationale for this change
>> https: //lwn.net/Articles/69419/
> 
> https://lwn.net/Articles/69419/
> 
>> Ie. snprintf() returns what *would* be the resulting length,
>> while scnprintf() returns the actual length.
> 
> Ok, but in most cases in this patch the return value is not checked.
> Is there any value in this change in those cases?
> 

Jules Irenge reported a coccinnelle warning to use scnprintf in 
show() functions [1]. (Thorsten Winkler changed these instances to
sysfs_emit in patch 3 of this series.)
We read the article as a call to implement the plan to upgrade the kernel
to the newer *scnprintf functions. Is that not intended?

I totally agree, that in these cases no real problem was fixed, it is
more of a style improvement.

[1] https://lore.kernel.org/netdev/YzHyniCyf+G%2F2xI8@fedora/T/

>> Reported-by: Jules Irenge <jbi.octave@gmail.com>
>> Reviewed-by: Alexandra Winkler <wintera@linux.ibm.com>
> 
> s/Winkler/Winter/ ?

Of course. Wow, you have good eyes!

> 
>> Signed-off-by: Thorsten Winkler <twinkler@linux.ibm.com>
>> Signed-off-by: Alexandra Winter <wintera@linux.ibm.com>
> 
> ...
> 
>> diff --git a/drivers/s390/net/qeth_l3_main.c b/drivers/s390/net/qeth_l3_main.c
>> index 1cf4e354693f..af4e60d2917e 100644
>> --- a/drivers/s390/net/qeth_l3_main.c
>> +++ b/drivers/s390/net/qeth_l3_main.c
>> @@ -47,9 +47,9 @@ int qeth_l3_ipaddr_to_string(enum qeth_prot_versions proto, const u8 *addr,
>>  			     char *buf)
>>  {
>>  	if (proto == QETH_PROT_IPV4)
>> -		return sprintf(buf, "%pI4", addr);
>> +		return scnprintf(buf, INET_ADDRSTRLEN, "%pI4", addr);
>>  	else
>> -		return sprintf(buf, "%pI6", addr);
>> +		return scnprintf(buf, INET6_ADDRSTRLEN, "%pI6", addr);
>>  }
> 
> 
> This seems to be the once case where the return value is not ignored.
> 
> Of the 4 callers of qeth_l3_ipaddr_to_string, two don't ignore the return
> value. And I agree in those cases this change seems correct.
> 
> However, amongst other usages of the return value,
> those callers also check for a return < 0 from this function.
> Can that occur, in the sprintf or scnprintf case?

I was under the impression this was a safeguard against a bad address format,
but I tried it out and it never resulted in a negative return.
Thanks a lot for pointing this out, we can further simplify patch 3 with that.

Re: [PATCH net-next 4/4] s390/qeth: Convert sprintf/snprintf to scnprintf

[Simon Horman]

On Wed, Feb 08, 2023 at 07:19:29PM +0100, Alexandra Winter wrote:
> 
> 
> On 07.02.23 16:42, Simon Horman wrote:
> > On Mon, Feb 06, 2023 at 06:27:54PM +0100, Alexandra Winter wrote:
> >> From: Thorsten Winkler <twinkler@linux.ibm.com>
> >>
> >> This LWN article explains the rationale for this change
> >> https: //lwn.net/Articles/69419/
> > 
> > https://lwn.net/Articles/69419/
> > 
> >> Ie. snprintf() returns what *would* be the resulting length,
> >> while scnprintf() returns the actual length.
> > 
> > Ok, but in most cases in this patch the return value is not checked.
> > Is there any value in this change in those cases?
> > 
> 
> Jules Irenge reported a coccinnelle warning to use scnprintf in 
> show() functions [1]. (Thorsten Winkler changed these instances to
> sysfs_emit in patch 3 of this series.)
> We read the article as a call to implement the plan to upgrade the kernel
> to the newer *scnprintf functions. Is that not intended?
>
> I totally agree, that in these cases no real problem was fixed, it is
> more of a style improvement.

My feeling is that it isn't an improvement and therefore probably
best not done. But that is just my opinion.

> [1] https://lore.kernel.org/netdev/YzHyniCyf+G%2F2xI8@fedora/T/
> 
> >> Reported-by: Jules Irenge <jbi.octave@gmail.com>
> >> Reviewed-by: Alexandra Winkler <wintera@linux.ibm.com>
> > 
> > s/Winkler/Winter/ ?
> 
> Of course. Wow, you have good eyes!

Only on my good days.

> >> Signed-off-by: Thorsten Winkler <twinkler@linux.ibm.com>
> >> Signed-off-by: Alexandra Winter <wintera@linux.ibm.com>
> > 
> > ...
> > 
> >> diff --git a/drivers/s390/net/qeth_l3_main.c b/drivers/s390/net/qeth_l3_main.c
> >> index 1cf4e354693f..af4e60d2917e 100644
> >> --- a/drivers/s390/net/qeth_l3_main.c
> >> +++ b/drivers/s390/net/qeth_l3_main.c
> >> @@ -47,9 +47,9 @@ int qeth_l3_ipaddr_to_string(enum qeth_prot_versions proto, const u8 *addr,
> >>  			     char *buf)
> >>  {
> >>  	if (proto == QETH_PROT_IPV4)
> >> -		return sprintf(buf, "%pI4", addr);
> >> +		return scnprintf(buf, INET_ADDRSTRLEN, "%pI4", addr);
> >>  	else
> >> -		return sprintf(buf, "%pI6", addr);
> >> +		return scnprintf(buf, INET6_ADDRSTRLEN, "%pI6", addr);
> >>  }
> > 
> > 
> > This seems to be the once case where the return value is not ignored.
> > 
> > Of the 4 callers of qeth_l3_ipaddr_to_string, two don't ignore the return
> > value. And I agree in those cases this change seems correct.
> > 
> > However, amongst other usages of the return value,
> > those callers also check for a return < 0 from this function.
> > Can that occur, in the sprintf or scnprintf case?
> 
> I was under the impression this was a safeguard against a bad address format,
> but I tried it out and it never resulted in a negative return.
> Thanks a lot for pointing this out, we can further simplify patch 3 with that.

The advice elsewhere in this thread is that perhaps leaving this as-is may
be best after all.

* https://lore.kernel.org/netdev/63c6825fc2c94ad19ac7de93a6f151f6@AcuMS.aculab.com/