netdev - RE: [PATCH 02/10] ravb: Rename "no_ptp_cfg_active" and "ptp_cfg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sun, 3 Oct 2021 07:05:26 +0000
From:   Biju Das <biju.das.jz@...renesas.com>
To:     Sergey Shtylyov <s.shtylyov@....ru>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
CC:     Sergei Shtylyov <sergei.shtylyov@...il.com>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Adam Ford <aford173@...il.com>, Andrew Lunn <andrew@...n.ch>,
        Yuusuke Ashizuka <ashiduka@...itsu.com>,
        Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-renesas-soc@...r.kernel.org" 
        <linux-renesas-soc@...r.kernel.org>,
        Chris Paterson <Chris.Paterson2@...esas.com>,
        Biju Das <biju.das@...renesas.com>,
        Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: RE: [PATCH 02/10] ravb: Rename "no_ptp_cfg_active" and
 "ptp_cfg_active" variables

Hi Sergei,

> Subject: Re: [PATCH 02/10] ravb: Rename "no_ptp_cfg_active" and
> "ptp_cfg_active" variables
> 
> Hello!
> 
>    Damn, DaveM continues ignoring my review efforts... :-( will finish
> reviewing the series anyway.
> 
> On 10/2/21 10:53 AM, Biju Das wrote:
> 
> >> Subject: Re: [PATCH 02/10] ravb: Rename "no_ptp_cfg_active" and
> >> "ptp_cfg_active" variables
> >>
> >> On 10/1/21 6:06 PM, Biju Das wrote:
> >>
> >>> Rename the variable "no_ptp_cfg_active" with "gptp" and
> >>
> >>    This shouldn't be a rename but the extension of the meaning
> instead...
> >
> > This is the original ptp support for both R-Car Gen3 and R-Car Gen2
> without config in active mode. Later we added feature support active in
> config mode for R-Car Gen3 by patch[1].
> > [1]
> > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.
> > kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2
> > Fcommit%2Fdrivers%2Fnet%2Fethernet%2Frenesas%2Fravb_main.c%3Fh%3Dv5.15
> > -rc3%26id%3Df5d7837f96e53a8c9b6c49e1bc95cf0ae88b99e8&amp;data=04%7C01%
> > 7Cbiju.das.jz%40bp.renesas.com%7Cb4a62982865a4f7cf38408d985d11fef%7C53
> > d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C637687955521294093%7CUnknown%
> > 7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> > VCI6Mn0%3D%7C1000&amp;sdata=6rpdh6hEAUl1yMng2ruFrKflBiDGmq6RylI90Ije3t
> > 4%3D&amp;reserved=0
> 
>    And? Do you think I don't remember the driver development history? :-)
> 
> >>> "ptp_cfg_active" with "ccc_gac" to match the HW features.
> >>>
> >>> There is no functional change.
> >>>
> >>> Signed-off-by: Biju Das <biju.das.jz@...renesas.com>
> >>> Suggested-by: Sergey Shtylyov <s.shtylyov@....ru>
> >>> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> >>> ---
> >>> RFc->v1:
> >>>  * Renamed the variable "no_ptp_cfg_active" with "gptp" and
> >>>    "ptp_cfg_active" with "ccc_gac
> >>> ---
> >>>  drivers/net/ethernet/renesas/ravb.h      |  4 ++--
> >>>  drivers/net/ethernet/renesas/ravb_main.c | 26
> >>> ++++++++++++------------
> >>>  2 files changed, 15 insertions(+), 15 deletions(-)
> >>
> >> [...]
> >>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> >>> b/drivers/net/ethernet/renesas/ravb_main.c
> >>> index 8f2358caef34..dc7654abfe55 100644
> >>> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> >>> @@ -1274,7 +1274,7 @@ static int ravb_set_ringparam(struct
> >>> net_device
> >> *ndev,
> >>>  	if (netif_running(ndev)) {
> >>>  		netif_device_detach(ndev);
> >>>  		/* Stop PTP Clock driver */
> >>> -		if (info->no_ptp_cfg_active)
> >>> +		if (info->gptp)
> >>
> >>    Where have you lost !info->ccc_gac?
> >
> >   As per patch[1], the check is for R-Car Gen2. Why do you need
> > additional check as per the current driver?
> 
>    Because the driver now supports not only gen2, but also gen3, and
> RZ/G2L, finally.
> 
> > I see below you are proposing to enable both "gptp" and "ccc_gac" for
> > R-Car Gen3,
> 
>    Yes, this is how the hardware evolved. gPTP hardware can (optionally)
> be active outside the config mode, otherwise there's no difference b/w
> gen2 and gen3.
> 
> > According to me it is a feature improvement for R-Car Gen3 in which,
> > you can have
> >
> > 1) gPTP support active in config mode
> > 2) gPTP support not active in config mode
> 
>    Right.
> 
> > But the existing driver code just support "gPTP support active in config
> mode" for R-Car Gen3.
> 
>    And?
> 
> > Do you want me to do feature improvement as well, as part of Gbethernet
> support?
> 
>    I thought we agreed on this patch in the previous iteration, To be more
> clear, by asking to remove the "double negation", I meant using:

I never thought of adding feature improvements as part of Gbethernet support. Any feature improvements after adding GbEthernet support.

If you expressed your ideas like adding gptp, ccc_gac for R-Car Gen3 earlier, then
I should have responded it is feature improvement. So please share your ideas in advance.

Regards,
Biju




> 
> 	if (info->gptp && !info->ccc_gac)
> 
> versus your:
> 
> 	if (!info->no_gptp && !info->ccc_gac)
> 
> > Please let me know your thoughts.
> >
> > The same comments applies to all the comments you have mentioned below.
> >
> > Regards,
> > Biju
> 
> [...]
> 
> MBR, Sergey