Re: [Linuxptp-devel] [PATCH 3/5] gptp: minimal support

[Richard C. <ric...@gm...>]

On Mon, Jul 16, 2012 at 12:49:12PM +0200, Delio Brignoli wrote:
> On Jul 14, 2012, at 11:58 AM, Richard Cochran wrote:
> >> diff --git a/sk.c b/sk.c
> >> index 05dd406..535da60 100644
> >> --- a/sk.c
> >> +++ b/sk.c
> [...]
> >> @@ -46,22 +47,35 @@ static int hwts_init(int fd, char *device)
> >> 
> >> 	ifreq.ifr_data = (void *) &cfg;
> >> 	cfg.tx_type    = HWTSTAMP_TX_ON;
> >> -	cfg.rx_filter  = HWTSTAMP_FILTER_PTP_V2_EVENT;
> >> +
> >> +	if (gptp_mode)
> >> +		req_rx_filter  = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
> >> +	else
> >> +		req_rx_filter  = HWTSTAMP_FILTER_PTP_V2_EVENT;
> >> +
> >> +	cfg.rx_filter = req_rx_filter;
> > 
> > I think there is a better way to handle this sort of thing. We should
> > start off with V2_EVENT, and, if that failes, then iterate over the
> > more specific variants while screaming loudly (warnings).
> 
> There could be a driver out there that implements 
> HWTSTAMP_FILTER_PTP_V2_L2_EVENT
> but not 
> HWTSTAMP_FILTER_PTP_V2_EVENT;
> 
> If the user selects transport = TRANS_IEEE_802_3 
> from the command line than a warning would be inappropriate IMO.
> In case of TRANS_IEEE_802_3 you would want to start from 
> V2_L2_EVENT and iterate over increasingly less specific variants.

Except that the code cannot work with the "sync-only" variants. Those
modes where introduced (not by me) for really broken, early hardware
designs.

But I get your point about user expectations WRT transport.
 
> For sk_timestamping_init() to make that call it needs to know 
> about the choice of transport, or delegate timestamping init to
> a transport specific module (raw, udp4 or udp6).

Yes, you are right. It should be easy to add the transport type as a
flag to sk_timestamping_init, if needed.

Thanks,
Richard