From: Krzysztof Opasiak <k.opasiak@samsung.com>
To: "'Michal Nazarewicz'" <mina86@mina86.com>, "'Felipe Balbi'" <balbi@ti.com>,
        Krzysztof Opasiak <k.opasiak@samsung.com>
Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
References: <1402045976-26580-1-git-send-email-mina86@mina86.com>
 <1402045976-26580-5-git-send-email-mina86@mina86.com>
In-reply-to: <1402045976-26580-5-git-send-email-mina86@mina86.com>
Subject: RE: [PATCHv2 5/5] tools: ffs-test: add compatibility code for old
 kernels
Date: Mon, 09 Jun 2014 10:25:39 +0200
Message-id: <006601cf83bc$66197b30$324c7190$%opasiak@samsung.com>
MIME-version: 1.0
Content-type: text/plain; charset=UTF-8
Content-transfer-encoding: 8BIT
Thread-index: Ac+BZ4Yw44op+tusSICAHbODtx1YJACUs8WQ
Content-language: en-us
Sender: linux-kernel-owner@vger.kernel.org

Hi Michal,

> -----Original Message-----
> From: Michal Nazarewicz [mailto:mina86@mina86.com]
> Sent: Friday, June 06, 2014 11:13 AM
> To: Felipe Balbi; Krzysztof Opasiak
> Cc: linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; Michal
> Nazarewicz
> Subject: [PATCHv2 5/5] tools: ffs-test: add compatibility code for
> old kernels

(... snip ...)

> +static size_t descs_to_legacy(void **legacy, const void
> *descriptors) {
> +	__u32 length, flags, fs_count = 0, hs_count = 0, count;
> +	const unsigned char *descs = descriptors, *usb_descs;
> +	unsigned char *out;
> +
> +	if (get_unaligned_le32(descs) !=
> FUNCTIONFS_DESCRIPTORS_MAGIC_V2)
> +		return 0;
> +
> +	length = get_unaligned_le32(descs + 4);
> +	if (length < 8)
> +		return 0;
> +	descs += 8;
> +	length -= 8;
> +
> +#define GET_LE32(ret) do {				\
> +		if (length < 4)				\
> +			return 0;			\
> +		ret = get_unaligned_le32(descs);	\
> +		descs += 4;				\
> +		length -= 4;				\
> +	} while (0)
> +
> +	GET_LE32(flags);
> +	if (flags & FUNCTIONFS_HAS_FS_DESC)
> +		GET_LE32(fs_count);
> +	if (flags & FUNCTIONFS_HAS_HS_DESC)
> +		GET_LE32(hs_count);
> +	if (!fs_count && !hs_count)
> +		return 0;
> +	if (flags & FUNCTIONFS_HAS_SS_DESC)
> +		GET_LE32(count);  /* The value is ignored later on
> anyway. */
> +	if (flags)
> +		return 0;

As far as I understand you are taking the flags from descriptor and then test them with possible ffs speed flags, after getting flags your are not assigning anything to this variable so in this place it will be != 0  unless none of the flags has been provided. I don't think this is intended behavior.

> +
> +#undef GET_LE32
> +
> +	usb_descs = descs;
> +	for (count = fs_count + hs_count; count; --count) {
> +		if (length < *descs)
> +			return 0;
> +		length -= *descs;
> +		descs += *descs;
> +	}
> +
> +	length = 16 + (descs - usb_descs);
> +	out = malloc(length);
> +	put_unaligned_le32(FUNCTIONFS_DESCRIPTORS_MAGIC, out);
> +	put_unaligned_le32(length, out + 4);
> +	put_unaligned_le32(fs_count, out + 8);
> +	put_unaligned_le32(hs_count, out + 12);
> +	memcpy(out + 16, usb_descs, length - 16);
> +	*legacy = out;
> +	return length;
> +}
> +

As this is an example which will be copy-paste by many users maybe you should you struct usb_functionfs_descs_head and struct usb_functionfs_descs_head_v2 instead of direct operations using hard-coded offsets to make this function more readable?

--
BR's
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
k.opasiak@samsung.com


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/