* [PATCH] usb: make printk messges more searchable
@ 2008-12-10 7:29 Wu Fengguang
2008-12-10 9:42 ` Laurent Pinchart
0 siblings, 1 reply; 6+ messages in thread
From: Wu Fengguang @ 2008-12-10 7:29 UTC (permalink / raw)
To: LKML; +Cc: Greg Kroah-Hartman, linux-usb
Make USB printk messages long and straightforward. One of these
decorated USB error messages cost me some smart efforts to locate.
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
---
drivers/usb/core/hub.c | 89 ++++++++++++++++++---------------------
1 file changed, 43 insertions(+), 46 deletions(-)
--- linux-2.6.orig/drivers/usb/core/hub.c
+++ linux-2.6/drivers/usb/core/hub.c
@@ -107,7 +107,8 @@ MODULE_PARM_DESC (blinkenlights, "true t
/* define initial 64-byte descriptor request timeout in milliseconds */
static int initial_descriptor_timeout = USB_CTRL_GET_TIMEOUT;
module_param(initial_descriptor_timeout, int, S_IRUGO|S_IWUSR);
-MODULE_PARM_DESC(initial_descriptor_timeout, "initial 64-byte descriptor request timeout in milliseconds (default 5000 - 5.0 seconds)");
+MODULE_PARM_DESC(initial_descriptor_timeout,
+ "initial 64-byte descriptor request timeout in milliseconds (default 5000 - 5.0 seconds)");
/*
* As of 2.6.10 we introduce a new USB device initialization scheme which
@@ -131,8 +132,7 @@ MODULE_PARM_DESC(old_scheme_first,
static int use_both_schemes = 1;
module_param(use_both_schemes, bool, S_IRUGO | S_IWUSR);
MODULE_PARM_DESC(use_both_schemes,
- "try the other device initialization scheme if the "
- "first one fails");
+ "try the other device initialization scheme if the first one fails");
/* Mutual exclusion for EHCI CF initialization. This interferes with
* port reset on some companion controllers.
@@ -545,8 +545,7 @@ static unsigned hub_power_on(struct usb_
if ((wHubCharacteristics & HUB_CHAR_LPSM) < 2)
dev_dbg(hub->intfdev, "enabling power on all ports\n");
else
- dev_dbg(hub->intfdev, "trying to enable port power on "
- "non-switchable hub\n");
+ dev_dbg(hub->intfdev, "trying to enable port power on non-switchable hub\n");
for (port1 = 1; port1 <= hub->descriptor->bNbrPorts; port1++)
set_port_feature(hub->hdev, port1, USB_PORT_FEAT_POWER);
@@ -1020,8 +1019,7 @@ static int hub_configure(struct usb_hub
if (remaining < hdev->maxchild * 100)
dev_warn(hub_dev,
- "insufficient power available "
- "to use all downstream ports\n");
+ "insufficient power available to use all downstream ports\n");
hub->mA_per_port = 100; /* 7.2.1.1 */
}
} else { /* Self-powered external hub */
@@ -1136,8 +1134,8 @@ static int hub_probe(struct usb_interfac
hdev = interface_to_usbdev(intf);
if (hdev->level == MAX_TOPO_LEVEL) {
- dev_err(&intf->dev, "Unsupported bus topology: "
- "hub nested too deep\n");
+ dev_err(&intf->dev,
+ "Unsupported bus topology: hub nested too deep\n");
return -E2BIG;
}
@@ -1476,8 +1474,8 @@ static void announce_device(struct usb_d
dev_info(&udev->dev, "New USB device found, idVendor=%04x, idProduct=%04x\n",
le16_to_cpu(udev->descriptor.idVendor),
le16_to_cpu(udev->descriptor.idProduct));
- dev_info(&udev->dev, "New USB device strings: Mfr=%d, Product=%d, "
- "SerialNumber=%d\n",
+ dev_info(&udev->dev,
+ "New USB device strings: Mfr=%d, Product=%d, SerialNumber=%d\n",
udev->descriptor.iManufacturer,
udev->descriptor.iProduct,
udev->descriptor.iSerialNumber);
@@ -1542,7 +1540,7 @@ static int usb_configure_device_otg(stru
* customize to match your product.
*/
dev_info(&udev->dev,
- "can't set HNP mode; %d\n",
+ "can't set HNP mode: %d\n",
err);
bus->b_hnp_enable = 0;
}
@@ -1723,8 +1721,9 @@ int usb_authorize_device(struct usb_devi
}
result = usb_get_device_descriptor(usb_dev, sizeof(usb_dev->descriptor));
if (result < 0) {
- dev_err(&usb_dev->dev, "can't re-read device descriptor for "
- "authorization: %d\n", result);
+ dev_err(&usb_dev->dev,
+ "can't re-read device descriptor for authorization: %d\n",
+ result);
goto error_device_descriptor;
}
usb_dev->authorized = 1;
@@ -2020,8 +2019,8 @@ int usb_port_suspend(struct usb_device *
USB_CTRL_SET_TIMEOUT);
} else {
/* device has up to 10 msec to fully suspend */
- dev_dbg(&udev->dev, "usb %ssuspend\n",
- udev->auto_pm ? "auto-" : "");
+ dev_dbg(&udev->dev, "%s\n",
+ udev->auto_pm ? "usb auto-suspend" : "usb suspend");
usb_set_device_state(udev, USB_STATE_SUSPENDED);
msleep(10);
}
@@ -2045,8 +2044,8 @@ static int finish_port_resume(struct usb
u16 devstatus;
/* caller owns the udev device lock */
- dev_dbg(&udev->dev, "finish %sresume\n",
- udev->reset_resume ? "reset-" : "");
+ dev_dbg(&udev->dev, "%s\n",
+ udev->reset_resume ? "finish reset-resume" : "finish resume");
/* usb ch9 identifies four variants of SUSPENDED, based on what
* state the device resumes to. Linux currently won't see the
@@ -2098,8 +2097,9 @@ static int finish_port_resume(struct usb
NULL, 0,
USB_CTRL_SET_TIMEOUT);
if (status)
- dev_dbg(&udev->dev, "disable remote "
- "wakeup, status %d\n", status);
+ dev_dbg(&udev->dev,
+ "disable remote wakeup, status %d\n",
+ status);
}
status = 0;
}
@@ -2582,9 +2582,9 @@ hub_port_init (struct usb_hub *hub, stru
goto fail;
}
if (r) {
- dev_err(&udev->dev, "device descriptor "
- "read/%s, error %d\n",
- "64", r);
+ dev_err(&udev->dev,
+ "device descriptor read/64, error %d\n",
+ r);
retval = -EMSGSIZE;
continue;
}
@@ -2621,9 +2621,9 @@ hub_port_init (struct usb_hub *hub, stru
retval = usb_get_device_descriptor(udev, 8);
if (retval < 8) {
- dev_err(&udev->dev, "device descriptor "
- "read/%s, error %d\n",
- "8", retval);
+ dev_err(&udev->dev,
+ "device descriptor read/8, error %d\n",
+ retval);
if (retval >= 0)
retval = -EMSGSIZE;
} else {
@@ -2650,8 +2650,8 @@ hub_port_init (struct usb_hub *hub, stru
retval = usb_get_device_descriptor(udev, USB_DT_DEVICE_SIZE);
if (retval < (signed)sizeof(udev->descriptor)) {
- dev_err(&udev->dev, "device descriptor read/%s, error %d\n",
- "all", retval);
+ dev_err(&udev->dev, "device descriptor read/all, error %d\n",
+ retval);
if (retval >= 0)
retval = -ENOMSG;
goto fail;
@@ -2681,8 +2681,8 @@ check_highspeed (struct usb_hub *hub, st
status = usb_get_descriptor (udev, USB_DT_DEVICE_QUALIFIER, 0,
qual, sizeof *qual);
if (status == sizeof *qual) {
- dev_info(&udev->dev, "not running at top speed; "
- "connect to a high speed hub\n");
+ dev_info(&udev->dev,
+ "not running at top speed; connect to a high speed hub\n");
/* hub LEDs are probably harder to miss than syslog */
if (hub->has_indicators) {
hub->indicator[port1-1] = INDICATOR_GREEN_BLINK;
@@ -2719,9 +2719,9 @@ hub_power_remaining (struct usb_hub *hub
else
delta = 8;
if (delta > hub->mA_per_port)
- dev_warn(&udev->dev, "%dmA is over %umA budget "
- "for port %d!\n",
- delta, hub->mA_per_port, port1);
+ dev_warn(&udev->dev,
+ "%dmA is over %umA budget for port %d!\n",
+ delta, hub->mA_per_port, port1);
remaining -= delta;
}
if (remaining < 0) {
@@ -2812,8 +2812,9 @@ static void hub_port_connect_change(stru
status = hub_port_debounce(hub, port1);
if (status < 0) {
if (printk_ratelimit())
- dev_err(hub_dev, "connect-debounce failed, "
- "port %d disabled\n", port1);
+ dev_err(hub_dev,
+ "connect-debounce failed, port %d disabled\n",
+ port1);
portstatus &= ~USB_PORT_STAT_CONNECTION;
} else {
portstatus = status;
@@ -2883,8 +2884,7 @@ static void hub_port_connect_change(stru
le16_to_cpus(&devstat);
if ((devstat & (1 << USB_DEVICE_SELF_POWERED)) == 0) {
dev_err(&udev->dev,
- "can't connect bus-powered hub "
- "to this port\n");
+ "can't connect bus-powered hub to this port\n");
if (hub->has_indicators) {
hub->indicator[port1-1] =
INDICATOR_AMBER_BLINK;
@@ -3067,9 +3067,8 @@ static void hub_events(void)
if (portchange & USB_PORT_STAT_C_ENABLE) {
if (!connect_change)
dev_dbg (hub_dev,
- "port %d enable change, "
- "status %08x\n",
- i, portstatus);
+ "port %d enable change, status %08x\n",
+ i, portstatus);
clear_port_feature(hdev, i,
USB_PORT_FEAT_C_ENABLE);
@@ -3083,10 +3082,8 @@ static void hub_events(void)
&& !connect_change
&& hdev->children[i-1]) {
dev_err (hub_dev,
- "port %i "
- "disabled by hub (EMI?), "
- "re-enabling...\n",
- i);
+ "port %i disabled by hub (EMI?), re-enabling...\n",
+ i);
connect_change = 1;
}
}
@@ -3420,8 +3417,8 @@ static int usb_reset_and_verify_device(s
ret = usb_set_interface(udev, desc->bInterfaceNumber,
desc->bAlternateSetting);
if (ret < 0) {
- dev_err(&udev->dev, "failed to restore interface %d "
- "altsetting %d (error=%d)\n",
+ dev_err(&udev->dev,
+ "failed to restore interface %d altsetting %d (error=%d)\n",
desc->bInterfaceNumber,
desc->bAlternateSetting,
ret);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: make printk messges more searchable
2008-12-10 7:29 [PATCH] usb: make printk messges more searchable Wu Fengguang
@ 2008-12-10 9:42 ` Laurent Pinchart
2008-12-10 9:56 ` Oliver Neukum
0 siblings, 1 reply; 6+ messages in thread
From: Laurent Pinchart @ 2008-12-10 9:42 UTC (permalink / raw)
To: Wu Fengguang; +Cc: LKML, Greg Kroah-Hartman, linux-usb
Hi Wu,
On Wednesday 10 December 2008, Wu Fengguang wrote:
> Make USB printk messages long and straightforward. One of these
> decorated USB error messages cost me some smart efforts to locate.
That would make the code break the 80 columns limit.
>From "Documentation/CodingStyle":
"The limit on the length of lines is 80 columns and this is a strongly
preferred limit."
Best regards,
Laurent Pinchart
> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
> ---
> drivers/usb/core/hub.c | 89 ++++++++++++++++++---------------------
> 1 file changed, 43 insertions(+), 46 deletions(-)
>
> --- linux-2.6.orig/drivers/usb/core/hub.c
> +++ linux-2.6/drivers/usb/core/hub.c
> @@ -107,7 +107,8 @@ MODULE_PARM_DESC (blinkenlights, "true t
> /* define initial 64-byte descriptor request timeout in milliseconds */
> static int initial_descriptor_timeout = USB_CTRL_GET_TIMEOUT;
> module_param(initial_descriptor_timeout, int, S_IRUGO|S_IWUSR);
> -MODULE_PARM_DESC(initial_descriptor_timeout, "initial 64-byte descriptor
> request timeout in milliseconds (default 5000 - 5.0 seconds)");
> +MODULE_PARM_DESC(initial_descriptor_timeout,
> + "initial 64-byte descriptor request timeout in milliseconds (default
> 5000 - 5.0 seconds)");
>
> /*
> * As of 2.6.10 we introduce a new USB device initialization scheme which
> @@ -131,8 +132,7 @@ MODULE_PARM_DESC(old_scheme_first,
> static int use_both_schemes = 1;
> module_param(use_both_schemes, bool, S_IRUGO | S_IWUSR);
> MODULE_PARM_DESC(use_both_schemes,
> - "try the other device initialization scheme if the "
> - "first one fails");
> + "try the other device initialization scheme if the first one fails");
>
> /* Mutual exclusion for EHCI CF initialization. This interferes with
> * port reset on some companion controllers.
> @@ -545,8 +545,7 @@ static unsigned hub_power_on(struct usb_
> if ((wHubCharacteristics & HUB_CHAR_LPSM) < 2)
> dev_dbg(hub->intfdev, "enabling power on all ports\n");
> else
> - dev_dbg(hub->intfdev, "trying to enable port power on "
> - "non-switchable hub\n");
> + dev_dbg(hub->intfdev, "trying to enable port power on non-switchable
> hub\n"); for (port1 = 1; port1 <= hub->descriptor->bNbrPorts; port1++)
> set_port_feature(hub->hdev, port1, USB_PORT_FEAT_POWER);
>
> @@ -1020,8 +1019,7 @@ static int hub_configure(struct usb_hub
>
> if (remaining < hdev->maxchild * 100)
> dev_warn(hub_dev,
> - "insufficient power available "
> - "to use all downstream ports\n");
> + "insufficient power available to use all downstream ports\n");
> hub->mA_per_port = 100; /* 7.2.1.1 */
> }
> } else { /* Self-powered external hub */
> @@ -1136,8 +1134,8 @@ static int hub_probe(struct usb_interfac
> hdev = interface_to_usbdev(intf);
>
> if (hdev->level == MAX_TOPO_LEVEL) {
> - dev_err(&intf->dev, "Unsupported bus topology: "
> - "hub nested too deep\n");
> + dev_err(&intf->dev,
> + "Unsupported bus topology: hub nested too deep\n");
> return -E2BIG;
> }
>
> @@ -1476,8 +1474,8 @@ static void announce_device(struct usb_d
> dev_info(&udev->dev, "New USB device found, idVendor=%04x,
> idProduct=%04x\n", le16_to_cpu(udev->descriptor.idVendor),
> le16_to_cpu(udev->descriptor.idProduct));
> - dev_info(&udev->dev, "New USB device strings: Mfr=%d, Product=%d, "
> - "SerialNumber=%d\n",
> + dev_info(&udev->dev,
> + "New USB device strings: Mfr=%d, Product=%d, SerialNumber=%d\n",
> udev->descriptor.iManufacturer,
> udev->descriptor.iProduct,
> udev->descriptor.iSerialNumber);
> @@ -1542,7 +1540,7 @@ static int usb_configure_device_otg(stru
> * customize to match your product.
> */
> dev_info(&udev->dev,
> - "can't set HNP mode; %d\n",
> + "can't set HNP mode: %d\n",
> err);
> bus->b_hnp_enable = 0;
> }
> @@ -1723,8 +1721,9 @@ int usb_authorize_device(struct usb_devi
> }
> result = usb_get_device_descriptor(usb_dev, sizeof(usb_dev->descriptor));
> if (result < 0) {
> - dev_err(&usb_dev->dev, "can't re-read device descriptor for "
> - "authorization: %d\n", result);
> + dev_err(&usb_dev->dev,
> + "can't re-read device descriptor for authorization: %d\n",
> + result);
> goto error_device_descriptor;
> }
> usb_dev->authorized = 1;
> @@ -2020,8 +2019,8 @@ int usb_port_suspend(struct usb_device *
> USB_CTRL_SET_TIMEOUT);
> } else {
> /* device has up to 10 msec to fully suspend */
> - dev_dbg(&udev->dev, "usb %ssuspend\n",
> - udev->auto_pm ? "auto-" : "");
> + dev_dbg(&udev->dev, "%s\n",
> + udev->auto_pm ? "usb auto-suspend" : "usb suspend");
> usb_set_device_state(udev, USB_STATE_SUSPENDED);
> msleep(10);
> }
> @@ -2045,8 +2044,8 @@ static int finish_port_resume(struct usb
> u16 devstatus;
>
> /* caller owns the udev device lock */
> - dev_dbg(&udev->dev, "finish %sresume\n",
> - udev->reset_resume ? "reset-" : "");
> + dev_dbg(&udev->dev, "%s\n",
> + udev->reset_resume ? "finish reset-resume" : "finish resume");
>
> /* usb ch9 identifies four variants of SUSPENDED, based on what
> * state the device resumes to. Linux currently won't see the
> @@ -2098,8 +2097,9 @@ static int finish_port_resume(struct usb
> NULL, 0,
> USB_CTRL_SET_TIMEOUT);
> if (status)
> - dev_dbg(&udev->dev, "disable remote "
> - "wakeup, status %d\n", status);
> + dev_dbg(&udev->dev,
> + "disable remote wakeup, status %d\n",
> + status);
> }
> status = 0;
> }
> @@ -2582,9 +2582,9 @@ hub_port_init (struct usb_hub *hub, stru
> goto fail;
> }
> if (r) {
> - dev_err(&udev->dev, "device descriptor "
> - "read/%s, error %d\n",
> - "64", r);
> + dev_err(&udev->dev,
> + "device descriptor read/64, error %d\n",
> + r);
> retval = -EMSGSIZE;
> continue;
> }
> @@ -2621,9 +2621,9 @@ hub_port_init (struct usb_hub *hub, stru
>
> retval = usb_get_device_descriptor(udev, 8);
> if (retval < 8) {
> - dev_err(&udev->dev, "device descriptor "
> - "read/%s, error %d\n",
> - "8", retval);
> + dev_err(&udev->dev,
> + "device descriptor read/8, error %d\n",
> + retval);
> if (retval >= 0)
> retval = -EMSGSIZE;
> } else {
> @@ -2650,8 +2650,8 @@ hub_port_init (struct usb_hub *hub, stru
>
> retval = usb_get_device_descriptor(udev, USB_DT_DEVICE_SIZE);
> if (retval < (signed)sizeof(udev->descriptor)) {
> - dev_err(&udev->dev, "device descriptor read/%s, error %d\n",
> - "all", retval);
> + dev_err(&udev->dev, "device descriptor read/all, error %d\n",
> + retval);
> if (retval >= 0)
> retval = -ENOMSG;
> goto fail;
> @@ -2681,8 +2681,8 @@ check_highspeed (struct usb_hub *hub, st
> status = usb_get_descriptor (udev, USB_DT_DEVICE_QUALIFIER, 0,
> qual, sizeof *qual);
> if (status == sizeof *qual) {
> - dev_info(&udev->dev, "not running at top speed; "
> - "connect to a high speed hub\n");
> + dev_info(&udev->dev,
> + "not running at top speed; connect to a high speed hub\n");
> /* hub LEDs are probably harder to miss than syslog */
> if (hub->has_indicators) {
> hub->indicator[port1-1] = INDICATOR_GREEN_BLINK;
> @@ -2719,9 +2719,9 @@ hub_power_remaining (struct usb_hub *hub
> else
> delta = 8;
> if (delta > hub->mA_per_port)
> - dev_warn(&udev->dev, "%dmA is over %umA budget "
> - "for port %d!\n",
> - delta, hub->mA_per_port, port1);
> + dev_warn(&udev->dev,
> + "%dmA is over %umA budget for port %d!\n",
> + delta, hub->mA_per_port, port1);
> remaining -= delta;
> }
> if (remaining < 0) {
> @@ -2812,8 +2812,9 @@ static void hub_port_connect_change(stru
> status = hub_port_debounce(hub, port1);
> if (status < 0) {
> if (printk_ratelimit())
> - dev_err(hub_dev, "connect-debounce failed, "
> - "port %d disabled\n", port1);
> + dev_err(hub_dev,
> + "connect-debounce failed, port %d disabled\n",
> + port1);
> portstatus &= ~USB_PORT_STAT_CONNECTION;
> } else {
> portstatus = status;
> @@ -2883,8 +2884,7 @@ static void hub_port_connect_change(stru
> le16_to_cpus(&devstat);
> if ((devstat & (1 << USB_DEVICE_SELF_POWERED)) == 0) {
> dev_err(&udev->dev,
> - "can't connect bus-powered hub "
> - "to this port\n");
> + "can't connect bus-powered hub to this port\n");
> if (hub->has_indicators) {
> hub->indicator[port1-1] =
> INDICATOR_AMBER_BLINK;
> @@ -3067,9 +3067,8 @@ static void hub_events(void)
> if (portchange & USB_PORT_STAT_C_ENABLE) {
> if (!connect_change)
> dev_dbg (hub_dev,
> - "port %d enable change, "
> - "status %08x\n",
> - i, portstatus);
> + "port %d enable change, status %08x\n",
> + i, portstatus);
> clear_port_feature(hdev, i,
> USB_PORT_FEAT_C_ENABLE);
>
> @@ -3083,10 +3082,8 @@ static void hub_events(void)
> && !connect_change
> && hdev->children[i-1]) {
> dev_err (hub_dev,
> - "port %i "
> - "disabled by hub (EMI?), "
> - "re-enabling...\n",
> - i);
> + "port %i disabled by hub (EMI?), re-enabling...\n",
> + i);
> connect_change = 1;
> }
> }
> @@ -3420,8 +3417,8 @@ static int usb_reset_and_verify_device(s
> ret = usb_set_interface(udev, desc->bInterfaceNumber,
> desc->bAlternateSetting);
> if (ret < 0) {
> - dev_err(&udev->dev, "failed to restore interface %d "
> - "altsetting %d (error=%d)\n",
> + dev_err(&udev->dev,
> + "failed to restore interface %d altsetting %d (error=%d)\n",
> desc->bInterfaceNumber,
> desc->bAlternateSetting,
> ret);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: make printk messges more searchable
2008-12-10 9:42 ` Laurent Pinchart
@ 2008-12-10 9:56 ` Oliver Neukum
2008-12-10 9:59 ` Laurent Pinchart
2008-12-10 15:39 ` Alan Stern
0 siblings, 2 replies; 6+ messages in thread
From: Oliver Neukum @ 2008-12-10 9:56 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Wu Fengguang, LKML, Greg Kroah-Hartman, linux-usb
Am Mittwoch, 10. Dezember 2008 10:42:17 schrieb Laurent Pinchart:
> Hi Wu,
>
> On Wednesday 10 December 2008, Wu Fengguang wrote:
> > Make USB printk messages long and straightforward. One of these
> > decorated USB error messages cost me some smart efforts to locate.
>
> That would make the code break the 80 columns limit.
>
> From "Documentation/CodingStyle":
>
> "The limit on the length of lines is 80 columns and this is a strongly
> preferred limit."
Too rigid an application is bad. The kernel must be greppable.
Regards
Oliver
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: make printk messges more searchable
2008-12-10 9:56 ` Oliver Neukum
@ 2008-12-10 9:59 ` Laurent Pinchart
2008-12-10 12:58 ` Wu Fengguang
2008-12-10 15:39 ` Alan Stern
1 sibling, 1 reply; 6+ messages in thread
From: Laurent Pinchart @ 2008-12-10 9:59 UTC (permalink / raw)
To: Oliver Neukum; +Cc: Wu Fengguang, LKML, Greg Kroah-Hartman, linux-usb
On Wednesday 10 December 2008, Oliver Neukum wrote:
> Am Mittwoch, 10. Dezember 2008 10:42:17 schrieb Laurent Pinchart:
> > Hi Wu,
> >
> > On Wednesday 10 December 2008, Wu Fengguang wrote:
> > > Make USB printk messages long and straightforward. One of these
> > > decorated USB error messages cost me some smart efforts to locate.
> >
> > That would make the code break the 80 columns limit.
> >
> > From "Documentation/CodingStyle":
> >
> > "The limit on the length of lines is 80 columns and this is a strongly
> > preferred limit."
>
> Too rigid an application is bad. The kernel must be greppable.
I've also been bitten by split strings when grepping kernel source code. A
cgrep that would unsplit strings before searching them would be nice :-)
Best regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: make printk messges more searchable
2008-12-10 9:59 ` Laurent Pinchart
@ 2008-12-10 12:58 ` Wu Fengguang
0 siblings, 0 replies; 6+ messages in thread
From: Wu Fengguang @ 2008-12-10 12:58 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Oliver Neukum, LKML, Greg Kroah-Hartman,
linux-usb@vger.kernel.org
On Wed, Dec 10, 2008 at 11:59:00AM +0200, Laurent Pinchart wrote:
> On Wednesday 10 December 2008, Oliver Neukum wrote:
> > Am Mittwoch, 10. Dezember 2008 10:42:17 schrieb Laurent Pinchart:
> > > Hi Wu,
> > >
> > > On Wednesday 10 December 2008, Wu Fengguang wrote:
> > > > Make USB printk messages long and straightforward. One of these
> > > > decorated USB error messages cost me some smart efforts to locate.
> > >
> > > That would make the code break the 80 columns limit.
> > >
> > > From "Documentation/CodingStyle":
> > >
> > > "The limit on the length of lines is 80 columns and this is a strongly
> > > preferred limit."
> >
> > Too rigid an application is bad. The kernel must be greppable.
>
> I've also been bitten by split strings when grepping kernel source code. A
> cgrep that would unsplit strings before searching them would be nice :-)
Such printk will sure bite more people. And it's amazing that the CodingStyle
document uses printk as an 80-column example:
80 The limit on the length of lines is 80 columns and this is a strongly
81 preferred limit.
82
83 Statements longer than 80 columns will be broken into sensible chunks.
84 Descendants are always substantially shorter than the parent and are placed
85 substantially to the right. The same applies to function headers with a long
86 argument list. Long strings are as well broken into shorter strings. The
87 only exception to this is where exceeding 80 columns significantly increases
88 readability and does not hide information.
89
90 void fun(int a, int b, int c)
91 {
92 if (condition)
93 printk(KERN_WARNING "Warning this is a long printk with "
94 "3 parameters a: %u b: %u "
95 "c: %u \n", a, b, c);
96 else
97 next_statement;
98 }
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: make printk messges more searchable
2008-12-10 9:56 ` Oliver Neukum
2008-12-10 9:59 ` Laurent Pinchart
@ 2008-12-10 15:39 ` Alan Stern
1 sibling, 0 replies; 6+ messages in thread
From: Alan Stern @ 2008-12-10 15:39 UTC (permalink / raw)
To: Oliver Neukum
Cc: Laurent Pinchart, Wu Fengguang, LKML, Greg Kroah-Hartman,
linux-usb
On Wed, 10 Dec 2008, Oliver Neukum wrote:
> Am Mittwoch, 10. Dezember 2008 10:42:17 schrieb Laurent Pinchart:
> > Hi Wu,
> >
> > On Wednesday 10 December 2008, Wu Fengguang wrote:
> > > Make USB printk messages long and straightforward. One of these
> > > decorated USB error messages cost me some smart efforts to locate.
> >
> > That would make the code break the 80 columns limit.
> >
> > From "Documentation/CodingStyle":
> >
> > "The limit on the length of lines is 80 columns and this is a strongly
> > preferred limit."
>
> Too rigid an application is bad. The kernel must be greppable.
The kernel is greppable in either case. It's foolish to rigidly insist
that long strings must not be broken. And don't forget that some
strings are created dynamically (with %s specifiers, for instance).
How are you going to grep for them?
It's also worth pointing out that much of the source code in usbcore
tends to indent continuation lines by two extra tab stops. This isn't
done all the time, admittedly. But deliberately changing the code so
that continuations use only one tab stop feels like a bad idea -- one
extra tab stop looks too much like a nested statement.
Alan Stern
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2008-12-10 15:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-12-10 7:29 [PATCH] usb: make printk messges more searchable Wu Fengguang
2008-12-10 9:42 ` Laurent Pinchart
2008-12-10 9:56 ` Oliver Neukum
2008-12-10 9:59 ` Laurent Pinchart
2008-12-10 12:58 ` Wu Fengguang
2008-12-10 15:39 ` Alan Stern
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox