| Message ID | 20221202151204.3318592-5-michael@walle.cc (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: mxl-gpy: broken interrupt fixes | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next, async |
| netdev/apply | fail | Patch does not apply to net-next |
On Fri, Dec 02, 2022 at 04:12:04PM +0100, Michael Walle wrote: > The interrupts on the GPY215B and GPY215C are broken and the only viable > fix is to disable them altogether. There is still the possibilty to > opt-in via the device tree. > > Signed-off-by: Michael Walle <michael@walle.cc> > --- > drivers/net/phy/mxl-gpy.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c > index 20e610dda891..edb8cd8313b0 100644 > --- a/drivers/net/phy/mxl-gpy.c > +++ b/drivers/net/phy/mxl-gpy.c > @@ -12,6 +12,7 @@ > #include <linux/mutex.h> > #include <linux/phy.h> > #include <linux/polynomial.h> > +#include <linux/property.h> > #include <linux/netdevice.h> > > /* PHY ID */ > @@ -290,6 +291,10 @@ static int gpy_probe(struct phy_device *phydev) > phydev->priv = priv; > mutex_init(&priv->mbox_lock); > > + if (gpy_has_broken_mdint(phydev) && > + !device_property_present(dev, "maxlinear,use-broken-interrupts")) > + phydev->irq = PHY_POLL; > + I'm not sure of ordering here. It could be phydev->irq is set after probe. The IRQ is requested as part of phy_connect_direct(), which is much later. I think a better place for this test is in gpy_config_intr(), return -EOPNOTSUPP. phy_enable_interrupts() failing should then cause phy_request_interrupt() to use polling. Andrew
Am 2022-12-02 19:42, schrieb Andrew Lunn: > On Fri, Dec 02, 2022 at 04:12:04PM +0100, Michael Walle wrote: >> The interrupts on the GPY215B and GPY215C are broken and the only >> viable >> fix is to disable them altogether. There is still the possibilty to >> opt-in via the device tree. >> >> Signed-off-by: Michael Walle <michael@walle.cc> >> --- >> drivers/net/phy/mxl-gpy.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c >> index 20e610dda891..edb8cd8313b0 100644 >> --- a/drivers/net/phy/mxl-gpy.c >> +++ b/drivers/net/phy/mxl-gpy.c >> @@ -12,6 +12,7 @@ >> #include <linux/mutex.h> >> #include <linux/phy.h> >> #include <linux/polynomial.h> >> +#include <linux/property.h> >> #include <linux/netdevice.h> >> >> /* PHY ID */ >> @@ -290,6 +291,10 @@ static int gpy_probe(struct phy_device *phydev) >> phydev->priv = priv; >> mutex_init(&priv->mbox_lock); >> >> + if (gpy_has_broken_mdint(phydev) && >> + !device_property_present(dev, >> "maxlinear,use-broken-interrupts")) >> + phydev->irq = PHY_POLL; >> + > > I'm not sure of ordering here. It could be phydev->irq is set after > probe. The IRQ is requested as part of phy_connect_direct(), which is > much later. I've did it that way, because phy_probe() also sets phydev->irq = PHY_POLL in some cases and the phy driver .probe() is called right after it. > I think a better place for this test is in gpy_config_intr(), return > -EOPNOTSUPP. phy_enable_interrupts() failing should then cause > phy_request_interrupt() to use polling. Which will then print a warning, which might be misleading. Or we disable the warning if -EOPNOTSUPP is returned? -michael
> > > @@ -290,6 +291,10 @@ static int gpy_probe(struct phy_device *phydev) > > > phydev->priv = priv; > > > mutex_init(&priv->mbox_lock); > > > > > > + if (gpy_has_broken_mdint(phydev) && > > > + !device_property_present(dev, > > > "maxlinear,use-broken-interrupts")) > > > + phydev->irq = PHY_POLL; > > > + > > > > I'm not sure of ordering here. It could be phydev->irq is set after > > probe. The IRQ is requested as part of phy_connect_direct(), which is > > much later. > > I've did it that way, because phy_probe() also sets phydev->irq = PHY_POLL > in some cases and the phy driver .probe() is called right after it. Yes, it is a valid point to do this check, but on its own i don't think it is sufficient. > > I think a better place for this test is in gpy_config_intr(), return > > -EOPNOTSUPP. phy_enable_interrupts() failing should then cause > > phy_request_interrupt() to use polling. > > Which will then print a warning, which might be misleading. > Or we disable the warning if -EOPNOTSUPP is returned? Disabling the warning is the right thing to do. Andrew
Am 2022-12-03 21:36, schrieb Andrew Lunn: >> > > @@ -290,6 +291,10 @@ static int gpy_probe(struct phy_device *phydev) >> > > phydev->priv = priv; >> > > mutex_init(&priv->mbox_lock); >> > > >> > > + if (gpy_has_broken_mdint(phydev) && >> > > + !device_property_present(dev, >> > > "maxlinear,use-broken-interrupts")) >> > > + phydev->irq = PHY_POLL; >> > > + >> > >> > I'm not sure of ordering here. It could be phydev->irq is set after >> > probe. The IRQ is requested as part of phy_connect_direct(), which is >> > much later. >> >> I've did it that way, because phy_probe() also sets phydev->irq = >> PHY_POLL >> in some cases and the phy driver .probe() is called right after it. > > Yes, it is a valid point to do this check, but on its own i don't > think it is sufficient. Care to elaborate a bit? E.g. what is the difference to the case the phy would have an interrupt described but no .config_intr() op. >> > I think a better place for this test is in gpy_config_intr(), return >> > -EOPNOTSUPP. phy_enable_interrupts() failing should then cause >> > phy_request_interrupt() to use polling. >> >> Which will then print a warning, which might be misleading. >> Or we disable the warning if -EOPNOTSUPP is returned? > > Disabling the warning is the right thing to do. There is more to this. .config_intr() is also used in phy_init_hw() and phy_drv_supports_irq(). The latter would still return true in our case. I'm not sure that is correct. After trying your suggestion, I'm still in favor of somehow tell the phy core to force polling mode during probe() of the driver. The same way it's done if there is no .config_intr(). It's not like we'd need change the mode after probe during runtime. Also with your proposed changed the attachment print is wrong/misleading as it still prints the original irq instead of PHY_POLL. -michael
> > Yes, it is a valid point to do this check, but on its own i don't > > think it is sufficient. > > Care to elaborate a bit? E.g. what is the difference to the case > the phy would have an interrupt described but no .config_intr() > op. > > > > > I think a better place for this test is in gpy_config_intr(), return > > > > -EOPNOTSUPP. phy_enable_interrupts() failing should then cause > > > > phy_request_interrupt() to use polling. > > > > > > Which will then print a warning, which might be misleading. > > > Or we disable the warning if -EOPNOTSUPP is returned? > > > > Disabling the warning is the right thing to do. > > There is more to this. .config_intr() is also used in > phy_init_hw() and phy_drv_supports_irq(). The latter would > still return true in our case. I'm not sure that is correct. > > After trying your suggestion, I'm still in favor of somehow > tell the phy core to force polling mode during probe() of the > driver. The problem is that the MAC can set the interrupt number after the PHY probe has been called. e.g. https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c#L524 The interrupt needs to be set by the time the PHY is connected to the MAC, which is often in the MAC open method, much later than the PHY probe. Andrew
Am 2022-12-20 14:33, schrieb Andrew Lunn: >> > > > I think a better place for this test is in gpy_config_intr(), return >> > > > -EOPNOTSUPP. phy_enable_interrupts() failing should then cause >> > > > phy_request_interrupt() to use polling. >> > > >> > > Which will then print a warning, which might be misleading. >> > > Or we disable the warning if -EOPNOTSUPP is returned? >> > >> > Disabling the warning is the right thing to do. >> >> There is more to this. .config_intr() is also used in >> phy_init_hw() and phy_drv_supports_irq(). The latter would >> still return true in our case. I'm not sure that is correct. >> >> After trying your suggestion, I'm still in favor of somehow >> tell the phy core to force polling mode during probe() of the >> driver. > > The problem is that the MAC can set the interrupt number after the PHY > probe has been called. e.g. > > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c#L524 > > The interrupt needs to be set by the time the PHY is connected to the > MAC, which is often in the MAC open method, much later than the PHY > probe. Ok, then phydev->irq should be updated within the phy_attach_direct(). Something like the following: diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index e865be3d7f01..c6c5830f5214 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -1537,6 +1537,14 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev, phydev->interrupts = PHY_INTERRUPT_DISABLED; + /* PHYs can request to use poll mode even though they have an + * associated interrupt line. This could be the case if they + * detect a broken interrupt handling. + */ + if (phydev->drv->force_polling_mode && + phydev->drv->force_polling_mode(phydev)) + phydev->irq = PHY_POLL; + /* Port is set to PORT_TP by default and the actual PHY driver will set * it to different value depending on the PHY configuration. If we have * the generic PHY driver we can't figure it out, thus set the old That callback could be too specifc, I don't know. We could also have phydev->drv->pre_attach() which then can update the phydev->irq itself. Btw. the phy_attached_info() in the stmmac seems to be a leftover from before the phylink conversion. phylink will print a similar info but when the PHY is actually attached. -michael
diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c index 20e610dda891..edb8cd8313b0 100644 --- a/drivers/net/phy/mxl-gpy.c +++ b/drivers/net/phy/mxl-gpy.c @@ -12,6 +12,7 @@ #include <linux/mutex.h> #include <linux/phy.h> #include <linux/polynomial.h> +#include <linux/property.h> #include <linux/netdevice.h> /* PHY ID */ @@ -290,6 +291,10 @@ static int gpy_probe(struct phy_device *phydev) phydev->priv = priv; mutex_init(&priv->mbox_lock); + if (gpy_has_broken_mdint(phydev) && + !device_property_present(dev, "maxlinear,use-broken-interrupts")) + phydev->irq = PHY_POLL; + fw_version = phy_read(phydev, PHY_FWV); if (fw_version < 0) return fw_version;
The interrupts on the GPY215B and GPY215C are broken and the only viable fix is to disable them altogether. There is still the possibilty to opt-in via the device tree. Signed-off-by: Michael Walle <michael@walle.cc> --- drivers/net/phy/mxl-gpy.c | 5 +++++ 1 file changed, 5 insertions(+)