Patch: Fix fec_mpc52xx driver to use net_device_ops

Patch: Fix fec_mpc52xx driver to use net_device_ops

[Henk Stegeman]

Fix fec_mpc52xx driver to use net_device_ops and to be careful not to
dereference phy_device if a phy has not yet been connected.

Signed-off-by: Henk Stegeman <henk.stegeman@gmail.com>

diff --git a/drivers/net/fec_mpc52xx.c b/drivers/net/fec_mpc52xx.c
index cd8e98b..ca76b95 100644
--- a/drivers/net/fec_mpc52xx.c
+++ b/drivers/net/fec_mpc52xx.c
@@ -847,24 +847,40 @@ static void mpc52xx_fec_get_drvinfo(struct
net_device *dev,
 static int mpc52xx_fec_get_settings(struct net_device *dev, struct
ethtool_cmd *cmd)
 {
 	struct mpc52xx_fec_priv *priv = netdev_priv(dev);
+
+	if (!priv->phydev)
+			return -ENODEV;
+
 	return phy_ethtool_gset(priv->phydev, cmd);
 }

 static int mpc52xx_fec_set_settings(struct net_device *dev, struct
ethtool_cmd *cmd)
 {
 	struct mpc52xx_fec_priv *priv = netdev_priv(dev);
+
+	if (!priv->phydev)
+			return -ENODEV;
+
 	return phy_ethtool_sset(priv->phydev, cmd);
 }

 static u32 mpc52xx_fec_get_msglevel(struct net_device *dev)
 {
 	struct mpc52xx_fec_priv *priv = netdev_priv(dev);
+
+	if (!priv->phydev)
+		return 0;
+
 	return priv->msg_enable;
 }

 static void mpc52xx_fec_set_msglevel(struct net_device *dev, u32 level)
 {
 	struct mpc52xx_fec_priv *priv = netdev_priv(dev);
+
+	if (!priv->phydev)
+			return;
+
 	priv->msg_enable = level;
 }

@@ -882,12 +898,31 @@ static int mpc52xx_fec_ioctl(struct net_device
*dev, struct ifreq *rq, int cmd)
 {
 	struct mpc52xx_fec_priv *priv = netdev_priv(dev);

+	if (!priv->phydev)
+			return -ENODEV;
+
 	return mpc52xx_fec_phy_mii_ioctl(priv, if_mii(rq), cmd);
 }

 /* ======================================================================== */
 /* OF Driver                                                                */
 /* ======================================================================== */
+static const struct net_device_ops mpc52xx_fec_netdev_ops = {
+       .ndo_open               = mpc52xx_fec_open,
+       .ndo_stop               = mpc52xx_fec_close,
+       .ndo_start_xmit         = mpc52xx_fec_hard_start_xmit,
+       .ndo_tx_timeout         = mpc52xx_fec_tx_timeout,
+       .ndo_get_stats          = mpc52xx_fec_get_stats,
+       .ndo_set_multicast_list = mpc52xx_fec_set_multicast_list,
+       .ndo_validate_addr      = eth_validate_addr,
+       .ndo_set_mac_address    = mpc52xx_fec_set_mac_address,
+       .ndo_do_ioctl           = mpc52xx_fec_ioctl,
+
+#ifdef CONFIG_NET_POLL_CONTROLLER
+       .ndo_poll_controller     = mpc52xx_fec_poll_controller,
+#endif
+};
+

 static int __devinit
 mpc52xx_fec_probe(struct of_device *op, const struct of_device_id *match)
@@ -929,20 +964,10 @@ mpc52xx_fec_probe(struct of_device *op, const
struct of_device_id *match)
 		return -EBUSY;

 	/* Init ether ndev with what we have */
-	ndev->open		= mpc52xx_fec_open;
-	ndev->stop		= mpc52xx_fec_close;
-	ndev->hard_start_xmit	= mpc52xx_fec_hard_start_xmit;
-	ndev->do_ioctl		= mpc52xx_fec_ioctl;
 	ndev->ethtool_ops	= &mpc52xx_fec_ethtool_ops;
-	ndev->get_stats		= mpc52xx_fec_get_stats;
-	ndev->set_mac_address	= mpc52xx_fec_set_mac_address;
-	ndev->set_multicast_list = mpc52xx_fec_set_multicast_list;
-	ndev->tx_timeout	= mpc52xx_fec_tx_timeout;
 	ndev->watchdog_timeo	= FEC_WATCHDOG_TIMEOUT;
 	ndev->base_addr		= mem.start;
-#ifdef CONFIG_NET_POLL_CONTROLLER
-	ndev->poll_controller = mpc52xx_fec_poll_controller;
-#endif
+	ndev->netdev_ops = &mpc52xx_fec_netdev_ops;

 	priv->t_irq = priv->r_irq = ndev->irq = NO_IRQ; /* IRQ are free for now */

Re: Patch: Fix fec_mpc52xx driver to use net_device_ops

[Stephen Hemminger]

On Tue, 31 Mar 2009 12:44:15 +0200
Henk Stegeman <henk.stegeman@gmail.com> wrote:

> Fix fec_mpc52xx driver to use net_device_ops and to be careful not to
> dereference phy_device if a phy has not yet been connected.
> 
> Signed-off-by: Henk Stegeman <henk.stegeman@gmail.com>
> 
> diff --git a/drivers/net/fec_mpc52xx.c b/drivers/net/fec_mpc52xx.c
> index cd8e98b..ca76b95 100644
> --- a/drivers/net/fec_mpc52xx.c
> +++ b/drivers/net/fec_mpc52xx.c
> @@ -847,24 +847,40 @@ static void mpc52xx_fec_get_drvinfo(struct
> net_device *dev,
>  static int mpc52xx_fec_get_settings(struct net_device *dev, struct
> ethtool_cmd *cmd)
>  {
>  	struct mpc52xx_fec_priv *priv = netdev_priv(dev);
> +
> +	if (!priv->phydev)
> +			return -ENODEV;
> +
>  	return phy_ethtool_gset(priv->phydev, cmd);
>  }
> 
>  static int mpc52xx_fec_set_settings(struct net_device *dev, struct
> ethtool_cmd *cmd)
>  {
>  	struct mpc52xx_fec_priv *priv = netdev_priv(dev);
> +
> +	if (!priv->phydev)
> +			return -ENODEV;
> +
>  	return phy_ethtool_sset(priv->phydev, cmd);
>  }
> 
>  static u32 mpc52xx_fec_get_msglevel(struct net_device *dev)
>  {
>  	struct mpc52xx_fec_priv *priv = netdev_priv(dev);
> +
> +	if (!priv->phydev)
> +		return 0;
> +
>  	return priv->msg_enable;
>  }
> 
>  static void mpc52xx_fec_set_msglevel(struct net_device *dev, u32 level)
>  {
>  	struct mpc52xx_fec_priv *priv = netdev_priv(dev);
> +
> +	if (!priv->phydev)
> +			return;
> +
>  	priv->msg_enable = level;
>  }
> 
> @@ -882,12 +898,31 @@ static int mpc52xx_fec_ioctl(struct net_device
> *dev, struct ifreq *rq, int cmd)
>  {
>  	struct mpc52xx_fec_priv *priv = netdev_priv(dev);
> 
> +	if (!priv->phydev)
> +			return -ENODEV;
> +
>  	return mpc52xx_fec_phy_mii_ioctl(priv, if_mii(rq), cmd);
>  }
> 
>  /* ======================================================================== */
>  /* OF Driver                                                                */
>  /* ======================================================================== */
> +static const struct net_device_ops mpc52xx_fec_netdev_ops = {
> +       .ndo_open               = mpc52xx_fec_open,
> +       .ndo_stop               = mpc52xx_fec_close,
> +       .ndo_start_xmit         = mpc52xx_fec_hard_start_xmit,
> +       .ndo_tx_timeout         = mpc52xx_fec_tx_timeout,
> +       .ndo_get_stats          = mpc52xx_fec_get_stats,
> +       .ndo_set_multicast_list = mpc52xx_fec_set_multicast_list,
> +       .ndo_validate_addr      = eth_validate_addr,
> +       .ndo_set_mac_address    = mpc52xx_fec_set_mac_address,
> +       .ndo_do_ioctl           = mpc52xx_fec_ioctl,
> 
What about change_mtu? Don't you want:
          .ndo_change_mtu         = eth_change_mtu,

Re: Patch: Fix fec_mpc52xx driver to use net_device_ops

[Grant Likely]

On Tue, Mar 31, 2009 at 8:48 AM, Stephen Hemminger
<shemminger@vyatta.com> wrote:
> On Tue, 31 Mar 2009 12:44:15 +0200
> Henk Stegeman <henk.stegeman@gmail.com> wrote:
>
>> Fix fec_mpc52xx driver to use net_device_ops and to be careful not to
>> dereference phy_device if a phy has not yet been connected.
>>
>> Signed-off-by: Henk Stegeman <henk.stegeman@gmail.com>

Hi Henk,

I hadn't heard from you for a while about the signed-off-by line, but
I really needed to get those changes queued because the 2.6.30 merge
window is open now, and some of my patches depend on it.  I ended up
rewriting your patch from scratch, testing it and posting it last
night.

I would really like to stick with the rewritten version since it is a
little tighter, and it is the one I've got tested.  However, now that
I've heard from you, I'm happy to change the patch author credit to
you.  You should look on the list for the patches I sent out last
night (labeled [PATCH 02/14] and [PATCH 03/14]) and reply to each of
them with an "Acked-by:" line if they look good to you.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: Patch: Fix fec_mpc52xx driver to use net_device_ops

[Grant Likely]

On Tue, Mar 31, 2009 at 8:48 AM, Stephen Hemminger
<shemminger@vyatta.com> wrote:
> On Tue, 31 Mar 2009 12:44:15 +0200
>> +static const struct net_device_ops mpc52xx_fec_netdev_ops =3D {
>> + =A0 =A0 =A0 .ndo_open =A0 =A0 =A0 =A0 =A0 =A0 =A0 =3D mpc52xx_fec_open=
,
>> + =A0 =A0 =A0 .ndo_stop =A0 =A0 =A0 =A0 =A0 =A0 =A0 =3D mpc52xx_fec_clos=
e,
>> + =A0 =A0 =A0 .ndo_start_xmit =A0 =A0 =A0 =A0 =3D mpc52xx_fec_hard_start=
_xmit,
>> + =A0 =A0 =A0 .ndo_tx_timeout =A0 =A0 =A0 =A0 =3D mpc52xx_fec_tx_timeout=
,
>> + =A0 =A0 =A0 .ndo_get_stats =A0 =A0 =A0 =A0 =A0=3D mpc52xx_fec_get_stat=
s,
>> + =A0 =A0 =A0 .ndo_set_multicast_list =3D mpc52xx_fec_set_multicast_list=
,
>> + =A0 =A0 =A0 .ndo_validate_addr =A0 =A0 =A0=3D eth_validate_addr,
>> + =A0 =A0 =A0 .ndo_set_mac_address =A0 =A0=3D mpc52xx_fec_set_mac_addres=
s,
>> + =A0 =A0 =A0 .ndo_do_ioctl =A0 =A0 =A0 =A0 =A0 =3D mpc52xx_fec_ioctl,
>>
> What about change_mtu? Don't you want:
> =A0 =A0 =A0 =A0 =A0.ndo_change_mtu =A0 =A0 =A0 =A0 =3D eth_change_mtu,

Yes, you're right.  fixed.

Thanks,
g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.