devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 0/5] net: vertexcom: mse102x: Improve RX handling
@ 2025-05-05 14:24 Stefan Wahren
  2025-05-05 14:24 ` [PATCH net-next 1/5] dt-bindings: vertexcom-mse102x: Fix IRQ type in example Stefan Wahren
                   ` (4 more replies)
  0 siblings, 5 replies; 20+ messages in thread
From: Stefan Wahren @ 2025-05-05 14:24 UTC (permalink / raw)
  To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley
  Cc: netdev, devicetree, Stefan Wahren

This series is the second part of two series for the Vertexcom driver.
It contains some improvements for the RX handling of the Vertexcom MSE102x.

Stefan Wahren (5):
  dt-bindings: vertexcom-mse102x: Fix IRQ type in example
  net: vertexcom: mse102x: Add warning about IRQ trigger type
  net: vertexcom: mse102x: drop invalid cmd stats
  net: vertexcom: mse102x: Return code for mse102x_rx_pkt_spi
  net: vertexcom: mse102x: Simplify mse102x_rx_pkt_spi

 .../bindings/net/vertexcom-mse102x.yaml       |  2 +-
 drivers/net/ethernet/vertexcom/mse102x.c      | 65 ++++++++++---------
 2 files changed, 36 insertions(+), 31 deletions(-)

-- 
2.34.1


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

* [PATCH net-next 1/5] dt-bindings: vertexcom-mse102x: Fix IRQ type in example
  2025-05-05 14:24 [PATCH net-next 0/5] net: vertexcom: mse102x: Improve RX handling Stefan Wahren
@ 2025-05-05 14:24 ` Stefan Wahren
  2025-05-05 16:31   ` Andrew Lunn
  2025-05-05 14:24 ` [PATCH net-next 2/5] net: vertexcom: mse102x: Add warning about IRQ trigger type Stefan Wahren
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 20+ messages in thread
From: Stefan Wahren @ 2025-05-05 14:24 UTC (permalink / raw)
  To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley
  Cc: netdev, devicetree, Stefan Wahren

According to the MSE102x documentation the trigger type is a
high level.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 Documentation/devicetree/bindings/net/vertexcom-mse102x.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/vertexcom-mse102x.yaml b/Documentation/devicetree/bindings/net/vertexcom-mse102x.yaml
index 4158673f723c..8359de7ad272 100644
--- a/Documentation/devicetree/bindings/net/vertexcom-mse102x.yaml
+++ b/Documentation/devicetree/bindings/net/vertexcom-mse102x.yaml
@@ -63,7 +63,7 @@ examples:
             compatible = "vertexcom,mse1021";
             reg = <0>;
             interrupt-parent = <&gpio>;
-            interrupts = <23 IRQ_TYPE_EDGE_RISING>;
+            interrupts = <23 IRQ_TYPE_LEVEL_HIGH>;
             spi-cpha;
             spi-cpol;
             spi-max-frequency = <7142857>;
-- 
2.34.1


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

* [PATCH net-next 2/5] net: vertexcom: mse102x: Add warning about IRQ trigger type
  2025-05-05 14:24 [PATCH net-next 0/5] net: vertexcom: mse102x: Improve RX handling Stefan Wahren
  2025-05-05 14:24 ` [PATCH net-next 1/5] dt-bindings: vertexcom-mse102x: Fix IRQ type in example Stefan Wahren
@ 2025-05-05 14:24 ` Stefan Wahren
  2025-05-05 16:32   ` Andrew Lunn
                     ` (3 more replies)
  2025-05-05 14:24 ` [PATCH net-next 3/5] net: vertexcom: mse102x: Drop invalid cmd stats Stefan Wahren
                   ` (2 subsequent siblings)
  4 siblings, 4 replies; 20+ messages in thread
From: Stefan Wahren @ 2025-05-05 14:24 UTC (permalink / raw)
  To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley
  Cc: netdev, devicetree, Stefan Wahren

The example of the initial DT binding of the Vertexcom MSE 102x suggested
a IRQ_TYPE_EDGE_RISING, which is wrong. So warn everyone to fix their
device tree to level based IRQ.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/net/ethernet/vertexcom/mse102x.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/net/ethernet/vertexcom/mse102x.c b/drivers/net/ethernet/vertexcom/mse102x.c
index e4d993f31374..33371438aa17 100644
--- a/drivers/net/ethernet/vertexcom/mse102x.c
+++ b/drivers/net/ethernet/vertexcom/mse102x.c
@@ -522,10 +522,25 @@ static irqreturn_t mse102x_irq(int irq, void *_mse)
 
 static int mse102x_net_open(struct net_device *ndev)
 {
+	struct irq_data *irq_data = irq_get_irq_data(ndev->irq);
 	struct mse102x_net *mse = netdev_priv(ndev);
 	struct mse102x_net_spi *mses = to_mse102x_spi(mse);
 	int ret;
 
+	if (!irq_data) {
+		netdev_err(ndev, "Invalid IRQ: %d\n", ndev->irq);
+		return -EINVAL;
+	}
+
+	switch (irqd_get_trigger_type(irq_data)) {
+	case IRQ_TYPE_LEVEL_HIGH:
+	case IRQ_TYPE_LEVEL_LOW:
+		break;
+	default:
+		netdev_warn_once(ndev, "Only IRQ type level recommended, please update your firmware.\n");
+		break;
+	}
+
 	ret = request_threaded_irq(ndev->irq, NULL, mse102x_irq, IRQF_ONESHOT,
 				   ndev->name, mse);
 	if (ret < 0) {
-- 
2.34.1


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

* [PATCH net-next 3/5] net: vertexcom: mse102x: Drop invalid cmd stats
  2025-05-05 14:24 [PATCH net-next 0/5] net: vertexcom: mse102x: Improve RX handling Stefan Wahren
  2025-05-05 14:24 ` [PATCH net-next 1/5] dt-bindings: vertexcom-mse102x: Fix IRQ type in example Stefan Wahren
  2025-05-05 14:24 ` [PATCH net-next 2/5] net: vertexcom: mse102x: Add warning about IRQ trigger type Stefan Wahren
@ 2025-05-05 14:24 ` Stefan Wahren
  2025-05-05 16:35   ` Andrew Lunn
  2025-05-05 14:24 ` [PATCH net-next 4/5] net: vertexcom: mse102x: Return code for mse102x_rx_pkt_spi Stefan Wahren
  2025-05-05 14:24 ` [PATCH net-next 5/5] net: vertexcom: mse102x: Simplify mse102x_rx_pkt_spi Stefan Wahren
  4 siblings, 1 reply; 20+ messages in thread
From: Stefan Wahren @ 2025-05-05 14:24 UTC (permalink / raw)
  To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley
  Cc: netdev, devicetree, Stefan Wahren

The SPI implementation on the MSE102x MCU is in software, as a result
it cannot reply to SPI commands in busy state and increase the invalid
command counter. So drop the confusing statistics about "invalid" command
replies.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/net/ethernet/vertexcom/mse102x.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/ethernet/vertexcom/mse102x.c b/drivers/net/ethernet/vertexcom/mse102x.c
index 33371438aa17..204ce8bdbaf8 100644
--- a/drivers/net/ethernet/vertexcom/mse102x.c
+++ b/drivers/net/ethernet/vertexcom/mse102x.c
@@ -45,7 +45,6 @@
 
 struct mse102x_stats {
 	u64 xfer_err;
-	u64 invalid_cmd;
 	u64 invalid_ctr;
 	u64 invalid_dft;
 	u64 invalid_len;
@@ -56,7 +55,6 @@ struct mse102x_stats {
 
 static const char mse102x_gstrings_stats[][ETH_GSTRING_LEN] = {
 	"SPI transfer errors",
-	"Invalid command",
 	"Invalid CTR",
 	"Invalid DFT",
 	"Invalid frame length",
@@ -194,7 +192,6 @@ static int mse102x_rx_cmd_spi(struct mse102x_net *mse, u8 *rxb)
 	} else if (*cmd != cpu_to_be16(DET_CMD)) {
 		net_dbg_ratelimited("%s: Unexpected response (0x%04x)\n",
 				    __func__, *cmd);
-		mse->stats.invalid_cmd++;
 		ret = -EIO;
 	} else {
 		memcpy(rxb, trx + 2, 2);
-- 
2.34.1


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

* [PATCH net-next 4/5] net: vertexcom: mse102x: Return code for mse102x_rx_pkt_spi
  2025-05-05 14:24 [PATCH net-next 0/5] net: vertexcom: mse102x: Improve RX handling Stefan Wahren
                   ` (2 preceding siblings ...)
  2025-05-05 14:24 ` [PATCH net-next 3/5] net: vertexcom: mse102x: Drop invalid cmd stats Stefan Wahren
@ 2025-05-05 14:24 ` Stefan Wahren
  2025-05-05 16:43   ` Andrew Lunn
  2025-05-05 14:24 ` [PATCH net-next 5/5] net: vertexcom: mse102x: Simplify mse102x_rx_pkt_spi Stefan Wahren
  4 siblings, 1 reply; 20+ messages in thread
From: Stefan Wahren @ 2025-05-05 14:24 UTC (permalink / raw)
  To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley
  Cc: netdev, devicetree, Stefan Wahren

The interrupt handler mse102x_irq always returns IRQ_HANDLED even
in case the SPI interrupt is not handled. In order to solve this,
let mse102x_rx_pkt_spi return the proper return code.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/net/ethernet/vertexcom/mse102x.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/vertexcom/mse102x.c b/drivers/net/ethernet/vertexcom/mse102x.c
index 204ce8bdbaf8..aeef144d0051 100644
--- a/drivers/net/ethernet/vertexcom/mse102x.c
+++ b/drivers/net/ethernet/vertexcom/mse102x.c
@@ -303,7 +303,7 @@ static void mse102x_dump_packet(const char *msg, int len, const char *data)
 		       data, len, true);
 }
 
-static void mse102x_rx_pkt_spi(struct mse102x_net *mse)
+static irqreturn_t mse102x_rx_pkt_spi(struct mse102x_net *mse)
 {
 	struct sk_buff *skb;
 	unsigned int rxalign;
@@ -324,7 +324,7 @@ static void mse102x_rx_pkt_spi(struct mse102x_net *mse)
 		mse102x_tx_cmd_spi(mse, CMD_CTR);
 		ret = mse102x_rx_cmd_spi(mse, (u8 *)&rx);
 		if (ret)
-			return;
+			return IRQ_NONE;
 
 		cmd_resp = be16_to_cpu(rx);
 		if ((cmd_resp & CMD_MASK) != CMD_RTS) {
@@ -357,7 +357,7 @@ static void mse102x_rx_pkt_spi(struct mse102x_net *mse)
 	rxalign = ALIGN(rxlen + DET_SOF_LEN + DET_DFT_LEN, 4);
 	skb = netdev_alloc_skb_ip_align(mse->ndev, rxalign);
 	if (!skb)
-		return;
+		return IRQ_NONE;
 
 	/* 2 bytes Start of frame (before ethernet header)
 	 * 2 bytes Data frame tail (after ethernet frame)
@@ -367,7 +367,7 @@ static void mse102x_rx_pkt_spi(struct mse102x_net *mse)
 	if (mse102x_rx_frame_spi(mse, rxpkt, rxlen, drop)) {
 		mse->ndev->stats.rx_errors++;
 		dev_kfree_skb(skb);
-		return;
+		return IRQ_HANDLED;
 	}
 
 	if (netif_msg_pktdata(mse))
@@ -378,6 +378,8 @@ static void mse102x_rx_pkt_spi(struct mse102x_net *mse)
 
 	mse->ndev->stats.rx_packets++;
 	mse->ndev->stats.rx_bytes += rxlen;
+
+	return IRQ_HANDLED;
 }
 
 static int mse102x_tx_pkt_spi(struct mse102x_net *mse, struct sk_buff *txb,
@@ -509,12 +511,13 @@ static irqreturn_t mse102x_irq(int irq, void *_mse)
 {
 	struct mse102x_net *mse = _mse;
 	struct mse102x_net_spi *mses = to_mse102x_spi(mse);
+	irqreturn_t ret;
 
 	mutex_lock(&mses->lock);
-	mse102x_rx_pkt_spi(mse);
+	ret = mse102x_rx_pkt_spi(mse);
 	mutex_unlock(&mses->lock);
 
-	return IRQ_HANDLED;
+	return ret;
 }
 
 static int mse102x_net_open(struct net_device *ndev)
-- 
2.34.1


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

* [PATCH net-next 5/5] net: vertexcom: mse102x: Simplify mse102x_rx_pkt_spi
  2025-05-05 14:24 [PATCH net-next 0/5] net: vertexcom: mse102x: Improve RX handling Stefan Wahren
                   ` (3 preceding siblings ...)
  2025-05-05 14:24 ` [PATCH net-next 4/5] net: vertexcom: mse102x: Return code for mse102x_rx_pkt_spi Stefan Wahren
@ 2025-05-05 14:24 ` Stefan Wahren
  4 siblings, 0 replies; 20+ messages in thread
From: Stefan Wahren @ 2025-05-05 14:24 UTC (permalink / raw)
  To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley
  Cc: netdev, devicetree, Stefan Wahren

The function mse102x_rx_pkt_spi is used in two cases:
* initial polling to re-arm interrupt
* level based interrupt handler

Both of them doesn't need an open-coded retry mechanism.
In the first case the function can be called again, if the return code
is IRQ_NONE. This keeps the error behavior during netdev open.

In the second case the proper retry would be handled implicit by
the SPI interrupt. So drop the retry code and simplify the receive path.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/net/ethernet/vertexcom/mse102x.c | 34 +++++++++---------------
 1 file changed, 12 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/vertexcom/mse102x.c b/drivers/net/ethernet/vertexcom/mse102x.c
index aeef144d0051..a474ceb77ece 100644
--- a/drivers/net/ethernet/vertexcom/mse102x.c
+++ b/drivers/net/ethernet/vertexcom/mse102x.c
@@ -312,31 +312,20 @@ static irqreturn_t mse102x_rx_pkt_spi(struct mse102x_net *mse)
 	__be16 rx = 0;
 	u16 cmd_resp;
 	u8 *rxpkt;
-	int ret;
 
 	mse102x_tx_cmd_spi(mse, CMD_CTR);
-	ret = mse102x_rx_cmd_spi(mse, (u8 *)&rx);
-	cmd_resp = be16_to_cpu(rx);
-
-	if (ret || ((cmd_resp & CMD_MASK) != CMD_RTS)) {
+	if (mse102x_rx_cmd_spi(mse, (u8 *)&rx)) {
 		usleep_range(50, 100);
+		return IRQ_NONE;
+	}
 
-		mse102x_tx_cmd_spi(mse, CMD_CTR);
-		ret = mse102x_rx_cmd_spi(mse, (u8 *)&rx);
-		if (ret)
-			return IRQ_NONE;
-
-		cmd_resp = be16_to_cpu(rx);
-		if ((cmd_resp & CMD_MASK) != CMD_RTS) {
-			net_dbg_ratelimited("%s: Unexpected response (0x%04x)\n",
-					    __func__, cmd_resp);
-			mse->stats.invalid_rts++;
-			drop = true;
-			goto drop;
-		}
-
-		net_dbg_ratelimited("%s: Unexpected response to first CMD\n",
-				    __func__);
+	cmd_resp = be16_to_cpu(rx);
+	if ((cmd_resp & CMD_MASK) != CMD_RTS) {
+		net_dbg_ratelimited("%s: Unexpected response (0x%04x)\n",
+				    __func__, cmd_resp);
+		mse->stats.invalid_rts++;
+		drop = true;
+		goto drop;
 	}
 
 	rxlen = cmd_resp & LEN_MASK;
@@ -558,7 +547,8 @@ static int mse102x_net_open(struct net_device *ndev)
 	 * So poll for possible packet(s) to re-arm the interrupt.
 	 */
 	mutex_lock(&mses->lock);
-	mse102x_rx_pkt_spi(mse);
+	if (mse102x_rx_pkt_spi(mse) == IRQ_NONE)
+		mse102x_rx_pkt_spi(mse);
 	mutex_unlock(&mses->lock);
 
 	netif_dbg(mse, ifup, ndev, "network device up\n");
-- 
2.34.1


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

* Re: [PATCH net-next 1/5] dt-bindings: vertexcom-mse102x: Fix IRQ type in example
  2025-05-05 14:24 ` [PATCH net-next 1/5] dt-bindings: vertexcom-mse102x: Fix IRQ type in example Stefan Wahren
@ 2025-05-05 16:31   ` Andrew Lunn
  0 siblings, 0 replies; 20+ messages in thread
From: Andrew Lunn @ 2025-05-05 16:31 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	netdev, devicetree

On Mon, May 05, 2025 at 04:24:23PM +0200, Stefan Wahren wrote:
> According to the MSE102x documentation the trigger type is a
> high level.
> 
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> ---
>  Documentation/devicetree/bindings/net/vertexcom-mse102x.yaml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/vertexcom-mse102x.yaml b/Documentation/devicetree/bindings/net/vertexcom-mse102x.yaml
> index 4158673f723c..8359de7ad272 100644
> --- a/Documentation/devicetree/bindings/net/vertexcom-mse102x.yaml
> +++ b/Documentation/devicetree/bindings/net/vertexcom-mse102x.yaml
> @@ -63,7 +63,7 @@ examples:
>              compatible = "vertexcom,mse1021";
>              reg = <0>;
>              interrupt-parent = <&gpio>;
> -            interrupts = <23 IRQ_TYPE_EDGE_RISING>;
> +            interrupts = <23 IRQ_TYPE_LEVEL_HIGH>;

This is a common problem with anything connected over some sort of
serial bus, I2C, SPI, MDIO.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

* Re: [PATCH net-next 2/5] net: vertexcom: mse102x: Add warning about IRQ trigger type
  2025-05-05 14:24 ` [PATCH net-next 2/5] net: vertexcom: mse102x: Add warning about IRQ trigger type Stefan Wahren
@ 2025-05-05 16:32   ` Andrew Lunn
  2025-05-06  8:38     ` Stefan Wahren
  2025-05-05 20:57   ` Jakub Kicinski
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 20+ messages in thread
From: Andrew Lunn @ 2025-05-05 16:32 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	netdev, devicetree

> +	if (!irq_data) {
> +		netdev_err(ndev, "Invalid IRQ: %d\n", ndev->irq);
> +		return -EINVAL;
> +	}
> +
> +	switch (irqd_get_trigger_type(irq_data)) {
> +	case IRQ_TYPE_LEVEL_HIGH:
> +	case IRQ_TYPE_LEVEL_LOW:
> +		break;
> +	default:
> +		netdev_warn_once(ndev, "Only IRQ type level recommended, please update your firmware.\n");

I would probably put DT in there somewhere. firmware is rather
generic.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

* Re: [PATCH net-next 3/5] net: vertexcom: mse102x: Drop invalid cmd stats
  2025-05-05 14:24 ` [PATCH net-next 3/5] net: vertexcom: mse102x: Drop invalid cmd stats Stefan Wahren
@ 2025-05-05 16:35   ` Andrew Lunn
  2025-05-05 17:28     ` Stefan Wahren
  0 siblings, 1 reply; 20+ messages in thread
From: Andrew Lunn @ 2025-05-05 16:35 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	netdev, devicetree

On Mon, May 05, 2025 at 04:24:25PM +0200, Stefan Wahren wrote:
> The SPI implementation on the MSE102x MCU is in software, as a result
> it cannot reply to SPI commands in busy state and increase the invalid
> command counter. So drop the confusing statistics about "invalid" command
> replies.
> 
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> ---
>  drivers/net/ethernet/vertexcom/mse102x.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/vertexcom/mse102x.c b/drivers/net/ethernet/vertexcom/mse102x.c
> index 33371438aa17..204ce8bdbaf8 100644
> --- a/drivers/net/ethernet/vertexcom/mse102x.c
> +++ b/drivers/net/ethernet/vertexcom/mse102x.c
> @@ -45,7 +45,6 @@
>  
>  struct mse102x_stats {
>  	u64 xfer_err;
> -	u64 invalid_cmd;
>  	u64 invalid_ctr;
>  	u64 invalid_dft;
>  	u64 invalid_len;
> @@ -56,7 +55,6 @@ struct mse102x_stats {
>  
>  static const char mse102x_gstrings_stats[][ETH_GSTRING_LEN] = {
>  	"SPI transfer errors",
> -	"Invalid command",
>  	"Invalid CTR",
>  	"Invalid DFT",
>  	"Invalid frame length",
> @@ -194,7 +192,6 @@ static int mse102x_rx_cmd_spi(struct mse102x_net *mse, u8 *rxb)
>  	} else if (*cmd != cpu_to_be16(DET_CMD)) {
>  		net_dbg_ratelimited("%s: Unexpected response (0x%04x)\n",
>  				    __func__, *cmd);
> -		mse->stats.invalid_cmd++;

If the net_dbg_ratelimited() is going to stay, maybe rename the
counter to unexpct_rsp, or similar?

	Andrew

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

* Re: [PATCH net-next 4/5] net: vertexcom: mse102x: Return code for mse102x_rx_pkt_spi
  2025-05-05 14:24 ` [PATCH net-next 4/5] net: vertexcom: mse102x: Return code for mse102x_rx_pkt_spi Stefan Wahren
@ 2025-05-05 16:43   ` Andrew Lunn
  2025-05-05 17:16     ` Stefan Wahren
  0 siblings, 1 reply; 20+ messages in thread
From: Andrew Lunn @ 2025-05-05 16:43 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	netdev, devicetree

On Mon, May 05, 2025 at 04:24:26PM +0200, Stefan Wahren wrote:
> The interrupt handler mse102x_irq always returns IRQ_HANDLED even
> in case the SPI interrupt is not handled. In order to solve this,
> let mse102x_rx_pkt_spi return the proper return code.
> 
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> ---
>  drivers/net/ethernet/vertexcom/mse102x.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/vertexcom/mse102x.c b/drivers/net/ethernet/vertexcom/mse102x.c
> index 204ce8bdbaf8..aeef144d0051 100644
> --- a/drivers/net/ethernet/vertexcom/mse102x.c
> +++ b/drivers/net/ethernet/vertexcom/mse102x.c
> @@ -303,7 +303,7 @@ static void mse102x_dump_packet(const char *msg, int len, const char *data)
>  		       data, len, true);
>  }
>  
> -static void mse102x_rx_pkt_spi(struct mse102x_net *mse)
> +static irqreturn_t mse102x_rx_pkt_spi(struct mse102x_net *mse)
>  {
>  	struct sk_buff *skb;
>  	unsigned int rxalign;
> @@ -324,7 +324,7 @@ static void mse102x_rx_pkt_spi(struct mse102x_net *mse)
>  		mse102x_tx_cmd_spi(mse, CMD_CTR);
>  		ret = mse102x_rx_cmd_spi(mse, (u8 *)&rx);
>  		if (ret)
> -			return;
> +			return IRQ_NONE;
>  
>  		cmd_resp = be16_to_cpu(rx);
>  		if ((cmd_resp & CMD_MASK) != CMD_RTS) {
> @@ -357,7 +357,7 @@ static void mse102x_rx_pkt_spi(struct mse102x_net *mse)
>  	rxalign = ALIGN(rxlen + DET_SOF_LEN + DET_DFT_LEN, 4);
>  	skb = netdev_alloc_skb_ip_align(mse->ndev, rxalign);
>  	if (!skb)
> -		return;
> +		return IRQ_NONE;

This is not my understanding of IRQ_NONE. To me, IRQ_NONE means the
driver has read the interrupt status register and determined that this
device did not generate the interrupt. It is probably some other
device which is sharing the interrupt.

       Andrew

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

* Re: [PATCH net-next 4/5] net: vertexcom: mse102x: Return code for mse102x_rx_pkt_spi
  2025-05-05 16:43   ` Andrew Lunn
@ 2025-05-05 17:16     ` Stefan Wahren
  2025-05-05 19:27       ` Andrew Lunn
  0 siblings, 1 reply; 20+ messages in thread
From: Stefan Wahren @ 2025-05-05 17:16 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	netdev, devicetree

Hi Andrew,

Am 05.05.25 um 18:43 schrieb Andrew Lunn:
> On Mon, May 05, 2025 at 04:24:26PM +0200, Stefan Wahren wrote:
>> The interrupt handler mse102x_irq always returns IRQ_HANDLED even
>> in case the SPI interrupt is not handled. In order to solve this,
>> let mse102x_rx_pkt_spi return the proper return code.
>>
>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
>> ---
>>   drivers/net/ethernet/vertexcom/mse102x.c | 15 +++++++++------
>>   1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/vertexcom/mse102x.c b/drivers/net/ethernet/vertexcom/mse102x.c
>> index 204ce8bdbaf8..aeef144d0051 100644
>> --- a/drivers/net/ethernet/vertexcom/mse102x.c
>> +++ b/drivers/net/ethernet/vertexcom/mse102x.c
>> @@ -303,7 +303,7 @@ static void mse102x_dump_packet(const char *msg, int len, const char *data)
>>   		       data, len, true);
>>   }
>>   
>> -static void mse102x_rx_pkt_spi(struct mse102x_net *mse)
>> +static irqreturn_t mse102x_rx_pkt_spi(struct mse102x_net *mse)
>>   {
>>   	struct sk_buff *skb;
>>   	unsigned int rxalign;
>> @@ -324,7 +324,7 @@ static void mse102x_rx_pkt_spi(struct mse102x_net *mse)
>>   		mse102x_tx_cmd_spi(mse, CMD_CTR);
>>   		ret = mse102x_rx_cmd_spi(mse, (u8 *)&rx);
>>   		if (ret)
>> -			return;
>> +			return IRQ_NONE;
>>   
>>   		cmd_resp = be16_to_cpu(rx);
>>   		if ((cmd_resp & CMD_MASK) != CMD_RTS) {
>> @@ -357,7 +357,7 @@ static void mse102x_rx_pkt_spi(struct mse102x_net *mse)
>>   	rxalign = ALIGN(rxlen + DET_SOF_LEN + DET_DFT_LEN, 4);
>>   	skb = netdev_alloc_skb_ip_align(mse->ndev, rxalign);
>>   	if (!skb)
>> -		return;
>> +		return IRQ_NONE;
> This is not my understanding of IRQ_NONE. To me, IRQ_NONE means the
> driver has read the interrupt status register and determined that this
> device did not generate the interrupt. It is probably some other
> device which is sharing the interrupt.
At first i wrote this patch for the not-shared interrupt use case in 
mind. Unfortunately this device doesn't have a interrupt status register 
and in the above cases the interrupt is not handled.

kernel-doc says:

@IRQ_NONE:        interrupt was not from this device or was not handled
@IRQ_HANDLED:    interrupt was handled by this device

So from my understanding IRQ_NONE fits better here (assuming a 
not-shared interrupt).

I think driver should only use not-shared interrupts, because there is 
no interrupt status register. Am I right?
May this SPI client protocol limitation should be documented in the 
dt-binding?

>
>         Andrew


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

* Re: [PATCH net-next 3/5] net: vertexcom: mse102x: Drop invalid cmd stats
  2025-05-05 16:35   ` Andrew Lunn
@ 2025-05-05 17:28     ` Stefan Wahren
  0 siblings, 0 replies; 20+ messages in thread
From: Stefan Wahren @ 2025-05-05 17:28 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	netdev, devicetree

Am 05.05.25 um 18:35 schrieb Andrew Lunn:
> On Mon, May 05, 2025 at 04:24:25PM +0200, Stefan Wahren wrote:
>> The SPI implementation on the MSE102x MCU is in software, as a result
>> it cannot reply to SPI commands in busy state and increase the invalid
>> command counter. So drop the confusing statistics about "invalid" command
>> replies.
>>
>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
>> ---
>>   drivers/net/ethernet/vertexcom/mse102x.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/vertexcom/mse102x.c b/drivers/net/ethernet/vertexcom/mse102x.c
>> index 33371438aa17..204ce8bdbaf8 100644
>> --- a/drivers/net/ethernet/vertexcom/mse102x.c
>> +++ b/drivers/net/ethernet/vertexcom/mse102x.c
>> @@ -45,7 +45,6 @@
>>   
>>   struct mse102x_stats {
>>   	u64 xfer_err;
>> -	u64 invalid_cmd;
>>   	u64 invalid_ctr;
>>   	u64 invalid_dft;
>>   	u64 invalid_len;
>> @@ -56,7 +55,6 @@ struct mse102x_stats {
>>   
>>   static const char mse102x_gstrings_stats[][ETH_GSTRING_LEN] = {
>>   	"SPI transfer errors",
>> -	"Invalid command",
>>   	"Invalid CTR",
>>   	"Invalid DFT",
>>   	"Invalid frame length",
>> @@ -194,7 +192,6 @@ static int mse102x_rx_cmd_spi(struct mse102x_net *mse, u8 *rxb)
>>   	} else if (*cmd != cpu_to_be16(DET_CMD)) {
>>   		net_dbg_ratelimited("%s: Unexpected response (0x%04x)\n",
>>   				    __func__, *cmd);
>> -		mse->stats.invalid_cmd++;
> If the net_dbg_ratelimited() is going to stay, maybe rename the
> counter to unexpct_rsp, or similar?
The problem here is that there are many reasons for the unexpected response:

  * line interferences
  * MSE102x has no firmware installed
  * MSE102x busy
  * no packet in MSE102x receive buffer

So without further context like the actual response this counter isn't 
very helpful and more likely to scare users. But I would like keep the 
debug message for hardware / wiring issues.

Regards

>
> 	Andrew


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

* Re: [PATCH net-next 4/5] net: vertexcom: mse102x: Return code for mse102x_rx_pkt_spi
  2025-05-05 17:16     ` Stefan Wahren
@ 2025-05-05 19:27       ` Andrew Lunn
  2025-05-06  8:24         ` Stefan Wahren
  0 siblings, 1 reply; 20+ messages in thread
From: Andrew Lunn @ 2025-05-05 19:27 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	netdev, devicetree

On Mon, May 05, 2025 at 07:16:51PM +0200, Stefan Wahren wrote:
> Hi Andrew,
> 
> Am 05.05.25 um 18:43 schrieb Andrew Lunn:
> > On Mon, May 05, 2025 at 04:24:26PM +0200, Stefan Wahren wrote:
> > > The interrupt handler mse102x_irq always returns IRQ_HANDLED even
> > > in case the SPI interrupt is not handled. In order to solve this,
> > > let mse102x_rx_pkt_spi return the proper return code.
> > > 
> > > Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> > > ---
> > >   drivers/net/ethernet/vertexcom/mse102x.c | 15 +++++++++------
> > >   1 file changed, 9 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/vertexcom/mse102x.c b/drivers/net/ethernet/vertexcom/mse102x.c
> > > index 204ce8bdbaf8..aeef144d0051 100644
> > > --- a/drivers/net/ethernet/vertexcom/mse102x.c
> > > +++ b/drivers/net/ethernet/vertexcom/mse102x.c
> > > @@ -303,7 +303,7 @@ static void mse102x_dump_packet(const char *msg, int len, const char *data)
> > >   		       data, len, true);
> > >   }
> > > -static void mse102x_rx_pkt_spi(struct mse102x_net *mse)
> > > +static irqreturn_t mse102x_rx_pkt_spi(struct mse102x_net *mse)
> > >   {
> > >   	struct sk_buff *skb;
> > >   	unsigned int rxalign;
> > > @@ -324,7 +324,7 @@ static void mse102x_rx_pkt_spi(struct mse102x_net *mse)
> > >   		mse102x_tx_cmd_spi(mse, CMD_CTR);
> > >   		ret = mse102x_rx_cmd_spi(mse, (u8 *)&rx);
> > >   		if (ret)
> > > -			return;
> > > +			return IRQ_NONE;
> > >   		cmd_resp = be16_to_cpu(rx);
> > >   		if ((cmd_resp & CMD_MASK) != CMD_RTS) {
> > > @@ -357,7 +357,7 @@ static void mse102x_rx_pkt_spi(struct mse102x_net *mse)
> > >   	rxalign = ALIGN(rxlen + DET_SOF_LEN + DET_DFT_LEN, 4);
> > >   	skb = netdev_alloc_skb_ip_align(mse->ndev, rxalign);
> > >   	if (!skb)
> > > -		return;
> > > +		return IRQ_NONE;
> > This is not my understanding of IRQ_NONE. To me, IRQ_NONE means the
> > driver has read the interrupt status register and determined that this
> > device did not generate the interrupt. It is probably some other
> > device which is sharing the interrupt.
> At first i wrote this patch for the not-shared interrupt use case in mind.
> Unfortunately this device doesn't have a interrupt status register and in
> the above cases the interrupt is not handled.
> 
> kernel-doc says:
> 
> @IRQ_NONE:        interrupt was not from this device or was not handled
> @IRQ_HANDLED:    interrupt was handled by this device

A memory allocation failure in netdev_alloc_skb_ip_align() does not
seem like a reason to return IRQ_NONE. I think the more normal case
is, there was an interrupt, there was an attempt to handle it, but the
handler failed. The driver should try to put the hardware into a state
the next interrupt will actually happen, and be serviced.

This is particularly important with level interrupts. If you fail to
clear the interrupt, it is going to fire again immediately after
exiting the interrupt handler and the interrupt is reenabled. You
don't want to die in an interrupt storm. Preventing such interrupt
storms is part of what the return value is used for. If the handler
continually returns IRQ_NONE, after a while the core declares there is
nobody actually interested in the interrupt, and it leaves it
disabled.

> So from my understanding IRQ_NONE fits better here (assuming a not-shared
> interrupt).
> 
> I think driver should only use not-shared interrupts, because there is no
> interrupt status register. Am I right?

I don't see why it cannot be shared. It is not very efficient, and
there will probable be a bias towards the first device which requests
the interrupt, but a shared interrupt should work.

	Andrew

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

* Re: [PATCH net-next 2/5] net: vertexcom: mse102x: Add warning about IRQ trigger type
  2025-05-05 14:24 ` [PATCH net-next 2/5] net: vertexcom: mse102x: Add warning about IRQ trigger type Stefan Wahren
  2025-05-05 16:32   ` Andrew Lunn
@ 2025-05-05 20:57   ` Jakub Kicinski
  2025-05-07 10:25   ` kernel test robot
  2025-05-08  9:00   ` kernel test robot
  3 siblings, 0 replies; 20+ messages in thread
From: Jakub Kicinski @ 2025-05-05 20:57 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, netdev,
	devicetree

On Mon,  5 May 2025 16:24:24 +0200 Stefan Wahren wrote:
> The example of the initial DT binding of the Vertexcom MSE 102x suggested
> a IRQ_TYPE_EDGE_RISING, which is wrong. So warn everyone to fix their
> device tree to level based IRQ.

Doesn't build on x86:

drivers/net/ethernet/vertexcom/mse102x.c:535:10: note: did you mean 'led_get_trigger_data'?
include/linux/leds.h:544:21: note: 'led_get_trigger_data' declared here
  544 | static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
      |                     ^
drivers/net/ethernet/vertexcom/mse102x.c:536:7: error: use of undeclared identifier 'IRQ_TYPE_LEVEL_HIGH'
  536 |         case IRQ_TYPE_LEVEL_HIGH:
      |              ^
drivers/net/ethernet/vertexcom/mse102x.c:537:7: error: use of undeclared identifier 'IRQ_TYPE_LEVEL_LOW'
  537 |         case IRQ_TYPE_LEVEL_LOW:
      |              ^
-- 
pw-bot: cr

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

* Re: [PATCH net-next 4/5] net: vertexcom: mse102x: Return code for mse102x_rx_pkt_spi
  2025-05-05 19:27       ` Andrew Lunn
@ 2025-05-06  8:24         ` Stefan Wahren
  0 siblings, 0 replies; 20+ messages in thread
From: Stefan Wahren @ 2025-05-06  8:24 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	netdev, devicetree

Hi Andrew,

Am 05.05.25 um 21:27 schrieb Andrew Lunn:
> On Mon, May 05, 2025 at 07:16:51PM +0200, Stefan Wahren wrote:
>> Hi Andrew,
>>
>> Am 05.05.25 um 18:43 schrieb Andrew Lunn:
>>> On Mon, May 05, 2025 at 04:24:26PM +0200, Stefan Wahren wrote:
>>>> The interrupt handler mse102x_irq always returns IRQ_HANDLED even
>>>> in case the SPI interrupt is not handled. In order to solve this,
>>>> let mse102x_rx_pkt_spi return the proper return code.
>>>>
>>>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
>>>> ---
>>>>    drivers/net/ethernet/vertexcom/mse102x.c | 15 +++++++++------
>>>>    1 file changed, 9 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/vertexcom/mse102x.c b/drivers/net/ethernet/vertexcom/mse102x.c
>>>> index 204ce8bdbaf8..aeef144d0051 100644
>>>> --- a/drivers/net/ethernet/vertexcom/mse102x.c
>>>> +++ b/drivers/net/ethernet/vertexcom/mse102x.c
>>>> @@ -303,7 +303,7 @@ static void mse102x_dump_packet(const char *msg, int len, const char *data)
>>>>    		       data, len, true);
>>>>    }
>>>> -static void mse102x_rx_pkt_spi(struct mse102x_net *mse)
>>>> +static irqreturn_t mse102x_rx_pkt_spi(struct mse102x_net *mse)
>>>>    {
>>>>    	struct sk_buff *skb;
>>>>    	unsigned int rxalign;
>>>> @@ -324,7 +324,7 @@ static void mse102x_rx_pkt_spi(struct mse102x_net *mse)
>>>>    		mse102x_tx_cmd_spi(mse, CMD_CTR);
>>>>    		ret = mse102x_rx_cmd_spi(mse, (u8 *)&rx);
>>>>    		if (ret)
>>>> -			return;
>>>> +			return IRQ_NONE;
>>>>    		cmd_resp = be16_to_cpu(rx);
>>>>    		if ((cmd_resp & CMD_MASK) != CMD_RTS) {
>>>> @@ -357,7 +357,7 @@ static void mse102x_rx_pkt_spi(struct mse102x_net *mse)
>>>>    	rxalign = ALIGN(rxlen + DET_SOF_LEN + DET_DFT_LEN, 4);
>>>>    	skb = netdev_alloc_skb_ip_align(mse->ndev, rxalign);
>>>>    	if (!skb)
>>>> -		return;
>>>> +		return IRQ_NONE;
>>> This is not my understanding of IRQ_NONE. To me, IRQ_NONE means the
>>> driver has read the interrupt status register and determined that this
>>> device did not generate the interrupt. It is probably some other
>>> device which is sharing the interrupt.
>> At first i wrote this patch for the not-shared interrupt use case in mind.
>> Unfortunately this device doesn't have a interrupt status register and in
>> the above cases the interrupt is not handled.
>>
>> kernel-doc says:
>>
>> @IRQ_NONE:        interrupt was not from this device or was not handled
>> @IRQ_HANDLED:    interrupt was handled by this device
> A memory allocation failure in netdev_alloc_skb_ip_align() does not
> seem like a reason to return IRQ_NONE. I think the more normal case
> is, there was an interrupt, there was an attempt to handle it, but the
> handler failed.
In my opinion this applies to both IRQ_NONE cases. The problem here is 
that there are no interrupt register provided by the MSE102x, so the 
only way to handle the SPI interrupt is to fetch the whole packet from 
the MSE102x internal buffer via SPI. But this requires a receive buffer 
which is big enough on the host side, so we use 
netdev_alloc_skb_ip_align() here to allocate this (SPI) receive buffer. 
The only way to avoid the second error case would be to preallocate a 
buffer which is big enough for the largest possible Ethernet frame.
>   The driver should try to put the hardware into a state
> the next interrupt will actually happen, and be serviced.
>
> This is particularly important with level interrupts. If you fail to
> clear the interrupt, it is going to fire again immediately after
> exiting the interrupt handler and the interrupt is reenabled. You
> don't want to die in an interrupt storm. Preventing such interrupt
> storms is part of what the return value is used for. If the handler
> continually returns IRQ_NONE, after a while the core declares there is
> nobody actually interested in the interrupt, and it leaves it
> disabled.
Yes, this was the intention of this patch. It's better to disable the 
IRQ completely because there is something wrong with the hardware 
instead of triggering a interrupt storm. So this is an argument to 
return IRQ_NONE and not IRQ_HANDLED.

I was dealing with interrupt storms on Raspberry Pi caused by dwc2 and 
it's a PITA.
>
>> So from my understanding IRQ_NONE fits better here (assuming a not-shared
>> interrupt).
>>
>> I think driver should only use not-shared interrupts, because there is no
>> interrupt status register. Am I right?
> I don't see why it cannot be shared. It is not very efficient, and
> there will probable be a bias towards the first device which requests
> the interrupt, but a shared interrupt should work.
I agree in general, but i don't recommend it.

Regards
>
> 	Andrew


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

* Re: [PATCH net-next 2/5] net: vertexcom: mse102x: Add warning about IRQ trigger type
  2025-05-05 16:32   ` Andrew Lunn
@ 2025-05-06  8:38     ` Stefan Wahren
  2025-05-06 12:24       ` Andrew Lunn
  0 siblings, 1 reply; 20+ messages in thread
From: Stefan Wahren @ 2025-05-06  8:38 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	netdev, devicetree

Hi Andrew,

Am 05.05.25 um 18:32 schrieb Andrew Lunn:
>> +	if (!irq_data) {
>> +		netdev_err(ndev, "Invalid IRQ: %d\n", ndev->irq);
>> +		return -EINVAL;
>> +	}
>> +
>> +	switch (irqd_get_trigger_type(irq_data)) {
>> +	case IRQ_TYPE_LEVEL_HIGH:
>> +	case IRQ_TYPE_LEVEL_LOW:
>> +		break;
>> +	default:
>> +		netdev_warn_once(ndev, "Only IRQ type level recommended, please update your firmware.\n");
> I would probably put DT in there somewhere. firmware is rather
> generic.
I'm fine with changing it to DT. I slightly remember of a patch for a 
BCM2835 driver, which also had a warning to update the DT and a reviewer 
requested it to change it to firmware. I don't remember the reason 
behind it, maybe it's the fact that not all user know what a DT / 
devicetree is. A quick grep shows both variants (DT vs firmware).

Regards
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>
>      Andrew


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

* Re: [PATCH net-next 2/5] net: vertexcom: mse102x: Add warning about IRQ trigger type
  2025-05-06  8:38     ` Stefan Wahren
@ 2025-05-06 12:24       ` Andrew Lunn
  0 siblings, 0 replies; 20+ messages in thread
From: Andrew Lunn @ 2025-05-06 12:24 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	netdev, devicetree

On Tue, May 06, 2025 at 10:38:53AM +0200, Stefan Wahren wrote:
> Hi Andrew,
> 
> Am 05.05.25 um 18:32 schrieb Andrew Lunn:
> > > +	if (!irq_data) {
> > > +		netdev_err(ndev, "Invalid IRQ: %d\n", ndev->irq);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	switch (irqd_get_trigger_type(irq_data)) {
> > > +	case IRQ_TYPE_LEVEL_HIGH:
> > > +	case IRQ_TYPE_LEVEL_LOW:
> > > +		break;
> > > +	default:
> > > +		netdev_warn_once(ndev, "Only IRQ type level recommended, please update your firmware.\n");
> > I would probably put DT in there somewhere. firmware is rather
> > generic.
> I'm fine with changing it to DT. I slightly remember of a patch for a
> BCM2835 driver, which also had a warning to update the DT and a reviewer
> requested it to change it to firmware. I don't remember the reason behind
> it, maybe it's the fact that not all user know what a DT / devicetree is. A
> quick grep shows both variants (DT vs firmware).


The line gets long, but "Only IRQ type level recommended, please
update your device tree firmware.\n" is good for me.

	Andrew

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

* Re: [PATCH net-next 2/5] net: vertexcom: mse102x: Add warning about IRQ trigger type
  2025-05-05 14:24 ` [PATCH net-next 2/5] net: vertexcom: mse102x: Add warning about IRQ trigger type Stefan Wahren
  2025-05-05 16:32   ` Andrew Lunn
  2025-05-05 20:57   ` Jakub Kicinski
@ 2025-05-07 10:25   ` kernel test robot
  2025-05-07 14:35     ` Stefan Wahren
  2025-05-08  9:00   ` kernel test robot
  3 siblings, 1 reply; 20+ messages in thread
From: kernel test robot @ 2025-05-07 10:25 UTC (permalink / raw)
  To: Stefan Wahren, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley
  Cc: oe-kbuild-all, netdev, devicetree, Stefan Wahren

Hi Stefan,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Stefan-Wahren/dt-bindings-vertexcom-mse102x-Fix-IRQ-type-in-example/20250505-222628
base:   net-next/main
patch link:    https://lore.kernel.org/r/20250505142427.9601-3-wahrenst%40gmx.net
patch subject: [PATCH net-next 2/5] net: vertexcom: mse102x: Add warning about IRQ trigger type
config: s390-allyesconfig (https://download.01.org/0day-ci/archive/20250507/202505071827.nbdcs1rW-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250507/202505071827.nbdcs1rW-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505071827.nbdcs1rW-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/net/ethernet/vertexcom/mse102x.c: In function 'mse102x_net_open':
>> drivers/net/ethernet/vertexcom/mse102x.c:525:37: error: implicit declaration of function 'irq_get_irq_data'; did you mean 'irq_set_irq_wake'? [-Wimplicit-function-declaration]
     525 |         struct irq_data *irq_data = irq_get_irq_data(ndev->irq);
         |                                     ^~~~~~~~~~~~~~~~
         |                                     irq_set_irq_wake
>> drivers/net/ethernet/vertexcom/mse102x.c:525:37: error: initialization of 'struct irq_data *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
>> drivers/net/ethernet/vertexcom/mse102x.c:535:17: error: implicit declaration of function 'irqd_get_trigger_type'; did you mean 'led_get_trigger_data'? [-Wimplicit-function-declaration]
     535 |         switch (irqd_get_trigger_type(irq_data)) {
         |                 ^~~~~~~~~~~~~~~~~~~~~
         |                 led_get_trigger_data
>> drivers/net/ethernet/vertexcom/mse102x.c:536:14: error: 'IRQ_TYPE_LEVEL_HIGH' undeclared (first use in this function)
     536 |         case IRQ_TYPE_LEVEL_HIGH:
         |              ^~~~~~~~~~~~~~~~~~~
   drivers/net/ethernet/vertexcom/mse102x.c:536:14: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/net/ethernet/vertexcom/mse102x.c:537:14: error: 'IRQ_TYPE_LEVEL_LOW' undeclared (first use in this function)
     537 |         case IRQ_TYPE_LEVEL_LOW:
         |              ^~~~~~~~~~~~~~~~~~


vim +525 drivers/net/ethernet/vertexcom/mse102x.c

   522	
   523	static int mse102x_net_open(struct net_device *ndev)
   524	{
 > 525		struct irq_data *irq_data = irq_get_irq_data(ndev->irq);
   526		struct mse102x_net *mse = netdev_priv(ndev);
   527		struct mse102x_net_spi *mses = to_mse102x_spi(mse);
   528		int ret;
   529	
   530		if (!irq_data) {
   531			netdev_err(ndev, "Invalid IRQ: %d\n", ndev->irq);
   532			return -EINVAL;
   533		}
   534	
 > 535		switch (irqd_get_trigger_type(irq_data)) {
 > 536		case IRQ_TYPE_LEVEL_HIGH:
 > 537		case IRQ_TYPE_LEVEL_LOW:
   538			break;
   539		default:
   540			netdev_warn_once(ndev, "Only IRQ type level recommended, please update your firmware.\n");
   541			break;
   542		}
   543	
   544		ret = request_threaded_irq(ndev->irq, NULL, mse102x_irq, IRQF_ONESHOT,
   545					   ndev->name, mse);
   546		if (ret < 0) {
   547			netdev_err(ndev, "Failed to get irq: %d\n", ret);
   548			return ret;
   549		}
   550	
   551		netif_dbg(mse, ifup, ndev, "opening\n");
   552	
   553		netif_start_queue(ndev);
   554	
   555		netif_carrier_on(ndev);
   556	
   557		/* The SPI interrupt can stuck in case of pending packet(s).
   558		 * So poll for possible packet(s) to re-arm the interrupt.
   559		 */
   560		mutex_lock(&mses->lock);
   561		mse102x_rx_pkt_spi(mse);
   562		mutex_unlock(&mses->lock);
   563	
   564		netif_dbg(mse, ifup, ndev, "network device up\n");
   565	
   566		return 0;
   567	}
   568	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

* Re: [PATCH net-next 2/5] net: vertexcom: mse102x: Add warning about IRQ trigger type
  2025-05-07 10:25   ` kernel test robot
@ 2025-05-07 14:35     ` Stefan Wahren
  0 siblings, 0 replies; 20+ messages in thread
From: Stefan Wahren @ 2025-05-07 14:35 UTC (permalink / raw)
  To: kernel test robot, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley
  Cc: oe-kbuild-all, netdev, devicetree

Am 07.05.25 um 12:25 schrieb kernel test robot:
> Hi Stefan,
>
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on net-next/main]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Stefan-Wahren/dt-bindings-vertexcom-mse102x-Fix-IRQ-type-in-example/20250505-222628
> base:   net-next/main
> patch link:    https://lore.kernel.org/r/20250505142427.9601-3-wahrenst%40gmx.net
> patch subject: [PATCH net-next 2/5] net: vertexcom: mse102x: Add warning about IRQ trigger type
> config: s390-allyesconfig (https://download.01.org/0day-ci/archive/20250507/202505071827.nbdcs1rW-lkp@intel.com/config)
> compiler: s390-linux-gcc (GCC) 14.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250507/202505071827.nbdcs1rW-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202505071827.nbdcs1rW-lkp@intel.com/
>
> All errors (new ones prefixed by >>):
>
>     drivers/net/ethernet/vertexcom/mse102x.c: In function 'mse102x_net_open':
>>> drivers/net/ethernet/vertexcom/mse102x.c:525:37: error: implicit declaration of function 'irq_get_irq_data'; did you mean 'irq_set_irq_wake'? [-Wimplicit-function-declaration]
>       525 |         struct irq_data *irq_data = irq_get_irq_data(ndev->irq);
>           |                                     ^~~~~~~~~~~~~~~~
>           |                                     irq_set_irq_wake
>
this issue has already been reported by Jakub. I will add the missing 
include in the next round. Sorry about that.

Regards

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

* Re: [PATCH net-next 2/5] net: vertexcom: mse102x: Add warning about IRQ trigger type
  2025-05-05 14:24 ` [PATCH net-next 2/5] net: vertexcom: mse102x: Add warning about IRQ trigger type Stefan Wahren
                     ` (2 preceding siblings ...)
  2025-05-07 10:25   ` kernel test robot
@ 2025-05-08  9:00   ` kernel test robot
  3 siblings, 0 replies; 20+ messages in thread
From: kernel test robot @ 2025-05-08  9:00 UTC (permalink / raw)
  To: Stefan Wahren, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley
  Cc: oe-kbuild-all, netdev, devicetree, Stefan Wahren

Hi Stefan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Stefan-Wahren/dt-bindings-vertexcom-mse102x-Fix-IRQ-type-in-example/20250505-222628
base:   net-next/main
patch link:    https://lore.kernel.org/r/20250505142427.9601-3-wahrenst%40gmx.net
patch subject: [PATCH net-next 2/5] net: vertexcom: mse102x: Add warning about IRQ trigger type
config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20250508/202505081612.wbRgFMC7-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250508/202505081612.wbRgFMC7-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505081612.wbRgFMC7-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/net/ethernet/vertexcom/mse102x.c: In function 'mse102x_net_open':
   drivers/net/ethernet/vertexcom/mse102x.c:525:37: error: implicit declaration of function 'irq_get_irq_data'; did you mean 'irq_set_irq_wake'? [-Werror=implicit-function-declaration]
     525 |         struct irq_data *irq_data = irq_get_irq_data(ndev->irq);
         |                                     ^~~~~~~~~~~~~~~~
         |                                     irq_set_irq_wake
>> drivers/net/ethernet/vertexcom/mse102x.c:525:37: warning: initialization of 'struct irq_data *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
   drivers/net/ethernet/vertexcom/mse102x.c:535:17: error: implicit declaration of function 'irqd_get_trigger_type'; did you mean 'led_get_trigger_data'? [-Werror=implicit-function-declaration]
     535 |         switch (irqd_get_trigger_type(irq_data)) {
         |                 ^~~~~~~~~~~~~~~~~~~~~
         |                 led_get_trigger_data
   drivers/net/ethernet/vertexcom/mse102x.c:536:14: error: 'IRQ_TYPE_LEVEL_HIGH' undeclared (first use in this function)
     536 |         case IRQ_TYPE_LEVEL_HIGH:
         |              ^~~~~~~~~~~~~~~~~~~
   drivers/net/ethernet/vertexcom/mse102x.c:536:14: note: each undeclared identifier is reported only once for each function it appears in
   drivers/net/ethernet/vertexcom/mse102x.c:537:14: error: 'IRQ_TYPE_LEVEL_LOW' undeclared (first use in this function)
     537 |         case IRQ_TYPE_LEVEL_LOW:
         |              ^~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +525 drivers/net/ethernet/vertexcom/mse102x.c

   522	
   523	static int mse102x_net_open(struct net_device *ndev)
   524	{
 > 525		struct irq_data *irq_data = irq_get_irq_data(ndev->irq);
   526		struct mse102x_net *mse = netdev_priv(ndev);
   527		struct mse102x_net_spi *mses = to_mse102x_spi(mse);
   528		int ret;
   529	
   530		if (!irq_data) {
   531			netdev_err(ndev, "Invalid IRQ: %d\n", ndev->irq);
   532			return -EINVAL;
   533		}
   534	
   535		switch (irqd_get_trigger_type(irq_data)) {
   536		case IRQ_TYPE_LEVEL_HIGH:
   537		case IRQ_TYPE_LEVEL_LOW:
   538			break;
   539		default:
   540			netdev_warn_once(ndev, "Only IRQ type level recommended, please update your firmware.\n");
   541			break;
   542		}
   543	
   544		ret = request_threaded_irq(ndev->irq, NULL, mse102x_irq, IRQF_ONESHOT,
   545					   ndev->name, mse);
   546		if (ret < 0) {
   547			netdev_err(ndev, "Failed to get irq: %d\n", ret);
   548			return ret;
   549		}
   550	
   551		netif_dbg(mse, ifup, ndev, "opening\n");
   552	
   553		netif_start_queue(ndev);
   554	
   555		netif_carrier_on(ndev);
   556	
   557		/* The SPI interrupt can stuck in case of pending packet(s).
   558		 * So poll for possible packet(s) to re-arm the interrupt.
   559		 */
   560		mutex_lock(&mses->lock);
   561		mse102x_rx_pkt_spi(mse);
   562		mutex_unlock(&mses->lock);
   563	
   564		netif_dbg(mse, ifup, ndev, "network device up\n");
   565	
   566		return 0;
   567	}
   568	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

end of thread, other threads:[~2025-05-08  9:00 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-05 14:24 [PATCH net-next 0/5] net: vertexcom: mse102x: Improve RX handling Stefan Wahren
2025-05-05 14:24 ` [PATCH net-next 1/5] dt-bindings: vertexcom-mse102x: Fix IRQ type in example Stefan Wahren
2025-05-05 16:31   ` Andrew Lunn
2025-05-05 14:24 ` [PATCH net-next 2/5] net: vertexcom: mse102x: Add warning about IRQ trigger type Stefan Wahren
2025-05-05 16:32   ` Andrew Lunn
2025-05-06  8:38     ` Stefan Wahren
2025-05-06 12:24       ` Andrew Lunn
2025-05-05 20:57   ` Jakub Kicinski
2025-05-07 10:25   ` kernel test robot
2025-05-07 14:35     ` Stefan Wahren
2025-05-08  9:00   ` kernel test robot
2025-05-05 14:24 ` [PATCH net-next 3/5] net: vertexcom: mse102x: Drop invalid cmd stats Stefan Wahren
2025-05-05 16:35   ` Andrew Lunn
2025-05-05 17:28     ` Stefan Wahren
2025-05-05 14:24 ` [PATCH net-next 4/5] net: vertexcom: mse102x: Return code for mse102x_rx_pkt_spi Stefan Wahren
2025-05-05 16:43   ` Andrew Lunn
2025-05-05 17:16     ` Stefan Wahren
2025-05-05 19:27       ` Andrew Lunn
2025-05-06  8:24         ` Stefan Wahren
2025-05-05 14:24 ` [PATCH net-next 5/5] net: vertexcom: mse102x: Simplify mse102x_rx_pkt_spi Stefan Wahren

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).