[PATCH net 0/2] sh_eth fixes

[PATCH net 0/2] sh_eth fixes

[Ben Hutchings]

I'm currently looking at Ethernet support on the R-Car H2 chip,
reviewing and testing the sh_eth driver.  Here are fixes for two fairly
obvious bugs in the driver; I will probably have some more later.

These are not tested on any of the other supported chips.

Ben.

Ben Hutchings (2):
  sh_eth: Fix promiscuous mode on chips without TSU
  sh_eth: Fix ethtool operation crash when net device is down

 drivers/net/ethernet/renesas/sh_eth.c |   28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

-- 
1.7.10.4

[PATCH net 1/2] sh_eth: Fix promiscuous mode on chips without TSU

[Ben Hutchings]

Currently net_device_ops::set_rx_mode is only implemented for
chips with a TSU (multiple address table).  However we do need
to turn the PRM (promiscuous) flag on and off for other chips.

- Remove the unlikely() from the TSU functions that we may safely
  call for chips without a TSU
- Make setting of the MCT flag conditional on the tsu capability flag
- Rename sh_eth_set_multicast_list() to sh_eth_set_rx_mode() and plumb
  it into both net_device_ops structures
- Remove the previously-unreachable branch in sh_eth_rx_mode() that
  would otherwise reset the flags to defaults for non-TSU chips

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
 drivers/net/ethernet/renesas/sh_eth.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 167737f..0c4d5b5 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -2417,7 +2417,7 @@ static int sh_eth_tsu_purge_all(struct net_device *ndev)
 	struct sh_eth_private *mdp = netdev_priv(ndev);
 	int i, ret;
 
-	if (unlikely(!mdp->cd->tsu))
+	if (!mdp->cd->tsu)
 		return 0;
 
 	for (i = 0; i < SH_ETH_TSU_CAM_ENTRIES; i++) {
@@ -2440,7 +2440,7 @@ static void sh_eth_tsu_purge_mcast(struct net_device *ndev)
 	void *reg_offset = sh_eth_tsu_get_offset(mdp, TSU_ADRH0);
 	int i;
 
-	if (unlikely(!mdp->cd->tsu))
+	if (!mdp->cd->tsu)
 		return;
 
 	for (i = 0; i < SH_ETH_TSU_CAM_ENTRIES; i++, reg_offset += 8) {
@@ -2450,8 +2450,8 @@ static void sh_eth_tsu_purge_mcast(struct net_device *ndev)
 	}
 }
 
-/* Multicast reception directions set */
-static void sh_eth_set_multicast_list(struct net_device *ndev)
+/* Update promiscuous flag and multicast filter */
+static void sh_eth_set_rx_mode(struct net_device *ndev)
 {
 	struct sh_eth_private *mdp = netdev_priv(ndev);
 	u32 ecmr_bits;
@@ -2462,7 +2462,9 @@ static void sh_eth_set_multicast_list(struct net_device *ndev)
 	/* Initial condition is MCT = 1, PRM = 0.
 	 * Depending on ndev->flags, set PRM or clear MCT
 	 */
-	ecmr_bits = (sh_eth_read(ndev, ECMR) & ~ECMR_PRM) | ECMR_MCT;
+	ecmr_bits = sh_eth_read(ndev, ECMR) & ~ECMR_PRM;
+	if (mdp->cd->tsu)
+		ecmr_bits |= ECMR_MCT;
 
 	if (!(ndev->flags & IFF_MULTICAST)) {
 		sh_eth_tsu_purge_mcast(ndev);
@@ -2491,9 +2493,6 @@ static void sh_eth_set_multicast_list(struct net_device *ndev)
 				}
 			}
 		}
-	} else {
-		/* Normal, unicast/broadcast-only mode. */
-		ecmr_bits = (ecmr_bits & ~ECMR_PRM) | ECMR_MCT;
 	}
 
 	/* update the ethernet mode */
@@ -2701,6 +2700,7 @@ static const struct net_device_ops sh_eth_netdev_ops = {
 	.ndo_stop		= sh_eth_close,
 	.ndo_start_xmit		= sh_eth_start_xmit,
 	.ndo_get_stats		= sh_eth_get_stats,
+	.ndo_set_rx_mode	= sh_eth_set_rx_mode,
 	.ndo_tx_timeout		= sh_eth_tx_timeout,
 	.ndo_do_ioctl		= sh_eth_do_ioctl,
 	.ndo_validate_addr	= eth_validate_addr,
@@ -2713,7 +2713,7 @@ static const struct net_device_ops sh_eth_netdev_ops_tsu = {
 	.ndo_stop		= sh_eth_close,
 	.ndo_start_xmit		= sh_eth_start_xmit,
 	.ndo_get_stats		= sh_eth_get_stats,
-	.ndo_set_rx_mode	= sh_eth_set_multicast_list,
+	.ndo_set_rx_mode	= sh_eth_set_rx_mode,
 	.ndo_vlan_rx_add_vid	= sh_eth_vlan_rx_add_vid,
 	.ndo_vlan_rx_kill_vid	= sh_eth_vlan_rx_kill_vid,
 	.ndo_tx_timeout		= sh_eth_tx_timeout,
-- 
1.7.10.4

[PATCH net 2/2] sh_eth: Fix ethtool operation crash when net device is down

[Ben Hutchings]

The driver connects and disconnects the PHY device whenever the
net device is brought up and down.  The ethtool get_settings,
set_settings and nway_reset operations will dereference a null
or dangling pointer if called while it is down.

I think it would be preferable to keep the PHY connected, but there
may be good reasons not to.

As an immediate fix for this bug:
- Set the phydev pointer to NULL after disconnecting the PHY
- Change those three operations to return -ENODEV while the PHY is
  not connected

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
 drivers/net/ethernet/renesas/sh_eth.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 0c4d5b5..28e3822 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -1827,6 +1827,9 @@ static int sh_eth_get_settings(struct net_device *ndev,
 	unsigned long flags;
 	int ret;
 
+	if (!mdp->phydev)
+		return -ENODEV;
+
 	spin_lock_irqsave(&mdp->lock, flags);
 	ret = phy_ethtool_gset(mdp->phydev, ecmd);
 	spin_unlock_irqrestore(&mdp->lock, flags);
@@ -1841,6 +1844,9 @@ static int sh_eth_set_settings(struct net_device *ndev,
 	unsigned long flags;
 	int ret;
 
+	if (!mdp->phydev)
+		return -ENODEV;
+
 	spin_lock_irqsave(&mdp->lock, flags);
 
 	/* disable tx and rx */
@@ -1875,6 +1881,9 @@ static int sh_eth_nway_reset(struct net_device *ndev)
 	unsigned long flags;
 	int ret;
 
+	if (!mdp->phydev)
+		return -ENODEV;
+
 	spin_lock_irqsave(&mdp->lock, flags);
 	ret = phy_start_aneg(mdp->phydev);
 	spin_unlock_irqrestore(&mdp->lock, flags);
@@ -2184,6 +2193,7 @@ static int sh_eth_close(struct net_device *ndev)
 	if (mdp->phydev) {
 		phy_stop(mdp->phydev);
 		phy_disconnect(mdp->phydev);
+		mdp->phydev = NULL;
 	}
 
 	free_irq(ndev->irq, ndev);
-- 
1.7.10.4

Re: [PATCH net 2/2] sh_eth: Fix ethtool operation crash when net device is down

[Florian Fainelli]

2015-01-16 9:51 GMT-08:00 Ben Hutchings <ben.hutchings@codethink.co.uk>:
> The driver connects and disconnects the PHY device whenever the
> net device is brought up and down.  The ethtool get_settings,
> set_settings and nway_reset operations will dereference a null
> or dangling pointer if called while it is down.
>
> I think it would be preferable to keep the PHY connected, but there
> may be good reasons not to.

phy_disconnect() is the canonical way to stop the PHY library state
machine, and avoid deferred work to be done and call the driver's
adjust_link function. This also boils down to calling phy_detach()
which can put the PHY in a low-power mode when implemented.

>
> As an immediate fix for this bug:
> - Set the phydev pointer to NULL after disconnecting the PHY
> - Change those three operations to return -ENODEV while the PHY is
>   not connected
>
> Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> ---
>  drivers/net/ethernet/renesas/sh_eth.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> index 0c4d5b5..28e3822 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> @@ -1827,6 +1827,9 @@ static int sh_eth_get_settings(struct net_device *ndev,
>         unsigned long flags;
>         int ret;
>
> +       if (!mdp->phydev)
> +               return -ENODEV;

Since the PHY is disconnected, would not checking for netif_running()
make sense here, unless there is a good reason to still allow
phy_ethtool_gset() to be called?

> +
>         spin_lock_irqsave(&mdp->lock, flags);
>         ret = phy_ethtool_gset(mdp->phydev, ecmd);
>         spin_unlock_irqrestore(&mdp->lock, flags);
> @@ -1841,6 +1844,9 @@ static int sh_eth_set_settings(struct net_device *ndev,
>         unsigned long flags;
>         int ret;
>
> +       if (!mdp->phydev)
> +               return -ENODEV;
> +
>         spin_lock_irqsave(&mdp->lock, flags);
>
>         /* disable tx and rx */
> @@ -1875,6 +1881,9 @@ static int sh_eth_nway_reset(struct net_device *ndev)
>         unsigned long flags;
>         int ret;
>
> +       if (!mdp->phydev)
> +               return -ENODEV;
> +
>         spin_lock_irqsave(&mdp->lock, flags);
>         ret = phy_start_aneg(mdp->phydev);
>         spin_unlock_irqrestore(&mdp->lock, flags);
> @@ -2184,6 +2193,7 @@ static int sh_eth_close(struct net_device *ndev)
>         if (mdp->phydev) {
>                 phy_stop(mdp->phydev);
>                 phy_disconnect(mdp->phydev);
> +               mdp->phydev = NULL;
>         }
>
>         free_irq(ndev->irq, ndev);
> --
> 1.7.10.4
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Florian

Re: [PATCH net 1/2] sh_eth: Fix promiscuous mode on chips without TSU

[Sergei Shtylyov]

Hello.

On 01/16/2015 08:51 PM, Ben Hutchings wrote:

> Currently net_device_ops::set_rx_mode is only implemented for
> chips with a TSU (multiple address table).  However we do need
> to turn the PRM (promiscuous) flag on and off for other chips.

> - Remove the unlikely() from the TSU functions that we may safely
>    call for chips without a TSU

    This is just optimization, worth pushing thru net-next instead.

> - Make setting of the MCT flag conditional on the tsu capability flag
> - Rename sh_eth_set_multicast_list() to sh_eth_set_rx_mode() and plumb
>    it into both net_device_ops structures
> - Remove the previously-unreachable branch in sh_eth_rx_mode() that
>    would otherwise reset the flags to defaults for non-TSU chips

    It couldn't be default for non-TSU chips, they don't seem to have 
ECMR.MCT. So it was just wrong.

    It would have been better if you did one thing per patch or at least 
didn't mix fixes with clean-ups...

> Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> ---
>   drivers/net/ethernet/renesas/sh_eth.c |   18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)

> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> index 167737f..0c4d5b5 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> @@ -2417,7 +2417,7 @@ static int sh_eth_tsu_purge_all(struct net_device *ndev)
>   	struct sh_eth_private *mdp = netdev_priv(ndev);
>   	int i, ret;
>
> -	if (unlikely(!mdp->cd->tsu))
> +	if (!mdp->cd->tsu)
>   		return 0;
>
>   	for (i = 0; i < SH_ETH_TSU_CAM_ENTRIES; i++) {
> @@ -2440,7 +2440,7 @@ static void sh_eth_tsu_purge_mcast(struct net_device *ndev)
>   	void *reg_offset = sh_eth_tsu_get_offset(mdp, TSU_ADRH0);
>   	int i;
>
> -	if (unlikely(!mdp->cd->tsu))
> +	if (!mdp->cd->tsu)

    But we don't call this function on non-TSU SoCs, do we?

[...]

> @@ -2462,7 +2462,9 @@ static void sh_eth_set_multicast_list(struct net_device *ndev)
>   	/* Initial condition is MCT = 1, PRM = 0.
>   	 * Depending on ndev->flags, set PRM or clear MCT
>   	 */
> -	ecmr_bits = (sh_eth_read(ndev, ECMR) & ~ECMR_PRM) | ECMR_MCT;
> +	ecmr_bits = sh_eth_read(ndev, ECMR) & ~ECMR_PRM;
> +	if (mdp->cd->tsu)
> +		ecmr_bits |= ECMR_MCT;

    Seems OK, looking at the RZ/A1H manuals (this SoC does have TSU and this 
bit too).

[...]

WBR, Sergei

Re: [PATCH net 1/2] sh_eth: Fix promiscuous mode on chips without TSU

[Sergei Shtylyov]

On 01/16/2015 10:27 PM, Sergei Shtylyov wrote:

>> Currently net_device_ops::set_rx_mode is only implemented for
>> chips with a TSU (multiple address table).  However we do need
>> to turn the PRM (promiscuous) flag on and off for other chips.

[...]

>> Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>

[...]

>> diff --git a/drivers/net/ethernet/renesas/sh_eth.c
>> b/drivers/net/ethernet/renesas/sh_eth.c
>> index 167737f..0c4d5b5 100644
>> --- a/drivers/net/ethernet/renesas/sh_eth.c
>> +++ b/drivers/net/ethernet/renesas/sh_eth.c
[...]
>> @@ -2440,7 +2440,7 @@ static void sh_eth_tsu_purge_mcast(struct net_device
>> *ndev)
>>       void *reg_offset = sh_eth_tsu_get_offset(mdp, TSU_ADRH0);
>>       int i;
>>
>> -    if (unlikely(!mdp->cd->tsu))
>> +    if (!mdp->cd->tsu)

>     But we don't call this function on non-TSU SoCs, do we?

    Sorry, we do.

> [...]

WBR, Sergei

Re: [PATCH net 2/2] sh_eth: Fix ethtool operation crash when net device is down

[Ben Hutchings]

On Fri, 2015-01-16 at 10:45 -0800, Florian Fainelli wrote:
> 2015-01-16 9:51 GMT-08:00 Ben Hutchings <ben.hutchings@codethink.co.uk>:
[...]
> > --- a/drivers/net/ethernet/renesas/sh_eth.c
> > +++ b/drivers/net/ethernet/renesas/sh_eth.c
> > @@ -1827,6 +1827,9 @@ static int sh_eth_get_settings(struct net_device *ndev,
> >         unsigned long flags;
> >         int ret;
> >
> > +       if (!mdp->phydev)
> > +               return -ENODEV;
> 
> Since the PHY is disconnected, would not checking for netif_running()
> make sense here, unless there is a good reason to still allow
> phy_ethtool_gset() to be called?
[...]

I think those two conditions will be equivalent, won't they?  Writing
the condition like this will also work if the driver later supports
PHY-less configurations.

Ben.

Re: [PATCH net 1/2] sh_eth: Fix promiscuous mode on chips without TSU

[Ben Hutchings]

On Fri, 2015-01-16 at 22:27 +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 01/16/2015 08:51 PM, Ben Hutchings wrote:
> 
> > Currently net_device_ops::set_rx_mode is only implemented for
> > chips with a TSU (multiple address table).  However we do need
> > to turn the PRM (promiscuous) flag on and off for other chips.
> 
> > - Remove the unlikely() from the TSU functions that we may safely
> >    call for chips without a TSU
> 
>     This is just optimization, worth pushing thru net-next instead.

This patch introduces calls to those functions for chips without a TSU,
and that makes the branch hint no longer correct.  (Not that these
functions are speed-critical, so it doesn't matter that much.)

> > - Make setting of the MCT flag conditional on the tsu capability flag
> > - Rename sh_eth_set_multicast_list() to sh_eth_set_rx_mode() and plumb
> >    it into both net_device_ops structures
> > - Remove the previously-unreachable branch in sh_eth_rx_mode() that
> >    would otherwise reset the flags to defaults for non-TSU chips
> 
>     It couldn't be default for non-TSU chips, they don't seem to have 
> ECMR.MCT. So it was just wrong.
> 
>     It would have been better if you did one thing per patch or at least 
> didn't mix fixes with clean-ups...
[...]

I think this is one logical change.

Ben.

Re: [PATCH net 2/2] sh_eth: Fix ethtool operation crash when net device is down

[Florian Fainelli]

On 19/01/15 02:41, Ben Hutchings wrote:
> On Fri, 2015-01-16 at 10:45 -0800, Florian Fainelli wrote:
>> 2015-01-16 9:51 GMT-08:00 Ben Hutchings <ben.hutchings@codethink.co.uk>:
> [...]
>>> --- a/drivers/net/ethernet/renesas/sh_eth.c
>>> +++ b/drivers/net/ethernet/renesas/sh_eth.c
>>> @@ -1827,6 +1827,9 @@ static int sh_eth_get_settings(struct net_device *ndev,
>>>         unsigned long flags;
>>>         int ret;
>>>
>>> +       if (!mdp->phydev)
>>> +               return -ENODEV;
>>
>> Since the PHY is disconnected, would not checking for netif_running()
>> make sense here, unless there is a good reason to still allow
>> phy_ethtool_gset() to be called?
> [...]
> 
> I think those two conditions will be equivalent, won't they?

They are indeed.

>  Writing the condition like this will also work if the driver later supports
> PHY-less configurations.

You could also represent a PHY-less configuration by using the emulated
fixed-PHY, such that the driver is effectively "driving" a PHY device,
even though this is not a real one, up to you.
-- 
Florian

Re: [PATCH net 0/2] sh_eth fixes

[David Miller]

From: Ben Hutchings <ben.hutchings@codethink.co.uk>
Date: Fri, 16 Jan 2015 17:49:52 +0000

> I'm currently looking at Ethernet support on the R-Car H2 chip,
> reviewing and testing the sh_eth driver.  Here are fixes for two fairly
> obvious bugs in the driver; I will probably have some more later.
> 
> These are not tested on any of the other supported chips.

Series applied, thanks Ben.