[PATCH net] net: wwan: Add net device name for error message print

[PATCH net] net: wwan: Add net device name for error message print

[Slark Xiao]

In my local, I got an error print in dmesg like below:
"sequence number glitch prev=487 curr=0"
After checking, it belongs to mhi_wwan_mbim.c. Refer to the usage
of this API in other files, I think we should add net device name
print before message context.

Signed-off-by: Slark Xiao <slark_xiao@163.com>
---
 drivers/net/wwan/mhi_wwan_mbim.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wwan/mhi_wwan_mbim.c b/drivers/net/wwan/mhi_wwan_mbim.c
index 3f72ae943b29..6cefee25efc4 100644
--- a/drivers/net/wwan/mhi_wwan_mbim.c
+++ b/drivers/net/wwan/mhi_wwan_mbim.c
@@ -186,14 +186,14 @@ static int mbim_rx_verify_nth16(struct mhi_mbim_context *mbim, struct sk_buff *s
 
 	if (skb->len < sizeof(struct usb_cdc_ncm_nth16) +
 			sizeof(struct usb_cdc_ncm_ndp16)) {
-		net_err_ratelimited("frame too short\n");
+		net_err_ratelimited("mbim: frame too short\n");
 		return -EINVAL;
 	}
 
 	nth16 = (struct usb_cdc_ncm_nth16 *)skb->data;
 
 	if (nth16->dwSignature != cpu_to_le32(USB_CDC_NCM_NTH16_SIGN)) {
-		net_err_ratelimited("invalid NTH16 signature <%#010x>\n",
+		net_err_ratelimited("mbim: invalid NTH16 signature <%#010x>\n",
 				    le32_to_cpu(nth16->dwSignature));
 		return -EINVAL;
 	}
@@ -201,7 +201,7 @@ static int mbim_rx_verify_nth16(struct mhi_mbim_context *mbim, struct sk_buff *s
 	/* No limit on the block length, except the size of the data pkt */
 	len = le16_to_cpu(nth16->wBlockLength);
 	if (len > skb->len) {
-		net_err_ratelimited("NTB does not fit into the skb %u/%u\n",
+		net_err_ratelimited("mbim: NTB does not fit into the skb %u/%u\n",
 				    len, skb->len);
 		return -EINVAL;
 	}
@@ -209,7 +209,7 @@ static int mbim_rx_verify_nth16(struct mhi_mbim_context *mbim, struct sk_buff *s
 	if (mbim->rx_seq + 1 != le16_to_cpu(nth16->wSequence) &&
 	    (mbim->rx_seq || le16_to_cpu(nth16->wSequence)) &&
 	    !(mbim->rx_seq == 0xffff && !le16_to_cpu(nth16->wSequence))) {
-		net_err_ratelimited("sequence number glitch prev=%d curr=%d\n",
+		net_err_ratelimited("mbim: sequence number glitch prev=%d curr=%d\n",
 				    mbim->rx_seq, le16_to_cpu(nth16->wSequence));
 	}
 	mbim->rx_seq = le16_to_cpu(nth16->wSequence);
@@ -222,7 +222,7 @@ static int mbim_rx_verify_ndp16(struct sk_buff *skb, struct usb_cdc_ncm_ndp16 *n
 	int ret;
 
 	if (le16_to_cpu(ndp16->wLength) < USB_CDC_NCM_NDP16_LENGTH_MIN) {
-		net_err_ratelimited("invalid DPT16 length <%u>\n",
+		net_err_ratelimited("mbim: invalid DPT16 length <%u>\n",
 				    le16_to_cpu(ndp16->wLength));
 		return -EINVAL;
 	}
@@ -233,7 +233,7 @@ static int mbim_rx_verify_ndp16(struct sk_buff *skb, struct usb_cdc_ncm_ndp16 *n
 
 	if (sizeof(struct usb_cdc_ncm_ndp16) +
 	     ret * sizeof(struct usb_cdc_ncm_dpe16) > skb->len) {
-		net_err_ratelimited("Invalid nframes = %d\n", ret);
+		net_err_ratelimited("mbim: Invalid nframes = %d\n", ret);
 		return -EINVAL;
 	}
 
-- 
2.25.1

[PATCH net] net: wwan: Add net device name for error message print

[Hariprasad Kelam]

> In my local, I got an error print in dmesg like below:
> "sequence number glitch prev=487 curr=0"
> After checking, it belongs to mhi_wwan_mbim.c. Refer to the usage of this
> API in other files, I think we should add net device name print before
> message context.
> 
> Signed-off-by: Slark Xiao <slark_xiao@163.com>
> ---
>  drivers/net/wwan/mhi_wwan_mbim.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/wwan/mhi_wwan_mbim.c
> b/drivers/net/wwan/mhi_wwan_mbim.c
> index 3f72ae943b29..6cefee25efc4 100644
> --- a/drivers/net/wwan/mhi_wwan_mbim.c
> +++ b/drivers/net/wwan/mhi_wwan_mbim.c
> @@ -186,14 +186,14 @@ static int mbim_rx_verify_nth16(struct
> mhi_mbim_context *mbim, struct sk_buff *s
> 
>  	if (skb->len < sizeof(struct usb_cdc_ncm_nth16) +
>  			sizeof(struct usb_cdc_ncm_ndp16)) {
> -		net_err_ratelimited("frame too short\n");
> +		net_err_ratelimited("mbim: frame too short\n");
>  		return -EINVAL;
>  	}
> 
>  	nth16 = (struct usb_cdc_ncm_nth16 *)skb->data;
> 
>  	if (nth16->dwSignature !=
> cpu_to_le32(USB_CDC_NCM_NTH16_SIGN)) {
> -		net_err_ratelimited("invalid NTH16 signature <%#010x>\n",
> +		net_err_ratelimited("mbim: invalid NTH16 signature
> <%#010x>\n",
>  				    le32_to_cpu(nth16->dwSignature));
>  		return -EINVAL;
>  	}
> @@ -201,7 +201,7 @@ static int mbim_rx_verify_nth16(struct
> mhi_mbim_context *mbim, struct sk_buff *s
>  	/* No limit on the block length, except the size of the data pkt */
>  	len = le16_to_cpu(nth16->wBlockLength);
>  	if (len > skb->len) {
> -		net_err_ratelimited("NTB does not fit into the skb %u/%u\n",
> +		net_err_ratelimited("mbim: NTB does not fit into the skb
> %u/%u\n",
>  				    len, skb->len);
>  		return -EINVAL;
>  	}
> @@ -209,7 +209,7 @@ static int mbim_rx_verify_nth16(struct
> mhi_mbim_context *mbim, struct sk_buff *s
>  	if (mbim->rx_seq + 1 != le16_to_cpu(nth16->wSequence) &&
>  	    (mbim->rx_seq || le16_to_cpu(nth16->wSequence)) &&
>  	    !(mbim->rx_seq == 0xffff && !le16_to_cpu(nth16->wSequence))) {
> -		net_err_ratelimited("sequence number glitch prev=%d
> curr=%d\n",
> +		net_err_ratelimited("mbim: sequence number glitch prev=%d
> curr=%d\n",
>  				    mbim->rx_seq, le16_to_cpu(nth16-
> >wSequence));
>  	}
>  	mbim->rx_seq = le16_to_cpu(nth16->wSequence); @@ -222,7 +222,7
> @@ static int mbim_rx_verify_ndp16(struct sk_buff *skb, struct
> usb_cdc_ncm_ndp16 *n
>  	int ret;
> 
>  	if (le16_to_cpu(ndp16->wLength) <
> USB_CDC_NCM_NDP16_LENGTH_MIN) {
> -		net_err_ratelimited("invalid DPT16 length <%u>\n",
> +		net_err_ratelimited("mbim: invalid DPT16 length <%u>\n",
>  				    le16_to_cpu(ndp16->wLength));
>  		return -EINVAL;
>  	}
> @@ -233,7 +233,7 @@ static int mbim_rx_verify_ndp16(struct sk_buff *skb,
> struct usb_cdc_ncm_ndp16 *n
> 
>  	if (sizeof(struct usb_cdc_ncm_ndp16) +
>  	     ret * sizeof(struct usb_cdc_ncm_dpe16) > skb->len) {
> -		net_err_ratelimited("Invalid nframes = %d\n", ret);
> +		net_err_ratelimited("mbim: Invalid nframes = %d\n", ret);
>  		return -EINVAL;
>  	}
> 
> --
> 2.25.1
> 
Nit: since this patch submitted to "net" , add fixes-tag

Reviewed-by: Hariprasad Kelam <hkelam@marvell.com>

Re: [PATCH net] net: wwan: Add net device name for error message print

[Andrew Lunn]

>  	if (skb->len < sizeof(struct usb_cdc_ncm_nth16) +
>  			sizeof(struct usb_cdc_ncm_ndp16)) {
> -		net_err_ratelimited("frame too short\n");
> +		net_err_ratelimited("mbim: frame too short\n");

I don't know this code at all, but i think you can do

dev_err_ratelimited(&mbim->mdev->dev, "frame too short\n");

That way, it tells you which of the 42 mhi devices has received too
short a frame.

      Andrew

Re:Re: [PATCH net] net: wwan: Add net device name for error message print

[Slark Xiao]

At 2024-04-27 06:21:03, "Andrew Lunn" <andrew@lunn.ch> wrote:
>>  	if (skb->len < sizeof(struct usb_cdc_ncm_nth16) +
>>  			sizeof(struct usb_cdc_ncm_ndp16)) {
>> -		net_err_ratelimited("frame too short\n");
>> +		net_err_ratelimited("mbim: frame too short\n");
>
>I don't know this code at all, but i think you can do
>
>dev_err_ratelimited(&mbim->mdev->dev, "frame too short\n");
>
>That way, it tells you which of the 42 mhi devices has received too
>short a frame.
>
>      Andrew
I tried in my case, and it will print:
mhi_wwan_mbim mhio_IP_HW0_MBIM: frame too short

I think it's much more complicated, isn't it?

Thanks

Re: Re: [PATCH net] net: wwan: Add net device name for error message print

[Andrew Lunn]

> At 2024-04-27 06:21:03, "Andrew Lunn" <andrew@lunn.ch> wrote:
> >>  	if (skb->len < sizeof(struct usb_cdc_ncm_nth16) +
> >>  			sizeof(struct usb_cdc_ncm_ndp16)) {
> >> -		net_err_ratelimited("frame too short\n");
> >> +		net_err_ratelimited("mbim: frame too short\n");
> >
> >I don't know this code at all, but i think you can do
> >
> >dev_err_ratelimited(&mbim->mdev->dev, "frame too short\n");
> >
> >That way, it tells you which of the 42 mhi devices has received too
> >short a frame.
> >
> >      Andrew
> I tried in my case, and it will print:
> mhi_wwan_mbim mhio_IP_HW0_MBIM: frame too short
> 
> I think it's much more complicated, isn't it?

As i said, this is about making it clear which device has problems.
mhio_IP_HW0_MBI is not a particularly good name, but it should be
unique. The question is, is this built into the silicon, and can there
only be one? The name is them pointless.

It is also not too unusual to see drivers define macros

mbim_err_ratelimited(mbin, "frame too short")

	Andrew