phy timestamping "library"

phy timestamping "library"

[Michael Walle]

Hi,

I'm working on PHY timestamping for the AR8031 PHY.

I try to factor out the timestamp handling of the dp83640 phy driver,
because my ar8031 phy driver uses almost the same logic. The dp83640 has
three lists, two skb queues for rx and tx and one lists to handle
out-of-order rx timestamps. The tx timestamps are handled in-order
because we already have the skbuf in the txqueue when a tx timestamp
arrives; I guess that is always true. The rx timestamps are allocated
from a pool and given back to it if there was a match between timestamp
and skb or if there was a timeout (just before passing the skb back to
the network stack).

So what I'd like to have in a seperate "library" is the following:
  - skb queues handling
  - rx timestamp pool handling
  - rx timestamp timeout handling
  - skb delivery to the network stack on either match or timeout

So what I'm looking to do is to have the following functions:

phyts_rx_skb(struct phyts *pt, struct sk_buff *skb);
phyts_tx_skb(struct phyts *pt, struct sk_buff *skb);
phyts_rx_timestamp(struct phyts *pt, struct phyts_ts *ts);
phyts_tx_timestamp(struct phyts *pt, struct phyts_ts *ts);
phyts_init(struct phyts *pt, <tbd>);

struct phyts {
	struct delayed_work ts_work;
	/* list of rx timestamps */
	struct list_head rxts;
	struct list_head rxpool;
	struct phyts_ts *rx_pool_data;
	/* protects above three fields from concurrent access */
	spinlock_t rx_lock;
	/* queues of incoming and outgoing packets */
	struct sk_buff_head rx_queue;
	struct sk_buff_head tx_queue;

	int (*match)(struct sk_buff *skb, struct phyts_ts *ts);
};

struct phyts_ts {
	struct list_head list;
	unsigned long tmo;
	ktime_t ts;
};

If the skb is accepted to be timestamped, the driver calls either
phyts_rx_skb() or phyts_tx_skb(). Once it received an timestamp, it
calls phyts_rx_timestamp() or phyts_tx_timestamp(). Everything else
would be handled by the lib.

Now I have the following problem: the original driver attaches more data
to the timestamp, as is my driver; this data is used by the match
function. Is there any pattern (1) where to allocate the memory for the
timestamp pool, (2) how to populate the pool and (3) how the driver can
request an element from the pool. I guess (1) has to be done in the
driver and I'd like to do (2) in the library.

So a driver part might look like:

struct rxts {
	struct phyts_ts phyts;
	u64 ns;
	u16 seqid;
	u8  msgtype;
	u16 hash;
};

The match function would get a pointer to this phyts element and can use
container_of() to cast the pointer back to its original "struct rxts".

To allocate the memory, the driver could do

   pool = kcalloc(num_rxts, sizeof(struct rxts), GFP_KERNEL);

But how can I give that memory to the lib in phyts_init() and how can
phyts_init() populate its rx_pool_data. Or maybe I'm getting it all
wrong and there is a better way ;)

Oh and the driver struct would look like:

struct dp83640_private {
	struct phyts pt;
..
};

And would then call phyts_init(&priv->pt) in its probe().

-michael

Re: phy timestamping "library"

[Richard Cochran]

On Tue, Nov 26, 2019 at 07:00:02PM +0100, Michael Walle wrote:
> So what I'd like to have in a seperate "library" is the following:
>  - skb queues handling
>  - rx timestamp pool handling
>  - rx timestamp timeout handling
>  - skb delivery to the network stack on either match or timeout

I would resist the temptation to turn this into a "library", unless
there is a clear benefit.

> Now I have the following problem: the original driver attaches more data
> to the timestamp, as is my driver; this data is used by the match
> function.

...

> But how can I give that memory to the lib in phyts_init() and how can
> phyts_init() populate its rx_pool_data. Or maybe I'm getting it all
> wrong and there is a better way ;)

Sounds like the re-factoring of a couple of lists into a "library" is
not worth the effort.  The differences in the matching logic will make
the "library" baroque.

I suggest not over-engineering your new driver.  In any case, it is
hard to say without seeing the patches.

Thanks,
Richard