| Message ID | 20221220113733.714233-1-enguerrand.de-ribaucourt@savoirfairelinux.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] net: lan78xx: prevent LAN88XX specific operations | expand |
On Tue, 2022-12-20 at 12:37 +0100, Enguerrand de Ribaucourt wrote: > Some operations during the cable switch workaround modify the register > LAN88XX_INT_MASK of the PHY. However, this register is specific to the > LAN8835 PHY. For instance, if a DP8322I PHY is connected to the LAN7801, > that register (0x19), corresponds to the LED and MAC address > configuration, resulting in unapropriate behavior. > > Use the generic phy interrupt functions instead. > > Fixes: 89b36fb5e532 ("lan78xx: Lan7801 Support for Fixed PHY") > Reviewed-by: Paolo Abeni <pabeni@redhat.com>; You should not attach this tag (or acked-by) on your own. The following is not even the code I was _asking_ about... > Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-ribaucourt@savoirfairelinux.com> > --- > drivers/net/usb/lan78xx.c | 14 +++----------- > 1 file changed, 3 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c > index f18ab8e220db..65d5d54994ff 100644 > --- a/drivers/net/usb/lan78xx.c > +++ b/drivers/net/usb/lan78xx.c > @@ -28,6 +28,7 @@ > #include <linux/phy_fixed.h> > #include <linux/of_mdio.h> > #include <linux/of_net.h> > +#include <linux/phy.h> > #include "lan78xx.h" > > #define DRIVER_AUTHOR "WOOJUNG HUH <woojung.huh@microchip.com>" > @@ -2123,10 +2124,7 @@ static void lan78xx_link_status_change(struct net_device *net) > * at forced 100 F/H mode. > */ > if (!phydev->autoneg && (phydev->speed == 100)) { > - /* disable phy interrupt */ > - temp = phy_read(phydev, LAN88XX_INT_MASK); > - temp &= ~LAN88XX_INT_MASK_MDINTPIN_EN_; > - phy_write(phydev, LAN88XX_INT_MASK, temp); > + phy_disable_interrupts(phydev); > > temp = phy_read(phydev, MII_BMCR); > temp &= ~(BMCR_SPEED100 | BMCR_SPEED1000); > @@ -2134,13 +2132,7 @@ static void lan78xx_link_status_change(struct net_device *net) > temp |= BMCR_SPEED100; > phy_write(phydev, MII_BMCR, temp); /* set to 100 later */ > > - /* clear pending interrupt generated while workaround */ > - temp = phy_read(phydev, LAN88XX_INT_STS); > - > - /* enable phy interrupt back */ > - temp = phy_read(phydev, LAN88XX_INT_MASK); > - temp |= LAN88XX_INT_MASK_MDINTPIN_EN_; > - phy_write(phydev, LAN88XX_INT_MASK, temp); > + phy_request_interrupt(phydev); This looks wrong. Should probably be: phy_enable_interrupts(phydev); Paolo
On Tue, 2022-12-20 at 12:37 +0100, Enguerrand de Ribaucourt wrote: > Some operations during the cable switch workaround modify the register > LAN88XX_INT_MASK of the PHY. However, this register is specific to the > LAN8835 PHY. For instance, if a DP8322I PHY is connected to the LAN7801, > that register (0x19), corresponds to the LED and MAC address > configuration, resulting in unapropriate behavior. > > Use the generic phy interrupt functions instead. > > Fixes: 89b36fb5e532 ("lan78xx: Lan7801 Support for Fixed PHY") > Reviewed-by: Paolo Abeni <pabeni@redhat.com>; > Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-ribaucourt@savoirfairelinux.com> > --- > drivers/net/usb/lan78xx.c | 14 +++----------- > 1 file changed, 3 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c > index f18ab8e220db..65d5d54994ff 100644 > --- a/drivers/net/usb/lan78xx.c > +++ b/drivers/net/usb/lan78xx.c > @@ -28,6 +28,7 @@ > #include <linux/phy_fixed.h> > #include <linux/of_mdio.h> > #include <linux/of_net.h> > +#include <linux/phy.h> > #include "lan78xx.h" > > #define DRIVER_AUTHOR "WOOJUNG HUH <woojung.huh@microchip.com>" > @@ -2123,10 +2124,7 @@ static void lan78xx_link_status_change(struct net_device *net) > * at forced 100 F/H mode. > */ > if (!phydev->autoneg && (phydev->speed == 100)) { > - /* disable phy interrupt */ > - temp = phy_read(phydev, LAN88XX_INT_MASK); > - temp &= ~LAN88XX_INT_MASK_MDINTPIN_EN_; > - phy_write(phydev, LAN88XX_INT_MASK, temp); > + phy_disable_interrupts(phydev); > > temp = phy_read(phydev, MII_BMCR); > temp &= ~(BMCR_SPEED100 | BMCR_SPEED1000); > @@ -2134,13 +2132,7 @@ static void lan78xx_link_status_change(struct net_device *net) > temp |= BMCR_SPEED100; > phy_write(phydev, MII_BMCR, temp); /* set to 100 later */ > > - /* clear pending interrupt generated while workaround */ > - temp = phy_read(phydev, LAN88XX_INT_STS); > - > - /* enable phy interrupt back */ > - temp = phy_read(phydev, LAN88XX_INT_MASK); > - temp |= LAN88XX_INT_MASK_MDINTPIN_EN_; > - phy_write(phydev, LAN88XX_INT_MASK, temp); > + phy_request_interrupt(phydev); > } > } > Oops, this does not even build... please take your time testing the code before sending patches to the ML. Paolo
----- Original Message ----- > From: "Paolo Abeni" <pabeni@redhat.com> > To: "Enguerrand de Ribaucourt" <enguerrand.de-ribaucourt@savoirfairelinux.com>, "netdev" <netdev@vger.kernel.org> > Cc: "woojung huh" <woojung.huh@microchip.com>, "davem" <davem@davemloft.net>, "UNGLinuxDriver" > <UNGLinuxDriver@microchip.com> > Sent: Tuesday, December 20, 2022 1:41:08 PM > Subject: Re: [PATCH v2] net: lan78xx: prevent LAN88XX specific operations > On Tue, 2022-12-20 at 12:37 +0100, Enguerrand de Ribaucourt wrote: > > Some operations during the cable switch workaround modify the register > > LAN88XX_INT_MASK of the PHY. However, this register is specific to the > > LAN8835 PHY. For instance, if a DP8322I PHY is connected to the LAN7801, > > that register (0x19), corresponds to the LED and MAC address > > configuration, resulting in unapropriate behavior. > > Use the generic phy interrupt functions instead. > > Fixes: 89b36fb5e532 ("lan78xx: Lan7801 Support for Fixed PHY") > > Reviewed-by: Paolo Abeni <pabeni@redhat.com>; > You should not attach this tag (or acked-by) on your own. Thanks, I'm still new with the patching process. > The following is not even the code I was _asking_ about... >> Signed-off-by: Enguerrand de Ribaucourt > > <enguerrand.de-ribaucourt@savoirfairelinux.com> > > --- > > drivers/net/usb/lan78xx.c | 14 +++----------- > > 1 file changed, 3 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c > > index f18ab8e220db..65d5d54994ff 100644 > > --- a/drivers/net/usb/lan78xx.c > > +++ b/drivers/net/usb/lan78xx.c > > @@ -28,6 +28,7 @@ > > #include <linux/phy_fixed.h> > > #include <linux/of_mdio.h> > > #include <linux/of_net.h> > > +#include <linux/phy.h> > > #include "lan78xx.h" > > #define DRIVER_AUTHOR "WOOJUNG HUH <woojung.huh@microchip.com>" >> @@ -2123,10 +2124,7 @@ static void lan78xx_link_status_change(struct net_device > > *net) > > * at forced 100 F/H mode. > > */ > > if (!phydev->autoneg && (phydev->speed == 100)) { > > - /* disable phy interrupt */ > > - temp = phy_read(phydev, LAN88XX_INT_MASK); > > - temp &= ~LAN88XX_INT_MASK_MDINTPIN_EN_; > > - phy_write(phydev, LAN88XX_INT_MASK, temp); > > + phy_disable_interrupts(phydev); > > temp = phy_read(phydev, MII_BMCR); > > temp &= ~(BMCR_SPEED100 | BMCR_SPEED1000); >> @@ -2134,13 +2132,7 @@ static void lan78xx_link_status_change(struct net_device > > *net) > > temp |= BMCR_SPEED100; > > phy_write(phydev, MII_BMCR, temp); /* set to 100 later */ > > - /* clear pending interrupt generated while workaround */ > > - temp = phy_read(phydev, LAN88XX_INT_STS); > > - > > - /* enable phy interrupt back */ > > - temp = phy_read(phydev, LAN88XX_INT_MASK); > > - temp |= LAN88XX_INT_MASK_MDINTPIN_EN_; > > - phy_write(phydev, LAN88XX_INT_MASK, temp); > > + phy_request_interrupt(phydev); > This looks wrong. Should probably be: > phy_enable_interrupts(phydev); phy_enable_interrupts isn't exported in the header. I'll add a dedicated commit for that. > Paolo
> From: "Paolo Abeni" <pabeni@redhat.com> > To: "Enguerrand de Ribaucourt" <enguerrand.de-ribaucourt@savoirfairelinux.com>, > "netdev" <netdev@vger.kernel.org> > Cc: "woojung huh" <woojung.huh@microchip.com>, "davem" <davem@davemloft.net>, > "UNGLinuxDriver" <UNGLinuxDriver@microchip.com> > Sent: Tuesday, December 20, 2022 1:45:08 PM > Subject: Re: [PATCH v2] net: lan78xx: prevent LAN88XX specific operations > On Tue, 2022-12-20 at 12:37 +0100, Enguerrand de Ribaucourt wrote: > > Some operations during the cable switch workaround modify the register > > LAN88XX_INT_MASK of the PHY. However, this register is specific to the > > LAN8835 PHY. For instance, if a DP8322I PHY is connected to the LAN7801, > > that register (0x19), corresponds to the LED and MAC address > > configuration, resulting in unapropriate behavior. > > Use the generic phy interrupt functions instead. > > Fixes: 89b36fb5e532 ("lan78xx: Lan7801 Support for Fixed PHY") > > Reviewed-by: Paolo Abeni <pabeni@redhat.com>; >> Signed-off-by: Enguerrand de Ribaucourt > > <enguerrand.de-ribaucourt@savoirfairelinux.com> > > --- > > drivers/net/usb/lan78xx.c | 14 +++----------- > > 1 file changed, 3 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c > > index f18ab8e220db..65d5d54994ff 100644 > > --- a/drivers/net/usb/lan78xx.c > > +++ b/drivers/net/usb/lan78xx.c > > @@ -28,6 +28,7 @@ > > #include <linux/phy_fixed.h> > > #include <linux/of_mdio.h> > > #include <linux/of_net.h> > > +#include <linux/phy.h> > > #include "lan78xx.h" > > #define DRIVER_AUTHOR "WOOJUNG HUH <woojung.huh@microchip.com>" >> @@ -2123,10 +2124,7 @@ static void lan78xx_link_status_change(struct net_device > > *net) > > * at forced 100 F/H mode. > > */ > > if (!phydev->autoneg && (phydev->speed == 100)) { > > - /* disable phy interrupt */ > > - temp = phy_read(phydev, LAN88XX_INT_MASK); > > - temp &= ~LAN88XX_INT_MASK_MDINTPIN_EN_; > > - phy_write(phydev, LAN88XX_INT_MASK, temp); > > + phy_disable_interrupts(phydev); > > temp = phy_read(phydev, MII_BMCR); > > temp &= ~(BMCR_SPEED100 | BMCR_SPEED1000); >> @@ -2134,13 +2132,7 @@ static void lan78xx_link_status_change(struct net_device > > *net) > > temp |= BMCR_SPEED100; > > phy_write(phydev, MII_BMCR, temp); /* set to 100 later */ > > - /* clear pending interrupt generated while workaround */ > > - temp = phy_read(phydev, LAN88XX_INT_STS); > > - > > - /* enable phy interrupt back */ > > - temp = phy_read(phydev, LAN88XX_INT_MASK); > > - temp |= LAN88XX_INT_MASK_MDINTPIN_EN_; > > - phy_write(phydev, LAN88XX_INT_MASK, temp); > > + phy_request_interrupt(phydev); > > } > > } > Oops, this does not even build... please take your time testing the > code before sending patches to the ML. I did verify that the patch built with the driver configured as built-in. I'll investigate if there's a problem when built as a module. > Paolo
Hi Enguerrand,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net/master]
[also build test ERROR on net-next/master linus/master v6.1 next-20221220]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Enguerrand-de-Ribaucourt/net-lan78xx-prevent-LAN88XX-specific-operations/20221220-194004
patch link: https://lore.kernel.org/r/20221220113733.714233-1-enguerrand.de-ribaucourt%40savoirfairelinux.com
patch subject: [PATCH v2] net: lan78xx: prevent LAN88XX specific operations
config: m68k-allmodconfig
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/4939bdafcb43f018636507f375f617d8d9d7902e
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Enguerrand-de-Ribaucourt/net-lan78xx-prevent-LAN88XX-specific-operations/20221220-194004
git checkout 4939bdafcb43f018636507f375f617d8d9d7902e
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "phy_disable_interrupts" [drivers/net/usb/lan78xx.ko] undefined!
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c index f18ab8e220db..65d5d54994ff 100644 --- a/drivers/net/usb/lan78xx.c +++ b/drivers/net/usb/lan78xx.c @@ -28,6 +28,7 @@ #include <linux/phy_fixed.h> #include <linux/of_mdio.h> #include <linux/of_net.h> +#include <linux/phy.h> #include "lan78xx.h" #define DRIVER_AUTHOR "WOOJUNG HUH <woojung.huh@microchip.com>" @@ -2123,10 +2124,7 @@ static void lan78xx_link_status_change(struct net_device *net) * at forced 100 F/H mode. */ if (!phydev->autoneg && (phydev->speed == 100)) { - /* disable phy interrupt */ - temp = phy_read(phydev, LAN88XX_INT_MASK); - temp &= ~LAN88XX_INT_MASK_MDINTPIN_EN_; - phy_write(phydev, LAN88XX_INT_MASK, temp); + phy_disable_interrupts(phydev); temp = phy_read(phydev, MII_BMCR); temp &= ~(BMCR_SPEED100 | BMCR_SPEED1000); @@ -2134,13 +2132,7 @@ static void lan78xx_link_status_change(struct net_device *net) temp |= BMCR_SPEED100; phy_write(phydev, MII_BMCR, temp); /* set to 100 later */ - /* clear pending interrupt generated while workaround */ - temp = phy_read(phydev, LAN88XX_INT_STS); - - /* enable phy interrupt back */ - temp = phy_read(phydev, LAN88XX_INT_MASK); - temp |= LAN88XX_INT_MASK_MDINTPIN_EN_; - phy_write(phydev, LAN88XX_INT_MASK, temp); + phy_request_interrupt(phydev); } }
Some operations during the cable switch workaround modify the register LAN88XX_INT_MASK of the PHY. However, this register is specific to the LAN8835 PHY. For instance, if a DP8322I PHY is connected to the LAN7801, that register (0x19), corresponds to the LED and MAC address configuration, resulting in unapropriate behavior. Use the generic phy interrupt functions instead. Fixes: 89b36fb5e532 ("lan78xx: Lan7801 Support for Fixed PHY") Reviewed-by: Paolo Abeni <pabeni@redhat.com>; Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-ribaucourt@savoirfairelinux.com> --- drivers/net/usb/lan78xx.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-)