I met below warning on raspberry pi 3B+:
[ 4.833207] ------------[ cut here ]------------
[ 4.833442] irq 79 handler irq_default_primary_handler+0x0/0x8 enabled interrupts
[ 4.833758] WARNING: CPU: 0 PID: 0 at __handle_irq_event_percpu+0x124/0x150
[ 4.834037] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.0.0+ #8
[ 4.834266] Hardware name: Raspberry Pi 3 Model B Plus Rev 1.3 (DT)
[ 4.834511] pstate: 60000005 (nZCv daif -PAN -UAO)
[ 4.834702] pc : __handle_irq_event_percpu+0x124/0x150
[ 4.834904] lr : __handle_irq_event_percpu+0x124/0x150
[ 4.835103] sp : ffffffc035fb5600
[ 4.835236] x29: ffffffc035fb5600 x28: ffffff8010720480
[ 4.835446] x27: 0000000000000001 x26: ffffff8010650fa0
[ 4.835655] x25: ffffff8010757ec6 x24: ffffffc034784e00
[ 4.835864] x23: 000000000000004f x22: ffffffc035fb568c
[ 4.836073] x21: 0000000000000000 x20: 0000000000000002
[ 4.836282] x19: ffffffc03330af00 x18: 0000000000000000
[ 4.836495] x17: 0000000000000000 x16: 0000000000000000
[ 4.836704] x15: 0000000000aaaaaa x14: 7265746e69206465
[ 4.836914] x13: 0000000000000001 x12: 00000000ffffffff
[ 4.837122] x11: ffffff801059cf48 x10: 0000000000000001
[ 4.837331] x9 : 0000000000000001 x8 : 0000000000000020
[ 4.837540] x7 : 0000000000000000 x6 : 00000000000000b7
[ 4.837748] x5 : ffffff80102a9f50 x4 : 0000000000000001
[ 4.837956] x3 : 0000000000000000 x2 : 0000000000000004
[ 4.838165] x1 : ffffff8010720480 x0 : 0000000000000045
[ 4.838374] Call trace:
[ 4.838480] __handle_irq_event_percpu+0x124/0x150
[ 4.838672] handle_irq_event_percpu+0x1c/0x68
[ 4.842134] handle_irq_event+0x48/0x78
[ 4.845527] handle_simple_irq+0x9c/0xd8
[ 4.848890] generic_handle_irq+0x28/0x40
[ 4.852186] intr_complete+0xb4/0xe8
[ 4.855315] __usb_hcd_giveback_urb+0x58/0xd0
[ 4.858368] usb_giveback_urb_bh+0x94/0xd8
[ 4.861323] tasklet_action_common.isra.1+0x88/0x130
[ 4.864268] tasklet_hi_action+0x24/0x30
[ 4.867112] __do_softirq+0x114/0x224
[ 4.869907] irq_exit+0x9c/0xb8
[ 4.872752] __handle_domain_irq+0x64/0xb8
[ 4.875604] bcm2836_arm_irqchip_handle_irq+0x60/0xc0
[ 4.878544] el1_irq+0xb0/0x128
[ 4.881472] arch_cpu_idle+0x10/0x18
[ 4.884411] do_idle+0x12c/0x140
[ 4.887324] cpu_startup_entry+0x24/0x28
[ 4.890285] rest_init+0xd0/0xdc
[ 4.893247] arch_call_rest_init+0xc/0x14
[ 4.896271] start_kernel+0x314/0x328
[ 4.899291] ---[ end trace 122fa03fe3c21930 ]---
Per my understanding, the proper handling of PHY irq is to make use of
PHY_IGNORE_INTERRUPT then call phy_mac_interrupt when INT_ENP_PHY_INT
is triggered.
Signed-off-by: Jisheng Zhang <[email protected]>
---
drivers/net/usb/lan78xx.c | 208 +++-----------------------------------
1 file changed, 16 insertions(+), 192 deletions(-)
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 3d92ea6fcc02..246a0d1bbc6c 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -20,10 +20,6 @@
#include <linux/mdio.h>
#include <linux/phy.h>
#include <net/ip6_checksum.h>
-#include <linux/interrupt.h>
-#include <linux/irqdomain.h>
-#include <linux/irq.h>
-#include <linux/irqchip/chained_irq.h>
#include <linux/microchipphy.h>
#include <linux/phy_fixed.h>
#include <linux/of_mdio.h>
@@ -87,38 +83,6 @@
/* statistic update interval (mSec) */
#define STAT_UPDATE_TIMER (1 * 1000)
-/* defines interrupts from interrupt EP */
-#define MAX_INT_EP (32)
-#define INT_EP_INTEP (31)
-#define INT_EP_OTP_WR_DONE (28)
-#define INT_EP_EEE_TX_LPI_START (26)
-#define INT_EP_EEE_TX_LPI_STOP (25)
-#define INT_EP_EEE_RX_LPI (24)
-#define INT_EP_MAC_RESET_TIMEOUT (23)
-#define INT_EP_RDFO (22)
-#define INT_EP_TXE (21)
-#define INT_EP_USB_STATUS (20)
-#define INT_EP_TX_DIS (19)
-#define INT_EP_RX_DIS (18)
-#define INT_EP_PHY (17)
-#define INT_EP_DP (16)
-#define INT_EP_MAC_ERR (15)
-#define INT_EP_TDFU (14)
-#define INT_EP_TDFO (13)
-#define INT_EP_UTX (12)
-#define INT_EP_GPIO_11 (11)
-#define INT_EP_GPIO_10 (10)
-#define INT_EP_GPIO_9 (9)
-#define INT_EP_GPIO_8 (8)
-#define INT_EP_GPIO_7 (7)
-#define INT_EP_GPIO_6 (6)
-#define INT_EP_GPIO_5 (5)
-#define INT_EP_GPIO_4 (4)
-#define INT_EP_GPIO_3 (3)
-#define INT_EP_GPIO_2 (2)
-#define INT_EP_GPIO_1 (1)
-#define INT_EP_GPIO_0 (0)
-
static const char lan78xx_gstrings[][ETH_GSTRING_LEN] = {
"RX FCS Errors",
"RX Alignment Errors",
@@ -350,15 +314,6 @@ struct statstage {
struct lan78xx_statstage64 curr_stat;
};
-struct irq_domain_data {
- struct irq_domain *irqdomain;
- unsigned int phyirq;
- struct irq_chip *irqchip;
- irq_flow_handler_t irq_handler;
- u32 irqenable;
- struct mutex irq_lock; /* for irq bus access */
-};
-
struct lan78xx_net {
struct net_device *net;
struct usb_device *udev;
@@ -415,8 +370,6 @@ struct lan78xx_net {
int delta;
struct statstage stats;
-
- struct irq_domain_data domain_data;
};
/* define external phy id */
@@ -1251,6 +1204,7 @@ static void lan78xx_defer_kevent(struct lan78xx_net *dev, int work)
static void lan78xx_status(struct lan78xx_net *dev, struct urb *urb)
{
u32 intdata;
+ struct phy_device *phydev = dev->net->phydev;
if (urb->actual_length != 4) {
netdev_warn(dev->net,
@@ -1264,9 +1218,7 @@ static void lan78xx_status(struct lan78xx_net *dev, struct urb *urb)
if (intdata & INT_ENP_PHY_INT) {
netif_dbg(dev, link, dev->net, "PHY INTR: 0x%08x\n", intdata);
lan78xx_defer_kevent(dev, EVENT_LINK_RESET);
-
- if (dev->domain_data.phyirq > 0)
- generic_handle_irq(dev->domain_data.phyirq);
+ phy_mac_interrupt(phydev);
} else
netdev_warn(dev->net,
"unexpected interrupt: 0x%08x\n", intdata);
@@ -1875,127 +1827,6 @@ static void lan78xx_link_status_change(struct net_device *net)
}
}
-static int irq_map(struct irq_domain *d, unsigned int irq,
- irq_hw_number_t hwirq)
-{
- struct irq_domain_data *data = d->host_data;
-
- irq_set_chip_data(irq, data);
- irq_set_chip_and_handler(irq, data->irqchip, data->irq_handler);
- irq_set_noprobe(irq);
-
- return 0;
-}
-
-static void irq_unmap(struct irq_domain *d, unsigned int irq)
-{
- irq_set_chip_and_handler(irq, NULL, NULL);
- irq_set_chip_data(irq, NULL);
-}
-
-static const struct irq_domain_ops chip_domain_ops = {
- .map = irq_map,
- .unmap = irq_unmap,
-};
-
-static void lan78xx_irq_mask(struct irq_data *irqd)
-{
- struct irq_domain_data *data = irq_data_get_irq_chip_data(irqd);
-
- data->irqenable &= ~BIT(irqd_to_hwirq(irqd));
-}
-
-static void lan78xx_irq_unmask(struct irq_data *irqd)
-{
- struct irq_domain_data *data = irq_data_get_irq_chip_data(irqd);
-
- data->irqenable |= BIT(irqd_to_hwirq(irqd));
-}
-
-static void lan78xx_irq_bus_lock(struct irq_data *irqd)
-{
- struct irq_domain_data *data = irq_data_get_irq_chip_data(irqd);
-
- mutex_lock(&data->irq_lock);
-}
-
-static void lan78xx_irq_bus_sync_unlock(struct irq_data *irqd)
-{
- struct irq_domain_data *data = irq_data_get_irq_chip_data(irqd);
- struct lan78xx_net *dev =
- container_of(data, struct lan78xx_net, domain_data);
- u32 buf;
- int ret;
-
- /* call register access here because irq_bus_lock & irq_bus_sync_unlock
- * are only two callbacks executed in non-atomic contex.
- */
- ret = lan78xx_read_reg(dev, INT_EP_CTL, &buf);
- if (buf != data->irqenable)
- ret = lan78xx_write_reg(dev, INT_EP_CTL, data->irqenable);
-
- mutex_unlock(&data->irq_lock);
-}
-
-static struct irq_chip lan78xx_irqchip = {
- .name = "lan78xx-irqs",
- .irq_mask = lan78xx_irq_mask,
- .irq_unmask = lan78xx_irq_unmask,
- .irq_bus_lock = lan78xx_irq_bus_lock,
- .irq_bus_sync_unlock = lan78xx_irq_bus_sync_unlock,
-};
-
-static int lan78xx_setup_irq_domain(struct lan78xx_net *dev)
-{
- struct device_node *of_node;
- struct irq_domain *irqdomain;
- unsigned int irqmap = 0;
- u32 buf;
- int ret = 0;
-
- of_node = dev->udev->dev.parent->of_node;
-
- mutex_init(&dev->domain_data.irq_lock);
-
- lan78xx_read_reg(dev, INT_EP_CTL, &buf);
- dev->domain_data.irqenable = buf;
-
- dev->domain_data.irqchip = &lan78xx_irqchip;
- dev->domain_data.irq_handler = handle_simple_irq;
-
- irqdomain = irq_domain_add_simple(of_node, MAX_INT_EP, 0,
- &chip_domain_ops, &dev->domain_data);
- if (irqdomain) {
- /* create mapping for PHY interrupt */
- irqmap = irq_create_mapping(irqdomain, INT_EP_PHY);
- if (!irqmap) {
- irq_domain_remove(irqdomain);
-
- irqdomain = NULL;
- ret = -EINVAL;
- }
- } else {
- ret = -EINVAL;
- }
-
- dev->domain_data.irqdomain = irqdomain;
- dev->domain_data.phyirq = irqmap;
-
- return ret;
-}
-
-static void lan78xx_remove_irq_domain(struct lan78xx_net *dev)
-{
- if (dev->domain_data.phyirq > 0) {
- irq_dispose_mapping(dev->domain_data.phyirq);
-
- if (dev->domain_data.irqdomain)
- irq_domain_remove(dev->domain_data.irqdomain);
- }
- dev->domain_data.phyirq = 0;
- dev->domain_data.irqdomain = NULL;
-}
-
static int lan8835_fixup(struct phy_device *phydev)
{
int buf;
@@ -2124,16 +1955,15 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
return -EIO;
}
- /* if phyirq is not set, use polling mode in phylib */
- if (dev->domain_data.phyirq > 0)
- phydev->irq = dev->domain_data.phyirq;
- else
- phydev->irq = 0;
- netdev_dbg(dev->net, "phydev->irq = %d\n", phydev->irq);
-
/* set to AUTOMDIX */
phydev->mdix = ETH_TP_MDI_AUTO;
+ ret = phy_read(phydev, LAN88XX_INT_STS);
+ ret = phy_write(phydev, LAN88XX_INT_MASK,
+ LAN88XX_INT_MASK_MDINTPIN_EN_ |
+ LAN88XX_INT_MASK_LINK_CHANGE_);
+ phydev->irq = PHY_IGNORE_INTERRUPT;
+
ret = phy_connect_direct(dev->net, phydev,
lan78xx_link_status_change,
dev->interface);
@@ -2571,6 +2401,11 @@ static int lan78xx_reset(struct lan78xx_net *dev)
}
ret = lan78xx_write_reg(dev, MAC_CR, buf);
+ /* enable PHY interrupts */
+ ret = lan78xx_read_reg(dev, INT_EP_CTL, &buf);
+ buf |= INT_ENP_PHY_INT;
+ ret = lan78xx_write_reg(dev, INT_EP_CTL, buf);
+
ret = lan78xx_read_reg(dev, MAC_TX, &buf);
buf |= MAC_TX_TXEN_;
ret = lan78xx_write_reg(dev, MAC_TX, buf);
@@ -2981,13 +2816,6 @@ static int lan78xx_bind(struct lan78xx_net *dev, struct usb_interface *intf)
dev->net->hw_features = dev->net->features;
- ret = lan78xx_setup_irq_domain(dev);
- if (ret < 0) {
- netdev_warn(dev->net,
- "lan78xx_setup_irq_domain() failed : %d", ret);
- goto out1;
- }
-
dev->net->hard_header_len += TX_OVERHEAD;
dev->hard_mtu = dev->net->mtu + dev->net->hard_header_len;
@@ -2995,13 +2823,13 @@ static int lan78xx_bind(struct lan78xx_net *dev, struct usb_interface *intf)
ret = lan78xx_reset(dev);
if (ret) {
netdev_warn(dev->net, "Registers INIT FAILED....");
- goto out2;
+ goto out;
}
ret = lan78xx_mdio_init(dev);
if (ret) {
netdev_warn(dev->net, "MDIO INIT FAILED.....");
- goto out2;
+ goto out;
}
dev->net->flags |= IFF_MULTICAST;
@@ -3010,10 +2838,7 @@ static int lan78xx_bind(struct lan78xx_net *dev, struct usb_interface *intf)
return ret;
-out2:
- lan78xx_remove_irq_domain(dev);
-
-out1:
+out:
netdev_warn(dev->net, "Bind routine FAILED");
cancel_work_sync(&pdata->set_multicast);
cancel_work_sync(&pdata->set_vlan);
@@ -3025,7 +2850,6 @@ static void lan78xx_unbind(struct lan78xx_net *dev, struct usb_interface *intf)
{
struct lan78xx_priv *pdata = (struct lan78xx_priv *)(dev->data[0]);
- lan78xx_remove_irq_domain(dev);
lan78xx_remove_mdio(dev);
--
2.20.1
>
> Per my understanding, the proper handling of PHY irq is to make use of
> PHY_IGNORE_INTERRUPT then call phy_mac_interrupt when
> INT_ENP_PHY_INT is triggered.
>
Hi Jisheng,
Thanks for the changes.
Is this warning specific to any linux version? Why do you think PHY irq domain handling is not proper? Maybe we can fix that rather removing complete IRQ domain code changes.
Also, Can you pls let us know how this changes fixed your warning.
Thanks
-Raghu
On Mon, 8 Apr 2019 07:46:03 +0000 wrote:
>
>
> >
> > Per my understanding, the proper handling of PHY irq is to make use of
> > PHY_IGNORE_INTERRUPT then call phy_mac_interrupt when
> > INT_ENP_PHY_INT is triggered.
> >
>
> Hi Jisheng,
Hi
> Thanks for the changes.
> Is this warning specific to any linux version?
In theory, no. I only tested 5.0, 4.20, both can reproduce this warning.
> Why do you think PHY irq domain handling is not proper?
+ Marc
The warning comes from calling generic_handle_irq() in usb tasklet context.
This is not correct.
Per my understanding, if there's chained irq, we could introduce extra
irqdomain. E.g
GIC <--> another irqchip controller <--> HW device
But in lan78xx, this is not the case. There's no chained irq at all.
In fact, there's a bit INT_ENP_PHY_INT in MAC's Interrupt Endpoint status
word to indicate this is PHY interrupt. So this is the case
PHY_IGNORE_INTERRUPT/phy_mac_interrupt intend for.
irq experts knows irqdomain etc better, maybe they can provide more inputs
> Maybe we can fix that rather removing complete IRQ domain code changes.
> Also, Can you pls let us know how this changes fixed your warning.
The patch removes the generic_handle_irq() calling from invalid context.
Thanks
> > Is this warning specific to any linux version?
>
> In theory, no. I only tested 5.0, 4.20, both can reproduce this warning.
>
This makes me think that code is fine because it occurs in 4.20 and greater. Or maybe the problem is masked in older. I maybe wrong in assuming that.
>
> The warning comes from calling generic_handle_irq() in usb tasklet context.
> This is not correct.
>
> Per my understanding, if there's chained irq, we could introduce extra
> irqdomain. E.g
>
> GIC <--> another irqchip controller <--> HW device
>
Correct, IRQ domain is generally used in chained irq controllers.
Yes, We need to check why irq domain is used in the current driver.
Thanks
On Mon, 8 Apr 2019 10:46:52 +0000 wrote:
>
>
> > > Is this warning specific to any linux version?
> >
> > In theory, no. I only tested 5.0, 4.20, both can reproduce this warning.
> >
> This makes me think that code is fine because it occurs in 4.20 and greater. Or maybe the problem is masked in older. I maybe wrong in assuming that.
Per my understanding, the warning should occur since commit cc89c323a30e
("lan78xx: Use irq_domain for phy interrupt from USB Int. EP")
I just bought a Raspberry Pi 3B+ a few days ago, so I didn't have chance
to check 4.19 and before.
>
> >
> > The warning comes from calling generic_handle_irq() in usb tasklet context.
> > This is not correct.
> >
> > Per my understanding, if there's chained irq, we could introduce extra
> > irqdomain. E.g
> >
> > GIC <--> another irqchip controller <--> HW device
> >
> Correct, IRQ domain is generally used in chained irq controllers.
> Yes, We need to check why irq domain is used in the current driver.
>
It's introduced in the commit cc89c323a30e
Thanks
> > Correct, IRQ domain is generally used in chained irq controllers.
> > Yes, We need to check why irq domain is used in the current driver.
> >
>
> It's introduced in the commit cc89c323a30e
>
Thanks Jisheng, Will check and get back ASAP.
Thanks
> > >
> > > The warning comes from calling generic_handle_irq() in usb tasklet
> context.
> > > This is not correct.
> > >
> > > Per my understanding, if there's chained irq, we could introduce
> > > extra irqdomain. E.g
> > >
> > > GIC <--> another irqchip controller <--> HW device
> > >
> > Correct, IRQ domain is generally used in chained irq controllers.
> > Yes, We need to check why irq domain is used in the current driver.
> >
>
> It's introduced in the commit cc89c323a30e
>
Hi Jisheng,
I had spent some time to look into the history of commit. The reason for having irq domain in driver is because of the unavailability of real hardware interrupt, here we have USB interrupt pipe which is not actual interrupt.
So changes were proposed to have pseudo phy interrupt and handlers in phy lib. But later it was suggested to implement linux interrupt controller in driver itself.
You can see the archive here https://patchwork.ozlabs.org/patch/564511/
I want to understand if there is any functionality impact with this warning? Because I'm afraid if the current changes are removed we might hit some other issues (or older ones). We have to go through rigorous testing before going ahead.
Thanks.
On Wed, 10 Apr 2019 09:20:38 +0000 wrote:
>
> > > >
> > > > The warning comes from calling generic_handle_irq() in usb tasklet
> > context.
> > > > This is not correct.
> > > >
> > > > Per my understanding, if there's chained irq, we could introduce
> > > > extra irqdomain. E.g
> > > >
> > > > GIC <--> another irqchip controller <--> HW device
> > > >
> > > Correct, IRQ domain is generally used in chained irq controllers.
> > > Yes, We need to check why irq domain is used in the current driver.
> > >
> >
> > It's introduced in the commit cc89c323a30e
> >
> Hi Jisheng,
Hi,
> I had spent some time to look into the history of commit. The reason for having irq domain in driver is because of the unavailability of real hardware interrupt, here we have USB interrupt pipe which is not actual interrupt.
> So changes were proposed to have pseudo phy interrupt and handlers in phy lib. But later it was suggested to implement linux interrupt controller in driver itself.
+ Andrew, Florian
USB net is different with Andrew's case.
there's no irq at all in usb net
but in Andrew's case, interrupt is connected to a GPIO line. As is known
GPIO can behave as irq chip.
> You can see the archive here https://patchwork.ozlabs.org/patch/564511/
Per my understanding, the purpose is to avoid phy_state_machine() poll
phy_read_status() to monitor phy link. But as pointed out by Florian,
phy_mac_interrupt() is intended for that purpose. And the poll should be
fixed in phy. Since the email was sent in 2016, maybe the bug has been fixed.
> I want to understand if there is any functionality impact with this warning? Because I'm afraid if the current changes are removed we might hit some other issues (or older ones). We have to go through rigorous testing before going ahead.
Warning indicates there's something wrong in the code.
IMHO phy_mac_interrupt() and PHY_IGNORE_INTERRUPT is the correct
solution. If the phy_mac_interrupt() poll is fixed, I think maybe
old issue which commit cc89c323a30e want to fix won't exist.
Thanks
On 10/04/2019 10:53, Jisheng Zhang wrote:
>
> On Wed, 10 Apr 2019 09:20:38 +0000 wrote:
>
>>
>>>>>
>>>>> The warning comes from calling generic_handle_irq() in usb tasklet
>>> context.
>>>>> This is not correct.
>>>>>
>>>>> Per my understanding, if there's chained irq, we could introduce
>>>>> extra irqdomain. E.g
>>>>>
>>>>> GIC <--> another irqchip controller <--> HW device
>>>>>
>>>> Correct, IRQ domain is generally used in chained irq controllers.
>>>> Yes, We need to check why irq domain is used in the current driver.
>>>>
>>>
>>> It's introduced in the commit cc89c323a30e
>>>
>> Hi Jisheng,
>
> Hi,
>
>> I had spent some time to look into the history of commit. The reason for having irq domain in driver is because of the unavailability of real hardware interrupt, here we have USB interrupt pipe which is not actual interrupt.
>> So changes were proposed to have pseudo phy interrupt and handlers in phy lib. But later it was suggested to implement linux interrupt controller in driver itself.
>
> + Andrew, Florian
>
> USB net is different with Andrew's case.
>
> there's no irq at all in usb net
>
> but in Andrew's case, interrupt is connected to a GPIO line. As is known
> GPIO can behave as irq chip.
>
>> You can see the archive here https://patchwork.ozlabs.org/patch/564511/
>
> Per my understanding, the purpose is to avoid phy_state_machine() poll
> phy_read_status() to monitor phy link. But as pointed out by Florian,
> phy_mac_interrupt() is intended for that purpose. And the poll should be
> fixed in phy. Since the email was sent in 2016, maybe the bug has been fixed.
>
>> I want to understand if there is any functionality impact with this warning? Because I'm afraid if the current changes are removed we might hit some other issues (or older ones). We have to go through rigorous testing before going ahead.
>
> Warning indicates there's something wrong in the code.
Most definitely. This code is completely busted (handling pseudo
interrupts out of a tasklet is a BUG), and things will crash or deadlock
under the right conditions.
>
> IMHO phy_mac_interrupt() and PHY_IGNORE_INTERRUPT is the correct
> solution. If the phy_mac_interrupt() poll is fixed, I think maybe
> old issue which commit cc89c323a30e want to fix won't exist.
Please also see this[1] thread, which has a potential solution for this.
Thanks,
M.
[1] https://www.spinics.net/lists/netdev/msg542290.html
--
Jazz is not dead. It just smells funny...
Hi Jisheng,
> > I want to understand if there is any functionality impact with this warning?
> Because I'm afraid if the current changes are removed we might hit some
> other issues (or older ones). We have to go through rigorous testing before
> going ahead.
>
> Warning indicates there's something wrong in the code.
Agree that the code is incorrect. Just wanted to understand if you had any functionality impact too.
>
> IMHO phy_mac_interrupt() and PHY_IGNORE_INTERRUPT is the correct
> solution. If the phy_mac_interrupt() poll is fixed, I think maybe old issue
> which commit cc89c323a30e want to fix won't exist.
>
I tried to reproduce the problem in PC environment but did not see the warnings.
However, I tried your patch and did plug/unplug tests(rmmod/insmod continuously) and observed call traces from kernel. I don't see these traces without your patch.
Attached log.
Thanks,
-Raghu
On Wed, 17 Apr 2019 03:49:46 +0000 <[email protected]> wrote:
>
> Hi Jisheng,
Hi,
>
> > > I want to understand if there is any functionality impact with this warning?
> > Because I'm afraid if the current changes are removed we might hit some
> > other issues (or older ones). We have to go through rigorous testing before
> > going ahead.
> >
> > Warning indicates there's something wrong in the code.
>
> Agree that the code is incorrect. Just wanted to understand if you had any functionality impact too.
>
> >
> > IMHO phy_mac_interrupt() and PHY_IGNORE_INTERRUPT is the correct
> > solution. If the phy_mac_interrupt() poll is fixed, I think maybe old issue
> > which commit cc89c323a30e want to fix won't exist.
> >
>
> I tried to reproduce the problem in PC environment but did not see the warnings.
> However, I tried your patch and did plug/unplug tests(rmmod/insmod continuously) and observed call traces from kernel. I don't see these traces without your patch.
> Attached log.
I believe this is another bug in lan78xx driver: after phy_disconnect() if
there's intr urb coming, we will call phy_mac_interrupt, so trigger
phy_state_machine(), but now the phydev->attached_dev is NULL, so NULL
pointer deference. Can you please try below patch:
--->8
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 246a0d1bbc6c..27d6fbdd58c1 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -3467,6 +3467,8 @@ static void lan78xx_disconnect(struct usb_interface *intf)
net = dev->net;
phydev = net->phydev;
+ usb_kill_urb(dev->urb_intr);
+
phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, 0xfffffff0);
phy_unregister_fixup_for_uid(PHY_LAN8835, 0xfffffff0);
@@ -3483,7 +3485,6 @@ static void lan78xx_disconnect(struct usb_interface *intf)
lan78xx_unbind(dev, intf);
- usb_kill_urb(dev->urb_intr);
usb_free_urb(dev->urb_intr);
free_netdev(net);
> I believe this is another bug in lan78xx driver: after phy_disconnect() if
> there's intr urb coming, we will call phy_mac_interrupt, so trigger
> phy_state_machine(), but now the phydev->attached_dev is NULL, so NULL
> pointer deference. Can you please try below patch:
>
Thanks Jisheng.
I still see the call trace even with updated patch. Log attached.
Thanks.