Re: [PATCH v5 net-next 3/9] net: mscc: ocelot: add MAC table stream learn and lookup operations

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Fri, Sep 24, 2021 at 05:52:20PM +0800, Xiaoliang Yang wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> ocelot_mact_learn_streamdata() can be used in VSC9959 to overwrite an
> FDB entry with stream data. The stream data includes SFID and SSID which
> can be used for PSFP and FRER set.
> 
> ocelot_mact_lookup() can be used to check if the given {DMAC, VID} FDB
> entry is exist, and also can retrieve the DEST_IDX and entry type for
> the FDB entry.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Signed-off-by: Xiaoliang Yang <xiaoliang.yang_1@nxp.com>

This is most certainly not my patch either, so you can drop my name from it.

> ---
>  drivers/net/ethernet/mscc/ocelot.c | 50 ++++++++++++++++++++++++++++++
>  include/soc/mscc/ocelot.h          |  5 +++
>  2 files changed, 55 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index 35006b0fb963..dc65247b91be 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -114,6 +114,56 @@ int ocelot_mact_forget(struct ocelot *ocelot,
>  }
>  EXPORT_SYMBOL(ocelot_mact_forget);
>  
> +int ocelot_mact_lookup(struct ocelot *ocelot, int *dst_idx,
> +		       struct ocelot_mact_entry *entry)

I think the arguments of this function can be improved.
Currently, "entry" is an inout argument: entry->mac and entry->vid are
input, and entry->type is output.

I think it would be better if the argument list looked like this:

int ocelot_mact_lookup(struct ocelot *ocelot, const unsigned char *addr,
		       u16 vid, enum macaccess_entry_type *type, int *dst_idx)

I think it's clearer this way which arguments are input and which are output.

Additionally, you can stop exporting struct ocelot_mact_entry, if this
is the only reason you needed it.

> +{
> +	int val;
> +
> +	mutex_lock(&ocelot->mact_lock);
> +
> +	ocelot_mact_select(ocelot, entry->mac, entry->vid);
> +
> +	/* Issue a read command with MACACCESS_VALID=1. */
> +	ocelot_write(ocelot, ANA_TABLES_MACACCESS_VALID |
> +		     ANA_TABLES_MACACCESS_MAC_TABLE_CMD(MACACCESS_CMD_READ),
> +		     ANA_TABLES_MACACCESS);
> +
> +	if (ocelot_mact_wait_for_completion(ocelot)) {
> +		mutex_unlock(&ocelot->mact_lock);
> +		return -ETIMEDOUT;
> +	}
> +
> +	/* Read back the entry flags */
> +	val = ocelot_read(ocelot, ANA_TABLES_MACACCESS);
> +
> +	mutex_unlock(&ocelot->mact_lock);
> +
> +	if (!(val & ANA_TABLES_MACACCESS_VALID))
> +		return -ENOENT;
> +
> +	*dst_idx = ANA_TABLES_MACACCESS_DEST_IDX_X(val);
> +	entry->type = ANA_TABLES_MACACCESS_ENTRYTYPE_X(val);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(ocelot_mact_lookup);
> +
> +int ocelot_mact_learn_streamdata(struct ocelot *ocelot, int dst_idx,
> +				 struct ocelot_mact_entry *entry, u32 data)

It would be nice if ocelot_mact_learn_streamdata would follow the basic
prototype of ocelot_mact_learn:

int ocelot_mact_learn(struct ocelot *ocelot, int port,
		      const unsigned char mac[ETH_ALEN],
		      unsigned int vid, enum macaccess_entry_type type)

just with the extra STREAMDATA argument at the end.
Also, could we format the STREAMDATA nicer than a raw u32?

> +{
> +	int ret;
> +
> +	mutex_lock(&ocelot->mact_lock);
> +	ocelot_write(ocelot, data, ANA_TABLES_STREAMDATA);
> +	mutex_unlock(&ocelot->mact_lock);
> +
> +	ret = ocelot_mact_learn(ocelot, dst_idx, entry->mac, entry->vid,
> +				entry->type);

Hm, if you temporarily drop the lock just for ocelot_mact_learn to take
it again, you allow somebody else to sneak in and learn another MAC
table entry, and the STREAMDATA will get associated with that other guy
and not with you, no?

> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(ocelot_mact_learn_streamdata);
> +
>  static void ocelot_mact_init(struct ocelot *ocelot)
>  {
>  	/* Configure the learning mode entries attributes:
> diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> index e6773f4d09ce..455293652257 100644
> --- a/include/soc/mscc/ocelot.h
> +++ b/include/soc/mscc/ocelot.h
> @@ -926,6 +926,11 @@ void ocelot_phylink_mac_link_up(struct ocelot *ocelot, int port,
>  				bool tx_pause, bool rx_pause,
>  				unsigned long quirks);
>  
> +int ocelot_mact_lookup(struct ocelot *ocelot, int *dst_idx,
> +		       struct ocelot_mact_entry *entry);
> +int ocelot_mact_learn_streamdata(struct ocelot *ocelot, int dst_idx,
> +				 struct ocelot_mact_entry *entry, u32 data);
> +
>  #if IS_ENABLED(CONFIG_BRIDGE_MRP)
>  int ocelot_mrp_add(struct ocelot *ocelot, int port,
>  		   const struct switchdev_obj_mrp *mrp);
> -- 
> 2.17.1
>