* [PATCH 0/4] xtensa: iss: clean up network driver
@ 2022-04-10 0:56 Max Filippov
2022-04-10 0:56 ` [PATCH 1/4] xtensa: iss: drop opened_list logic from the " Max Filippov
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Max Filippov @ 2022-04-10 0:56 UTC (permalink / raw)
To: linux-xtensa
Cc: Chris Zankel, linux-kernel, Duoming Zhou, Jakub Kicinski,
Greg Kroah-Hartman, David S. Miller, Max Filippov
Hello,
this series cleans up ISS network driver and fixes potential deadlock
reported by Duoming Zhou <duoming@zju.edu.cn> here:
https://lore.kernel.org/lkml/20220409055135.54921-1-duoming@zju.edu.cn/
Max Filippov (4):
xtensa: iss: drop opened_list logic from the network driver
xtensa: iss: replace iss_net_set_mac with eth_mac_addr
xtensa: iss: clean up per-device locking in network driver
xtensa: iss: extract and constify network callbacks
arch/xtensa/platforms/iss/network.c | 150 +++++++++++-----------------
1 file changed, 59 insertions(+), 91 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/4] xtensa: iss: drop opened_list logic from the network driver
2022-04-10 0:56 [PATCH 0/4] xtensa: iss: clean up network driver Max Filippov
@ 2022-04-10 0:56 ` Max Filippov
2022-04-10 0:56 ` [PATCH 2/4] xtensa: iss: replace iss_net_set_mac with eth_mac_addr Max Filippov
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Max Filippov @ 2022-04-10 0:56 UTC (permalink / raw)
To: linux-xtensa
Cc: Chris Zankel, linux-kernel, Duoming Zhou, Jakub Kicinski,
Greg Kroah-Hartman, David S. Miller, Max Filippov
opened_list is used to poll all opened devices in the timer callback,
but there's individual timer that is associated with each device.
Drop opened_list and only poll the device that is associated with the
timer in the timer callback.
Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
arch/xtensa/platforms/iss/network.c | 53 ++++++++---------------------
1 file changed, 14 insertions(+), 39 deletions(-)
diff --git a/arch/xtensa/platforms/iss/network.c b/arch/xtensa/platforms/iss/network.c
index be3aaaad8bee..409def002f55 100644
--- a/arch/xtensa/platforms/iss/network.c
+++ b/arch/xtensa/platforms/iss/network.c
@@ -38,9 +38,6 @@
#define ISS_NET_TIMER_VALUE (HZ / 10)
-static DEFINE_SPINLOCK(opened_lock);
-static LIST_HEAD(opened);
-
static DEFINE_SPINLOCK(devices_lock);
static LIST_HEAD(devices);
@@ -63,7 +60,6 @@ struct tuntap_info {
struct iss_net_private {
struct list_head device_list;
- struct list_head opened_list;
spinlock_t lock;
struct net_device *dev;
@@ -311,38 +307,28 @@ static int iss_net_rx(struct net_device *dev)
return pkt_len;
}
-static int iss_net_poll(void)
+static int iss_net_poll(struct iss_net_private *lp)
{
- struct list_head *ele;
int err, ret = 0;
- spin_lock(&opened_lock);
-
- list_for_each(ele, &opened) {
- struct iss_net_private *lp;
-
- lp = list_entry(ele, struct iss_net_private, opened_list);
-
- if (!netif_running(lp->dev))
- break;
+ if (!netif_running(lp->dev))
+ return 0;
- spin_lock(&lp->lock);
+ spin_lock(&lp->lock);
- while ((err = iss_net_rx(lp->dev)) > 0)
- ret++;
+ while ((err = iss_net_rx(lp->dev)) > 0)
+ ret++;
- spin_unlock(&lp->lock);
+ spin_unlock(&lp->lock);
- if (err < 0) {
- pr_err("Device '%s' read returned %d, shutting it down\n",
- lp->dev->name, err);
- dev_close(lp->dev);
- } else {
- /* FIXME reactivate_fd(lp->fd, ISS_ETH_IRQ); */
- }
+ if (err < 0) {
+ pr_err("Device '%s' read returned %d, shutting it down\n",
+ lp->dev->name, err);
+ dev_close(lp->dev);
+ } else {
+ /* FIXME reactivate_fd(lp->fd, ISS_ETH_IRQ); */
}
- spin_unlock(&opened_lock);
return ret;
}
@@ -351,7 +337,7 @@ static void iss_net_timer(struct timer_list *t)
{
struct iss_net_private *lp = from_timer(lp, t, timer);
- iss_net_poll();
+ iss_net_poll(lp);
spin_lock(&lp->lock);
mod_timer(&lp->timer, jiffies + lp->timer_val);
spin_unlock(&lp->lock);
@@ -378,12 +364,6 @@ static int iss_net_open(struct net_device *dev)
while ((err = iss_net_rx(dev)) > 0)
;
- spin_unlock_bh(&lp->lock);
- spin_lock_bh(&opened_lock);
- list_add(&lp->opened_list, &opened);
- spin_unlock_bh(&opened_lock);
- spin_lock_bh(&lp->lock);
-
timer_setup(&lp->timer, iss_net_timer, 0);
lp->timer_val = ISS_NET_TIMER_VALUE;
mod_timer(&lp->timer, jiffies + lp->timer_val);
@@ -399,10 +379,6 @@ static int iss_net_close(struct net_device *dev)
netif_stop_queue(dev);
spin_lock_bh(&lp->lock);
- spin_lock(&opened_lock);
- list_del(&opened);
- spin_unlock(&opened_lock);
-
del_timer_sync(&lp->timer);
lp->tp.close(lp);
@@ -520,7 +496,6 @@ static int iss_net_configure(int index, char *init)
lp = netdev_priv(dev);
*lp = (struct iss_net_private) {
.device_list = LIST_HEAD_INIT(lp->device_list),
- .opened_list = LIST_HEAD_INIT(lp->opened_list),
.dev = dev,
.index = index,
};
--
2.30.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/4] xtensa: iss: replace iss_net_set_mac with eth_mac_addr
2022-04-10 0:56 [PATCH 0/4] xtensa: iss: clean up network driver Max Filippov
2022-04-10 0:56 ` [PATCH 1/4] xtensa: iss: drop opened_list logic from the " Max Filippov
@ 2022-04-10 0:56 ` Max Filippov
2022-04-10 0:56 ` [PATCH 3/4] xtensa: iss: clean up per-device locking in network driver Max Filippov
2022-04-10 0:56 ` [PATCH 4/4] xtensa: iss: extract and constify network callbacks Max Filippov
3 siblings, 0 replies; 5+ messages in thread
From: Max Filippov @ 2022-04-10 0:56 UTC (permalink / raw)
To: linux-xtensa
Cc: Chris Zankel, linux-kernel, Duoming Zhou, Jakub Kicinski,
Greg Kroah-Hartman, David S. Miller, Max Filippov
iss_net_set_mac is just a copy of eth_mac_addr with pointless locking.
Drop this function and replace it with eth_mac_addr.
Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
arch/xtensa/platforms/iss/network.c | 15 +--------------
1 file changed, 1 insertion(+), 14 deletions(-)
diff --git a/arch/xtensa/platforms/iss/network.c b/arch/xtensa/platforms/iss/network.c
index 409def002f55..e9454652551b 100644
--- a/arch/xtensa/platforms/iss/network.c
+++ b/arch/xtensa/platforms/iss/network.c
@@ -436,19 +436,6 @@ static void iss_net_tx_timeout(struct net_device *dev, unsigned int txqueue)
{
}
-static int iss_net_set_mac(struct net_device *dev, void *addr)
-{
- struct iss_net_private *lp = netdev_priv(dev);
- struct sockaddr *hwaddr = addr;
-
- if (!is_valid_ether_addr(hwaddr->sa_data))
- return -EADDRNOTAVAIL;
- spin_lock_bh(&lp->lock);
- eth_hw_addr_set(dev, hwaddr->sa_data);
- spin_unlock_bh(&lp->lock);
- return 0;
-}
-
static int iss_net_change_mtu(struct net_device *dev, int new_mtu)
{
return -EINVAL;
@@ -474,7 +461,7 @@ static const struct net_device_ops iss_netdev_ops = {
.ndo_start_xmit = iss_net_start_xmit,
.ndo_validate_addr = eth_validate_addr,
.ndo_change_mtu = iss_net_change_mtu,
- .ndo_set_mac_address = iss_net_set_mac,
+ .ndo_set_mac_address = eth_mac_addr,
.ndo_tx_timeout = iss_net_tx_timeout,
.ndo_set_rx_mode = iss_net_set_multicast_list,
};
--
2.30.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 3/4] xtensa: iss: clean up per-device locking in network driver
2022-04-10 0:56 [PATCH 0/4] xtensa: iss: clean up network driver Max Filippov
2022-04-10 0:56 ` [PATCH 1/4] xtensa: iss: drop opened_list logic from the " Max Filippov
2022-04-10 0:56 ` [PATCH 2/4] xtensa: iss: replace iss_net_set_mac with eth_mac_addr Max Filippov
@ 2022-04-10 0:56 ` Max Filippov
2022-04-10 0:56 ` [PATCH 4/4] xtensa: iss: extract and constify network callbacks Max Filippov
3 siblings, 0 replies; 5+ messages in thread
From: Max Filippov @ 2022-04-10 0:56 UTC (permalink / raw)
To: linux-xtensa
Cc: Chris Zankel, linux-kernel, Duoming Zhou, Jakub Kicinski,
Greg Kroah-Hartman, David S. Miller, Max Filippov
Per-device locking in the ISS network driver is used to protect poll
timer and stats updates. Stat collection is not protected.
Remove per-device locking everywhere except the stats updates. Replace
ndo_get_stats callback with ndo_get_stats64 and use proper locking there
as well.
As a side effect this fixes possible deadlock between iss_net_close
and iss_net_timer.
Reported by: Duoming Zhou <duoming@zju.edu.cn>
Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
arch/xtensa/platforms/iss/network.c | 39 +++++++++++++----------------
1 file changed, 18 insertions(+), 21 deletions(-)
diff --git a/arch/xtensa/platforms/iss/network.c b/arch/xtensa/platforms/iss/network.c
index e9454652551b..13802babba17 100644
--- a/arch/xtensa/platforms/iss/network.c
+++ b/arch/xtensa/platforms/iss/network.c
@@ -65,7 +65,7 @@ struct iss_net_private {
struct net_device *dev;
struct platform_device pdev;
struct timer_list tl;
- struct net_device_stats stats;
+ struct rtnl_link_stats64 stats;
struct timer_list timer;
unsigned int timer_val;
@@ -281,7 +281,9 @@ static int iss_net_rx(struct net_device *dev)
skb = dev_alloc_skb(dev->mtu + 2 + ETH_HEADER_OTHER);
if (skb == NULL) {
+ spin_lock_bh(&lp->lock);
lp->stats.rx_dropped++;
+ spin_unlock_bh(&lp->lock);
return 0;
}
@@ -298,8 +300,10 @@ static int iss_net_rx(struct net_device *dev)
skb_trim(skb, pkt_len);
skb->protocol = lp->tp.protocol(skb);
+ spin_lock_bh(&lp->lock);
lp->stats.rx_bytes += skb->len;
lp->stats.rx_packets++;
+ spin_unlock_bh(&lp->lock);
netif_rx(skb);
return pkt_len;
}
@@ -314,13 +318,9 @@ static int iss_net_poll(struct iss_net_private *lp)
if (!netif_running(lp->dev))
return 0;
- spin_lock(&lp->lock);
-
while ((err = iss_net_rx(lp->dev)) > 0)
ret++;
- spin_unlock(&lp->lock);
-
if (err < 0) {
pr_err("Device '%s' read returned %d, shutting it down\n",
lp->dev->name, err);
@@ -338,9 +338,7 @@ static void iss_net_timer(struct timer_list *t)
struct iss_net_private *lp = from_timer(lp, t, timer);
iss_net_poll(lp);
- spin_lock(&lp->lock);
mod_timer(&lp->timer, jiffies + lp->timer_val);
- spin_unlock(&lp->lock);
}
@@ -349,11 +347,9 @@ static int iss_net_open(struct net_device *dev)
struct iss_net_private *lp = netdev_priv(dev);
int err;
- spin_lock_bh(&lp->lock);
-
err = lp->tp.open(lp);
if (err < 0)
- goto out;
+ return err;
netif_start_queue(dev);
@@ -368,22 +364,17 @@ static int iss_net_open(struct net_device *dev)
lp->timer_val = ISS_NET_TIMER_VALUE;
mod_timer(&lp->timer, jiffies + lp->timer_val);
-out:
- spin_unlock_bh(&lp->lock);
return err;
}
static int iss_net_close(struct net_device *dev)
{
struct iss_net_private *lp = netdev_priv(dev);
- netif_stop_queue(dev);
- spin_lock_bh(&lp->lock);
+ netif_stop_queue(dev);
del_timer_sync(&lp->timer);
-
lp->tp.close(lp);
- spin_unlock_bh(&lp->lock);
return 0;
}
@@ -393,13 +384,14 @@ static int iss_net_start_xmit(struct sk_buff *skb, struct net_device *dev)
int len;
netif_stop_queue(dev);
- spin_lock_bh(&lp->lock);
len = lp->tp.write(lp, &skb);
if (len == skb->len) {
+ spin_lock_bh(&lp->lock);
lp->stats.tx_packets++;
lp->stats.tx_bytes += skb->len;
+ spin_unlock_bh(&lp->lock);
netif_trans_update(dev);
netif_start_queue(dev);
@@ -408,24 +400,29 @@ static int iss_net_start_xmit(struct sk_buff *skb, struct net_device *dev)
} else if (len == 0) {
netif_start_queue(dev);
+ spin_lock_bh(&lp->lock);
lp->stats.tx_dropped++;
+ spin_unlock_bh(&lp->lock);
} else {
netif_start_queue(dev);
pr_err("%s: %s failed(%d)\n", dev->name, __func__, len);
}
- spin_unlock_bh(&lp->lock);
dev_kfree_skb(skb);
return NETDEV_TX_OK;
}
-static struct net_device_stats *iss_net_get_stats(struct net_device *dev)
+static void iss_net_get_stats64(struct net_device *dev,
+ struct rtnl_link_stats64 *stats)
{
struct iss_net_private *lp = netdev_priv(dev);
- return &lp->stats;
+
+ spin_lock_bh(&lp->lock);
+ *stats = lp->stats;
+ spin_unlock_bh(&lp->lock);
}
static void iss_net_set_multicast_list(struct net_device *dev)
@@ -457,7 +454,7 @@ static int driver_registered;
static const struct net_device_ops iss_netdev_ops = {
.ndo_open = iss_net_open,
.ndo_stop = iss_net_close,
- .ndo_get_stats = iss_net_get_stats,
+ .ndo_get_stats64 = iss_net_get_stats64,
.ndo_start_xmit = iss_net_start_xmit,
.ndo_validate_addr = eth_validate_addr,
.ndo_change_mtu = iss_net_change_mtu,
--
2.30.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 4/4] xtensa: iss: extract and constify network callbacks
2022-04-10 0:56 [PATCH 0/4] xtensa: iss: clean up network driver Max Filippov
` (2 preceding siblings ...)
2022-04-10 0:56 ` [PATCH 3/4] xtensa: iss: clean up per-device locking in network driver Max Filippov
@ 2022-04-10 0:56 ` Max Filippov
3 siblings, 0 replies; 5+ messages in thread
From: Max Filippov @ 2022-04-10 0:56 UTC (permalink / raw)
To: linux-xtensa
Cc: Chris Zankel, linux-kernel, Duoming Zhou, Jakub Kicinski,
Greg Kroah-Hartman, David S. Miller, Max Filippov
Instead of storing pointers to callback functions in the
struct iss_net_private::tp move them to struct struct iss_net_ops and
store a const pointer to it. Make static const tuntap_ops structure with
tuntap callbacks and initialize tp.net_ops with it in the tuntap_probe.
Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
arch/xtensa/platforms/iss/network.c | 47 +++++++++++++++++------------
1 file changed, 28 insertions(+), 19 deletions(-)
diff --git a/arch/xtensa/platforms/iss/network.c b/arch/xtensa/platforms/iss/network.c
index 13802babba17..fd84d4891758 100644
--- a/arch/xtensa/platforms/iss/network.c
+++ b/arch/xtensa/platforms/iss/network.c
@@ -56,6 +56,17 @@ struct tuntap_info {
/* ------------------------------------------------------------------------- */
+struct iss_net_private;
+
+struct iss_net_ops {
+ int (*open)(struct iss_net_private *lp);
+ void (*close)(struct iss_net_private *lp);
+ int (*read)(struct iss_net_private *lp, struct sk_buff **skb);
+ int (*write)(struct iss_net_private *lp, struct sk_buff **skb);
+ unsigned short (*protocol)(struct sk_buff *skb);
+ int (*poll)(struct iss_net_private *lp);
+};
+
/* This structure contains out private information for the driver. */
struct iss_net_private {
@@ -78,12 +89,7 @@ struct iss_net_private {
struct tuntap_info tuntap;
} info;
- int (*open)(struct iss_net_private *lp);
- void (*close)(struct iss_net_private *lp);
- int (*read)(struct iss_net_private *lp, struct sk_buff **skb);
- int (*write)(struct iss_net_private *lp, struct sk_buff **skb);
- unsigned short (*protocol)(struct sk_buff *skb);
- int (*poll)(struct iss_net_private *lp);
+ const struct iss_net_ops *net_ops;
} tp;
};
@@ -211,6 +217,15 @@ static int tuntap_poll(struct iss_net_private *lp)
return simc_poll(lp->tp.info.tuntap.fd);
}
+static const struct iss_net_ops tuntap_ops = {
+ .open = tuntap_open,
+ .close = tuntap_close,
+ .read = tuntap_read,
+ .write = tuntap_write,
+ .protocol = tuntap_protocol,
+ .poll = tuntap_poll,
+};
+
/*
* ethX=tuntap,[mac address],device name
*/
@@ -253,13 +268,7 @@ static int tuntap_probe(struct iss_net_private *lp, int index, char *init)
lp->mtu = TRANSPORT_TUNTAP_MTU;
lp->tp.info.tuntap.fd = -1;
-
- lp->tp.open = tuntap_open;
- lp->tp.close = tuntap_close;
- lp->tp.read = tuntap_read;
- lp->tp.write = tuntap_write;
- lp->tp.protocol = tuntap_protocol;
- lp->tp.poll = tuntap_poll;
+ lp->tp.net_ops = &tuntap_ops;
return 1;
}
@@ -274,7 +283,7 @@ static int iss_net_rx(struct net_device *dev)
/* Check if there is any new data. */
- if (lp->tp.poll(lp) == 0)
+ if (lp->tp.net_ops->poll(lp) == 0)
return 0;
/* Try to allocate memory, if it fails, try again next round. */
@@ -293,12 +302,12 @@ static int iss_net_rx(struct net_device *dev)
skb->dev = dev;
skb_reset_mac_header(skb);
- pkt_len = lp->tp.read(lp, &skb);
+ pkt_len = lp->tp.net_ops->read(lp, &skb);
skb_put(skb, pkt_len);
if (pkt_len > 0) {
skb_trim(skb, pkt_len);
- skb->protocol = lp->tp.protocol(skb);
+ skb->protocol = lp->tp.net_ops->protocol(skb);
spin_lock_bh(&lp->lock);
lp->stats.rx_bytes += skb->len;
@@ -347,7 +356,7 @@ static int iss_net_open(struct net_device *dev)
struct iss_net_private *lp = netdev_priv(dev);
int err;
- err = lp->tp.open(lp);
+ err = lp->tp.net_ops->open(lp);
if (err < 0)
return err;
@@ -373,7 +382,7 @@ static int iss_net_close(struct net_device *dev)
netif_stop_queue(dev);
del_timer_sync(&lp->timer);
- lp->tp.close(lp);
+ lp->tp.net_ops->close(lp);
return 0;
}
@@ -385,7 +394,7 @@ static int iss_net_start_xmit(struct sk_buff *skb, struct net_device *dev)
netif_stop_queue(dev);
- len = lp->tp.write(lp, &skb);
+ len = lp->tp.net_ops->write(lp, &skb);
if (len == skb->len) {
spin_lock_bh(&lp->lock);
--
2.30.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-04-10 0:57 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-10 0:56 [PATCH 0/4] xtensa: iss: clean up network driver Max Filippov
2022-04-10 0:56 ` [PATCH 1/4] xtensa: iss: drop opened_list logic from the " Max Filippov
2022-04-10 0:56 ` [PATCH 2/4] xtensa: iss: replace iss_net_set_mac with eth_mac_addr Max Filippov
2022-04-10 0:56 ` [PATCH 3/4] xtensa: iss: clean up per-device locking in network driver Max Filippov
2022-04-10 0:56 ` [PATCH 4/4] xtensa: iss: extract and constify network callbacks Max Filippov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).