* [RFC PATCH 0/1] net: arcnet: Fix RESET sequence
@ 2020-12-22 9:03 Ahmed S. Darwish
2020-12-22 9:03 ` [RFC PATCH 1/1] net: arcnet: Fix RESET flag handling Ahmed S. Darwish
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Ahmed S. Darwish @ 2020-12-22 9:03 UTC (permalink / raw)
To: Michael Grzeschik, David S. Miller, Jakub Kicinski, netdev
Cc: LKML, Thomas Gleixner, Sebastian A. Siewior, Ahmed S. Darwish
Folks,
At drivers/net/arcnet/arcnet.c, there is:
irqreturn_t arcnet_interrupt(int irq, void *dev_id)
{
...
if (status & RESETflag) {
arcnet_close(dev);
arcnet_open(dev);
}
...
}
struct net_device_ops arcnet_netdev_ops = {
.ndo_open = arcnet_open,
.ndo_stop = arcnet_close,
...
};
which is wrong, in many ways:
1) In general, interrupt handlers should never call ->ndo_stop() and
->ndo_open() functions. They are usually full of blocking calls and
other methods that are expected to be called only from drivers
init/exit code paths.
2) arcnet_close() contains a del_timer_sync(). If the irq handler
interrupts the to-be-deleted timer then call del_timer_sync(), it
will just loop forever.
3) arcnet_close() also calls tasklet_kill(), which has a warning if
called from irq context.
4) For device reset, the sequence "arcnet_close(); arcnet_open();" is
not complete. Some children arcnet drivers have special init/exit
code sequences, which then embed a call to arcnet_open() and
arcnet_close() accordingly. Check drivers/net/arcnet/com20020.c.
Included is an RFC patch to fix the points above: if the RESET flag is
encountered, a workqueue is scheduled to run the generic reset sequence.
Note: Only compile-tested, as I do not have the hardware in question.
Thanks,
8<--------------
Ahmed S. Darwish (1):
net: arcnet: Fix RESET flag handling
drivers/net/arcnet/arc-rimi.c | 4 +-
drivers/net/arcnet/arcdevice.h | 6 +++
drivers/net/arcnet/arcnet.c | 69 +++++++++++++++++++++++++++++--
drivers/net/arcnet/com20020-isa.c | 4 +-
drivers/net/arcnet/com20020-pci.c | 2 +-
drivers/net/arcnet/com20020_cs.c | 2 +-
drivers/net/arcnet/com90io.c | 4 +-
drivers/net/arcnet/com90xx.c | 4 +-
8 files changed, 81 insertions(+), 14 deletions(-)
base-commit: 2c85ebc57b3e1817b6ce1a6b703928e113a90442
--
2.29.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFC PATCH 1/1] net: arcnet: Fix RESET flag handling
2020-12-22 9:03 [RFC PATCH 0/1] net: arcnet: Fix RESET sequence Ahmed S. Darwish
@ 2020-12-22 9:03 ` Ahmed S. Darwish
2021-01-11 10:09 ` [RFC PATCH 0/1] net: arcnet: Fix RESET sequence Sebastian A. Siewior
2021-01-11 13:54 ` Ahmed S. Darwish
2 siblings, 0 replies; 6+ messages in thread
From: Ahmed S. Darwish @ 2020-12-22 9:03 UTC (permalink / raw)
To: Michael Grzeschik, David S. Miller, Jakub Kicinski, netdev
Cc: LKML, Thomas Gleixner, Sebastian A. Siewior, Ahmed S. Darwish
The main arcnet interrupt handler calls arcnet_close() then
arcnet_open(), if the RESET status flag is encountered.
This is invalid:
1) In general, interrupt handlers should never call ->ndo_stop() and
->ndo_open() functions. They are usually full of blocking calls and
other methods that are expected to be called only from drivers
init and exit code paths.
2) arcnet_close() contains a del_timer_sync(). If the irq handler
interrupts the to-be-deleted timer, del_timer_sync() will just loop
forever.
3) arcnet_close() also calls tasklet_kill(), which has a warning if
called from irq context.
4) For device reset, the sequence "arcnet_close(); arcnet_open();" is
not complete. Some children arcnet drivers have special init/exit
code sequences, which then embed a call to arcnet_open() and
arcnet_close() accordingly. Check drivers/net/arcnet/com20020.c.
Run the device RESET sequence from a scheduled workqueue instead.
Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
---
drivers/net/arcnet/arc-rimi.c | 4 +-
drivers/net/arcnet/arcdevice.h | 6 +++
drivers/net/arcnet/arcnet.c | 69 +++++++++++++++++++++++++++++--
drivers/net/arcnet/com20020-isa.c | 4 +-
drivers/net/arcnet/com20020-pci.c | 2 +-
drivers/net/arcnet/com20020_cs.c | 2 +-
drivers/net/arcnet/com90io.c | 4 +-
drivers/net/arcnet/com90xx.c | 4 +-
8 files changed, 81 insertions(+), 14 deletions(-)
diff --git a/drivers/net/arcnet/arc-rimi.c b/drivers/net/arcnet/arc-rimi.c
index 98df38fe553c..12d085405bd0 100644
--- a/drivers/net/arcnet/arc-rimi.c
+++ b/drivers/net/arcnet/arc-rimi.c
@@ -332,7 +332,7 @@ static int __init arc_rimi_init(void)
dev->irq = 9;
if (arcrimi_probe(dev)) {
- free_netdev(dev);
+ free_arcdev(dev);
return -EIO;
}
@@ -349,7 +349,7 @@ static void __exit arc_rimi_exit(void)
iounmap(lp->mem_start);
release_mem_region(dev->mem_start, dev->mem_end - dev->mem_start + 1);
free_irq(dev->irq, dev);
- free_netdev(dev);
+ free_arcdev(dev);
}
#ifndef MODULE
diff --git a/drivers/net/arcnet/arcdevice.h b/drivers/net/arcnet/arcdevice.h
index 22a49c6d7ae6..5d4a4c7efbbf 100644
--- a/drivers/net/arcnet/arcdevice.h
+++ b/drivers/net/arcnet/arcdevice.h
@@ -298,6 +298,10 @@ struct arcnet_local {
int excnak_pending; /* We just got an excesive nak interrupt */
+ /* RESET flag handling */
+ int reset_in_progress;
+ struct work_struct reset_work;
+
struct {
uint16_t sequence; /* sequence number (incs with each packet) */
__be16 aborted_seq;
@@ -350,7 +354,9 @@ void arcnet_dump_skb(struct net_device *dev, struct sk_buff *skb, char *desc)
void arcnet_unregister_proto(struct ArcProto *proto);
irqreturn_t arcnet_interrupt(int irq, void *dev_id);
+
struct net_device *alloc_arcdev(const char *name);
+void free_arcdev(struct net_device *dev);
int arcnet_open(struct net_device *dev);
int arcnet_close(struct net_device *dev);
diff --git a/drivers/net/arcnet/arcnet.c b/drivers/net/arcnet/arcnet.c
index e04efc0a5c97..563c43ae5cce 100644
--- a/drivers/net/arcnet/arcnet.c
+++ b/drivers/net/arcnet/arcnet.c
@@ -387,10 +387,46 @@ static void arcnet_timer(struct timer_list *t)
struct arcnet_local *lp = from_timer(lp, t, timer);
struct net_device *dev = lp->dev;
- if (!netif_carrier_ok(dev)) {
+ spin_lock_irq(&lp->lock);
+
+ if (!lp->reset_in_progress && !netif_carrier_ok(dev)) {
netif_carrier_on(dev);
netdev_info(dev, "link up\n");
}
+
+ spin_unlock_irq(&lp->lock);
+}
+
+static void reset_device_work(struct work_struct *work)
+{
+ struct arcnet_local *lp;
+ struct net_device *dev;
+
+ lp = container_of(work, struct arcnet_local, reset_work);
+ dev = lp->dev;
+
+ /*
+ * Do not bring the network interface back up if an ifdown
+ * was already done.
+ */
+ if (!netif_running(dev) || !lp->reset_in_progress)
+ return;
+
+ rtnl_lock();
+
+ /*
+ * Do another check, in case of an ifdown that was triggered in
+ * the small race window between the exit condition above and
+ * acquiring RTNL.
+ */
+ if (!netif_running(dev) || !lp->reset_in_progress)
+ goto out;
+
+ dev_close(dev);
+ dev_open(dev, NULL);
+
+out:
+ rtnl_unlock();
}
static void arcnet_reply_tasklet(unsigned long data)
@@ -452,12 +488,26 @@ struct net_device *alloc_arcdev(const char *name)
lp->dev = dev;
spin_lock_init(&lp->lock);
timer_setup(&lp->timer, arcnet_timer, 0);
+ INIT_WORK(&lp->reset_work, reset_device_work);
}
return dev;
}
EXPORT_SYMBOL(alloc_arcdev);
+void free_arcdev(struct net_device *dev)
+{
+ struct arcnet_local *lp = netdev_priv(dev);
+
+ /*
+ * Do not cancel this at ->ndo_close(), as the workqueue itself
+ * indirectly calls the ifdown path through dev_close().
+ */
+ cancel_work_sync(&lp->reset_work);
+ free_netdev(dev);
+}
+EXPORT_SYMBOL(free_arcdev);
+
/* Open/initialize the board. This is called sometime after booting when
* the 'ifconfig' program is run.
*
@@ -587,6 +637,10 @@ int arcnet_close(struct net_device *dev)
/* shut down the card */
lp->hw.close(dev);
+
+ /* reset counters */
+ lp->reset_in_progress = 0;
+
module_put(lp->hw.owner);
return 0;
}
@@ -820,6 +874,9 @@ irqreturn_t arcnet_interrupt(int irq, void *dev_id)
spin_lock_irqsave(&lp->lock, flags);
+ if (lp->reset_in_progress)
+ goto out;
+
/* RESET flag was enabled - if device is not running, we must
* clear it right away (but nothing else).
*/
@@ -852,11 +909,14 @@ irqreturn_t arcnet_interrupt(int irq, void *dev_id)
if (status & RESETflag) {
arc_printk(D_NORMAL, dev, "spurious reset (status=%Xh)\n",
status);
- arcnet_close(dev);
- arcnet_open(dev);
+
+ lp->reset_in_progress = 1;
+ netif_stop_queue(dev);
+ netif_carrier_off(dev);
+ schedule_work(&lp->reset_work);
/* get out of the interrupt handler! */
- break;
+ goto out;
}
/* RX is inhibited - we must have received something.
* Prepare to receive into the next buffer.
@@ -1052,6 +1112,7 @@ irqreturn_t arcnet_interrupt(int irq, void *dev_id)
udelay(1);
lp->hw.intmask(dev, lp->intmask);
+out:
spin_unlock_irqrestore(&lp->lock, flags);
return retval;
}
diff --git a/drivers/net/arcnet/com20020-isa.c b/drivers/net/arcnet/com20020-isa.c
index f983c4ce6b07..b50d4c33d2a4 100644
--- a/drivers/net/arcnet/com20020-isa.c
+++ b/drivers/net/arcnet/com20020-isa.c
@@ -169,7 +169,7 @@ static int __init com20020_init(void)
dev->irq = 9;
if (com20020isa_probe(dev)) {
- free_netdev(dev);
+ free_arcdev(dev);
return -EIO;
}
@@ -182,7 +182,7 @@ static void __exit com20020_exit(void)
unregister_netdev(my_dev);
free_irq(my_dev->irq, my_dev);
release_region(my_dev->base_addr, ARCNET_TOTAL_SIZE);
- free_netdev(my_dev);
+ free_ardev(my_dev);
}
#ifndef MODULE
diff --git a/drivers/net/arcnet/com20020-pci.c b/drivers/net/arcnet/com20020-pci.c
index eb7f76753c9c..8bdc44b7e09a 100644
--- a/drivers/net/arcnet/com20020-pci.c
+++ b/drivers/net/arcnet/com20020-pci.c
@@ -291,7 +291,7 @@ static void com20020pci_remove(struct pci_dev *pdev)
unregister_netdev(dev);
free_irq(dev->irq, dev);
- free_netdev(dev);
+ free_arcdev(dev);
}
}
diff --git a/drivers/net/arcnet/com20020_cs.c b/drivers/net/arcnet/com20020_cs.c
index cf607ffcf358..9cc5eb6a8e90 100644
--- a/drivers/net/arcnet/com20020_cs.c
+++ b/drivers/net/arcnet/com20020_cs.c
@@ -177,7 +177,7 @@ static void com20020_detach(struct pcmcia_device *link)
dev = info->dev;
if (dev) {
dev_dbg(&link->dev, "kfree...\n");
- free_netdev(dev);
+ free_arcdev(dev);
}
dev_dbg(&link->dev, "kfree2...\n");
kfree(info);
diff --git a/drivers/net/arcnet/com90io.c b/drivers/net/arcnet/com90io.c
index cf214b730671..3856b447d38e 100644
--- a/drivers/net/arcnet/com90io.c
+++ b/drivers/net/arcnet/com90io.c
@@ -396,7 +396,7 @@ static int __init com90io_init(void)
err = com90io_probe(dev);
if (err) {
- free_netdev(dev);
+ free_arcdev(dev);
return err;
}
@@ -419,7 +419,7 @@ static void __exit com90io_exit(void)
free_irq(dev->irq, dev);
release_region(dev->base_addr, ARCNET_TOTAL_SIZE);
- free_netdev(dev);
+ free_arcdev(dev);
}
module_init(com90io_init)
diff --git a/drivers/net/arcnet/com90xx.c b/drivers/net/arcnet/com90xx.c
index 3dc3d533cb19..d8dfb9ea0de8 100644
--- a/drivers/net/arcnet/com90xx.c
+++ b/drivers/net/arcnet/com90xx.c
@@ -554,7 +554,7 @@ static int __init com90xx_found(int ioaddr, int airq, u_long shmem,
err_release_mem:
release_mem_region(dev->mem_start, dev->mem_end - dev->mem_start + 1);
err_free_dev:
- free_netdev(dev);
+ free_arcdev(dev);
return -EIO;
}
@@ -672,7 +672,7 @@ static void __exit com90xx_exit(void)
release_region(dev->base_addr, ARCNET_TOTAL_SIZE);
release_mem_region(dev->mem_start,
dev->mem_end - dev->mem_start + 1);
- free_netdev(dev);
+ free_arcdev(dev);
}
}
--
2.29.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 0/1] net: arcnet: Fix RESET sequence
2020-12-22 9:03 [RFC PATCH 0/1] net: arcnet: Fix RESET sequence Ahmed S. Darwish
2020-12-22 9:03 ` [RFC PATCH 1/1] net: arcnet: Fix RESET flag handling Ahmed S. Darwish
@ 2021-01-11 10:09 ` Sebastian A. Siewior
2021-01-11 13:54 ` Ahmed S. Darwish
2 siblings, 0 replies; 6+ messages in thread
From: Sebastian A. Siewior @ 2021-01-11 10:09 UTC (permalink / raw)
To: Ahmed S. Darwish
Cc: Michael Grzeschik, David S. Miller, Jakub Kicinski, netdev, LKML,
Thomas Gleixner
On 2020-12-22 10:03:37 [+0100], Ahmed S. Darwish wrote:
> 2) arcnet_close() contains a del_timer_sync(). If the irq handler
> interrupts the to-be-deleted timer then call del_timer_sync(), it
> will just loop forever.
del_timer_sync() will trigger a warning if invoked from interrupt
handler.
Sebastian
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 0/1] net: arcnet: Fix RESET sequence
2020-12-22 9:03 [RFC PATCH 0/1] net: arcnet: Fix RESET sequence Ahmed S. Darwish
2020-12-22 9:03 ` [RFC PATCH 1/1] net: arcnet: Fix RESET flag handling Ahmed S. Darwish
2021-01-11 10:09 ` [RFC PATCH 0/1] net: arcnet: Fix RESET sequence Sebastian A. Siewior
@ 2021-01-11 13:54 ` Ahmed S. Darwish
2021-01-18 10:45 ` Ahmed S. Darwish
2 siblings, 1 reply; 6+ messages in thread
From: Ahmed S. Darwish @ 2021-01-11 13:54 UTC (permalink / raw)
To: Michael Grzeschik, David S. Miller, Jakub Kicinski, netdev
Cc: LKML, Thomas Gleixner, Sebastian A. Siewior
Hi,
On Tue, Dec 22, 2020 at 10:03:37AM +0100, Ahmed S. Darwish wrote:
...
>
> Included is an RFC patch to fix the points above: if the RESET flag is
> encountered, a workqueue is scheduled to run the generic reset sequence.
>
...
Kind reminder.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 0/1] net: arcnet: Fix RESET sequence
2021-01-11 13:54 ` Ahmed S. Darwish
@ 2021-01-18 10:45 ` Ahmed S. Darwish
2021-01-18 17:31 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: Ahmed S. Darwish @ 2021-01-18 10:45 UTC (permalink / raw)
To: Michael Grzeschik, David S. Miller, Jakub Kicinski, netdev
Cc: LKML, Thomas Gleixner, Sebastian A. Siewior
On Mon, Jan 11, 2021 at 02:54:06PM +0100, Ahmed S. Darwish wrote:
> Hi,
>
> On Tue, Dec 22, 2020 at 10:03:37AM +0100, Ahmed S. Darwish wrote:
> ...
> >
> > Included is an RFC patch to fix the points above: if the RESET flag is
> > encountered, a workqueue is scheduled to run the generic reset sequence.
> >
> ...
>
> Kind reminder.
Ping. Will anyone look at this?
Thanks,
--
Ahmed S. Darwish
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 0/1] net: arcnet: Fix RESET sequence
2021-01-18 10:45 ` Ahmed S. Darwish
@ 2021-01-18 17:31 ` Jakub Kicinski
0 siblings, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2021-01-18 17:31 UTC (permalink / raw)
To: Ahmed S. Darwish
Cc: Michael Grzeschik, David S. Miller, netdev, LKML,
Thomas Gleixner, Sebastian A. Siewior
On Mon, 18 Jan 2021 11:45:44 +0100 Ahmed S. Darwish wrote:
> On Mon, Jan 11, 2021 at 02:54:06PM +0100, Ahmed S. Darwish wrote:
> > Hi,
> >
> > On Tue, Dec 22, 2020 at 10:03:37AM +0100, Ahmed S. Darwish wrote:
> > ...
> > >
> > > Included is an RFC patch to fix the points above: if the RESET flag is
> > > encountered, a workqueue is scheduled to run the generic reset sequence.
> > >
> > ...
> >
> > Kind reminder.
>
> Ping. Will anyone look at this?
Clearly not, if you think the fix is correct please repost as non-RFC.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-18 17:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-22 9:03 [RFC PATCH 0/1] net: arcnet: Fix RESET sequence Ahmed S. Darwish
2020-12-22 9:03 ` [RFC PATCH 1/1] net: arcnet: Fix RESET flag handling Ahmed S. Darwish
2021-01-11 10:09 ` [RFC PATCH 0/1] net: arcnet: Fix RESET sequence Sebastian A. Siewior
2021-01-11 13:54 ` Ahmed S. Darwish
2021-01-18 10:45 ` Ahmed S. Darwish
2021-01-18 17:31 ` Jakub Kicinski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.