DriverDev-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 net] staging: octeon: repair "fixed-link" support
@ 2020-10-16 10:18 Alexander A Sverdlin
  2020-10-16 10:18 ` [PATCH v2 net] staging: octeon: Drop on uncorrectable alignment or FCS error Alexander A Sverdlin
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Alexander A Sverdlin @ 2020-10-16 10:18 UTC (permalink / raw)
  To: devel
  Cc: Aaro Koskinen, Greg Kroah-Hartman, Ralf Baechle,
	Alexander Sverdlin, netdev, David S. Miller

From: Alexander Sverdlin <alexander.sverdlin@nokia.com>

The PHYs must be registered once in device probe function, not in device
open callback because it's only possible to register them once.

Fixes: a25e278020 ("staging: octeon: support fixed-link phys")
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
---
 drivers/staging/octeon/ethernet-mdio.c | 6 ------
 drivers/staging/octeon/ethernet.c      | 9 +++++++++
 2 files changed, 9 insertions(+), 6 deletions(-)

Changes in v2:
- removed the usage of non-upstream local variable "r"

diff --git a/drivers/staging/octeon/ethernet-mdio.c b/drivers/staging/octeon/ethernet-mdio.c
index cfb673a..0bf54584 100644
--- a/drivers/staging/octeon/ethernet-mdio.c
+++ b/drivers/staging/octeon/ethernet-mdio.c
@@ -147,12 +147,6 @@ int cvm_oct_phy_setup_device(struct net_device *dev)
 
 	phy_node = of_parse_phandle(priv->of_node, "phy-handle", 0);
 	if (!phy_node && of_phy_is_fixed_link(priv->of_node)) {
-		int rc;
-
-		rc = of_phy_register_fixed_link(priv->of_node);
-		if (rc)
-			return rc;
-
 		phy_node = of_node_get(priv->of_node);
 	}
 	if (!phy_node)
diff --git a/drivers/staging/octeon/ethernet.c b/drivers/staging/octeon/ethernet.c
index 204f0b1..5dea6e9 100644
--- a/drivers/staging/octeon/ethernet.c
+++ b/drivers/staging/octeon/ethernet.c
@@ -13,6 +13,7 @@
 #include <linux/phy.h>
 #include <linux/slab.h>
 #include <linux/interrupt.h>
+#include <linux/of_mdio.h>
 #include <linux/of_net.h>
 #include <linux/if_ether.h>
 #include <linux/if_vlan.h>
@@ -892,6 +893,14 @@ static int cvm_oct_probe(struct platform_device *pdev)
 				break;
 			}
 
+			if (priv->of_node && of_phy_is_fixed_link(priv->of_node)) {
+				if (of_phy_register_fixed_link(priv->of_node)) {
+					netdev_err(dev, "Failed to register fixed link for interface %d, port %d\n",
+						   interface, priv->port);
+					dev->netdev_ops = NULL;
+				}
+			}
+
 			if (!dev->netdev_ops) {
 				free_netdev(dev);
 			} else if (register_netdev(dev) < 0) {
-- 
2.10.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 net] staging: octeon: Drop on uncorrectable alignment or FCS error
  2020-10-16 10:18 [PATCH v2 net] staging: octeon: repair "fixed-link" support Alexander A Sverdlin
@ 2020-10-16 10:18 ` Alexander A Sverdlin
  2020-10-16 11:17   ` Greg Kroah-Hartman
                     ` (2 more replies)
  2020-10-17 20:45 ` [PATCH v2 net] staging: octeon: repair "fixed-link" support Andrew Lunn
  2020-10-17 20:48 ` Andrew Lunn
  2 siblings, 3 replies; 8+ messages in thread
From: Alexander A Sverdlin @ 2020-10-16 10:18 UTC (permalink / raw)
  To: devel
  Cc: Aaro Koskinen, Greg Kroah-Hartman, Ralf Baechle,
	Alexander Sverdlin, netdev, David S. Miller

From: Alexander Sverdlin <alexander.sverdlin@nokia.com>

Currently in case of alignment or FCS error if the packet cannot be
corrected it's still not dropped. Report the error properly and drop the
packet while making the code around a little bit more readable.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
Fixes: 80ff0fd3ab ("Staging: Add octeon-ethernet driver files.")

Change-Id: Ie1fadcc57cb5e221cf3e83c169b53a5533b8edff
---
 drivers/staging/octeon/ethernet-rx.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

Changes in v2:
- whitespace alignment fix as suggested by Dan Carpenter
- fixed the logic to accept "corrected" packets (preambles 0xd, 0xd5)

diff --git a/drivers/staging/octeon/ethernet-rx.c b/drivers/staging/octeon/ethernet-rx.c
index 2c16230..9ebd665 100644
--- a/drivers/staging/octeon/ethernet-rx.c
+++ b/drivers/staging/octeon/ethernet-rx.c
@@ -69,15 +69,17 @@ static inline int cvm_oct_check_rcv_error(struct cvmx_wqe *work)
 	else
 		port = work->word1.cn38xx.ipprt;
 
-	if ((work->word2.snoip.err_code == 10) && (work->word1.len <= 64)) {
+	if ((work->word2.snoip.err_code == 10) && (work->word1.len <= 64))
 		/*
 		 * Ignore length errors on min size packets. Some
 		 * equipment incorrectly pads packets to 64+4FCS
 		 * instead of 60+4FCS.  Note these packets still get
 		 * counted as frame errors.
 		 */
-	} else if (work->word2.snoip.err_code == 5 ||
-		   work->word2.snoip.err_code == 7) {
+		return 0;
+
+	if (work->word2.snoip.err_code == 5 ||
+	    work->word2.snoip.err_code == 7) {
 		/*
 		 * We received a packet with either an alignment error
 		 * or a FCS error. This may be signalling that we are
@@ -108,7 +110,10 @@ static inline int cvm_oct_check_rcv_error(struct cvmx_wqe *work)
 				/* Port received 0xd5 preamble */
 				work->packet_ptr.s.addr += i + 1;
 				work->word1.len -= i + 5;
-			} else if ((*ptr & 0xf) == 0xd) {
+				return 0;
+			}
+
+			if ((*ptr & 0xf) == 0xd) {
 				/* Port received 0xd preamble */
 				work->packet_ptr.s.addr += i;
 				work->word1.len -= i + 4;
@@ -118,21 +123,20 @@ static inline int cvm_oct_check_rcv_error(struct cvmx_wqe *work)
 					    ((*(ptr + 1) & 0xf) << 4);
 					ptr++;
 				}
-			} else {
-				printk_ratelimited("Port %d unknown preamble, packet dropped\n",
-						   port);
-				cvm_oct_free_work(work);
-				return 1;
+				return 0;
 			}
+
+			printk_ratelimited("Port %d unknown preamble, packet dropped\n",
+					   port);
+			cvm_oct_free_work(work);
+			return 1;
 		}
-	} else {
-		printk_ratelimited("Port %d receive error code %d, packet dropped\n",
-				   port, work->word2.snoip.err_code);
-		cvm_oct_free_work(work);
-		return 1;
 	}
 
-	return 0;
+	printk_ratelimited("Port %d receive error code %d, packet dropped\n",
+			   port, work->word2.snoip.err_code);
+	cvm_oct_free_work(work);
+	return 1;
 }
 
 static void copy_segments_to_skb(struct cvmx_wqe *work, struct sk_buff *skb)
-- 
2.10.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 net] staging: octeon: Drop on uncorrectable alignment or FCS error
  2020-10-16 10:18 ` [PATCH v2 net] staging: octeon: Drop on uncorrectable alignment or FCS error Alexander A Sverdlin
@ 2020-10-16 11:17   ` Greg Kroah-Hartman
  2020-10-16 13:14   ` Greg Kroah-Hartman
  2020-10-17 21:02   ` Andrew Lunn
  2 siblings, 0 replies; 8+ messages in thread
From: Greg Kroah-Hartman @ 2020-10-16 11:17 UTC (permalink / raw)
  To: Alexander A Sverdlin
  Cc: devel, netdev, David S. Miller, Ralf Baechle, Aaro Koskinen

On Fri, Oct 16, 2020 at 12:18:58PM +0200, Alexander A Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> 
> Currently in case of alignment or FCS error if the packet cannot be
> corrected it's still not dropped. Report the error properly and drop the
> packet while making the code around a little bit more readable.
> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> Fixes: 80ff0fd3ab ("Staging: Add octeon-ethernet driver files.")
> 
> Change-Id: Ie1fadcc57cb5e221cf3e83c169b53a5533b8edff

You didn't run checkpatch :(

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 net] staging: octeon: Drop on uncorrectable alignment or FCS error
  2020-10-16 10:18 ` [PATCH v2 net] staging: octeon: Drop on uncorrectable alignment or FCS error Alexander A Sverdlin
  2020-10-16 11:17   ` Greg Kroah-Hartman
@ 2020-10-16 13:14   ` Greg Kroah-Hartman
  2020-10-17 21:02   ` Andrew Lunn
  2 siblings, 0 replies; 8+ messages in thread
From: Greg Kroah-Hartman @ 2020-10-16 13:14 UTC (permalink / raw)
  To: Alexander A Sverdlin
  Cc: devel, netdev, David S. Miller, Ralf Baechle, Aaro Koskinen

On Fri, Oct 16, 2020 at 12:18:58PM +0200, Alexander A Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> 
> Currently in case of alignment or FCS error if the packet cannot be
> corrected it's still not dropped. Report the error properly and drop the
> packet while making the code around a little bit more readable.
> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> Fixes: 80ff0fd3ab ("Staging: Add octeon-ethernet driver files.")
> 
> Change-Id: Ie1fadcc57cb5e221cf3e83c169b53a5533b8edff

Why is the change-id still here???
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 net] staging: octeon: repair "fixed-link" support
  2020-10-16 10:18 [PATCH v2 net] staging: octeon: repair "fixed-link" support Alexander A Sverdlin
  2020-10-16 10:18 ` [PATCH v2 net] staging: octeon: Drop on uncorrectable alignment or FCS error Alexander A Sverdlin
@ 2020-10-17 20:45 ` Andrew Lunn
  2020-10-17 20:48 ` Andrew Lunn
  2 siblings, 0 replies; 8+ messages in thread
From: Andrew Lunn @ 2020-10-17 20:45 UTC (permalink / raw)
  To: Alexander A Sverdlin
  Cc: devel, Aaro Koskinen, Greg Kroah-Hartman, Ralf Baechle, netdev,
	David S. Miller

> +			if (priv->of_node && of_phy_is_fixed_link(priv->of_node)) {
> +				if (of_phy_register_fixed_link(priv->of_node)) {
> +					netdev_err(dev, "Failed to register fixed link for interface %d, port %d\n",
> +						   interface, priv->port);
> +					dev->netdev_ops = NULL;
> +				}
> +			}
> +
>  			if (!dev->netdev_ops) {
>  				free_netdev(dev);

Setting dev->netdev_ops to NULL to indicate an error is pretty
odd. But this is staging. How about cleaning this
up. of_phy_register_fixed_link() returns an -errno which you should
return.

	Andrew
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 net] staging: octeon: repair "fixed-link" support
  2020-10-16 10:18 [PATCH v2 net] staging: octeon: repair "fixed-link" support Alexander A Sverdlin
  2020-10-16 10:18 ` [PATCH v2 net] staging: octeon: Drop on uncorrectable alignment or FCS error Alexander A Sverdlin
  2020-10-17 20:45 ` [PATCH v2 net] staging: octeon: repair "fixed-link" support Andrew Lunn
@ 2020-10-17 20:48 ` Andrew Lunn
  2 siblings, 0 replies; 8+ messages in thread
From: Andrew Lunn @ 2020-10-17 20:48 UTC (permalink / raw)
  To: Alexander A Sverdlin
  Cc: devel, Aaro Koskinen, Greg Kroah-Hartman, Ralf Baechle, netdev,
	David S. Miller

> --- a/drivers/staging/octeon/ethernet.c
> +++ b/drivers/staging/octeon/ethernet.c
> @@ -13,6 +13,7 @@
>  #include <linux/phy.h>
>  #include <linux/slab.h>
>  #include <linux/interrupt.h>
> +#include <linux/of_mdio.h>
>  #include <linux/of_net.h>
>  #include <linux/if_ether.h>
>  #include <linux/if_vlan.h>
> @@ -892,6 +893,14 @@ static int cvm_oct_probe(struct platform_device *pdev)
>  				break;
>  			}
>  
> +			if (priv->of_node && of_phy_is_fixed_link(priv->of_node)) {
> +				if (of_phy_register_fixed_link(priv->of_node)) {
> +					netdev_err(dev, "Failed to register fixed link for interface %d, port %d\n",
> +						   interface, priv->port);
> +					dev->netdev_ops = NULL;
> +				}
> +			}
> +
>  			if (!dev->netdev_ops) {
>  				free_netdev(dev);
>  			} else if (register_netdev(dev) < 0) {
> -- 
> 2.10.2

I would also expect a call to of_phy_deregister_fixed_link() somewhere
in the driver.

   Andrew
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 net] staging: octeon: Drop on uncorrectable alignment or FCS error
  2020-10-16 10:18 ` [PATCH v2 net] staging: octeon: Drop on uncorrectable alignment or FCS error Alexander A Sverdlin
  2020-10-16 11:17   ` Greg Kroah-Hartman
  2020-10-16 13:14   ` Greg Kroah-Hartman
@ 2020-10-17 21:02   ` Andrew Lunn
  2020-10-19  9:11     ` Alexander Sverdlin
  2 siblings, 1 reply; 8+ messages in thread
From: Andrew Lunn @ 2020-10-17 21:02 UTC (permalink / raw)
  To: Alexander A Sverdlin
  Cc: devel, Aaro Koskinen, Greg Kroah-Hartman, Ralf Baechle, netdev,
	David S. Miller

> diff --git a/drivers/staging/octeon/ethernet-rx.c b/drivers/staging/octeon/ethernet-rx.c
> index 2c16230..9ebd665 100644
> --- a/drivers/staging/octeon/ethernet-rx.c
> +++ b/drivers/staging/octeon/ethernet-rx.c
> @@ -69,15 +69,17 @@ static inline int cvm_oct_check_rcv_error(struct cvmx_wqe *work)
>  	else
>  		port = work->word1.cn38xx.ipprt;
>  
> -	if ((work->word2.snoip.err_code == 10) && (work->word1.len <= 64)) {
> +	if ((work->word2.snoip.err_code == 10) && (work->word1.len <= 64))

It would be nice to replace all these err_code magic numbers with #defines.

You should also replace 64 with ETH_ZLEN + ETH_FCS_LEN. I also wonder
if <= should be just < ?

>  		/*
>  		 * Ignore length errors on min size packets. Some
>  		 * equipment incorrectly pads packets to 64+4FCS
>  		 * instead of 60+4FCS.  Note these packets still get
>  		 * counted as frame errors.
>  		 */
> -	} else if (work->word2.snoip.err_code == 5 ||
> -		   work->word2.snoip.err_code == 7) {
> +		return 0;
> +
> +	if (work->word2.snoip.err_code == 5 ||
> +	    work->word2.snoip.err_code == 7) {
>  		/*
>  		 * We received a packet with either an alignment error
>  		 * or a FCS error. This may be signalling that we are
> @@ -108,7 +110,10 @@ static inline int cvm_oct_check_rcv_error(struct cvmx_wqe *work)
>  				/* Port received 0xd5 preamble */
>  				work->packet_ptr.s.addr += i + 1;
>  				work->word1.len -= i + 5;
> -			} else if ((*ptr & 0xf) == 0xd) {
> +				return 0;
> +			}
> +
> +			if ((*ptr & 0xf) == 0xd) {

The comments are not so clear what is going on here. Can this
incorrectly match a destination MAC address of xD:XX:XX:XX:XX:XX.

>  				/* Port received 0xd preamble */
>  				work->packet_ptr.s.addr += i;
>  				work->word1.len -= i + 4;

	Andrew
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 net] staging: octeon: Drop on uncorrectable alignment or FCS error
  2020-10-17 21:02   ` Andrew Lunn
@ 2020-10-19  9:11     ` Alexander Sverdlin
  0 siblings, 0 replies; 8+ messages in thread
From: Alexander Sverdlin @ 2020-10-19  9:11 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: devel, Aaro Koskinen, Greg Kroah-Hartman, Ralf Baechle, netdev,
	David S. Miller

Hello Andrew,

thank you for your review!

On 17/10/2020 23:02, Andrew Lunn wrote:
>> diff --git a/drivers/staging/octeon/ethernet-rx.c b/drivers/staging/octeon/ethernet-rx.c
>> index 2c16230..9ebd665 100644
>> --- a/drivers/staging/octeon/ethernet-rx.c
>> +++ b/drivers/staging/octeon/ethernet-rx.c
>> @@ -69,15 +69,17 @@ static inline int cvm_oct_check_rcv_error(struct cvmx_wqe *work)
>>  	else
>>  		port = work->word1.cn38xx.ipprt;
>>  
>> -	if ((work->word2.snoip.err_code == 10) && (work->word1.len <= 64)) {
>> +	if ((work->word2.snoip.err_code == 10) && (work->word1.len <= 64))
> It would be nice to replace all these err_code magic numbers with #defines.
> 
> You should also replace 64 with ETH_ZLEN + ETH_FCS_LEN. I also wonder
> if <= should be just < ?

I think all your comments are valid points, but are rather topics
for separate patches. In this one I've addressed one issue: the structure of
ifs and elses is so deeply nested, that it lead to one logic mistake:
broken packets which cannot be corrected are still not dropped.

Even my patch has two changes in one: error correction and style correction,
but I consider this justified because one was a result of another.

>>  		/*
>>  		 * Ignore length errors on min size packets. Some
>>  		 * equipment incorrectly pads packets to 64+4FCS
>>  		 * instead of 60+4FCS.  Note these packets still get
>>  		 * counted as frame errors.
>>  		 */
>> -	} else if (work->word2.snoip.err_code == 5 ||
>> -		   work->word2.snoip.err_code == 7) {
>> +		return 0;
>> +
>> +	if (work->word2.snoip.err_code == 5 ||
>> +	    work->word2.snoip.err_code == 7) {
>>  		/*
>>  		 * We received a packet with either an alignment error
>>  		 * or a FCS error. This may be signalling that we are
>> @@ -108,7 +110,10 @@ static inline int cvm_oct_check_rcv_error(struct cvmx_wqe *work)
>>  				/* Port received 0xd5 preamble */
>>  				work->packet_ptr.s.addr += i + 1;
>>  				work->word1.len -= i + 5;
>> -			} else if ((*ptr & 0xf) == 0xd) {
>> +				return 0;
>> +			}
>> +
>> +			if ((*ptr & 0xf) == 0xd) {
> The comments are not so clear what is going on here. Can this
> incorrectly match a destination MAC address of xD:XX:XX:XX:XX:XX.
> 
>>  				/* Port received 0xd preamble */
>>  				work->packet_ptr.s.addr += i;
>>  				work->word1.len -= i + 4;

-- 
Best regards,
Alexander Sverdlin.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-16 10:18 [PATCH v2 net] staging: octeon: repair "fixed-link" support Alexander A Sverdlin
2020-10-16 10:18 ` [PATCH v2 net] staging: octeon: Drop on uncorrectable alignment or FCS error Alexander A Sverdlin
2020-10-16 11:17   ` Greg Kroah-Hartman
2020-10-16 13:14   ` Greg Kroah-Hartman
2020-10-17 21:02   ` Andrew Lunn
2020-10-19  9:11     ` Alexander Sverdlin
2020-10-17 20:45 ` [PATCH v2 net] staging: octeon: repair "fixed-link" support Andrew Lunn
2020-10-17 20:48 ` Andrew Lunn

DriverDev-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/driverdev-devel/0 driverdev-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 driverdev-devel driverdev-devel/ https://lore.kernel.org/driverdev-devel \
		driverdev-devel@linuxdriverproject.org devel@driverdev.osuosl.org
	public-inbox-index driverdev-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linuxdriverproject.driverdev-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git