* [PATCH] staging: octeon-ethernet: trivial: Avoid OOPS if phydev is not set @ 2013-10-13 18:59 Sebastian Pöhn 2013-10-13 21:28 ` Greg KH 0 siblings, 1 reply; 7+ messages in thread From: Sebastian Pöhn @ 2013-10-13 18:59 UTC (permalink / raw) To: driverdev-devel; +Cc: support, netdev A zero pointer deref on priv->phydev->link was causing oops on our systems. Might be related to improper configuration but we should fail gracefully here ;-) Signed-off-by: Sebastian Poehn <sebastian.poehn@googlemail.com> --- diff --git a/drivers/staging/octeon/ethernet-mdio.c b/drivers/staging/octeon/ethernet-mdio.c index 83b1030..bc8c503 100644 --- a/drivers/staging/octeon/ethernet-mdio.c +++ b/drivers/staging/octeon/ethernet-mdio.c @@ -121,6 +121,9 @@ static void cvm_oct_adjust_link(struct net_device *dev) struct octeon_ethernet *priv = netdev_priv(dev); cvmx_helper_link_info_t link_info; + if(!priv->phydev) + return ; + if (priv->last_link != priv->phydev->link) { priv->last_link = priv->phydev->link; link_info.u64 = 0; ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: octeon-ethernet: trivial: Avoid OOPS if phydev is not set 2013-10-13 18:59 [PATCH] staging: octeon-ethernet: trivial: Avoid OOPS if phydev is not set Sebastian Pöhn @ 2013-10-13 21:28 ` Greg KH 2013-10-14 10:10 ` Dan Carpenter 0 siblings, 1 reply; 7+ messages in thread From: Greg KH @ 2013-10-13 21:28 UTC (permalink / raw) To: Sebastian Pöhn; +Cc: support, netdev, driverdev-devel On Sun, Oct 13, 2013 at 08:59:54PM +0200, Sebastian Pöhn wrote: > A zero pointer deref on priv->phydev->link was causing oops on our systems. > Might be related to improper configuration but we should fail gracefully here ;-) > > Signed-off-by: Sebastian Poehn <sebastian.poehn@googlemail.com> > > --- > > diff --git a/drivers/staging/octeon/ethernet-mdio.c b/drivers/staging/octeon/ethernet-mdio.c > index 83b1030..bc8c503 100644 > --- a/drivers/staging/octeon/ethernet-mdio.c > +++ b/drivers/staging/octeon/ethernet-mdio.c > @@ -121,6 +121,9 @@ static void cvm_oct_adjust_link(struct net_device *dev) > struct octeon_ethernet *priv = netdev_priv(dev); > cvmx_helper_link_info_t link_info; > > + if(!priv->phydev) > + return ; Please always run your patches through the scripts/checkpatch.pl tool so that maintainers don't have to point out the obvious coding syle errors by hand each time :) Care to try again? Also, how was phydev NULL? What was causing that? thanks, greg k-h ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: octeon-ethernet: trivial: Avoid OOPS if phydev is not set 2013-10-13 21:28 ` Greg KH @ 2013-10-14 10:10 ` Dan Carpenter 2013-10-14 18:39 ` Aaro Koskinen 0 siblings, 1 reply; 7+ messages in thread From: Dan Carpenter @ 2013-10-14 10:10 UTC (permalink / raw) To: Greg KH; +Cc: support, netdev, driverdev-devel, Sebastian Pöhn On Sun, Oct 13, 2013 at 02:28:10PM -0700, Greg KH wrote: > On Sun, Oct 13, 2013 at 08:59:54PM +0200, Sebastian Pöhn wrote: > > A zero pointer deref on priv->phydev->link was causing oops on our systems. > > Might be related to improper configuration but we should fail gracefully here ;-) > > > > Signed-off-by: Sebastian Poehn <sebastian.poehn@googlemail.com> > > > > --- > > > > diff --git a/drivers/staging/octeon/ethernet-mdio.c b/drivers/staging/octeon/ethernet-mdio.c > > index 83b1030..bc8c503 100644 > > --- a/drivers/staging/octeon/ethernet-mdio.c > > +++ b/drivers/staging/octeon/ethernet-mdio.c > > @@ -121,6 +121,9 @@ static void cvm_oct_adjust_link(struct net_device *dev) > > struct octeon_ethernet *priv = netdev_priv(dev); > > cvmx_helper_link_info_t link_info; > > > > + if(!priv->phydev) > > + return ; > > Please always run your patches through the scripts/checkpatch.pl tool so > that maintainers don't have to point out the obvious coding syle errors > by hand each time :) Also it's whitespace damaged and doesn't apply. > > Care to try again? > > Also, how was phydev NULL? What was causing that? To me it looks like phydev is always NULL. regards, dan carpenter ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: octeon-ethernet: trivial: Avoid OOPS if phydev is not set 2013-10-14 10:10 ` Dan Carpenter @ 2013-10-14 18:39 ` Aaro Koskinen 2013-10-14 19:16 ` Dan Carpenter 0 siblings, 1 reply; 7+ messages in thread From: Aaro Koskinen @ 2013-10-14 18:39 UTC (permalink / raw) To: Dan Carpenter Cc: Greg KH, support, netdev, driverdev-devel, Sebastian Pöhn, David Daney Hi, On Mon, Oct 14, 2013 at 01:10:51PM +0300, Dan Carpenter wrote: > On Sun, Oct 13, 2013 at 02:28:10PM -0700, Greg KH wrote: > > On Sun, Oct 13, 2013 at 08:59:54PM +0200, Sebastian Pöhn wrote: > > > A zero pointer deref on priv->phydev->link was causing oops on our systems. > > > Might be related to improper configuration but we should fail gracefully here ;-) > > > > > > Signed-off-by: Sebastian Poehn <sebastian.poehn@googlemail.com> > > > > > > --- > > > > > > diff --git a/drivers/staging/octeon/ethernet-mdio.c b/drivers/staging/octeon/ethernet-mdio.c > > > index 83b1030..bc8c503 100644 > > > --- a/drivers/staging/octeon/ethernet-mdio.c > > > +++ b/drivers/staging/octeon/ethernet-mdio.c > > > @@ -121,6 +121,9 @@ static void cvm_oct_adjust_link(struct net_device *dev) > > > struct octeon_ethernet *priv = netdev_priv(dev); > > > cvmx_helper_link_info_t link_info; > > > > > > + if(!priv->phydev) > > > + return ; > > > > Please always run your patches through the scripts/checkpatch.pl tool so > > that maintainers don't have to point out the obvious coding syle errors > > by hand each time :) > > Also it's whitespace damaged and doesn't apply. > > > > > Care to try again? > > > > Also, how was phydev NULL? What was causing that? > > To me it looks like phydev is always NULL. It's initialized in cvm_oct_phy_setup_device(): priv->phydev = of_phy_connect(dev, phy_node, cvm_oct_adjust_link, 0, PHY_INTERFACE_MODE_GMII); So maybe there is a chance that cvm_oct_adjust_link() callback gets called already before the function returns? Getting a copy of the original OOPS report/crash dump could help to confirm this. A. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: octeon-ethernet: trivial: Avoid OOPS if phydev is not set 2013-10-14 18:39 ` Aaro Koskinen @ 2013-10-14 19:16 ` Dan Carpenter 2013-10-14 19:49 ` Aaro Koskinen 0 siblings, 1 reply; 7+ messages in thread From: Dan Carpenter @ 2013-10-14 19:16 UTC (permalink / raw) To: Aaro Koskinen Cc: support, David Daney, Greg KH, driverdev-devel, Sebastian Pöhn, netdev On Mon, Oct 14, 2013 at 09:39:06PM +0300, Aaro Koskinen wrote: > Hi, > > On Mon, Oct 14, 2013 at 01:10:51PM +0300, Dan Carpenter wrote: > > On Sun, Oct 13, 2013 at 02:28:10PM -0700, Greg KH wrote: > > > On Sun, Oct 13, 2013 at 08:59:54PM +0200, Sebastian Pöhn wrote: > > > > A zero pointer deref on priv->phydev->link was causing oops on our systems. > > > > Might be related to improper configuration but we should fail gracefully here ;-) > > > > > > > > Signed-off-by: Sebastian Poehn <sebastian.poehn@googlemail.com> > > > > > > > > --- > > > > > > > > diff --git a/drivers/staging/octeon/ethernet-mdio.c b/drivers/staging/octeon/ethernet-mdio.c > > > > index 83b1030..bc8c503 100644 > > > > --- a/drivers/staging/octeon/ethernet-mdio.c > > > > +++ b/drivers/staging/octeon/ethernet-mdio.c > > > > @@ -121,6 +121,9 @@ static void cvm_oct_adjust_link(struct net_device *dev) > > > > struct octeon_ethernet *priv = netdev_priv(dev); > > > > cvmx_helper_link_info_t link_info; > > > > > > > > + if(!priv->phydev) > > > > + return ; > > > > > > Please always run your patches through the scripts/checkpatch.pl tool so > > > that maintainers don't have to point out the obvious coding syle errors > > > by hand each time :) > > > > Also it's whitespace damaged and doesn't apply. > > > > > > > > Care to try again? > > > > > > Also, how was phydev NULL? What was causing that? > > > > To me it looks like phydev is always NULL. > > It's initialized in cvm_oct_phy_setup_device(): > > priv->phydev = of_phy_connect(dev, phy_node, cvm_oct_adjust_link, 0, ^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^ Sorry I should have explained better. We use cvm_oct_adjust_link() to initialize priv->phydev but cvm_oct_adjust_link() depends on priv->phydev. It seems like we would hit the NULL dereference every time. Weird huh? regards, dan carpenter ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: octeon-ethernet: trivial: Avoid OOPS if phydev is not set 2013-10-14 19:16 ` Dan Carpenter @ 2013-10-14 19:49 ` Aaro Koskinen 2013-10-15 9:47 ` Dan Carpenter 0 siblings, 1 reply; 7+ messages in thread From: Aaro Koskinen @ 2013-10-14 19:49 UTC (permalink / raw) To: Dan Carpenter Cc: support, David Daney, Greg KH, driverdev-devel, Sebastian Pöhn, netdev On Mon, Oct 14, 2013 at 10:16:49PM +0300, Dan Carpenter wrote: > On Mon, Oct 14, 2013 at 09:39:06PM +0300, Aaro Koskinen wrote: > > It's initialized in cvm_oct_phy_setup_device(): > > > > priv->phydev = of_phy_connect(dev, phy_node, cvm_oct_adjust_link, 0, > ^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^ > > Sorry I should have explained better. > > We use cvm_oct_adjust_link() to initialize priv->phydev but > cvm_oct_adjust_link() depends on priv->phydev. It seems like we would > hit the NULL dereference every time. Weird huh? It doesn't happen on my system (EdgeRouter Lite). I think you need to explain even more better. :-) What you mean by "We use cvm_oct_adjust_link() to initialize priv->phydev..."? Sorry, maybe I'm just missing something really obvious... A. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: octeon-ethernet: trivial: Avoid OOPS if phydev is not set 2013-10-14 19:49 ` Aaro Koskinen @ 2013-10-15 9:47 ` Dan Carpenter 0 siblings, 0 replies; 7+ messages in thread From: Dan Carpenter @ 2013-10-15 9:47 UTC (permalink / raw) To: Aaro Koskinen Cc: support, David Daney, Greg KH, driverdev-devel, Sebastian Pöhn, netdev On Mon, Oct 14, 2013 at 10:49:37PM +0300, Aaro Koskinen wrote: > On Mon, Oct 14, 2013 at 10:16:49PM +0300, Dan Carpenter wrote: > > On Mon, Oct 14, 2013 at 09:39:06PM +0300, Aaro Koskinen wrote: > > > It's initialized in cvm_oct_phy_setup_device(): > > > > > > priv->phydev = of_phy_connect(dev, phy_node, cvm_oct_adjust_link, 0, > > ^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^ > > > > Sorry I should have explained better. > > > > We use cvm_oct_adjust_link() to initialize priv->phydev but > > cvm_oct_adjust_link() depends on priv->phydev. It seems like we would > > hit the NULL dereference every time. Weird huh? > > It doesn't happen on my system (EdgeRouter Lite). I think you need to > explain even more better. :-) > > What you mean by "We use cvm_oct_adjust_link() to initialize > priv->phydev..."? Sorry, maybe I'm just missing something really > obvious... Hm... I didn't followed the state machine all the way through so I'm not actually sure how this is called. But it's weird that priv->phydev is initialized on the left side of the assignment but dereferenced in the function mentioned on the right side if the assignment. Also this patch should not be marked as "trivial" in the subject. "trivial" is only for spelling mistakes in comments etc. regards, dan carpenter ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-10-15 9:52 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-10-13 18:59 [PATCH] staging: octeon-ethernet: trivial: Avoid OOPS if phydev is not set Sebastian Pöhn 2013-10-13 21:28 ` Greg KH 2013-10-14 10:10 ` Dan Carpenter 2013-10-14 18:39 ` Aaro Koskinen 2013-10-14 19:16 ` Dan Carpenter 2013-10-14 19:49 ` Aaro Koskinen 2013-10-15 9:47 ` Dan Carpenter
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).