Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1580004 > unrolled thread

[PATCH v4 net-next 5/8] sunvnet: straighten up message event handling logic

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v4 net-next 5/8] sunvnet: straighten up message event handling logic Shannon Nelson <shannon.nelson@oracle.com> - 2017-02-13 20:10 +0100
    Re: [PATCH v4 net-next 5/8] sunvnet: straighten up message event  handling logic Joe Perches <joe@perches.com> - 2017-02-13 20:10 +0100
      Re: [PATCH v4 net-next 5/8] sunvnet: straighten up message event  handling logic Shannon Nelson <shannon.nelson@oracle.com> - 2017-02-13 20:20 +0100

#1580004 — [PATCH v4 net-next 5/8] sunvnet: straighten up message event handling logic

The use of gotos for handling the incoming events made this code
harder to read and support than it should be.  This patch straightens
out and clears up the logic.

Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
---
 drivers/net/ethernet/sun/sunvnet_common.c |   94 ++++++++++++++---------------
 1 files changed, 45 insertions(+), 49 deletions(-)

diff --git a/drivers/net/ethernet/sun/sunvnet_common.c b/drivers/net/ethernet/sun/sunvnet_common.c
index 623363b..d124bde 100644
--- a/drivers/net/ethernet/sun/sunvnet_common.c
+++ b/drivers/net/ethernet/sun/sunvnet_common.c
@@ -738,41 +738,37 @@ static int vnet_event_napi(struct vnet_port *port, int budget)
 	struct vio_driver_state *vio = &port->vio;
 	int tx_wakeup, err;
 	int npkts = 0;
-	int event = (port->rx_event & LDC_EVENT_RESET);
-
-ldc_ctrl:
-	if (unlikely(event == LDC_EVENT_RESET ||
-		     event == LDC_EVENT_UP)) {
-		vio_link_state_change(vio, event);
-
-		if (event == LDC_EVENT_RESET) {
-			vnet_port_reset(port);
-			vio_port_up(vio);
-
-			/* If the device is running but its tx queue was
-			 * stopped (due to flow control), restart it.
-			 * This is necessary since vnet_port_reset()
-			 * clears the tx drings and thus we may never get
-			 * back a VIO_TYPE_DATA ACK packet - which is
-			 * the normal mechanism to restart the tx queue.
-			 */
-			if (netif_running(dev))
-				maybe_tx_wakeup(port);
-		}
+
+	/* we don't expect any other bits */
+	BUG_ON(port->rx_event & ~(LDC_EVENT_DATA_READY |
+				  LDC_EVENT_RESET |
+				  LDC_EVENT_UP));
+
+	/* RESET takes precedent over any other event */
+	if (port->rx_event & LDC_EVENT_RESET) {
+		vio_link_state_change(vio, LDC_EVENT_RESET);
+		vnet_port_reset(port);
+		vio_port_up(vio);
+
+		/* If the device is running but its tx queue was
+		 * stopped (due to flow control), restart it.
+		 * This is necessary since vnet_port_reset()
+		 * clears the tx drings and thus we may never get
+		 * back a VIO_TYPE_DATA ACK packet - which is
+		 * the normal mechanism to restart the tx queue.
+		 */
+		if (netif_running(dev))
+			maybe_tx_wakeup(port);
+
 		port->rx_event = 0;
 		return 0;
 	}
-	/* We may have multiple LDC events in rx_event. Unroll send_events() */
-	event = (port->rx_event & LDC_EVENT_UP);
-	port->rx_event &= ~(LDC_EVENT_RESET | LDC_EVENT_UP);
-	if (event == LDC_EVENT_UP)
-		goto ldc_ctrl;
-	event = port->rx_event;
-	if (!(event & LDC_EVENT_DATA_READY))
-		return 0;
 
-	/* we dont expect any other bits than RESET, UP, DATA_READY */
-	BUG_ON(event != LDC_EVENT_DATA_READY);
+	if (port->rx_event & LDC_EVENT_UP) {
+		vio_link_state_change(vio, LDC_EVENT_UP);
+		port->rx_event = 0;
+		return 0;
+	}
 
 	err = 0;
 	tx_wakeup = 0;
@@ -795,25 +791,25 @@ static int vnet_event_napi(struct vnet_port *port, int budget)
 			pkt->start_idx = vio_dring_next(dr,
 							port->napi_stop_idx);
 			pkt->end_idx = -1;
-			goto napi_resume;
-		}
-		err = ldc_read(vio->lp, &msgbuf, sizeof(msgbuf));
-		if (unlikely(err < 0)) {
-			if (err == -ECONNRESET)
-				vio_conn_reset(vio);
-			break;
+		} else {
+			err = ldc_read(vio->lp, &msgbuf, sizeof(msgbuf));
+			if (unlikely(err < 0)) {
+				if (err == -ECONNRESET)
+					vio_conn_reset(vio);
+				break;
+			}
+			if (err == 0)
+				break;
+			viodbg(DATA, "TAG [%02x:%02x:%04x:%08x]\n",
+			       msgbuf.tag.type,
+			       msgbuf.tag.stype,
+			       msgbuf.tag.stype_env,
+			       msgbuf.tag.sid);
+			err = vio_validate_sid(vio, &msgbuf.tag);
+			if (err < 0)
+				break;
 		}
-		if (err == 0)
-			break;
-		viodbg(DATA, "TAG [%02x:%02x:%04x:%08x]\n",
-		       msgbuf.tag.type,
-		       msgbuf.tag.stype,
-		       msgbuf.tag.stype_env,
-		       msgbuf.tag.sid);
-		err = vio_validate_sid(vio, &msgbuf.tag);
-		if (err < 0)
-			break;
-napi_resume:
+
 		if (likely(msgbuf.tag.type == VIO_TYPE_DATA)) {
 			if (msgbuf.tag.stype == VIO_SUBTYPE_INFO) {
 				if (!sunvnet_port_is_up_common(port)) {
-- 
1.7.1

[toc] | [next] | [standalone]

#1580011 — Re: [PATCH v4 net-next 5/8] sunvnet: straighten up message event handling logic

On Mon, 2017-02-13 at 10:57 -0800, Shannon Nelson wrote:
> The use of gotos for handling the incoming events made this code
> harder to read and support than it should be.  This patch straightens
> out and clears up the logic.
> 
> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
> ---
>  drivers/net/ethernet/sun/sunvnet_common.c |   94 ++++++++++++++---------------
>  1 files changed, 45 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sun/sunvnet_common.c b/drivers/net/ethernet/sun/sunvnet_common.c
[]
> @@ -738,41 +738,37 @@ static int vnet_event_napi(struct vnet_port *port, int budget)
[]
> +	/* we don't expect any other bits */
> +	BUG_ON(port->rx_event & ~(LDC_EVENT_DATA_READY |
> +				  LDC_EVENT_RESET |
> +				  LDC_EVENT_UP));

Is it really necessary to use BUG_ON here?

[toc] | [prev] | [next] | [standalone]

#1580029 — Re: [PATCH v4 net-next 5/8] sunvnet: straighten up message event handling logic

On 2/13/2017 11:06 AM, Joe Perches wrote:
> On Mon, 2017-02-13 at 10:57 -0800, Shannon Nelson wrote:
>> The use of gotos for handling the incoming events made this code
>> harder to read and support than it should be.  This patch straightens
>> out and clears up the logic.
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
>> ---
>>  drivers/net/ethernet/sun/sunvnet_common.c |   94 ++++++++++++++---------------
>>  1 files changed, 45 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/sun/sunvnet_common.c b/drivers/net/ethernet/sun/sunvnet_common.c
> []
>> @@ -738,41 +738,37 @@ static int vnet_event_napi(struct vnet_port *port, int budget)
> []
>> +	/* we don't expect any other bits */
>> +	BUG_ON(port->rx_event & ~(LDC_EVENT_DATA_READY |
>> +				  LDC_EVENT_RESET |
>> +				  LDC_EVENT_UP));
>
> Is it really necessary to use BUG_ON here?
>

I'm carrying this from the original code because we want to know asap if 
we have a low level protocol issue.  It should never happen in the 
field, but we want to notice it as soon as we can when doing development 
and testing.  In this patch I've simply made it more obvious and up 
front that we're doing this test rather than having it buried in the 
logic a few lines further down.

sln

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web