| Message ID | 20240426080733.819633-1-slark_xiao@163.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: wwan: Add net device name for error message print | expand |
> 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>
> 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
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
> 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
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; }
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(-)