| Message ID | 1231805322.12962.4.camel@tux.localhost |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Am Tuesday 13 January 2009 01:08:42 schrieb Alexey Klimov: > @@ -675,7 +677,7 @@ static int kaweth_open(struct net_device *net) > > res = usb_autopm_get_interface(kaweth->intf); > if (res) { > - err("Interface cannot be resumed."); > + dev_err(&net->dev, "Interface cannot be resumed.\n"); > return -EIO; > } > res = kaweth_resubmit_rx_urb(kaweth, GFP_KERNEL); > @@ -902,7 +904,7 @@ static void kaweth_async_set_rx_mode(struct kaweth_device *kaweth) > KAWETH_CONTROL_TIMEOUT); > > if(result < 0) { > - err("Failed to set Rx mode: %d", result); > + dev_err(&kaweth->dev->dev, "Failed to set Rx mode: %d\n", result); > } > else { > dbg("Set Rx mode to %d", packet_filter_bitmap); > @@ -1038,7 +1040,7 @@ static int kaweth_probe( > "kaweth/new_code.bin", > 100, > 2)) < 0) { > - err("Error downloading firmware (%d)", result); > + dev_err(&intf->dev, "Error downloading firmware (%d)\n", result); I have to veto this. You are using three different device structures to refer to the same device. That will make it next to impossible to find out which device originated a message. Regards Oliver
On Tue, Jan 13, 2009 at 11:40 AM, Oliver Neukum <oliver@neukum.org> wrote: > Am Tuesday 13 January 2009 01:08:42 schrieb Alexey Klimov: >> @@ -675,7 +677,7 @@ static int kaweth_open(struct net_device *net) >> >> res = usb_autopm_get_interface(kaweth->intf); >> if (res) { >> - err("Interface cannot be resumed."); >> + dev_err(&net->dev, "Interface cannot be resumed.\n"); >> return -EIO; >> } >> res = kaweth_resubmit_rx_urb(kaweth, GFP_KERNEL); >> @@ -902,7 +904,7 @@ static void kaweth_async_set_rx_mode(struct kaweth_device *kaweth) >> KAWETH_CONTROL_TIMEOUT); >> >> if(result < 0) { >> - err("Failed to set Rx mode: %d", result); >> + dev_err(&kaweth->dev->dev, "Failed to set Rx mode: %d\n", result); >> } >> else { >> dbg("Set Rx mode to %d", packet_filter_bitmap); >> @@ -1038,7 +1040,7 @@ static int kaweth_probe( >> "kaweth/new_code.bin", >> 100, >> 2)) < 0) { >> - err("Error downloading firmware (%d)", result); >> + dev_err(&intf->dev, "Error downloading firmware (%d)\n", result); > > I have to veto this. You are using three different device structures > to refer to the same device. That will make it next to impossible to > find out which device originated a message. > > Regards > Oliver Ok, i'll reformat patch. Is it okay if i use kaweth->dev->dev, kaweth->net->dev and intf->dev ? Or switch to kaweth->net->dev (instead of dev->dev)?
Am Tuesday 13 January 2009 09:57:12 schrieb Alexey Klimov: > On Tue, Jan 13, 2009 at 11:40 AM, Oliver Neukum <oliver@neukum.org> wrote: > > I have to veto this. You are using three different device structures > > to refer to the same device. That will make it next to impossible to > > find out which device originated a message. > > > Ok, i'll reformat patch. Is it okay if i use kaweth->dev->dev, > kaweth->net->dev and intf->dev ? Or switch to kaweth->net->dev > (instead of dev->dev)? kaweth in kaweth_disconnect() frees the network device. Therefore you cannot use it. You'd access freed memory. Pick one of the others and stay with it. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 13 January 2009, Oliver Neukum wrote: > > Ok, i'll reformat patch. Is it okay if i use kaweth->dev->dev, > > kaweth->net->dev and intf->dev ? Or switch to kaweth->net->dev > > (instead of dev->dev)? > > kaweth in kaweth_disconnect() frees the network device. Therefore > you cannot use it. You'd access freed memory. Pick one of the others > and stay with it. intf->dev is going to be "obviously correct", the others aren't. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 12 January 2009, Alexey Klimov wrote: > - err("submit(rx_urb) status %d", res); > + dev_err(&catc->usbdev->dev, "submit(rx_urb) status %d\n", res); That's the same as urb->dev though. Use the interface passed into probe(): dev_err(&intf->dev, ...) etc -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 13, 2009 at 12:44 PM, David Brownell <david-b@pacbell.net> wrote: > On Tuesday 13 January 2009, Oliver Neukum wrote: >> > Ok, i'll reformat patch. Is it okay if i use kaweth->dev->dev, >> > kaweth->net->dev and intf->dev ? Or switch to kaweth->net->dev >> > (instead of dev->dev)? >> >> kaweth in kaweth_disconnect() frees the network device. Therefore >> you cannot use it. You'd access freed memory. Pick one of the others >> and stay with it. > > intf->dev is going to be "obviously correct", the others aren't. > Is this patch touch kaweth_disconnect() ? I see dev_warn and dev_info there. No dev_err.
Am Tuesday 13 January 2009 11:08:27 schrieb Alexey Klimov: > On Tue, Jan 13, 2009 at 12:44 PM, David Brownell <david-b@pacbell.net> wrote: > > On Tuesday 13 January 2009, Oliver Neukum wrote: > >> > Ok, i'll reformat patch. Is it okay if i use kaweth->dev->dev, > >> > kaweth->net->dev and intf->dev ? Or switch to kaweth->net->dev > >> > (instead of dev->dev)? > >> > >> kaweth in kaweth_disconnect() frees the network device. Therefore > >> you cannot use it. You'd access freed memory. Pick one of the others > >> and stay with it. > > > > intf->dev is going to be "obviously correct", the others aren't. > > > > Is this patch touch kaweth_disconnect() ? I see dev_warn and dev_info > there. No dev_err. No, it does not. But you need to make sure that the devices you refer to in debug messages are always valid. Functions that you do touch may run after disconnect() has run. A debug message that oopses does no good. If you cannot make sure you pass a valid device to dev_err/info don't use them. It is as simple as that. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, all On Tue, Jan 13, 2009 at 2:07 PM, Oliver Neukum <oliver@neukum.org> wrote: > Am Tuesday 13 January 2009 11:08:27 schrieb Alexey Klimov: >> On Tue, Jan 13, 2009 at 12:44 PM, David Brownell <david-b@pacbell.net> wrote: >> > On Tuesday 13 January 2009, Oliver Neukum wrote: >> >> > Ok, i'll reformat patch. Is it okay if i use kaweth->dev->dev, >> >> > kaweth->net->dev and intf->dev ? Or switch to kaweth->net->dev >> >> > (instead of dev->dev)? >> >> >> >> kaweth in kaweth_disconnect() frees the network device. Therefore >> >> you cannot use it. You'd access freed memory. Pick one of the others >> >> and stay with it. >> > >> > intf->dev is going to be "obviously correct", the others aren't. >> > >> >> Is this patch touch kaweth_disconnect() ? I see dev_warn and dev_info >> there. No dev_err. > > No, it does not. But you need to make sure that the devices you refer to > in debug messages are always valid. Functions that you do touch may > run after disconnect() has run. A debug message that oopses does no good. > If you cannot make sure you pass a valid device to dev_err/info don't use > them. It is as simple as that. Yes, that's obviously right. I'll review and check patch again. Thank you for your advices. On Tue, Jan 13, 2009 at 12:46 PM, David Brownell <david-b@pacbell.net> wrote: > On Monday 12 January 2009, Alexey Klimov wrote: >> - err("submit(rx_urb) status %d", res); >> + dev_err(&catc->usbdev->dev, "submit(rx_urb) status %d\n", res); > > > That's the same as urb->dev though. > > Use the interface passed into probe(): dev_err(&intf->dev, ...) etc Well, in few v4l-dvb drivers successfully used &radio->usbdev->dev in debug messages and that doesn't look wrong. Hmm, i see that struct usb_interface passed to probe function for example, and here in catc_irq_done struct urb passed, and then we have struct catc. So, messages based on catc->usbdev->dev. I can switch to &catc->netdev->dev when it's safe to do, right? I didn't see way we can use &intv->dev in such functions. Am i wrong ? In probe functions &intf->dev is right. Btw, in patch "USB: remove info() macro from usb network drivers" http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=880c9c66a60c0aa4fb4dac2da9679da5f8f41903 few messages based on &urb->dev->dev.
On Tuesday 13 January 2009, Alexey Klimov wrote: > > Use the interface passed into probe(): dev_err(&intf->dev, ...) etc > > Well, in few v4l-dvb drivers successfully used &radio->usbdev->dev in > debug messages and that doesn't look wrong. Use intf->dev ... always. You may need to save the interface in probe(), but that's easy. "Doesn't look wrong" is incorrect; it doesn't list the driver which issued it. Which makes it harder to track down exactly what issued the message, and thence what it really means. - Dave -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Wednesday 14 January 2009 01:22:26 schrieb Alexey Klimov: > > Use the interface passed into probe(): dev_err(&intf->dev, ...) etc A USB driver is fundamentally a driver for a device's interfaces. Therefore the interface's device struct is the obviously correct choice. > Well, in few v4l-dvb drivers successfully used &radio->usbdev->dev in > debug messages and that doesn't look wrong. That will not allow you to identify which interface caused the message. > Hmm, i see that struct usb_interface passed to probe function for > example, and here in catc_irq_done struct urb passed, and then we have > struct catc. So, messages based on catc->usbdev->dev. I can switch to > &catc->netdev->dev when it's safe to do, right? You cannot. The point of passing a device pointer is to identify the device. The device in the network view and the USB view is not identical. You have to pick one choice and stick with it and if for some reason that is not possible you cannot use this debugging functions. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/usb/catc.c b/drivers/net/usb/catc.c index cb7acbb..bf1e881 100644 --- a/drivers/net/usb/catc.c +++ b/drivers/net/usb/catc.c @@ -339,14 +339,14 @@ static void catc_irq_done(struct urb *urb) } else { catc->rx_urb->dev = catc->usbdev; if ((res = usb_submit_urb(catc->rx_urb, GFP_ATOMIC)) < 0) { - err("submit(rx_urb) status %d", res); + dev_err(&catc->usbdev->dev, "submit(rx_urb) status %d\n", res); } } } resubmit: res = usb_submit_urb (urb, GFP_ATOMIC); if (res) - err ("can't resubmit intr, %s-%s, status %d", + dev_err(&catc->usbdev->dev, "can't resubmit intr, %s-%s, status %d\n", catc->usbdev->bus->bus_name, catc->usbdev->devpath, res); } @@ -367,7 +367,7 @@ static int catc_tx_run(struct catc *catc) catc->tx_urb->dev = catc->usbdev; if ((status = usb_submit_urb(catc->tx_urb, GFP_ATOMIC)) < 0) - err("submit(tx_urb), status %d", status); + dev_err(&catc->usbdev->dev, "submit(tx_urb), status %d\n", status); catc->tx_idx = !catc->tx_idx; catc->tx_ptr = 0; @@ -496,7 +496,7 @@ static void catc_ctrl_run(struct catc *catc) memcpy(catc->ctrl_buf, q->buf, q->len); if ((status = usb_submit_urb(catc->ctrl_urb, GFP_KERNEL))) - err("submit(ctrl_urb) status %d", status); + dev_err(&usbdev->dev, "submit(ctrl_urb) status %d\n", status); } static void catc_ctrl_done(struct urb *urb) @@ -555,7 +555,7 @@ static int catc_ctrl_async(struct catc *catc, u8 dir, u8 request, u16 value, catc->ctrl_head = (catc->ctrl_head + 1) & (CTRL_QUEUE - 1); if (catc->ctrl_head == catc->ctrl_tail) { - err("ctrl queue full"); + dev_err(&catc->usbdev->dev, "ctrl queue full\n"); catc->ctrl_tail = (catc->ctrl_tail + 1) & (CTRL_QUEUE - 1); retval = -1; } @@ -721,7 +721,7 @@ static int catc_open(struct net_device *netdev) catc->irq_urb->dev = catc->usbdev; if ((status = usb_submit_urb(catc->irq_urb, GFP_KERNEL)) < 0) { - err("submit(irq_urb) status %d", status); + dev_err(&netdev->dev, "submit(irq_urb) status %d\n", status); return -1; } @@ -764,7 +764,7 @@ static int catc_probe(struct usb_interface *intf, const struct usb_device_id *id if (usb_set_interface(usbdev, intf->altsetting->desc.bInterfaceNumber, 1)) { - err("Can't set altsetting 1."); + dev_err(&intf->dev, "Can't set altsetting 1.\n"); return -EIO; } @@ -799,7 +799,7 @@ static int catc_probe(struct usb_interface *intf, const struct usb_device_id *id catc->irq_urb = usb_alloc_urb(0, GFP_KERNEL); if ((!catc->ctrl_urb) || (!catc->tx_urb) || (!catc->rx_urb) || (!catc->irq_urb)) { - err("No free urbs available."); + dev_err(&intf->dev, "No free urbs available.\n"); usb_free_urb(catc->ctrl_urb); usb_free_urb(catc->tx_urb); usb_free_urb(catc->rx_urb); diff --git a/drivers/net/usb/kaweth.c b/drivers/net/usb/kaweth.c index 7cb10a0..454b745 100644 --- a/drivers/net/usb/kaweth.c +++ b/drivers/net/usb/kaweth.c @@ -397,12 +397,12 @@ static int kaweth_download_firmware(struct kaweth_device *kaweth, ret = request_firmware(&fw, fwname, &kaweth->dev->dev); if (ret) { - err("Firmware request failed\n"); + dev_err(&kaweth->net->dev, "Firmware request failed\n"); return ret; } if (fw->size > KAWETH_FIRMWARE_BUF_SIZE) { - err("Firmware too big: %zu", fw->size); + dev_err(&kaweth->net->dev, "Firmware too big: %zu\n", fw->size); return -ENOSPC; } data_len = fw->size; @@ -506,7 +506,7 @@ static void kaweth_resubmit_int_urb(struct kaweth_device *kaweth, gfp_t mf) } if (status) - err ("can't resubmit intr, %s-%s, status %d", + dev_err(&kaweth->dev->dev, "can't resubmit intr, %s-%s, status %d\n", kaweth->dev->bus->bus_name, kaweth->dev->devpath, status); } @@ -581,7 +581,7 @@ static int kaweth_resubmit_rx_urb(struct kaweth_device *kaweth, kaweth->suspend_lowmem_rx = 1; schedule_delayed_work(&kaweth->lowmem_work, HZ/4); } - err("resubmitting rx_urb %d failed", result); + dev_err(&kaweth->dev->dev, "resubmitting rx_urb %d failed\n", result); } else { kaweth->suspend_lowmem_rx = 0; } @@ -623,7 +623,7 @@ static void kaweth_usb_receive(struct urb *urb) spin_unlock(&kaweth->device_lock); if(status && status != -EREMOTEIO && count != 1) { - err("%s RX status: %d count: %d packet_len: %d", + dev_err(&kaweth->net->dev, "%s RX status: %d count: %d packet_len: %d\n", net->name, status, count, @@ -634,9 +634,11 @@ static void kaweth_usb_receive(struct urb *urb) if(kaweth->net && (count > 2)) { if(pkt_len > (count - 2)) { - err("Packet length too long for USB frame (pkt_len: %x, count: %x)",pkt_len, count); - err("Packet len & 2047: %x", pkt_len & 2047); - err("Count 2: %x", count2); + dev_err(&kaweth->net->dev, + "Packet length too long for USB frame (pkt_len: %x, count: %x)\n", + pkt_len, count); + dev_err(&kaweth->net->dev, "Packet len & 2047: %x\n", pkt_len & 2047); + dev_err(&kaweth->net->dev, "Count 2: %x\n", count2); kaweth_resubmit_rx_urb(kaweth, GFP_ATOMIC); return; } @@ -675,7 +677,7 @@ static int kaweth_open(struct net_device *net) res = usb_autopm_get_interface(kaweth->intf); if (res) { - err("Interface cannot be resumed."); + dev_err(&net->dev, "Interface cannot be resumed.\n"); return -EIO; } res = kaweth_resubmit_rx_urb(kaweth, GFP_KERNEL); @@ -902,7 +904,7 @@ static void kaweth_async_set_rx_mode(struct kaweth_device *kaweth) KAWETH_CONTROL_TIMEOUT); if(result < 0) { - err("Failed to set Rx mode: %d", result); + dev_err(&kaweth->dev->dev, "Failed to set Rx mode: %d\n", result); } else { dbg("Set Rx mode to %d", packet_filter_bitmap); @@ -1038,7 +1040,7 @@ static int kaweth_probe( "kaweth/new_code.bin", 100, 2)) < 0) { - err("Error downloading firmware (%d)", result); + dev_err(&intf->dev, "Error downloading firmware (%d)\n", result); goto err_fw; } @@ -1046,7 +1048,7 @@ static int kaweth_probe( "kaweth/new_code_fix.bin", 100, 3)) < 0) { - err("Error downloading firmware fix (%d)", result); + dev_err(&intf->dev, "Error downloading firmware fix (%d)\n", result); goto err_fw; } @@ -1054,7 +1056,7 @@ static int kaweth_probe( "kaweth/trigger_code.bin", 126, 2)) < 0) { - err("Error downloading trigger code (%d)", result); + dev_err(&intf->dev, "Error downloading trigger code (%d)\n", result); goto err_fw; } @@ -1063,13 +1065,13 @@ static int kaweth_probe( "kaweth/trigger_code_fix.bin", 126, 3)) < 0) { - err("Error downloading trigger code fix (%d)", result); + dev_err(&intf->dev, "Error downloading trigger code fix (%d)\n", result); goto err_fw; } if ((result = kaweth_trigger_firmware(kaweth, 126)) < 0) { - err("Error triggering firmware (%d)", result); + dev_err(&intf->dev, "Error triggering firmware (%d)\n", result); goto err_fw; } @@ -1084,7 +1086,7 @@ err_fw: result = kaweth_read_configuration(kaweth); if(result < 0) { - err("Error reading configuration (%d), no net device created", result); + dev_err(&intf->dev, "Error reading configuration (%d), no net device created\n", result); goto err_free_netdev; } @@ -1102,7 +1104,7 @@ err_fw: if(!memcmp(&kaweth->configuration.hw_addr, &bcast_addr, sizeof(bcast_addr))) { - err("Firmware not functioning properly, no net device created"); + dev_err(&intf->dev, "Firmware not functioning properly, no net device created\n"); goto err_free_netdev; } @@ -1112,7 +1114,7 @@ err_fw: } if(kaweth_set_sofs_wait(kaweth, KAWETH_SOFS_TO_WAIT) < 0) { - err("Error setting SOFS wait"); + dev_err(&intf->dev, "Error setting SOFS wait\n"); goto err_free_netdev; } @@ -1122,7 +1124,7 @@ err_fw: KAWETH_PACKET_FILTER_MULTICAST); if(result < 0) { - err("Error setting receive filter"); + dev_err(&intf->dev, "Error setting receive filter\n"); goto err_free_netdev; } @@ -1174,7 +1176,7 @@ err_fw: SET_NETDEV_DEV(netdev, &intf->dev); if (register_netdev(netdev) != 0) { - err("Error registering netdev."); + dev_err(&intf->dev, "Error registering netdev.\n"); goto err_intfdata; } diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c index d8664bf..ae19675 100644 --- a/drivers/net/usb/rtl8150.c +++ b/drivers/net/usb/rtl8150.c @@ -247,7 +247,8 @@ static int async_set_registers(rtl8150_t * dev, u16 indx, u16 size) if ((ret = usb_submit_urb(dev->ctrl_urb, GFP_ATOMIC))) { if (ret == -ENODEV) netif_device_detach(dev->netdev); - err("control request submission failed: %d", ret); + dev_err(&dev->udev->dev, + "control request submission failed: %d\n", ret); } else set_bit(RX_REG_SET, &dev->flags); @@ -599,7 +600,8 @@ resubmit: if (res == -ENODEV) netif_device_detach(dev->netdev); else if (res) - err ("can't resubmit intr, %s-%s/input0, status %d", + dev_err(&dev->udev->dev, + "can't resubmit intr, %s-%s/input0, status %d\n", dev->udev->bus->bus_name, dev->udev->devpath, res); } @@ -906,7 +908,7 @@ static int rtl8150_probe(struct usb_interface *intf, netdev = alloc_etherdev(sizeof(rtl8150_t)); if (!netdev) { - err("Out of memory"); + dev_err(&intf->dev, "Out of memory\n"); return -ENOMEM; } @@ -936,11 +938,11 @@ static int rtl8150_probe(struct usb_interface *intf, dev->intr_interval = 100; /* 100ms */ if (!alloc_all_urbs(dev)) { - err("out of memory"); + dev_err(&intf->dev, "out of memory\n"); goto out; } if (!rtl8150_reset(dev)) { - err("couldn't reset the device"); + dev_err(&intf->dev, "couldn't reset the device\n"); goto out1; } fill_skb_pool(dev); @@ -949,7 +951,7 @@ static int rtl8150_probe(struct usb_interface *intf, usb_set_intfdata(intf, dev); SET_NETDEV_DEV(netdev, &intf->dev); if (register_netdev(netdev) != 0) { - err("couldn't register the device"); + dev_err(&intf->dev, "couldn't register the device\n"); goto out2; }
Remove err-messages and place dev_err instead in catc.c, kaweth.c and rtl8150.c. Signed-off-by: Alexey Klimov <klimov.linux@gmail.com> --