[PATCH 0/4] Refactoring for GMA command

[PATCH 0/4] Refactoring for GMA command

[Ivan Mikhaylov]

Make one GMA function for all manufacturers, change ndo_set_mac_address
to dev_set_mac_address for notifiying net layer about MAC change which
ndo_set_mac_address doesn't do. 

Add mac-address-increment option for possibility to control MAC address
assignment on BMC via GMA command. 

Ivan Mikhaylov (4):
  net/ncsi: make one oem_gma function for all mfr id
  net/ncsi: change from ndo_set_mac_address to dev_set_mac_address
  net/ftgmac100: add mac-address-increment option for GMA command from
    NC-SI
  net/ncsi: add shift MAC address property

 .../devicetree/bindings/net/ftgmac100.txt     |   4 +
 net/ncsi/ncsi-rsp.c                           | 108 ++++++------------
 2 files changed, 41 insertions(+), 71 deletions(-)

-- 
2.40.0

[PATCH 1/4] net/ncsi: make one oem_gma function for all mfr id

[Ivan Mikhaylov]

Make the one Get Mac Address function for all manufacturers and change
this call in handlers accordingly.

Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
---
 net/ncsi/ncsi-rsp.c | 88 ++++++++++-----------------------------------
 1 file changed, 19 insertions(+), 69 deletions(-)

diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 6447a09932f5..91c42253a711 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -611,14 +611,15 @@ static int ncsi_rsp_handler_snfc(struct ncsi_request *nr)
 	return 0;
 }
 
-/* Response handler for Mellanox command Get Mac Address */
-static int ncsi_rsp_handler_oem_mlx_gma(struct ncsi_request *nr)
+/* Response handler for Get Mac Address command */
+static int ncsi_rsp_handler_oem_gma(struct ncsi_request *nr, int mfr_id)
 {
 	struct ncsi_dev_priv *ndp = nr->ndp;
 	struct net_device *ndev = ndp->ndev.dev;
 	const struct net_device_ops *ops = ndev->netdev_ops;
 	struct ncsi_rsp_oem_pkt *rsp;
 	struct sockaddr saddr;
+	u32 mac_addr_off = 0;
 	int ret = 0;
 
 	/* Get the response header */
@@ -626,7 +627,19 @@ static int ncsi_rsp_handler_oem_mlx_gma(struct ncsi_request *nr)
 
 	saddr.sa_family = ndev->type;
 	ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
-	memcpy(saddr.sa_data, &rsp->data[MLX_MAC_ADDR_OFFSET], ETH_ALEN);
+	if (mfr_id == NCSI_OEM_MFR_BCM_ID)
+		mac_addr_off = BCM_MAC_ADDR_OFFSET;
+	else if (mfr_id == NCSI_OEM_MFR_MLX_ID)
+		mac_addr_off = MLX_MAC_ADDR_OFFSET;
+	else if (mfr_id == NCSI_OEM_MFR_INTEL_ID)
+		mac_addr_off = INTEL_MAC_ADDR_OFFSET;
+
+	memcpy(saddr.sa_data, &rsp->data[mac_addr_off], ETH_ALEN);
+	if (mfr_id == NCSI_OEM_MFR_BCM_ID || mfr_id == NCSI_OEM_MFR_INTEL_ID)
+		eth_addr_inc((u8 *)saddr.sa_data);
+	if (!is_valid_ether_addr((const u8 *)saddr.sa_data))
+		return -ENXIO;
+
 	/* Set the flag for GMA command which should only be called once */
 	ndp->gma_flag = 1;
 
@@ -649,41 +662,10 @@ static int ncsi_rsp_handler_oem_mlx(struct ncsi_request *nr)
 
 	if (mlx->cmd == NCSI_OEM_MLX_CMD_GMA &&
 	    mlx->param == NCSI_OEM_MLX_CMD_GMA_PARAM)
-		return ncsi_rsp_handler_oem_mlx_gma(nr);
+		return ncsi_rsp_handler_oem_gma(nr, NCSI_OEM_MFR_MLX_ID);
 	return 0;
 }
 
-/* Response handler for Broadcom command Get Mac Address */
-static int ncsi_rsp_handler_oem_bcm_gma(struct ncsi_request *nr)
-{
-	struct ncsi_dev_priv *ndp = nr->ndp;
-	struct net_device *ndev = ndp->ndev.dev;
-	const struct net_device_ops *ops = ndev->netdev_ops;
-	struct ncsi_rsp_oem_pkt *rsp;
-	struct sockaddr saddr;
-	int ret = 0;
-
-	/* Get the response header */
-	rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
-
-	saddr.sa_family = ndev->type;
-	ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
-	memcpy(saddr.sa_data, &rsp->data[BCM_MAC_ADDR_OFFSET], ETH_ALEN);
-	/* Increase mac address by 1 for BMC's address */
-	eth_addr_inc((u8 *)saddr.sa_data);
-	if (!is_valid_ether_addr((const u8 *)saddr.sa_data))
-		return -ENXIO;
-
-	/* Set the flag for GMA command which should only be called once */
-	ndp->gma_flag = 1;
-
-	ret = ops->ndo_set_mac_address(ndev, &saddr);
-	if (ret < 0)
-		netdev_warn(ndev, "NCSI: 'Writing mac address to device failed\n");
-
-	return ret;
-}
-
 /* Response handler for Broadcom card */
 static int ncsi_rsp_handler_oem_bcm(struct ncsi_request *nr)
 {
@@ -695,42 +677,10 @@ static int ncsi_rsp_handler_oem_bcm(struct ncsi_request *nr)
 	bcm = (struct ncsi_rsp_oem_bcm_pkt *)(rsp->data);
 
 	if (bcm->type == NCSI_OEM_BCM_CMD_GMA)
-		return ncsi_rsp_handler_oem_bcm_gma(nr);
+		return ncsi_rsp_handler_oem_gma(nr, NCSI_OEM_MFR_BCM_ID);
 	return 0;
 }
 
-/* Response handler for Intel command Get Mac Address */
-static int ncsi_rsp_handler_oem_intel_gma(struct ncsi_request *nr)
-{
-	struct ncsi_dev_priv *ndp = nr->ndp;
-	struct net_device *ndev = ndp->ndev.dev;
-	const struct net_device_ops *ops = ndev->netdev_ops;
-	struct ncsi_rsp_oem_pkt *rsp;
-	struct sockaddr saddr;
-	int ret = 0;
-
-	/* Get the response header */
-	rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
-
-	saddr.sa_family = ndev->type;
-	ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
-	memcpy(saddr.sa_data, &rsp->data[INTEL_MAC_ADDR_OFFSET], ETH_ALEN);
-	/* Increase mac address by 1 for BMC's address */
-	eth_addr_inc((u8 *)saddr.sa_data);
-	if (!is_valid_ether_addr((const u8 *)saddr.sa_data))
-		return -ENXIO;
-
-	/* Set the flag for GMA command which should only be called once */
-	ndp->gma_flag = 1;
-
-	ret = ops->ndo_set_mac_address(ndev, &saddr);
-	if (ret < 0)
-		netdev_warn(ndev,
-			    "NCSI: 'Writing mac address to device failed\n");
-
-	return ret;
-}
-
 /* Response handler for Intel card */
 static int ncsi_rsp_handler_oem_intel(struct ncsi_request *nr)
 {
@@ -742,7 +692,7 @@ static int ncsi_rsp_handler_oem_intel(struct ncsi_request *nr)
 	intel = (struct ncsi_rsp_oem_intel_pkt *)(rsp->data);
 
 	if (intel->cmd == NCSI_OEM_INTEL_CMD_GMA)
-		return ncsi_rsp_handler_oem_intel_gma(nr);
+		return ncsi_rsp_handler_oem_gma(nr, NCSI_OEM_MFR_INTEL_ID);
 
 	return 0;
 }
-- 
2.40.0

[PATCH 2/4] net/ncsi: change from ndo_set_mac_address to dev_set_mac_address

[Ivan Mikhaylov]

Change ndo_set_mac_address to dev_set_mac_address because
dev_set_mac_address provides a way to notify network layer about MAC
change. In other case, services may not aware about MAC change and keep
using old one which set from network adapter driver.

As example, DHCP client from systemd do not update MAC address without
notification from net subsystem which leads to the problem with acquiring
the right address from DHCP server.

Signed-off-by: Paul Fertser <fercerpav@gmail.com>
Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
---
 net/ncsi/ncsi-rsp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 91c42253a711..069c2659074b 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -616,7 +616,6 @@ static int ncsi_rsp_handler_oem_gma(struct ncsi_request *nr, int mfr_id)
 {
 	struct ncsi_dev_priv *ndp = nr->ndp;
 	struct net_device *ndev = ndp->ndev.dev;
-	const struct net_device_ops *ops = ndev->netdev_ops;
 	struct ncsi_rsp_oem_pkt *rsp;
 	struct sockaddr saddr;
 	u32 mac_addr_off = 0;
@@ -643,7 +642,9 @@ static int ncsi_rsp_handler_oem_gma(struct ncsi_request *nr, int mfr_id)
 	/* Set the flag for GMA command which should only be called once */
 	ndp->gma_flag = 1;
 
-	ret = ops->ndo_set_mac_address(ndev, &saddr);
+	rtnl_lock();
+	ret = dev_set_mac_address(ndev, &saddr, NULL);
+	rtnl_unlock();
 	if (ret < 0)
 		netdev_warn(ndev, "NCSI: 'Writing mac address to device failed\n");
 
-- 
2.40.0

[PATCH 3/4] net/ftgmac100: add mac-address-increment option for GMA command from NC-SI

[Ivan Mikhaylov]

Add s32 mac-address-increment option for Get MAC Address command from
NC-SI.

Signed-off-by: Paul Fertser <fercerpav@gmail.com>
Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
---
 Documentation/devicetree/bindings/net/ftgmac100.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ftgmac100.txt b/Documentation/devicetree/bindings/net/ftgmac100.txt
index 29234021f601..7ef5329d888d 100644
--- a/Documentation/devicetree/bindings/net/ftgmac100.txt
+++ b/Documentation/devicetree/bindings/net/ftgmac100.txt
@@ -22,6 +22,10 @@ Optional properties:
 - use-ncsi: Use the NC-SI stack instead of an MDIO PHY. Currently assumes
   rmii (100bT) but kept as a separate property in case NC-SI grows support
   for a gigabit link.
+- mac-address-increment: Increment the MAC address taken by GMA command via
+  NC-SI. Specifies a signed number to be added to the host MAC address as
+  obtained by the OEM GMA command. If not specified, 1 is used by default
+  for Broadcom and Intel network cards, 0 otherwise.
 - no-hw-checksum: Used to disable HW checksum support. Here for backward
   compatibility as the driver now should have correct defaults based on
   the SoC.
-- 
2.40.0

Re: [PATCH 3/4] net/ftgmac100: add mac-address-increment option for GMA command from NC-SI

[Krzysztof Kozlowski]

On 13/04/2023 02:29, Ivan Mikhaylov wrote:
> Add s32 mac-address-increment option for Get MAC Address command from
> NC-SI.
> 
> Signed-off-by: Paul Fertser <fercerpav@gmail.com>
> Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> ---
>  Documentation/devicetree/bindings/net/ftgmac100.txt | 4 ++++

Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching).

>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ftgmac100.txt b/Documentation/devicetree/bindings/net/ftgmac100.txt
> index 29234021f601..7ef5329d888d 100644
> --- a/Documentation/devicetree/bindings/net/ftgmac100.txt
> +++ b/Documentation/devicetree/bindings/net/ftgmac100.txt
> @@ -22,6 +22,10 @@ Optional properties:
>  - use-ncsi: Use the NC-SI stack instead of an MDIO PHY. Currently assumes
>    rmii (100bT) but kept as a separate property in case NC-SI grows support
>    for a gigabit link.
> +- mac-address-increment: Increment the MAC address taken by GMA command via
> +  NC-SI. Specifies a signed number to be added to the host MAC address as
> +  obtained by the OEM GMA command. If not specified, 1 is used by default
> +  for Broadcom and Intel network cards, 0 otherwise.

First conversion to DT Schema. New properties are not accepted to TXT files.

Best regards,
Krzysztof

Re: [PATCH 3/4] net/ftgmac100: add mac-address-increment option for GMA command from NC-SI

[Rob Herring]

On Thu, Apr 13, 2023 at 12:29:04AM +0000, Ivan Mikhaylov wrote:
> Add s32 mac-address-increment option for Get MAC Address command from
> NC-SI.
> 
> Signed-off-by: Paul Fertser <fercerpav@gmail.com>
> Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> ---
>  Documentation/devicetree/bindings/net/ftgmac100.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ftgmac100.txt b/Documentation/devicetree/bindings/net/ftgmac100.txt
> index 29234021f601..7ef5329d888d 100644
> --- a/Documentation/devicetree/bindings/net/ftgmac100.txt
> +++ b/Documentation/devicetree/bindings/net/ftgmac100.txt
> @@ -22,6 +22,10 @@ Optional properties:
>  - use-ncsi: Use the NC-SI stack instead of an MDIO PHY. Currently assumes
>    rmii (100bT) but kept as a separate property in case NC-SI grows support
>    for a gigabit link.
> +- mac-address-increment: Increment the MAC address taken by GMA command via
> +  NC-SI. Specifies a signed number to be added to the host MAC address as
> +  obtained by the OEM GMA command. If not specified, 1 is used by default
> +  for Broadcom and Intel network cards, 0 otherwise.

This would need to be common. There's been some attempts around how to 
support a base MAC address with a transform per instance. So far it's 
not clear that something in DT works for everyone. Until there's 
something common (if ever), you need platform specific code somewhere to 
handle this. The nvmem binding has had some extensions to support that.

Rob

Re: [PATCH 3/4] net/ftgmac100: add mac-address-increment option for GMA command from NC-SI

[Ivan Mikhaylov]

On Tue, 2023-04-18 at 13:54 -0500, Rob Herring wrote:
> On Thu, Apr 13, 2023 at 12:29:04AM +0000, Ivan Mikhaylov wrote:
> > Add s32 mac-address-increment option for Get MAC Address command
> > from
> > NC-SI.
> > 
> > Signed-off-by: Paul Fertser <fercerpav@gmail.com>
> > Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> > ---
> >  Documentation/devicetree/bindings/net/ftgmac100.txt | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/ftgmac100.txt
> > b/Documentation/devicetree/bindings/net/ftgmac100.txt
> > index 29234021f601..7ef5329d888d 100644
> > --- a/Documentation/devicetree/bindings/net/ftgmac100.txt
> > +++ b/Documentation/devicetree/bindings/net/ftgmac100.txt
> > @@ -22,6 +22,10 @@ Optional properties:
> >  - use-ncsi: Use the NC-SI stack instead of an MDIO PHY. Currently
> > assumes
> >    rmii (100bT) but kept as a separate property in case NC-SI grows
> > support
> >    for a gigabit link.
> > +- mac-address-increment: Increment the MAC address taken by GMA
> > command via
> > +  NC-SI. Specifies a signed number to be added to the host MAC
> > address as
> > +  obtained by the OEM GMA command. If not specified, 1 is used by
> > default
> > +  for Broadcom and Intel network cards, 0 otherwise.
> 
> This would need to be common. There's been some attempts around how
> to 
> support a base MAC address with a transform per instance. So far it's
> not clear that something in DT works for everyone. Until there's 
> something common (if ever), you need platform specific code somewhere
> to 
> handle this. The nvmem binding has had some extensions to support
> that.
> 
> Rob

Rob, I agree but unfortunately there isn't a generic option for such
case, maybe something should be added into net/ethernet-
controller.yaml? As example, `mac-address-increment` option using
widely in openwrt project. About nvmem, are we talking `nvmem-cell-
names` option or reverse_mac_address in drivers/nvmem/imx-ocotp.c?

I'll do the transfer into DT schema, that's not a problem but after
naming resolve.

Adding openbmc community, maybe they have some ideas about this one.

Thanks.

Re: [PATCH 3/4] net/ftgmac100: add mac-address-increment option for GMA command from NC-SI

[Ivan Mikhaylov]

On Tue, 2023-04-18 at 13:54 -0500, Rob Herring wrote:
> On Thu, Apr 13, 2023 at 12:29:04AM +0000, Ivan Mikhaylov wrote:
> > Add s32 mac-address-increment option for Get MAC Address command
> > from
> > NC-SI.
> > 
> > Signed-off-by: Paul Fertser <fercerpav@gmail.com>
> > Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> > ---
> >  Documentation/devicetree/bindings/net/ftgmac100.txt | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/ftgmac100.txt
> > b/Documentation/devicetree/bindings/net/ftgmac100.txt
> > index 29234021f601..7ef5329d888d 100644
> > --- a/Documentation/devicetree/bindings/net/ftgmac100.txt
> > +++ b/Documentation/devicetree/bindings/net/ftgmac100.txt
> > @@ -22,6 +22,10 @@ Optional properties:
> >  - use-ncsi: Use the NC-SI stack instead of an MDIO PHY. Currently
> > assumes
> >    rmii (100bT) but kept as a separate property in case NC-SI grows
> > support
> >    for a gigabit link.
> > +- mac-address-increment: Increment the MAC address taken by GMA
> > command via
> > +  NC-SI. Specifies a signed number to be added to the host MAC
> > address as
> > +  obtained by the OEM GMA command. If not specified, 1 is used by
> > default
> > +  for Broadcom and Intel network cards, 0 otherwise.
> 
> This would need to be common. There's been some attempts around how
> to 
> support a base MAC address with a transform per instance. So far it's
> not clear that something in DT works for everyone. Until there's 
> something common (if ever), you need platform specific code somewhere
> to 
> handle this. The nvmem binding has had some extensions to support
> that.
> 
> Rob

Rob, I agree but unfortunately there isn't a generic option for such
case, maybe something should be added into net/ethernet-
controller.yaml? As example, `mac-address-increment` option using
widely in openwrt project. About nvmem, are we talking `nvmem-cell-
names` option or reverse_mac_address in drivers/nvmem/imx-ocotp.c?

I'll do the transfer into DT schema, that's not a problem but after
naming resolve.

Adding openbmc community, maybe they have some ideas about this one.

Thanks.

[PATCH 4/4] net/ncsi: add shift MAC address property

[Ivan Mikhaylov]

Add the shift MAC address property for GMA command which provides which
shift should be used but keep old one values for backward compatibility.

Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
---
 net/ncsi/ncsi-rsp.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 069c2659074b..1f108db34d85 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -9,6 +9,8 @@
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
 #include <linux/skbuff.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
 
 #include <net/ncsi.h>
 #include <net/net_namespace.h>
@@ -616,9 +618,12 @@ static int ncsi_rsp_handler_oem_gma(struct ncsi_request *nr, int mfr_id)
 {
 	struct ncsi_dev_priv *ndp = nr->ndp;
 	struct net_device *ndev = ndp->ndev.dev;
+	struct platform_device *pdev;
 	struct ncsi_rsp_oem_pkt *rsp;
 	struct sockaddr saddr;
 	u32 mac_addr_off = 0;
+	s32 shift_mac_addr = 0;
+	u64 mac_addr;
 	int ret = 0;
 
 	/* Get the response header */
@@ -635,7 +640,17 @@ static int ncsi_rsp_handler_oem_gma(struct ncsi_request *nr, int mfr_id)
 
 	memcpy(saddr.sa_data, &rsp->data[mac_addr_off], ETH_ALEN);
 	if (mfr_id == NCSI_OEM_MFR_BCM_ID || mfr_id == NCSI_OEM_MFR_INTEL_ID)
-		eth_addr_inc((u8 *)saddr.sa_data);
+		shift_mac_addr = 1;
+
+	pdev = to_platform_device(ndev->dev.parent);
+	if (pdev)
+		of_property_read_s32(pdev->dev.of_node,
+				     "mac-address-increment", &shift_mac_addr);
+
+	/* Increase mac address by shift value for BMC's address */
+	mac_addr = ether_addr_to_u64((u8 *)saddr.sa_data);
+	mac_addr += shift_mac_addr;
+	u64_to_ether_addr(mac_addr, (u8 *)saddr.sa_data);
 	if (!is_valid_ether_addr((const u8 *)saddr.sa_data))
 		return -ENXIO;
 
-- 
2.40.0