Re: [PATCH 16/20] net: dsa: xilinx: drive per-MAC PTP TX/RX hardware paths

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
    net: dsa: xilinx: drive per-MAC PTP TX/RX hardware paths

This adds the per-MAC PTP TX and RX hardware paths to the Xilinx TSN DSA
driver: the tag protocol diverts PTP frames into a per-MAC hardware TX
slot through a ds->tagger_data callback, and two new per-MAC interrupts
drive TX timestamp completion and direct RX delivery to the user port.
It also wires up port_hwtstamp_get/set and get_ts_info.

A few questions about the new paths below.

> diff --git a/drivers/net/dsa/xilinx/xilinx_tsn.c b/drivers/net/dsa/xilinx/xilinx_tsn.c
> index 318d8b332208..e48c81f18d6e 100644
> --- a/drivers/net/dsa/xilinx/xilinx_tsn.c
> +++ b/drivers/net/dsa/xilinx/xilinx_tsn.c

[ ... ]

> @@ -692,7 +710,10 @@ static void xlnx_tsn_teardown(struct dsa_switch *ds)
>  {
>  	struct xlnx_tsn *sw = ds->priv;
>  	struct dsa_port *dp;
> +	int port;
>  
> +	for (port = XLNX_TSN_PORT_MAC1; port <= XLNX_TSN_PORT_MAC2; port++)
> +		xlnx_tsn_port_ptp_exit(sw, port);
>  	xlnx_tsn_ptp_exit(sw);
>  	unregister_netdevice_notifier(&sw->nb);
>  	xlnx_tsn_mdio_unregister_all(sw);

[ ... ]

> diff --git a/drivers/net/dsa/xilinx/xilinx_tsn.h b/drivers/net/dsa/xilinx/xilinx_tsn.h
> index 6063763b5c17..d46150535775 100644
> --- a/drivers/net/dsa/xilinx/xilinx_tsn.h
> +++ b/drivers/net/dsa/xilinx/xilinx_tsn.h

[ ... ]

> @@ -127,7 +131,44 @@ enum tsn_port_state {
>  #define TSN_TIMER_PULSES_PER_PPS	128
>  #define TSN_TIMER_GTX_CLK_FREQ		125000000U
>  
> +/* Per-MAC PTP TX / RX register windows, sitting inside each per-MAC
> + * reg space. Each PTP TX slot is 256 B wide; the first 8 B hold the
> + * cmd1/cmd2 header, leaving 248 B for frame data. HW provides 8 slots.
> + * The PTP RX buffer mirrors the layout with a 252 B usable area and
> + * an 8 B HW timestamp footer.
> + */

[ ... ]

> +#define TSN_PTP_TX_CMD_FIELD_LEN	8
> +#define TSN_PTP_TX_MAX_FRAME_SIZE	(TSN_PTP_TX_HWBUF_SIZE - \
> +					 TSN_PTP_TX_CMD_FIELD_LEN)

[ ... ]

> +#define TSN_PTP_HW_TSTAMP_SIZE		8
> +#define TSN_PTP_RX_HWBUF_SIZE		256
> +#define TSN_PTP_RX_FRAME_SIZE		252
> +#define TSN_PTP_HW_TSTAMP_OFFSET	(TSN_PTP_RX_HWBUF_SIZE - \
> +					 TSN_PTP_HW_TSTAMP_SIZE)

[Severity: Medium]
Do these numbers add up?  The comment says "a 252 B usable area and an 8 B
HW timestamp footer", but 252 + 8 is larger than the 256 B slot.

TSN_PTP_HW_TSTAMP_OFFSET is 256 - 8 = 248, so the footer sits at slot bytes
248..255, which leaves at most 248 usable bytes, not 252.

In xlnx_tsn_ptp_recv() both the copy length and the bound check use 252:

	memcpy_fromio_32(m, frame_base, skb->data, TSN_PTP_RX_FRAME_SIZE);
	...
	if (msg_len + ETH_HLEN > TSN_PTP_RX_FRAME_SIZE) {
	...
	skb_put(skb, msg_len + ETH_HLEN);

so for a frame where msg_len + ETH_HLEN lands in (248, 252] the skb ends up
carrying up to 4 bytes of the timestamp footer as payload.  Should
TSN_PTP_RX_FRAME_SIZE be 248?

The TX side has the mirror of this.  TSN_PTP_TX_MAX_FRAME_SIZE is 248 and
frame data starts at slot offset 8, so a maximum-length frame writes slot
bytes 8..255 - including the address xlnx_tsn_tx_tstamp_work() then reads
back as the transmit timestamp:

	ts_off = TSN_PTP_TX_BUFFER_OFFSET(index) +
		 TSN_PTP_HW_TSTAMP_OFFSET;

Should the accepted TX length be 240 for this layout?  As a side note,
TSN_PTP_HW_TSTAMP_OFFSET is derived from TSN_PTP_RX_HWBUF_SIZE but is also
applied to TX slots; it only works because both happen to be 256.

[ ... ]

> diff --git a/drivers/net/dsa/xilinx/xilinx_tsn_ptp.c b/drivers/net/dsa/xilinx/xilinx_tsn_ptp.c
> index 75c177f752f1..740d92e3e80d 100644
> --- a/drivers/net/dsa/xilinx/xilinx_tsn_ptp.c
> +++ b/drivers/net/dsa/xilinx/xilinx_tsn_ptp.c

[ ... ]

> @@ -233,3 +241,350 @@ void xlnx_tsn_ptp_exit(struct xlnx_tsn *sw)
>  	ptp_clock_unregister(sw->ptp_clock);
>  	sw->ptp_clock = NULL;
>  }
> +
> +static void memcpy_toio_32(struct xlnx_tsn_mac *m, unsigned long off,
> +			   const u8 *data, size_t len)
> +{
> +	while (len >= 4) {
> +		mac_iow(m, off, get_unaligned((const u32 *)data));
> +		off += 4;
> +		data += 4;
> +		len -= 4;
> +	}
> +
> +	if (len) {
> +		u32 leftover = 0;
> +		u8 *dst = (u8 *)&leftover;
> +
> +		while (len--)
> +			*dst++ = *data++;
> +		mac_iow(m, off, leftover);
> +	}
> +}
> +
> +static void memcpy_fromio_32(struct xlnx_tsn_mac *m, unsigned long off,
> +			     u8 *data, size_t len)
> +{
> +	while (len >= 4) {
> +		put_unaligned(mac_ior(m, off), (u32 *)data);
> +		off += 4;
> +		data += 4;
> +		len -= 4;
> +	}

[Severity: Medium]
Is it intentional that frame data is moved with the byte-swapping register
accessors?  mac_iow()/mac_ior() in xilinx_tsn.h are iowrite32()/ioread32():

	static inline void mac_iow(struct xlnx_tsn_mac *m, u32 off, u32 val)
	{
		iowrite32(val, m->regs + off);
	}

	static inline u32 mac_ior(struct xlnx_tsn_mac *m, u32 off)
	{
		return ioread32(m->regs + off);
	}

Those apply cpu_to_le32()/le32_to_cpu(), which is right for control and
status registers but not for a frame byte stream.  On a big-endian host
memcpy_toio_32() assembles a host-order u32 out of four consecutive frame
bytes and writes it byte-reversed into the slot, so the MAC transmits a
corrupted frame; memcpy_fromio_32() has the mirrored effect on receive, so
eth_type_trans(), the messageType byte at ETH_HLEN and get_unaligned_be16()
all parse swapped bytes.

The Kconfig entry only depends on OF && HAS_IOMEM && NET_DSA &&
PTP_1588_CLOCK, so big-endian hosts of this soft IP are not excluded.  Would
raw/stream accessors (__raw_writel/__raw_readl, iowrite32_rep/ioread32_rep,
or memcpy_toio/memcpy_fromio) be more appropriate here?

[ ... ]

> +void xlnx_tsn_ptp_tx(struct dsa_port *dp, struct sk_buff *skb)
> +{
> +	struct xlnx_tsn *sw = dp->ds->priv;
> +	u32 frame_waiting, cmd1, cmd2 = 0;
> +	struct xlnx_tsn_mac *m;
> +	u8 free_index;
> +
> +	m = &sw->mac[dp->index];
> +
> +	if (unlikely(skb->len > TSN_PTP_TX_MAX_FRAME_SIZE)) {
> +		dev_kfree_skb_any(skb);
> +		return;
> +	}
> +
> +	scoped_guard(spinlock_irqsave, &m->ptp_tx_lock) {
> +		frame_waiting = FIELD_GET(TSN_PTP_TX_FRAME_WAITING_MASK,
> +					  mac_ior(m, TSN_PTP_TX_CONTROL_OFFSET));
> +		if (frame_waiting & TSN_PTP_TX_BUFFERS_FULL_MASK) {
> +			dev_kfree_skb_any(skb);
> +			return;
> +		}
> +
> +		free_index = fls(frame_waiting);

[Severity: High]
Can this slot allocator hand out a slot that an skb still sitting in
m->ptp_txq owns?

free_index comes from the hardware "frames waiting to be transmitted"
bitmap, which clears when a frame departs, while ptp_txq holds frames that
have not yet been timestamped - the TX IRQ only calls schedule_work().  So:

	frame A: free_index = fls(0) = 0, queued with cb[0] = 0, departs
	         HW clears bit 0, TX IRQ schedules tx_tstamp_work
	frame B: reads frame_waiting == 0, free_index = fls(0) = 0 again,
	         queued with cb[0] = 0

xlnx_tsn_tx_tstamp_work() then reads tx_packet == 0 and drains both skbs,
handing frame B a timestamp read out of slot 0 that belongs to frame A.

The reverse ordering looks worse: if the queue head carries a high index
(say 5) and later frames get low indices, this test in
xlnx_tsn_tx_tstamp_work() never becomes true again:

	if (index > tx_packet) {
		__skb_queue_head(&m->ptp_txq, skb);
		break;
	}

so the head is requeued forever, no further TX timestamps are delivered for
that port, and skbs accumulate until the driver is unbound.

TSN_PTP_TX_PACKET_FIELD_MASK is GENMASK(18, 16), i.e. 3 bits, so the status
value wraps 0..7 - can it be used as a monotonic ordering key at all?
Would tracking slot ownership in software (a used-slot bitmap plus the
queued index) be more robust?

> +		cmd1 = skb->len;
> +
> +		mac_iow(m, TSN_PTP_TX_BUFFER_OFFSET(free_index), cmd1);
> +		mac_iow(m, TSN_PTP_TX_BUFFER_OFFSET(free_index) +
> +			TSN_PTP_TX_BUFFER_CMD2_FIELD, cmd2);
> +		memcpy_toio_32(m,
> +			       TSN_PTP_TX_BUFFER_OFFSET(free_index) +
> +			       TSN_PTP_TX_CMD_FIELD_LEN,
> +			       skb->data, skb->len);

[Severity: High]
Can this read past the end of the linear part of the skb?  memcpy_toio_32()
walks skb->len bytes linearly from skb->data, but nothing here checks
skb_headlen(skb) or calls skb_linearize(), so for a paged skb it reads
beyond the head buffer (into skb_shared_info and past the kmalloc'd area)
and pushes those bytes into the hardware slot, i.e. out onto the wire.

Non-linear skbs do not appear to be filtered out on the way in.  This
tagger sets no needed_tailroom, so dsa_user_setup_tagger() does not strip
the features:

	if (user->needed_tailroom)
		user->features &= ~(NETIF_F_SG | NETIF_F_FRAGLIST);

and skb_ensure_writable_head_tail() only does pskb_expand_head(), it never
linearizes.  An AF_PACKET send with PACKET_VNET_HDR (attacker-chosen
hdr_len picking a small linear area) or a bridge-forwarded frame whose
ingress driver put payload in fragments would reach this copy.

The same assumption exists one layer up in xlnx_tsn_xmit(), which
dereferences eth_hdr(skb)->h_proto with no guarantee that the 14 byte MAC
header is in the linear area.

> +
> +		skb->cb[0] = free_index;
> +		__skb_queue_tail(&m->ptp_txq, skb);

[Severity: Medium]
What releases these skbs when the port or the link goes down?  The only
paths that dequeue ptp_txq are xlnx_tsn_tx_tstamp_work() (which requires the
completion match to succeed) and xlnx_tsn_port_ptp_exit() at unbind:

	scoped_guard(spinlock_irqsave, &m->ptp_tx_lock)
		while ((skb = __skb_dequeue(&m->ptp_txq)) != NULL)
			dev_kfree_skb_any(skb);

TSN_TC_TX_EN is only set in xlnx_tsn_mac_link_up(), so frames written into a
slot while the link is down never depart, never complete, and hold their
skbs - and with them the owning socket's sk_wmem_alloc charge via
skb->sk/sock_wfree - until the driver is unbound.  Stale skbs from a
previous link-up also stay queued while fls(frame_waiting) re-allocates the
same slot indices.  Would a drain on link down / port disable, or a
timeout, be worth adding?

> +
> +		if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)
> +			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;

[Severity: Medium]
Is m->hwtstamp_tx_type ever consumed?  xlnx_tsn_port_hwtstamp_set() stores
it and xlnx_tsn_port_hwtstamp_get() reports it back, but the TX path gates
only on the skb flag here, and xlnx_tsn_tx_tstamp_work() then gates only on
SKBTX_IN_PROGRESS:

	if (skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) {
		xlnx_tsn_read_tstamp(m, &hwtstamps, ts_off);
		skb_tstamp_tx(skb, &hwtstamps);
	}

So a socket with SOF_TIMESTAMPING_TX_HARDWARE still gets hardware TX
timestamps on the error queue while the port is configured
HWTSTAMP_TX_OFF.  The RX half of the same ABI does honour its stored state
(READ_ONCE(m->hwtstamp_rx_filter) gates xlnx_tsn_read_tstamp()), so the two
halves disagree.

The commit message says "TX timestamping accepts HWTSTAMP_TX_OFF and
HWTSTAMP_TX_ON" and the kernel-doc calls @hwtstamp_tx_type the "current
SO_TIMESTAMPING TX type for this port", which both suggest the value has an
effect.

Separately, SKBTX_HW_TSTAMP is (SKBTX_HW_TSTAMP_NOBPF | SKBTX_BPF), so this
test also fires for BPF-only timestamping requests; the DSA core uses
SKBTX_HW_TSTAMP_NOBPF in dsa_skb_tx_timestamp().  Was
SKBTX_HW_TSTAMP_NOBPF intended here?

> +
> +		skb_tx_timestamp(skb);
> +		mac_iow(m, TSN_PTP_TX_CONTROL_OFFSET, BIT(free_index));
> +	}
> +}
> +
> +static void xlnx_tsn_ptp_recv(struct xlnx_tsn *sw, int port)
> +{
> +	struct net_device *user = dsa_to_port(&sw->ds, port)->user;
> +	struct xlnx_tsn_mac *m = &sw->mac[port];
> +	unsigned long frame_base;
> +	struct sk_buff *skb;
> +	u16 msg_len;
> +	u8 msg_type;
> +
> +	if (!user || !netif_running(user))
> +		return;

[Severity: High]
Can this dereference a freed net_device during teardown?  dp->user is read
here from hardirq context (xlnx_tsn_ptp_rx_isr() calls this directly) with
no RCU section, lock, or netdev reference, while the PTP RX interrupt is
only released much later, in xlnx_tsn_port_ptp_exit() from ds->ops->
teardown().

The DSA teardown order is ports first, switches second:

net/dsa/dsa.c:dsa_tree_teardown()
	dsa_tree_teardown_ports(dst);	-> dsa_port_teardown()
	dsa_tree_teardown_switches(dst);	-> ds->ops->teardown()

and dsa_port_teardown() frees the netdev before clearing the pointer:

	case DSA_PORT_TYPE_USER:
		if (dp->user) {
			dsa_user_destroy(dp->user);
			dp->user = NULL;
		}

So between dsa_user_destroy() and the dp->user = NULL store, a PTP RX
interrupt reads a non-NULL dangling pointer and runs netif_running(),
netdev_alloc_skb(), eth_type_trans(), dev_sw_netstats_rx_add() and
netif_rx() on it.  Should the PTP RX capture be masked in hardware and the
IRQ freed before the user netdevs go away (or the RX work moved out of
hardirq and synchronised against teardown)?

> +
> +	while ((m->ptp_rx_hw_pointer & 0xf) != (m->ptp_rx_sw_pointer & 0xf)) {

[ ... ]

> +		memcpy_fromio_32(m, frame_base, skb->data,
> +				 TSN_PTP_RX_FRAME_SIZE);
> +
> +		msg_type = *(u8 *)(skb->data + ETH_HLEN) & 0xf;
> +		msg_len = get_unaligned_be16(skb->data + ETH_HLEN + 2);
> +
> +		if (msg_len + ETH_HLEN > TSN_PTP_RX_FRAME_SIZE) {
> +			dev_kfree_skb_any(skb);
> +			DEV_STATS_INC(user, rx_length_errors);
> +			continue;
> +		}
> +
> +		skb_put(skb, msg_len + ETH_HLEN);

[Severity: High]
Does this leak stale slot contents to the network stack?  msg_len is the PTP
messageLength field taken from the wire, not a hardware-reported receive
length, and the only validation is the upper bound above.

memcpy_fromio_32() always copies the full slot, so if a peer sends a short
0x88f7 frame that declares a large messageLength, skb_put() hands up to
~238 bytes of whatever was previously in that per-MAC RX slot (earlier
frames from other hosts, or uninitialised FPGA buffer content - the slot is
never zeroed) to netif_rx(), and from there to any listener or bridge port.

There is also no lower bound: msg_len may be 0 or shorter than the PTP
common header, and nothing cross-checks it against the number of bytes the
hardware actually received.  Is there a hardware length field for the slot
that could be used instead?

> +		skb->protocol = eth_type_trans(skb, user);
> +		skb->ip_summed = CHECKSUM_UNNECESSARY;

[ ... ]

> +		dev_sw_netstats_rx_add(user, skb->len);

[Severity: Medium]
Should this add ETH_HLEN back?  eth_type_trans() has already pulled the 14
byte MAC header, while the core DSA receive path feeds the same per-CPU
tstats counter on the same netdev in the other unit:

net/dsa/tag.c:dsa_switch_rcv()
	dev_sw_netstats_rx_add(skb->dev, skb->len + ETH_HLEN);

As it stands, rx_bytes reported through rtnl_link_stats64 under-counts PTP
frames by 14 bytes relative to every other frame on the same interface.

[Severity: Medium]
Is the non-irqsave u64_stats writer safe here?  This runs in hardirq
context (request_irq() with flags 0 and no threaded handler), and
dev_sw_netstats_rx_add() uses plain u64_stats_update_begin()/
u64_stats_update_end() on the DSA user device's per-CPU syncp, which is
also written from the transmit path:

net/dsa/user.c:dsa_user_xmit()
	dev_sw_netstats_tx_add(dev, 1, skb->len);

On 32-bit SMP those are real seqcount operations, so:

	dsa_user_xmit() -> u64_stats_update_begin()   (seq odd)
	  PTP RX hardirq on the same CPU
	    dev_sw_netstats_rx_add() -> begin()/end() (seq even again)
	  reader in dev_get_tstats64() sees an even seq mid-update

There is no _irqsave flavour of dev_sw_netstats_rx_add(), so would a
threaded IRQ / NAPI context for the RX path, or open-coded _irqsave
counter updates, be needed?

> +		netif_rx(skb);
> +	}
> +}

[ ... ]

> +static irqreturn_t xlnx_tsn_ptp_tx_isr(int irq, void *data)
> +{
> +	struct xlnx_tsn_mac *m = data;
> +
> +	mac_ior(m, TSN_PTP_TX_CONTROL_OFFSET);
> +	schedule_work(&m->tx_tstamp_work);
> +
> +	return IRQ_HANDLED;
> +}

[Severity: Medium]
How are these two interrupts acknowledged?  This handler reads
TSN_PTP_TX_CONTROL_OFFSET and throws the value away, and
xlnx_tsn_ptp_rx_isr() only samples the RX packet pointer without ever
writing TSN_PTP_RX_PACKET_CLEAR, while the PPS handler in the same file
does an explicit write to clear:

	mac_iow(&sw->mac[XLNX_TSN_PORT_MAC1], TSN_TIMER_INTERRUPT,
		TSN_TIMER_INT_CLEAR);

If the discarded read is a read-to-clear acknowledge, then the
TSN_PTP_TX_PACKET_FIELD_MASK status has already been consumed by the time
xlnx_tsn_tx_tstamp_work() re-reads the register for tx_packet.  If it is
not an acknowledge, does the line stay asserted until genirq's spurious
interrupt detector disables it?  Either way, a bare register read with no
comment is hard to follow.

> +
> +int xlnx_tsn_port_ptp_init(struct xlnx_tsn *sw, int port,
> +			   const char *rx_name, const char *tx_name)
> +{

[ ... ]

> +	mac_iow(m, TSN_PTP_RX_CONTROL_OFFSET, TSN_PTP_RX_PACKET_CLEAR);
> +
> +	ret = request_irq(m->ptp_rx_irq, xlnx_tsn_ptp_rx_isr, 0, rx_name, m);

[Severity: Medium]
Why is the RX pointer reset here but the TX block left untouched?  The
first transmit consumes TSN_PTP_TX_FRAME_WAITING_MASK and the first
completion pass consumes TSN_PTP_TX_PACKET_FIELD_MASK exactly as the
hardware left them.

After a rebind, or a warm reset, or a boot stage that already used the PTP
TX block, those fields can be non-zero:

  - a stale bit 7 makes xlnx_tsn_ptp_tx() take the "buffers full" branch and
    free every PTP frame from then on
  - other stale bits make fls(frame_waiting) pick a slot while ptp_txq is
    empty
  - a stale TX_PACKET makes the first xlnx_tsn_tx_tstamp_work() pass read a
    timestamp footer out of a slot that never transmitted and report it to
    userspace

xlnx_tsn_port_ptp_exit() also frees the IRQs and drains ptp_txq but leaves
the PTP TX/RX paths armed and any waiting bit set, so the next bind starts
from that state.  Should init clear the TX control fields (and exit quiesce
the hardware)?  Also, both this write and mac_iow(m,
TSN_PTP_TX_CONTROL_OFFSET, BIT(free_index)) are full-register writes to
registers that carry other fields - is a read-modify-write needed?

[ ... ]

> diff --git a/net/dsa/tag_xlnx_tsn.c b/net/dsa/tag_xlnx_tsn.c
> index de352aa3d9a8..6f487ad7bcf9 100644
> --- a/net/dsa/tag_xlnx_tsn.c
> +++ b/net/dsa/tag_xlnx_tsn.c
> @@ -3,14 +3,41 @@
>   * AMD/Xilinx TSN Endpoint Ethernet MAC tag protocol.
>   */
>  

[ ... ]

>  static struct sk_buff *xlnx_tsn_xmit(struct sk_buff *skb,
>  				     struct net_device *dev)
>  {
> -	return skb;
> +	struct xlnx_tsn_tagger_data *tagger_data;
> +	struct dsa_port *dp;
> +
> +	dp = dsa_user_to_port(dev);
> +	tagger_data = dp->ds->tagger_data;
> +
> +	if (!tagger_data || !tagger_data->ptp_tx)
> +		return skb;
> +
> +	if (eth_hdr(skb)->h_proto != htons(ETH_P_1588) ||
> +	    skb_vlan_tag_present(skb))
> +		return skb;
> +
> +	tagger_data->ptp_tx(dp, skb);

[Severity: Medium]
This matches on the ethertype only, so every non-VLAN 0x88f7 frame is
diverted into the 248 byte hardware slot, not just the PTP event frames the
commit message describes ("PTP event frames bypass the switch fabric").
Announce, signalling and management messages carrying TLVs can exceed 248
bytes.

xlnx_tsn_ptp_tx() then drops such a frame with no accounting:

	if (unlikely(skb->len > TSN_PTP_TX_MAX_FRAME_SIZE)) {
		dev_kfree_skb_any(skb);
		return;
	}
	...
		if (frame_waiting & TSN_PTP_TX_BUFFERS_FULL_MASK) {
			dev_kfree_skb_any(skb);
			return;
		}

Before this patch xlnx_tsn_xmit() returned the skb and it egressed through
the conduit, so this turns those frames into a silent black hole.
dsa_user_xmit() has already counted the frame as transmitted
(dev_sw_netstats_tx_add(dev, 1, skb->len)) before calling the tagger, so
nothing shows up in tx_dropped or tx_errors either.  Should oversized
frames fall back to the normal path, and should the drops bump a counter?

> +
> +	return NULL;
>  }