[PATCH net-next] net: sxgbe: change conditional statement from if to switch

[PATCH net-next] net: sxgbe: change conditional statement from if to switch

[Jeongjun Park]

Change the if conditional statement in sxgbe_rx_ctxt_wbstatus() to a switch
conditional statement to improve readability, and also add processing for
cases where all conditions are not satisfied.

Signed-off-by: Jeongjun Park <aha310510@gmail.com>
---
 .../net/ethernet/samsung/sxgbe/sxgbe_desc.c   | 43 +++++++++++++------
 1 file changed, 30 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_desc.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_desc.c
index b33ebf2dca47..5e69ab8a4b90 100644
--- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_desc.c
+++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_desc.c
@@ -421,31 +421,48 @@ static void sxgbe_rx_ctxt_wbstatus(struct sxgbe_rx_ctxt_desc *p,
 	if (p->tstamp_dropped)
 		x->timestamp_dropped++;
 
-	/* ptp */
-	if (p->ptp_msgtype == RX_NO_PTP)
+	/* PTP Msg type */
+	switch (p->ptp_msgtype) {
+	case RX_NO_PTP:
 		x->rx_msg_type_no_ptp++;
-	else if (p->ptp_msgtype == RX_PTP_SYNC)
+		break;
+	case RX_PTP_SYNC:
 		x->rx_ptp_type_sync++;
-	else if (p->ptp_msgtype == RX_PTP_FOLLOW_UP)
+		break;
+	case RX_PTP_FOLLOW_UP:
 		x->rx_ptp_type_follow_up++;
-	else if (p->ptp_msgtype == RX_PTP_DELAY_REQ)
+		break;
+	case RX_PTP_DELAY_REQ:
 		x->rx_ptp_type_delay_req++;
-	else if (p->ptp_msgtype == RX_PTP_DELAY_RESP)
+		break;
+	case RX_PTP_DELAY_RESP:
 		x->rx_ptp_type_delay_resp++;
-	else if (p->ptp_msgtype == RX_PTP_PDELAY_REQ)
+		break;
+	case RX_PTP_PDELAY_REQ:
 		x->rx_ptp_type_pdelay_req++;
-	else if (p->ptp_msgtype == RX_PTP_PDELAY_RESP)
+		break;
+	case RX_PTP_PDELAY_RESP:
 		x->rx_ptp_type_pdelay_resp++;
-	else if (p->ptp_msgtype == RX_PTP_PDELAY_FOLLOW_UP)
+		break;
+	case RX_PTP_PDELAY_FOLLOW_UP:
 		x->rx_ptp_type_pdelay_follow_up++;
-	else if (p->ptp_msgtype == RX_PTP_ANNOUNCE)
+		break;
+	case RX_PTP_ANNOUNCE:
 		x->rx_ptp_announce++;
-	else if (p->ptp_msgtype == RX_PTP_MGMT)
+		break;
+	case RX_PTP_MGMT:
 		x->rx_ptp_mgmt++;
-	else if (p->ptp_msgtype == RX_PTP_SIGNAL)
+		break;
+	case RX_PTP_SIGNAL:
 		x->rx_ptp_signal++;
-	else if (p->ptp_msgtype == RX_PTP_RESV_MSG)
+		break;
+	case RX_PTP_RESV_MSG:
 		x->rx_ptp_resv_msg_type++;
+		break;
+	default:
+		pr_err("Invalid PTP Message type\n");
+		break;
+	}
 }
 
 /* Get rx timestamp status */
--

Re: [PATCH net-next] net: sxgbe: change conditional statement from if to switch

[Simon Horman]

On Fri, Jan 17, 2025 at 01:03:14AM +0900, Jeongjun Park wrote:
> Change the if conditional statement in sxgbe_rx_ctxt_wbstatus() to a switch
> conditional statement to improve readability, and also add processing for
> cases where all conditions are not satisfied.
> 
> Signed-off-by: Jeongjun Park <aha310510@gmail.com>
> ---
>  .../net/ethernet/samsung/sxgbe/sxgbe_desc.c   | 43 +++++++++++++------
>  1 file changed, 30 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_desc.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_desc.c
> index b33ebf2dca47..5e69ab8a4b90 100644
> --- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_desc.c
> +++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_desc.c
> @@ -421,31 +421,48 @@ static void sxgbe_rx_ctxt_wbstatus(struct sxgbe_rx_ctxt_desc *p,

...

> +	default:
> +		pr_err("Invalid PTP Message type\n");
> +		break;
> +	}
>  }

Hi Jeongjun,

I was wondering if it would be best if the error message above should be
rate limited, or perhaps the callback enhanced to return an error in such
cases. But that depends on where sxgbe_rx_ctxt_wbstatus is called.
And I'm unable to find where the it called.

I see that sxgbe_rx_ctxt_wbstatus is registered as a get_rx_ctxt_tstamp_status
callback. But is the get_rx_ctxt_tstamp_status callback called anywhere?

Re: [PATCH net-next] net: sxgbe: change conditional statement from if to switch

[Jeongjun Park]

Simon Horman <horms@kernel.org> wrote:
>
> On Fri, Jan 17, 2025 at 01:03:14AM +0900, Jeongjun Park wrote:
> > Change the if conditional statement in sxgbe_rx_ctxt_wbstatus() to a switch
> > conditional statement to improve readability, and also add processing for
> > cases where all conditions are not satisfied.
> >
> > Signed-off-by: Jeongjun Park <aha310510@gmail.com>
> > ---
> >  .../net/ethernet/samsung/sxgbe/sxgbe_desc.c   | 43 +++++++++++++------
> >  1 file changed, 30 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_desc.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_desc.c
> > index b33ebf2dca47..5e69ab8a4b90 100644
> > --- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_desc.c
> > +++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_desc.c
> > @@ -421,31 +421,48 @@ static void sxgbe_rx_ctxt_wbstatus(struct sxgbe_rx_ctxt_desc *p,
>
> ...
>
> > +     default:
> > +             pr_err("Invalid PTP Message type\n");
> > +             break;
> > +     }
> >  }
>
> Hi Jeongjun,
>
> I was wondering if it would be best if the error message above should be
> rate limited, or perhaps the callback enhanced to return an error in such
> cases. But that depends on where sxgbe_rx_ctxt_wbstatus is called.
> And I'm unable to find where the it called.
>
> I see that sxgbe_rx_ctxt_wbstatus is registered as a get_rx_ctxt_tstamp_status
> callback. But is the get_rx_ctxt_tstamp_status callback called anywhere?

Hello. Sorry for the late reply.

I still don't know exactly where sxgbe_rx_ctxt_wbstatus() is called. What I
do know is that I can't find a function that calls this function within the
Linux kernel. When I wrote this patch, I thought it would be good to refactor
this function, so I wrote the patch, but I didn't know exactly where
this function
was being used. I think it might be a function called from an external
kernel driver
written by Samsung itself.

Regards,

Jeongjun Park