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

[Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>]

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