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]
Message-ID: <20241008174512.GA4146181@ragnatech.se>
Date: Tue, 8 Oct 2024 19:45:12 +0200
From: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
To: Sergey Shtylyov <s.shtylyov@....ru>
Cc: "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Claudiu Beznea <claudiu.beznea.uj@...renesas.com>,
	Paul Barker <paul.barker.ct@...renesas.com>,
	Biju Das <biju.das.jz@...renesas.com>,
	Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>,
	netdev@...r.kernel.org, linux-renesas-soc@...r.kernel.org
Subject: Re: [net-next] net: ravb: Only advertise Rx/Tx timestamps if
 hardware supports it

Hi Sergey,

Sorry for missing your comment earlier.

On 2024-10-08 20:26:21 +0300, Sergey Shtylyov wrote:
> On 10/7/24 10:05 PM, Sergey Shtylyov wrote:
> [...]
> 
> >> Recent work moving the reporting of Rx software timestamps to the core
> >> [1] highlighted an issue where hardware time stamping where advertised
> >> for the platforms where it is not supported.
> >>
> >> Fix this by covering advertising support for hardware timestamps only if
> >> the hardware supports it. Due to the Tx implementation in RAVB software
> >> Tx timestamping is also only considered if the hardware supports
> >> hardware timestamps. This should be addressed in future, but this fix
> >> only reflects what the driver currently implements.
> >>
> >> 1. Commit 277901ee3a26 ("ravb: Remove setting of RX software timestamp")
> >>
> >> Fixes: 7e09a052dc4e ("ravb: Exclude gPTP feature support for RZ/G2L")
> >> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
> > [...]
> > 
> > Reviewed-by: Sergey Shtylyov <s.shtylyov@....ru>
> > 
> >> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> >> index d2a6518532f3..907af4651c55 100644
> >> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> >> @@ -1750,20 +1750,19 @@ static int ravb_get_ts_info(struct net_device *ndev,
> >>  	struct ravb_private *priv = netdev_priv(ndev);
> >>  	const struct ravb_hw_info *hw_info = priv->info;
> >>  
> >> -	info->so_timestamping =
> >> -		SOF_TIMESTAMPING_TX_SOFTWARE |
> >> -		SOF_TIMESTAMPING_TX_HARDWARE |
> >> -		SOF_TIMESTAMPING_RX_HARDWARE |
> >> -		SOF_TIMESTAMPING_RAW_HARDWARE;
> >> -	info->tx_types = (1 << HWTSTAMP_TX_OFF) | (1 << HWTSTAMP_TX_ON);
> >> -	info->rx_filters =
> >> -		(1 << HWTSTAMP_FILTER_NONE) |
> >> -		(1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
> >> -		(1 << HWTSTAMP_FILTER_ALL);
> >> -	if (hw_info->gptp || hw_info->ccc_gac)
> >> +	if (hw_info->gptp || hw_info->ccc_gac) {
> >> +		info->so_timestamping =
> >> +			SOF_TIMESTAMPING_TX_SOFTWARE |
> >> +			SOF_TIMESTAMPING_TX_HARDWARE |
> >> +			SOF_TIMESTAMPING_RX_HARDWARE |
> >> +			SOF_TIMESTAMPING_RAW_HARDWARE;
> >> +		info->tx_types = (1 << HWTSTAMP_TX_OFF) | (1 << HWTSTAMP_TX_ON);
> >> +		info->rx_filters =
> >> +			(1 << HWTSTAMP_FILTER_NONE) |
> >> +			(1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
> >> +			(1 << HWTSTAMP_FILTER_ALL);
> >>  		info->phc_index = ptp_clock_index(priv->ptp.clock);
> >> -	else
> >> -		info->phc_index = 0;
> > 
> >    Is it OK to remove this line?

Yes it is OK, see the discussion that sparked this patch.

https://lore.kernel.org/netdev/20240829204429.GA3708622@ragnatech.se/

> 
>    Also, how about inverting the *if* condition above (and doing an early
> *return*) and avoiding reindenting the code below it?

I thought about that but opted not to do so. The same check is used all 
over the code and I think it's value in keeping it similar. I will go 
over all this code again as Gen4 will need more work to fully enable 
gPTP. My hope is to abstract the check into something bore descriptive 
instead of sprinkling yet more conditions on to this one. Is it OK for 
you to keep them aligned for now?

> 
> [...]
> 
> MBR, Sergey
> 

-- 
Kind Regards,
Niklas Söderlund

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ