linux-kernel - Re: [PATCH v4 10/13] USB: ch341: clean up messages

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160429134012.GN22229@localhost>
Date:	Fri, 29 Apr 2016 15:40:12 +0200
From:	Johan Hovold <johan@...nel.org>
To:	Grigori Goronzy <greg@...wn.ath.cx>
Cc:	Johan Hovold <johan@...nel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 10/13] USB: ch341: clean up messages

On Fri, Apr 15, 2016 at 11:14:13PM +0200, Grigori Goronzy wrote:
> No functional change.  Remove explicit function name printing, it's
> easy to use dynamic debug to print it every time, if required.

While that is true, we currently use __func__ in a lot of debug messages
as a compact form for a self-contained message (e.g. "%s - x=y\n",
__func__) and that should be ok.

> Fix capitalization and phrasing in some cases.

No need to capitalise either. I'd even prefer sticking to lower-case
consistently.

> Drop useless
> information like a USB buffer pointer, which is not helpful.
> 
> Signed-off-by: Grigori Goronzy <greg@...wn.ath.cx>
> ---
>  drivers/usb/serial/ch341.c | 48 +++++++++++++++++++++-------------------------
>  1 file changed, 22 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index f524aa9..22cfd88 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -106,7 +106,7 @@ static int ch341_control_out(struct usb_device *dev, u8 request,
>  {
>  	int r;
>  
> -	dev_dbg(&dev->dev, "ch341_control_out(%02x,%02x,%04x,%04x)\n",
> +	dev_dbg(&dev->dev, "control_out(%02x,%02x,%04x,%04x)\n",

Drop this one or do use __func__ here.

>  		USB_DIR_OUT|0x40, (int)request, (int)value, (int)index);
>  
>  	r = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), request,
> @@ -122,8 +122,8 @@ static int ch341_control_in(struct usb_device *dev,
>  {
>  	int r;
>  
> -	dev_dbg(&dev->dev, "ch341_control_in(%02x,%02x,%04x,%04x,%p,%u)\n",
> -		USB_DIR_IN|0x40, (int)request, (int)value, (int)index, buf,
> +	dev_dbg(&dev->dev, "control_in(%02x,%02x,%04x,%04x,%u)\n",
> +		USB_DIR_IN|0x40, (int)request, (int)value, (int)index,

Ditto.

>  		(int)bufsize);
>  
>  	r = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0), request,
> @@ -327,11 +327,11 @@ static int ch341_open(struct tty_struct *tty, struct usb_serial_port *port)
>  	if (tty)
>  		ch341_set_termios(tty, port, NULL);
>  
> -	dev_dbg(&port->dev, "%s - submitting interrupt urb\n", __func__);
> +	dev_dbg(&port->dev, "Submitting interrupt URB\n");

Drop.

>  	r = usb_submit_urb(port->interrupt_in_urb, GFP_KERNEL);
>  	if (r) {
> -		dev_err(&port->dev, "%s - failed to submit interrupt urb: %d\n",
> -			__func__, r);
> +		dev_err(&port->dev,
> +			"Failed to submit interrupt URB: %d\n", r);

Error message, so I agree that this should be spelled out, but not need
to capitalise "failed".

>  		goto out;
>  	}
>  
> @@ -409,8 +409,7 @@ static void ch341_set_termios(struct tty_struct *tty,
>  				CH341_REG_RTSCTS | ((uint16_t)CH341_REG_RTSCTS << 8),
>  				0x0101);
>  		if (r < 0) {
> -			dev_err(&port->dev, "%s - USB control write error (%d)\n",
> -					__func__, r);
> +			dev_err(&port->dev, "USB control write error: %d\n", r);

Please say what went wrong instead of what function failed. (I think I
already commented on this one when you added it, fix it at the source).

>  			tty->termios.c_cflag &= ~CRTSCTS;
>  		}
>  	}
> @@ -432,29 +431,27 @@ static void ch341_break_ctl(struct tty_struct *tty, int break_state)
>  	r = ch341_control_in(port->serial->dev, CH341_REQ_READ_REG,
>  			ch341_break_reg, 0, break_reg, 2);
>  	if (r < 0) {
> -		dev_err(&port->dev, "%s - USB control read error (%d)\n",
> -				__func__, r);
> +		dev_err(&port->dev, "USB control read error: %d\n", r);

"failed to read break status: %d\n", or similar.

>  		goto out;
>  	}
> -	dev_dbg(&port->dev, "%s - initial ch341 break register contents - reg1: %x, reg2: %x\n",
> -		__func__, break_reg[0], break_reg[1]);
> +	dev_dbg(&port->dev, "Initial break register contents - reg1: %x, reg2: %x\n",
> +		break_reg[0], break_reg[1]);

__func__ is fine for compact debug messages, but you can remove the
verbose text if you want. I'd prefer the form

	"%s - x = a, y = b\n"

>  	if (break_state != 0) {
> -		dev_dbg(&port->dev, "%s - Enter break state requested\n", __func__);
> +		dev_dbg(&port->dev, "Enter break state requested\n");
>  		break_reg[0] &= ~CH341_NBREAK_BITS_REG1;
>  		break_reg[1] &= ~CH341_LCR_ENABLE_TX;
>  	} else {
> -		dev_dbg(&port->dev, "%s - Leave break state requested\n", __func__);
> +		dev_dbg(&port->dev, "Leave break state requested\n");
>  		break_reg[0] |= CH341_NBREAK_BITS_REG1;
>  		break_reg[1] |= CH341_LCR_ENABLE_TX;
>  	}

A common "%s - break = %d\n" would do instead of the two above.

> -	dev_dbg(&port->dev, "%s - New ch341 break register contents - reg1: %x, reg2: %x\n",
> -		__func__, break_reg[0], break_reg[1]);
> +	dev_dbg(&port->dev, "New break register contents - reg1: %x, reg2: %x\n",
> +		break_reg[0], break_reg[1]);

I think you get the idea, so won't comment on the rest.

Thanks,
Johan