diff mbox series

[net-next,v2,13/14] ravb: Update EMAC configuration mode comment

Message ID 20211010072920.20706-14-biju.das.jz@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Add functional support for Gigabit Ethernet driver | expand

Checks

Context Check Description
netdev/cover_letter success Series has a cover letter
netdev/fixes_present success Fixes tag not required for -next series
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success No Fixes tag
netdev/checkpatch warning WARNING: line length of 89 exceeds 80 columns
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success No static functions without inline keyword in header files

Commit Message

Biju Das Oct. 10, 2021, 7:29 a.m. UTC
Update EMAC configuration mode comment from "PAUSE prohibition"
to "EMAC Mode: PAUSE prohibition; Duplex; TX; RX; CRC Pass Through;
Promiscuous".

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
v1->v2:
 * No change
V1:
 * New patch.
---
 drivers/net/ethernet/renesas/ravb_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Sergey Shtylyov Oct. 10, 2021, 9:27 a.m. UTC | #1
On 10.10.2021 10:29, Biju Das wrote:

> Update EMAC configuration mode comment from "PAUSE prohibition"
> to "EMAC Mode: PAUSE prohibition; Duplex; TX; RX; CRC Pass Through;
> Promiscuous".
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> ---
> v1->v2:
>   * No change
> V1:
>   * New patch.
> ---
>   drivers/net/ethernet/renesas/ravb_main.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 9a770a05c017..b78aca235c37 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -519,7 +519,7 @@ static void ravb_emac_init_gbeth(struct net_device *ndev)
>   	/* Receive frame limit set register */
>   	ravb_write(ndev, GBETH_RX_BUFF_MAX + ETH_FCS_LEN, RFLR);
>   
> -	/* PAUSE prohibition */
> +	/* EMAC Mode: PAUSE prohibition; Duplex; TX; RX; CRC Pass Through; Promiscuous */

    Promiscuous mode, really? Why?!

>   	ravb_write(ndev, ECMR_ZPF | ((priv->duplex > 0) ? ECMR_DM : 0) |
>   			 ECMR_TE | ECMR_RE | ECMR_RCPT |
>   			 ECMR_TXF | ECMR_RXF | ECMR_PRM, ECMR);

MBR, Sergey
Biju Das Oct. 10, 2021, 9:37 a.m. UTC | #2
Hi Sergey,

> -----Original Message-----
> From: Sergei Shtylyov <sergei.shtylyov@gmail.com>
> Sent: 10 October 2021 10:28
> To: Biju Das <biju.das.jz@bp.renesas.com>; David S. Miller
> <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>
> Cc: Sergey Shtylyov <s.shtylyov@omp.ru>; Geert Uytterhoeven
> <geert+renesas@glider.be>; Sergey Shtylyov <s.shtylyov@omprussia.ru>; Adam
> Ford <aford173@gmail.com>; Andrew Lunn <andrew@lunn.ch>; Yuusuke Ashizuka
> <ashiduka@fujitsu.com>; Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com>; netdev@vger.kernel.org; linux-renesas-
> soc@vger.kernel.org; Chris Paterson <Chris.Paterson2@renesas.com>; Biju
> Das <biju.das@bp.renesas.com>; Prabhakar Mahadev Lad <prabhakar.mahadev-
> lad.rj@bp.renesas.com>
> Subject: Re: [PATCH net-next v2 13/14] ravb: Update EMAC configuration
> mode comment
> 
> On 10.10.2021 10:29, Biju Das wrote:
> 
> > Update EMAC configuration mode comment from "PAUSE prohibition"
> > to "EMAC Mode: PAUSE prohibition; Duplex; TX; RX; CRC Pass Through;
> > Promiscuous".
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> > ---
> > v1->v2:
> >   * No change
> > V1:
> >   * New patch.
> > ---
> >   drivers/net/ethernet/renesas/ravb_main.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> > b/drivers/net/ethernet/renesas/ravb_main.c
> > index 9a770a05c017..b78aca235c37 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -519,7 +519,7 @@ static void ravb_emac_init_gbeth(struct net_device
> *ndev)
> >   	/* Receive frame limit set register */
> >   	ravb_write(ndev, GBETH_RX_BUFF_MAX + ETH_FCS_LEN, RFLR);
> >
> > -	/* PAUSE prohibition */
> > +	/* EMAC Mode: PAUSE prohibition; Duplex; TX; RX; CRC Pass Through;
> > +Promiscuous */
> 
>     Promiscuous mode, really? Why?!

This is TOE related and is recommendation from BSP/HW team. If you think it is wrong.
I can take this out. Please let me know. Currently the board is booting and everything works without issues.

The meaning of promiscuous in H/W manual as follows.

Promiscuous Mode
1: All the frames except for PAUSE frame are received. Self-addressed unicast,
different address unicast, multicast, and broadcast frames are all transferred to
TOE. PAUSE frame reception is controlled by PFR bit.
0: Self-addressed unicast, multicast, and broadcast frames are received, then
transferred to TOE.

Regards,
Biju

> 
> >   	ravb_write(ndev, ECMR_ZPF | ((priv->duplex > 0) ? ECMR_DM : 0) |
> >   			 ECMR_TE | ECMR_RE | ECMR_RCPT |
> >   			 ECMR_TXF | ECMR_RXF | ECMR_PRM, ECMR);
> 
> MBR, Sergey
Sergey Shtylyov Oct. 10, 2021, 9:49 a.m. UTC | #3
On 10.10.2021 12:37, Biju Das wrote:
> Hi Sergey,
> 
>> -----Original Message-----
>> From: Sergei Shtylyov <sergei.shtylyov@gmail.com>
>> Sent: 10 October 2021 10:28
>> To: Biju Das <biju.das.jz@bp.renesas.com>; David S. Miller
>> <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>
>> Cc: Sergey Shtylyov <s.shtylyov@omp.ru>; Geert Uytterhoeven
>> <geert+renesas@glider.be>; Sergey Shtylyov <s.shtylyov@omprussia.ru>; Adam
>> Ford <aford173@gmail.com>; Andrew Lunn <andrew@lunn.ch>; Yuusuke Ashizuka
>> <ashiduka@fujitsu.com>; Yoshihiro Shimoda
>> <yoshihiro.shimoda.uh@renesas.com>; netdev@vger.kernel.org; linux-renesas-
>> soc@vger.kernel.org; Chris Paterson <Chris.Paterson2@renesas.com>; Biju
>> Das <biju.das@bp.renesas.com>; Prabhakar Mahadev Lad <prabhakar.mahadev-
>> lad.rj@bp.renesas.com>
>> Subject: Re: [PATCH net-next v2 13/14] ravb: Update EMAC configuration
>> mode comment
>>
>> On 10.10.2021 10:29, Biju Das wrote:
>>
>>> Update EMAC configuration mode comment from "PAUSE prohibition"
>>> to "EMAC Mode: PAUSE prohibition; Duplex; TX; RX; CRC Pass Through;
>>> Promiscuous".
>>>
>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
>>> Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>> ---
>>> v1->v2:
>>>    * No change
>>> V1:
>>>    * New patch.
>>> ---
>>>    drivers/net/ethernet/renesas/ravb_main.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
>>> b/drivers/net/ethernet/renesas/ravb_main.c
>>> index 9a770a05c017..b78aca235c37 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -519,7 +519,7 @@ static void ravb_emac_init_gbeth(struct net_device
>> *ndev)
>>>    	/* Receive frame limit set register */
>>>    	ravb_write(ndev, GBETH_RX_BUFF_MAX + ETH_FCS_LEN, RFLR);
>>>
>>> -	/* PAUSE prohibition */
>>> +	/* EMAC Mode: PAUSE prohibition; Duplex; TX; RX; CRC Pass Through;
>>> +Promiscuous */
>>
>>      Promiscuous mode, really? Why?!
> 
> This is TOE related 

    The promiscuous mode is supported by _all_ Ethernet controllers, I think.

> and is recommendation from BSP/HW team.

    On what grounds?

> If you think it is wrong.
> I can take this out. Please let me know. Currently the board is booting and everything works without issues.

    Please do take it out. It'll needlessly overload the controller when 
there's much traffic on the local network.

> The meaning of promiscuous in H/W manual as follows.

    I know what the promiscuous mode is. :-)
    It's needed by things like 'tcpdump' and normally shoild be off.

> Promiscuous Mode
> 1: All the frames except for PAUSE frame are received. Self-addressed unicast,
> different address unicast, multicast, and broadcast frames are all transferred to
> TOE. PAUSE frame reception is controlled by PFR bit.
> 0: Self-addressed unicast, multicast, and broadcast frames are received, then
> transferred to TOE.
> 
> Regards,
> Biju

MBR, Sergey
Biju Das Oct. 10, 2021, 10:56 a.m. UTC | #4
> -----Original Message-----
> From: Sergei Shtylyov <sergei.shtylyov@gmail.com>
> Sent: 10 October 2021 10:49
> To: Biju Das <biju.das.jz@bp.renesas.com>; David S. Miller
> <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>
> Cc: Sergey Shtylyov <s.shtylyov@omp.ru>; Geert Uytterhoeven
> <geert+renesas@glider.be>; Sergey Shtylyov <s.shtylyov@omprussia.ru>; Adam
> Ford <aford173@gmail.com>; Andrew Lunn <andrew@lunn.ch>; Yuusuke Ashizuka
> <ashiduka@fujitsu.com>; Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com>; netdev@vger.kernel.org; linux-renesas-
> soc@vger.kernel.org; Chris Paterson <Chris.Paterson2@renesas.com>; Biju
> Das <biju.das@bp.renesas.com>; Prabhakar Mahadev Lad <prabhakar.mahadev-
> lad.rj@bp.renesas.com>
> Subject: Re: [PATCH net-next v2 13/14] ravb: Update EMAC configuration
> mode comment
> 
> On 10.10.2021 12:37, Biju Das wrote:
> > Hi Sergey,
> >
> >> -----Original Message-----
> >> From: Sergei Shtylyov <sergei.shtylyov@gmail.com>
> >> Sent: 10 October 2021 10:28
> >> To: Biju Das <biju.das.jz@bp.renesas.com>; David S. Miller
> >> <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>
> >> Cc: Sergey Shtylyov <s.shtylyov@omp.ru>; Geert Uytterhoeven
> >> <geert+renesas@glider.be>; Sergey Shtylyov <s.shtylyov@omprussia.ru>;
> >> Adam Ford <aford173@gmail.com>; Andrew Lunn <andrew@lunn.ch>; Yuusuke
> >> Ashizuka <ashiduka@fujitsu.com>; Yoshihiro Shimoda
> >> <yoshihiro.shimoda.uh@renesas.com>; netdev@vger.kernel.org;
> >> linux-renesas- soc@vger.kernel.org; Chris Paterson
> >> <Chris.Paterson2@renesas.com>; Biju Das <biju.das@bp.renesas.com>;
> >> Prabhakar Mahadev Lad <prabhakar.mahadev- lad.rj@bp.renesas.com>
> >> Subject: Re: [PATCH net-next v2 13/14] ravb: Update EMAC
> >> configuration mode comment
> >>
> >> On 10.10.2021 10:29, Biju Das wrote:
> >>
> >>> Update EMAC configuration mode comment from "PAUSE prohibition"
> >>> to "EMAC Mode: PAUSE prohibition; Duplex; TX; RX; CRC Pass Through;
> >>> Promiscuous".
> >>>
> >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> >>> Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> >>> ---
> >>> v1->v2:
> >>>    * No change
> >>> V1:
> >>>    * New patch.
> >>> ---
> >>>    drivers/net/ethernet/renesas/ravb_main.c | 2 +-
> >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> >>> b/drivers/net/ethernet/renesas/ravb_main.c
> >>> index 9a770a05c017..b78aca235c37 100644
> >>> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> >>> @@ -519,7 +519,7 @@ static void ravb_emac_init_gbeth(struct
> >>> net_device
> >> *ndev)
> >>>    	/* Receive frame limit set register */
> >>>    	ravb_write(ndev, GBETH_RX_BUFF_MAX + ETH_FCS_LEN, RFLR);
> >>>
> >>> -	/* PAUSE prohibition */
> >>> +	/* EMAC Mode: PAUSE prohibition; Duplex; TX; RX; CRC Pass Through;
> >>> +Promiscuous */
> >>
> >>      Promiscuous mode, really? Why?!
> >
> > This is TOE related,

I meant the context here is TOE register related. That is what I meant.

> 
>     The promiscuous mode is supported by _all_ Ethernet controllers, I
> think.
> 
> > and is recommendation from BSP team.
> 
>     On what grounds?

The reference implementation has this on. Any way it is good catch. 
I will turn it off and check.

by looking at the RJ LED's there is not much activity and packet statistics also show not much activity by default.

How can we check, it is overloading the controller? So that I can compare with and without this setting

> 
> > If you think it is wrong.
> > I can take this out. Please let me know. Currently the board is booting
> and everything works without issues.
> 
>     Please do take it out. It'll needlessly overload the controller when
> there's much traffic on the local network.


I can see much activity only on RJ45 LED's when I call tcpdump or by setting IP link set eth0 promisc on.
Otherwise there is no traffic at all.

Regards,
Biju

> 
> > The meaning of promiscuous in H/W manual as follows.
> 
>     I know what the promiscuous mode is. :-)
>     It's needed by things like 'tcpdump' and normally shoild be off.
> 
> > Promiscuous Mode
> > 1: All the frames except for PAUSE frame are received. Self-addressed
> > unicast, different address unicast, multicast, and broadcast frames
> > are all transferred to TOE. PAUSE frame reception is controlled by PFR
> bit.
> > 0: Self-addressed unicast, multicast, and broadcast frames are
> > received, then transferred to TOE.



> >
> > Regards,
> > Biju
> 
> MBR, Sergey
Biju Das Oct. 10, 2021, 11:29 a.m. UTC | #5
Hi Sergey,

Thanks for the suggestion.

> Subject: RE: [PATCH net-next v2 13/14] ravb: Update EMAC configuration
> mode comment
> 
> 
> 
> > -----Original Message-----
> > From: Sergei Shtylyov <sergei.shtylyov@gmail.com>
> > Sent: 10 October 2021 10:49
> > To: Biju Das <biju.das.jz@bp.renesas.com>; David S. Miller
> > <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>
> > Cc: Sergey Shtylyov <s.shtylyov@omp.ru>; Geert Uytterhoeven
> > <geert+renesas@glider.be>; Sergey Shtylyov <s.shtylyov@omprussia.ru>;
> > Adam Ford <aford173@gmail.com>; Andrew Lunn <andrew@lunn.ch>; Yuusuke
> > Ashizuka <ashiduka@fujitsu.com>; Yoshihiro Shimoda
> > <yoshihiro.shimoda.uh@renesas.com>; netdev@vger.kernel.org;
> > linux-renesas- soc@vger.kernel.org; Chris Paterson
> > <Chris.Paterson2@renesas.com>; Biju Das <biju.das@bp.renesas.com>;
> > Prabhakar Mahadev Lad <prabhakar.mahadev- lad.rj@bp.renesas.com>
> > Subject: Re: [PATCH net-next v2 13/14] ravb: Update EMAC configuration
> > mode comment
> >
> > On 10.10.2021 12:37, Biju Das wrote:
> > > Hi Sergey,
> > >
> > >> -----Original Message-----
> > >> From: Sergei Shtylyov <sergei.shtylyov@gmail.com>
> > >> Sent: 10 October 2021 10:28
> > >> To: Biju Das <biju.das.jz@bp.renesas.com>; David S. Miller
> > >> <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>
> > >> Cc: Sergey Shtylyov <s.shtylyov@omp.ru>; Geert Uytterhoeven
> > >> <geert+renesas@glider.be>; Sergey Shtylyov
> > >> <s.shtylyov@omprussia.ru>; Adam Ford <aford173@gmail.com>; Andrew
> > >> Lunn <andrew@lunn.ch>; Yuusuke Ashizuka <ashiduka@fujitsu.com>;
> > >> Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>;
> > >> netdev@vger.kernel.org;
> > >> linux-renesas- soc@vger.kernel.org; Chris Paterson
> > >> <Chris.Paterson2@renesas.com>; Biju Das <biju.das@bp.renesas.com>;
> > >> Prabhakar Mahadev Lad <prabhakar.mahadev- lad.rj@bp.renesas.com>
> > >> Subject: Re: [PATCH net-next v2 13/14] ravb: Update EMAC
> > >> configuration mode comment
> > >>
> > >> On 10.10.2021 10:29, Biju Das wrote:
> > >>
> > >>> Update EMAC configuration mode comment from "PAUSE prohibition"
> > >>> to "EMAC Mode: PAUSE prohibition; Duplex; TX; RX; CRC Pass
> > >>> Through; Promiscuous".
> > >>>
> > >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > >>> Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> > >>> ---
> > >>> v1->v2:
> > >>>    * No change
> > >>> V1:
> > >>>    * New patch.
> > >>> ---
> > >>>    drivers/net/ethernet/renesas/ravb_main.c | 2 +-
> > >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> > >>> b/drivers/net/ethernet/renesas/ravb_main.c
> > >>> index 9a770a05c017..b78aca235c37 100644
> > >>> --- a/drivers/net/ethernet/renesas/ravb_main.c
> > >>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > >>> @@ -519,7 +519,7 @@ static void ravb_emac_init_gbeth(struct
> > >>> net_device
> > >> *ndev)
> > >>>    	/* Receive frame limit set register */
> > >>>    	ravb_write(ndev, GBETH_RX_BUFF_MAX + ETH_FCS_LEN, RFLR);
> > >>>
> > >>> -	/* PAUSE prohibition */
> > >>> +	/* EMAC Mode: PAUSE prohibition; Duplex; TX; RX; CRC Pass
> > >>> +Through; Promiscuous */
> > >>
> > >>      Promiscuous mode, really? Why?!
> > >
> > > This is TOE related,
> 
> I meant the context here is TOE register related. That is what I meant.
> 
> >
> >     The promiscuous mode is supported by _all_ Ethernet controllers, I
> > think.
> >
> > > and is recommendation from BSP team.
> >
> >     On what grounds?
> 
> The reference implementation has this on. Any way it is good catch.
> I will turn it off and check.
> 
> by looking at the RJ LED's there is not much activity and packet
> statistics also show not much activity by default.
> 
> How can we check, it is overloading the controller? So that I can compare
> with and without this setting
> 
> >
> > > If you think it is wrong.
> > > I can take this out. Please let me know. Currently the board is
> > > booting
> > and everything works without issues.
> >
> >     Please do take it out. It'll needlessly overload the controller
> > when there's much traffic on the local network.

I have tested without this as well and I don't find any difference.
So I plan to take this out.

Do you have any idea how to check the "overloading the controller" with PRM bit ON/OFF 
to check the actual impact? Please let me know, so that I can compare the same.

Regards,
Biju

> 
> 
> I can see much activity only on RJ45 LED's when I call tcpdump or by
> setting IP link set eth0 promisc on.
> Otherwise there is no traffic at all.
> 
> Regards,
> Biju
> 
> >
> > > The meaning of promiscuous in H/W manual as follows.
> >
> >     I know what the promiscuous mode is. :-)
> >     It's needed by things like 'tcpdump' and normally shoild be off.
> >
> > > Promiscuous Mode
> > > 1: All the frames except for PAUSE frame are received.
> > > Self-addressed unicast, different address unicast, multicast, and
> > > broadcast frames are all transferred to TOE. PAUSE frame reception
> > > is controlled by PFR
> > bit.
> > > 0: Self-addressed unicast, multicast, and broadcast frames are
> > > received, then transferred to TOE.
> 
> 
> 
> > >
> > > Regards,
> > > Biju
> >
> > MBR, Sergey
Andrew Lunn Oct. 10, 2021, 3:06 p.m. UTC | #6
> by looking at the RJ LED's there is not much activity and packet
> statistics also show not much activity by default.

> How can we check, it is overloading the controller? So that I can
> compare with and without this setting

What is you link peer? A switch? That will be doing some filtering, so
you probably don't see unicast traffic from other devices. So you need
to flood your link with traffic the switch does not filter. Try
multicast traffic for a group you are not a member off. You might need
to disable IGMP snooping on the switch.

Or use a traffic generator as a link peer and have it generate streams
with mixed sources and destinations.

     Andrew
Sergey Shtylyov Oct. 10, 2021, 5:24 p.m. UTC | #7
On 10/10/21 1:56 PM, Biju Das wrote:

[...]
>>>>> Update EMAC configuration mode comment from "PAUSE prohibition"
>>>>> to "EMAC Mode: PAUSE prohibition; Duplex; TX; RX; CRC Pass Through;
>>>>> Promiscuous".
>>>>>
>>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
>>>>> Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>>>> ---
>>>>> v1->v2:
>>>>>    * No change
>>>>> V1:
>>>>>    * New patch.
>>>>> ---
>>>>>    drivers/net/ethernet/renesas/ravb_main.c | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
>>>>> b/drivers/net/ethernet/renesas/ravb_main.c
>>>>> index 9a770a05c017..b78aca235c37 100644
>>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>>>> @@ -519,7 +519,7 @@ static void ravb_emac_init_gbeth(struct
>>>>> net_device
>>>> *ndev)
>>>>>    	/* Receive frame limit set register */
>>>>>    	ravb_write(ndev, GBETH_RX_BUFF_MAX + ETH_FCS_LEN, RFLR);
>>>>>
>>>>> -	/* PAUSE prohibition */
>>>>> +	/* EMAC Mode: PAUSE prohibition; Duplex; TX; RX; CRC Pass Through;
>>>>> +Promiscuous */
>>>>
>>>>      Promiscuous mode, really? Why?!
>>>
>>> This is TOE related,
> 
> I meant the context here is TOE register related. That is what I meant.
> 
>>
>>     The promiscuous mode is supported by _all_ Ethernet controllers, I
>> think.
>>
>>> and is recommendation from BSP team.
>>
>>     On what grounds?
> 
> The reference implementation has this on. Any way it is good catch. 
> I will turn it off and check.
> 
> by looking at the RJ LED's there is not much activity and packet statistics also show not much activity by default.
> 
> How can we check, it is overloading the controller? So that I can compare with and without this setting

   Maybe it doesn't get overloaded that simply, but definitely the promiscuous mode is not the thing
for the normal driver use...

>>> If you think it is wrong.
>>> I can take this out. Please let me know. Currently the board is booting
>> and everything works without issues.
>>
>>     Please do take it out. It'll needlessly overload the controller when
>> there's much traffic on the local network.
> 
> 
> I can see much activity only on RJ45 LED's when I call tcpdump or by setting IP link set eth0 promisc on.
> Otherwise there is no traffic at all.

   Sounds like the kernel initially sets the RX mode with IFF_PROMISC = 0 and thus clear ECMR.PRM but I don't
see where it does this? Could you instrument ravb_set_tx_mode() plz?

> Regards,
> Biju

[...]

MBR, Sergey
Sergey Shtylyov Oct. 10, 2021, 5:45 p.m. UTC | #8
On 10/10/21 8:24 PM, Sergey Shtylyov wrote:

[...]
>>>>>> Update EMAC configuration mode comment from "PAUSE prohibition"
>>>>>> to "EMAC Mode: PAUSE prohibition; Duplex; TX; RX; CRC Pass Through;
>>>>>> Promiscuous".
>>>>>>
>>>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
>>>>>> Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>>>>> ---
>>>>>> v1->v2:
>>>>>>    * No change
>>>>>> V1:
>>>>>>    * New patch.
>>>>>> ---
>>>>>>    drivers/net/ethernet/renesas/ravb_main.c | 2 +-
>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
>>>>>> b/drivers/net/ethernet/renesas/ravb_main.c
>>>>>> index 9a770a05c017..b78aca235c37 100644
>>>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>>>>> @@ -519,7 +519,7 @@ static void ravb_emac_init_gbeth(struct
>>>>>> net_device
>>>>> *ndev)
>>>>>>    	/* Receive frame limit set register */
>>>>>>    	ravb_write(ndev, GBETH_RX_BUFF_MAX + ETH_FCS_LEN, RFLR);
>>>>>>
>>>>>> -	/* PAUSE prohibition */
>>>>>> +	/* EMAC Mode: PAUSE prohibition; Duplex; TX; RX; CRC Pass Through;
>>>>>> +Promiscuous */
>>>>>
>>>>>      Promiscuous mode, really? Why?!
>>>>
>>>> This is TOE related,
>>
>> I meant the context here is TOE register related. That is what I meant.
>>
>>>
>>>     The promiscuous mode is supported by _all_ Ethernet controllers, I
>>> think.
>>>
>>>> and is recommendation from BSP team.
>>>
>>>     On what grounds?
>>
>> The reference implementation has this on. Any way it is good catch. 
>> I will turn it off and check.
>>
>> by looking at the RJ LED's there is not much activity and packet statistics also show not much activity by default.
>>
>> How can we check, it is overloading the controller? So that I can compare with and without this setting
> 
>    Maybe it doesn't get overloaded that simply, but definitely the promiscuous mode is not the thing
> for the normal driver use...
> 
>>>> If you think it is wrong.
>>>> I can take this out. Please let me know. Currently the board is booting
>>> and everything works without issues.
>>>
>>>     Please do take it out. It'll needlessly overload the controller when
>>> there's much traffic on the local network.
>>
>>
>> I can see much activity only on RJ45 LED's when I call tcpdump or by setting IP link set eth0 promisc on.
>> Otherwise there is no traffic at all.
> 
>    Sounds like the kernel initially sets the RX mode with IFF_PROMISC = 0 and thus clear ECMR.PRM but I don't
> see where it does this? Could you instrument ravb_set_tx_mode() plz?

   Sorry, ravb_set_rx_mode(), of/c. :-)

>> Regards,
>> Biju
> 
> [...]

MBR, Sergey
Biju Das Oct. 12, 2021, 1:34 p.m. UTC | #9
Hi Andrew,

Thanks for the feedback.

> Subject: Re: [PATCH net-next v2 13/14] ravb: Update EMAC configuration
> mode comment
> 
> > by looking at the RJ LED's there is not much activity and packet
> > statistics also show not much activity by default.
> 
> > How can we check, it is overloading the controller? So that I can
> > compare with and without this setting
> 
> What is you link peer? A switch? That will be doing some filtering, so you
> probably don't see unicast traffic from other devices. So you need to
> flood your link with traffic the switch does not filter. Try multicast
> traffic for a group you are not a member off. You might need to disable
> IGMP snooping on the switch.
> 

I have tested in below environments
Setup 1: 
  Machine1: RZ/G2L platform connected to Ubuntu Guest VM(bridged),Host oS windows via SWITCH and
  Machine2: RZ/G2M platform connected to Ubuntu Guest VM(bridged),Host oS windows via SWITCH

Then ran multicast_sender app from machine 2 and ran tcpdump on machine 1.
using devmem, I have controlled on/off PRM bit.

In both cases, on tcpdump from machine 1, I see multicast packets which I am not a member off.

Setup2:-

  RZ/G2L platform directly connected to RZ/G2M platform. 

Ran UDP unicast sockets to send data from RZ/G2M platform

ran tcpdump from from RZ/G2L platform

using devmem, I am controlling on/off PRM bit.

But for different addressed packet, I see RZ/G2L platfrom is trying to do ARP request for
different address. So packets are handled, with and without PRM bit set.

Regards,
Biju

> Or use a traffic generator as a link peer and have it generate streams
> with mixed sources and destinations.
Biju Das Oct. 12, 2021, 1:39 p.m. UTC | #10
Hi Jakub Kicinski,

> Subject: Re: [PATCH net-next v2 13/14] ravb: Update EMAC configuration
> mode comment
> 
> On Sun, 10 Oct 2021 10:56:32 +0000 Biju Das wrote:
> > > > This is TOE related,
> >
> > I meant the context here is TOE register related. That is what I meant.
> 
> Did you test TCP packets with bad checksums? The description you posted
> earlier could indicate this is about dropping such packets, not about
> address filtering?

I have made changes similar to R-Car for HW Checksum on RX and passed wrong checksum(0x0000 or 0xffff) and it crashed the system.

Regards,
Biju
diff mbox series

Patch

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 9a770a05c017..b78aca235c37 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -519,7 +519,7 @@  static void ravb_emac_init_gbeth(struct net_device *ndev)
 	/* Receive frame limit set register */
 	ravb_write(ndev, GBETH_RX_BUFF_MAX + ETH_FCS_LEN, RFLR);
 
-	/* PAUSE prohibition */
+	/* EMAC Mode: PAUSE prohibition; Duplex; TX; RX; CRC Pass Through; Promiscuous */
 	ravb_write(ndev, ECMR_ZPF | ((priv->duplex > 0) ? ECMR_DM : 0) |
 			 ECMR_TE | ECMR_RE | ECMR_RCPT |
 			 ECMR_TXF | ECMR_RXF | ECMR_PRM, ECMR);