| Message ID | 20250120170026.1880-1-vulab@iscas.ac.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | net: usb: qmi_wwan: Add error handling for usbnet_get_ethernet_addr | expand |
On Tue, Jan 21, 2025 at 01:00:26AM +0800, Wentao Liang wrote: > In qmi_wwan_bind(), usbnet_get_ethernet_addr() is called > without error handling, risking unnoticed failures and > inconsistent behavior compared to other parts of the code. > > Fix this issue by adding an error handling for the > usbnet_get_ethernet_addr(), improving code robustness. > > Fixes: 423ce8caab7e ("net: usb: qmi_wwan: New driver for Huawei QMI based WWAN devices") > Signed-off-by: Wentao Liang <vulab@iscas.ac.cn> Reviewed-by: Breno Leitao <leitao@debian.org>
Wentao Liang <vulab@iscas.ac.cn> writes: > In qmi_wwan_bind(), usbnet_get_ethernet_addr() is called > without error handling, risking unnoticed failures and > inconsistent behavior compared to other parts of the code. > > Fix this issue by adding an error handling for the > usbnet_get_ethernet_addr(), improving code robustness. > > Fixes: 423ce8caab7e ("net: usb: qmi_wwan: New driver for Huawei QMI based WWAN devices") > Signed-off-by: Wentao Liang <vulab@iscas.ac.cn> > --- > drivers/net/usb/qmi_wwan.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c > index e9208a8d2bfa..7aa576bfe76b 100644 > --- a/drivers/net/usb/qmi_wwan.c > +++ b/drivers/net/usb/qmi_wwan.c > @@ -779,7 +779,9 @@ static int qmi_wwan_bind(struct usbnet *dev, struct usb_interface *intf) > /* errors aren't fatal - we can live with the dynamic address */ > if (cdc_ether && cdc_ether->wMaxSegmentSize) { > dev->hard_mtu = le16_to_cpu(cdc_ether->wMaxSegmentSize); > - usbnet_get_ethernet_addr(dev, cdc_ether->iMACAddress); > + status = usbnet_get_ethernet_addr(dev, cdc_ether->iMACAddress); > + if (status < 0) > + goto err; > } > > /* claim data interface and set it up */ Did you read the comment? This intentinonally ignores any errors. I don't know how to make it clear anough for these AI bots to understand. Any advice there? NAK (and why weren't I CCed? Noticed this by chance only...) Bjørn
On 20.01.25 18:00, Wentao Liang wrote: > In qmi_wwan_bind(), usbnet_get_ethernet_addr() is called > without error handling, risking unnoticed failures and > inconsistent behavior compared to other parts of the code. Hi, unfortunately this makes no difference between a genuine IO error and the device simply providing no string. Does the device need to provide a string for the MAC in order to function? Frankly, this patch does not look like a good idea, as it disregards the reasoning expressed in the comment seen in the code. Regards Oliver
On Mon, Jan 20, 2025 at 07:06:52PM +0100, Bjørn Mork wrote: > Wentao Liang <vulab@iscas.ac.cn> writes: > > > In qmi_wwan_bind(), usbnet_get_ethernet_addr() is called > > without error handling, risking unnoticed failures and > > inconsistent behavior compared to other parts of the code. > > > > Fix this issue by adding an error handling for the > > usbnet_get_ethernet_addr(), improving code robustness. > > > > Fixes: 423ce8caab7e ("net: usb: qmi_wwan: New driver for Huawei QMI based WWAN devices") > > Signed-off-by: Wentao Liang <vulab@iscas.ac.cn> > > --- > > drivers/net/usb/qmi_wwan.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c > > index e9208a8d2bfa..7aa576bfe76b 100644 > > --- a/drivers/net/usb/qmi_wwan.c > > +++ b/drivers/net/usb/qmi_wwan.c > > @@ -779,7 +779,9 @@ static int qmi_wwan_bind(struct usbnet *dev, struct usb_interface *intf) > > /* errors aren't fatal - we can live with the dynamic address */ > > if (cdc_ether && cdc_ether->wMaxSegmentSize) { > > dev->hard_mtu = le16_to_cpu(cdc_ether->wMaxSegmentSize); > > - usbnet_get_ethernet_addr(dev, cdc_ether->iMACAddress); > > + status = usbnet_get_ethernet_addr(dev, cdc_ether->iMACAddress); > > + if (status < 0) > > + goto err; > > } > > > > /* claim data interface and set it up */ > > > > Did you read the comment? > > This intentinonally ignores any errors. I don't know how to make it > clear anough for these AI bots to understand. Any advice there? The problem is not really the AI bot, but the user of the AI bot. We often see this problem, and need to teach bot drivers that the bot is just the start. The bot points out a potential issue, but it needs a human developer to verify the issue and consider what the real fix is, which might be, as in this case, a false positive and no fix is needed. I don't know how to really fix this issue, other than try to teach the bot drivers to actually think. > NAK > > (and why weren't I CCed? Noticed this by chance only...) Probably the same issue. The bot driver does not know the processes, has not run the get_maintainers script. This is again part of the issue with these sorts of patched, they are often low quality and need careful review. Andrew
diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c index e9208a8d2bfa..7aa576bfe76b 100644 --- a/drivers/net/usb/qmi_wwan.c +++ b/drivers/net/usb/qmi_wwan.c @@ -779,7 +779,9 @@ static int qmi_wwan_bind(struct usbnet *dev, struct usb_interface *intf) /* errors aren't fatal - we can live with the dynamic address */ if (cdc_ether && cdc_ether->wMaxSegmentSize) { dev->hard_mtu = le16_to_cpu(cdc_ether->wMaxSegmentSize); - usbnet_get_ethernet_addr(dev, cdc_ether->iMACAddress); + status = usbnet_get_ethernet_addr(dev, cdc_ether->iMACAddress); + if (status < 0) + goto err; } /* claim data interface and set it up */
In qmi_wwan_bind(), usbnet_get_ethernet_addr() is called without error handling, risking unnoticed failures and inconsistent behavior compared to other parts of the code. Fix this issue by adding an error handling for the usbnet_get_ethernet_addr(), improving code robustness. Fixes: 423ce8caab7e ("net: usb: qmi_wwan: New driver for Huawei QMI based WWAN devices") Signed-off-by: Wentao Liang <vulab@iscas.ac.cn> --- drivers/net/usb/qmi_wwan.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)