All of lore.kernel.org
 help / color / mirror / Atom feed
From: Vladimir Oltean <vladimir.oltean@nxp.com>
To: Horatiu Vultur <horatiu.vultur@microchip.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"UNGLinuxDriver@microchip.com" <UNGLinuxDriver@microchip.com>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"kuba@kernel.org" <kuba@kernel.org>,
	"f.fainelli@gmail.com" <f.fainelli@gmail.com>,
	"vivien.didelot@gmail.com" <vivien.didelot@gmail.com>,
	"andrew@lunn.ch" <andrew@lunn.ch>
Subject: Re: [PATCH net-next 1/3] net: lan966x: Add function lan966x_mac_cpu_copy()
Date: Mon, 3 Jan 2022 14:27:55 +0000	[thread overview]
Message-ID: <20220103142754.vtotw3clkwdrvcrd@skbuf> (raw)
In-Reply-To: <20220103131039.3473876-2-horatiu.vultur@microchip.com>

On Mon, Jan 03, 2022 at 02:10:37PM +0100, Horatiu Vultur wrote:
> Extend mac functionality with the function lan966x_mac_cpu_copy. This
> function adds an entry in the MAC table where a frame is copy to the CPU

s/copy/copied/

> but also can be forward on the front ports.

s/forward/forwarded/

> This functionality is needed for mdb support. In case the CPU and some
> of the front ports subscribe to an IP multicast address.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  .../ethernet/microchip/lan966x/lan966x_mac.c  | 27 ++++++++++++++++---
>  .../ethernet/microchip/lan966x/lan966x_main.h |  5 ++++
>  .../ethernet/microchip/lan966x/lan966x_regs.h |  6 +++++
>  3 files changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c b/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c
> index efadb8d326cc..ae3a7a10e0d6 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c
> @@ -68,16 +68,18 @@ static void lan966x_mac_select(struct lan966x *lan966x,
>  	lan_wr(mach, lan966x, ANA_MACHDATA);
>  }
>  
> -int lan966x_mac_learn(struct lan966x *lan966x, int port,
> -		      const unsigned char mac[ETH_ALEN],
> -		      unsigned int vid,
> -		      enum macaccess_entry_type type)
> +static int lan966x_mac_learn_impl(struct lan966x *lan966x, int port,
> +				  bool cpu_copy,
> +				  const unsigned char mac[ETH_ALEN],
> +				  unsigned int vid,
> +				  enum macaccess_entry_type type)

In the kernel, the __lan966x_mac_learn naming scheme seems to be more
popular than _impl.

Also, may I suggest that the "int port" argument may be better named
"int pgid"?

>  {
>  	lan966x_mac_select(lan966x, mac, vid);
>  
>  	/* Issue a write command */
>  	lan_wr(ANA_MACACCESS_VALID_SET(1) |
>  	       ANA_MACACCESS_CHANGE2SW_SET(0) |
> +	       ANA_MACACCESS_MAC_CPU_COPY_SET(cpu_copy) |
>  	       ANA_MACACCESS_DEST_IDX_SET(port) |
>  	       ANA_MACACCESS_ENTRYTYPE_SET(type) |
>  	       ANA_MACACCESS_MAC_TABLE_CMD_SET(MACACCESS_CMD_LEARN),
> @@ -86,6 +88,23 @@ int lan966x_mac_learn(struct lan966x *lan966x, int port,
>  	return lan966x_mac_wait_for_completion(lan966x);
>  }
>  
> +int lan966x_mac_cpu_copy(struct lan966x *lan966x, int port,
> +			 bool cpu_copy,
> +			 const unsigned char mac[ETH_ALEN],
> +			 unsigned int vid,
> +			 enum macaccess_entry_type type)

This doesn't seem to use the "port" argument from any of its call sites.
It is always 0. What would it even mean?

> +{
> +	return lan966x_mac_learn_impl(lan966x, port, cpu_copy, mac, vid, type);
> +}
> +
> +int lan966x_mac_learn(struct lan966x *lan966x, int port,
> +		      const unsigned char mac[ETH_ALEN],
> +		      unsigned int vid,
> +		      enum macaccess_entry_type type)
> +{
> +	return lan966x_mac_learn_impl(lan966x, port, false, mac, vid, type);

If you call lan966x_mac_cpu_copy() on an address and then
lan966x_mac_learn() on the same address but on an external port, how
does that work (why doesn't the "false" here overwrite the cpu_copy in
the previous command, breaking the copy-to-CPU feature)?

> +}
> +
>  int lan966x_mac_forget(struct lan966x *lan966x,
>  		       const unsigned char mac[ETH_ALEN],
>  		       unsigned int vid,
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> index c399b1256edc..a7a2a3537036 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> @@ -157,6 +157,11 @@ int lan966x_port_pcs_set(struct lan966x_port *port,
>  			 struct lan966x_port_config *config);
>  void lan966x_port_init(struct lan966x_port *port);
>  
> +int lan966x_mac_cpu_copy(struct lan966x *lan966x, int port,
> +			 bool cpu_copy,
> +			 const unsigned char mac[ETH_ALEN],
> +			 unsigned int vid,
> +			 enum macaccess_entry_type type);
>  int lan966x_mac_learn(struct lan966x *lan966x, int port,
>  		      const unsigned char mac[ETH_ALEN],
>  		      unsigned int vid,
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h b/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h
> index a13c469e139a..797560172aca 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h
> @@ -169,6 +169,12 @@ enum lan966x_target {
>  #define ANA_MACACCESS_CHANGE2SW_GET(x)\
>  	FIELD_GET(ANA_MACACCESS_CHANGE2SW, x)
>  
> +#define ANA_MACACCESS_MAC_CPU_COPY               BIT(16)
> +#define ANA_MACACCESS_MAC_CPU_COPY_SET(x)\
> +	FIELD_PREP(ANA_MACACCESS_MAC_CPU_COPY, x)
> +#define ANA_MACACCESS_MAC_CPU_COPY_GET(x)\
> +	FIELD_GET(ANA_MACACCESS_MAC_CPU_COPY, x)
> +
>  #define ANA_MACACCESS_VALID                      BIT(12)
>  #define ANA_MACACCESS_VALID_SET(x)\
>  	FIELD_PREP(ANA_MACACCESS_VALID, x)
> -- 
> 2.33.0
>

  reply	other threads:[~2022-01-03 14:29 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-03 13:10 [PATCH net-next 0/3] net: lan966x: Extend switchdev with mdb support Horatiu Vultur
2022-01-03 13:10 ` [PATCH net-next 1/3] net: lan966x: Add function lan966x_mac_cpu_copy() Horatiu Vultur
2022-01-03 14:27   ` Vladimir Oltean [this message]
2022-01-03 15:39     ` Horatiu Vultur
2022-01-03 15:46       ` Vladimir Oltean
2022-01-03 13:10 ` [PATCH net-next 2/3] net: lan966x: Add PGID_FIRST and PGID_LAST Horatiu Vultur
2022-01-03 13:10 ` [PATCH net-next 3/3] net: lan966x: Extend switchdev with mdb support Horatiu Vultur
2022-01-03 14:43   ` Vladimir Oltean
2022-01-03 16:22     ` Horatiu Vultur
2022-01-03 16:57       ` Vladimir Oltean

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220103142754.vtotw3clkwdrvcrd@skbuf \
    --to=vladimir.oltean@nxp.com \
    --cc=UNGLinuxDriver@microchip.com \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=f.fainelli@gmail.com \
    --cc=horatiu.vultur@microchip.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=vivien.didelot@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.