RE: [PATCH v3 net-next 5/8] net: dsa: netc: check return value of ntmp_ipft_delete_entry()

[Wei Fang]

> > -	ntmp_ipft_delete_entry(&priv->ntmp, entry_id);
> > +	np->ipft_hf_eid = NTMP_NULL_ENTRY_ID;
> > +	np->uc = false;
> > +	np->mc = false;
> > +	netc_port_wr(np, NETC_PIPFCR, 0);
> 
> [Severity: Medium]
> Is every error from ntmp_ipft_delete_entry() safe to interpret as "the
> entry is still programmed in hardware"?  The NTMP command path does not
> look transactional:
> 
> drivers/net/ethernet/freescale/enetc/ntmp.c:netc_xmit_ntmp_cmd() {
> 	...
> 	cbdr->next_to_use = i;
> 	netc_write(cbdr->regs.pir, i);
> 
> 	err = read_poll_timeout(netc_read, val, (val & NETC_CBDRCIR_INDEX) == i,
> 				NETC_CBDR_DELAY_US, NETC_CBDR_TIMEOUT, true,
> 				cbdr->regs.cir);
> 	if (unlikely(err))
> 		return err;
> 	...
> }
> 
> The DELETE BD and the producer index are published to hardware before the
> 1 ms completion poll, so -ETIMEDOUT means "completion not observed", not
> "command not executed"; -EIO from the writeback error status is similarly
> indeterminate.  In those cases np->ipft_hf_eid keeps an entry ID the
> hardware may no longer own, and there is no other release path:
> ntmp_ipft_delete_entry() has this single caller and nothing else clears
> np->ipft_hf_eid (this is still the case at the end of the series).

I think this concern is a bit over-cautious.

For a driver-allocated, currently-valid entry ID,
ntmp_ipft_delete_entry() will not return -ETIMEDOUT/-EBUSY/-EIO(error_rr)
on any normal path:

- -ETIMEDOUT: the command BD ring is only accessed while it is enabled
  (between ntmp_init_cbdr() and ntmp_free_cbdr()); nothing disables it
  at runtime. With the ring enabled, 1 ms is far more than enough for a
  single command, so the completion poll cannot time out.

- -EBUSY: commands are issued synchronously and serialized under
  ring_lock (post one BD, poll, return), so in-flight BDs never exceed
  one against a 256-entry ring. A full ring would require the hardware
  to stop advancing CIR, which would already surface as -ETIMEDOUT.

- -EIO(error_rr): the writeback error status is only set for malformed
  request data. Validated interfaces always build correct requests.

These three checks are defensive programming to help locate wrong
configuration or malformed request data when developing a new NTMP
interface; they do not fire for validated ones.

In short: with the CBDR enabled, commands serialized, and a valid,
driver-allocated entry ID, -ETIMEDOUT/-EBUSY/-EIO(error_rr) never occur
on the normal path -- they are defensive checks for new-interface
development. -EIO(SBE) and -ENOMEM are bus/memory-level catastrophic or
transient failures; there the command did not take effect, so keeping
ipft_hf_eid and returning the error is the intended behavior -- the
hardware entry is still present.