linux-kernel - Re: [PATCH v10 2/6] usb:common Separated decoding functions from dwc3 driver.

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]

Date:   Sun, 21 Jul 2019 12:05:37 -0700
From:   Joe Perches <joe@...ches.com>
To:     Pawel Laszczak <pawell@...ence.com>, felipe.balbi@...ux.intel.com
Cc:     gregkh@...uxfoundation.org, linux-usb@...r.kernel.org,
        rogerq@...com, linux-kernel@...r.kernel.org, jbergsagel@...com,
        nsekhar@...com, nm@...com, sureshp@...ence.com, jpawar@...ence.com,
        kurahul@...ence.com, aniljoy@...ence.com
Subject: Re: [PATCH v10 2/6] usb:common Separated decoding functions from
 dwc3 driver.

On Sun, 2019-07-21 at 19:32 +0100, Pawel Laszczak wrote:
> Patch moves some decoding functions from driver/usb/dwc3/debug.h driver
> to driver/usb/common/debug.c file. These moved functions include:
[]
> diff --git a/drivers/usb/common/debug.c b/drivers/usb/common/debug.c
[]
> +static void usb_decode_set_clear_feature(__u8 bRequestType, __u8 bRequest,
> +					 __u16 wValue, __u16 wIndex,
> +					 char *str, size_t size)

It's probably not necessary to use Hungarian
when moving these functions into generic code.

> +{
> +	switch (bRequestType & USB_RECIP_MASK) {
> +	case USB_RECIP_DEVICE:
> +		snprintf(str, size, "%s Device Feature(%s%s)",
> +			 bRequest == USB_REQ_CLEAR_FEATURE ? "Clear" : "Set",
> +			 ({char *s;
> +				switch (wValue) {
> +				case USB_DEVICE_SELF_POWERED:
> +					s = "Self Powered";
> +					break;
> +				case USB_DEVICE_REMOTE_WAKEUP:
> +					s = "Remote Wakeup";
> +					break;
> +				case USB_DEVICE_TEST_MODE:
> +					s = "Test Mode";
> +					break;
> +				case USB_DEVICE_U1_ENABLE:
> +					s = "U1 Enable";
> +					break;
> +				case USB_DEVICE_U2_ENABLE:
> +					s = "U2 Enable";
> +					break;
> +				case USB_DEVICE_LTM_ENABLE:
> +					s = "LTM Enable";
> +					break;
> +				default:
> +					s = "UNKNOWN";
> +				} s; }),
> +			 wValue == USB_DEVICE_TEST_MODE ?
> +			 ({ char *s;
> +				switch (wIndex) {
> +				case TEST_J:
> +					s = ": TEST_J";
> +					break;
> +				case TEST_K:
> +					s = ": TEST_K";
> +					break;
> +				case TEST_SE0_NAK:
> +					s = ": TEST_SE0_NAK";
> +					break;
> +				case TEST_PACKET:
> +					s = ": TEST_PACKET";
> +					break;
> +				case TEST_FORCE_EN:
> +					s = ": TEST_FORCE_EN";
> +					break;
> +				default:
> +					s = ": UNKNOWN";
> +				} s; }) : "");

I always find this sort of embedded switch/case char *
statement expressions difficult to read and think it's
better to use separate lookup functions.

I would much prefer something like:

static const char *usb_device_mode_desc(u16 mode)
{
	switch (mode) {
	case USB_DEVICE_SELF_POWERED:
		return "Self Powered";
	case USB_DEVICE_REMOTE_WAKEUP:
		return "Remote Wakeup";
	case USB_DEVICE_TEST_MODE:
		return "Test Mode";
	case USB_DEVICE_U1_ENABLE:
		return "U1 Enable";
	case USB_DEVICE_U2_ENABLE:
		return "U2 Enable";
	case USB_DEVICE_LTM_ENABLE:
		return "LTM Enable";
	}
	return "UNKNOWN";
}

		snprintf(str, size, "%s Device Feature(%s%s)",
> 			 bRequest == USB_REQ_CLEAR_FEATURE ? "Clear" : "Set",
			 usb_device_mode_desc(wValue),
			 etc...);


etc...