dfu:usb: Composite USB download gadget with DFU function

Composite USB download gadget support (g_dnl) for download functions. This code works on top of composite gadget.

Signed-off-by Lukasz Majewski <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

Cc Marek Vasut <[email protected]>

---
 drivers/usb/gadget/Makefile |    1 +
 drivers/usb/gadget/g_dnl.c  |  235
+++++++++++++++++++++++++++++++++++++++++++ include/g_dnl.h             | 
 33 ++++++
 3 files changed, 269 insertions(+), 0 deletions(-)
 create mode 100644 drivers/usb/gadget/g_dnl.c
 create mode 100644 include/g_dnl.h

drivers/usb/gadget/Makefile +1 -0

index 87d1918..2c067c8 100644
@@ -29,6 +29,7 @@ LIB	:= $(obj)libusb_gadget.o
 ifdef CONFIG_USB_GADGET
 COBJS-y += epautoconf.o config.o usbstring.o
 COBJS-$(CONFIG_USB_GADGET_S3C_UDC_OTG) += s3c_udc_otg.o
+COBJS-$(CONFIG_USBDOWNLOAD_GADGET) += g_dnl.o
 endif
 ifdef CONFIG_USB_ETHER
 COBJS-y += ether.o epautoconf.o config.o usbstring.o

drivers/usb/gadget/g_dnl.c new file +19 -0

new file mode 100644
index 0000000..7925d0e
@@ -0,0 +1,235 @@
+/*
+ * g_dnl.c -- USB Downloader Gadget
+ *
+ * Copyright (C) 2012 Samsung Electronics
+ * Lukasz Majewski  <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 

USA + */

drivers/usb/gadget/g_dnl.c new file +10 -0

+
+#include <errno.h>
+#include <common.h>
+#include <malloc.h>
+#include <linux/usb/ch9.h>
+#include <usbdescriptors.h>
+#include <linux/usb/gadget.h>
+
+#include <mmc.h>
+#include <part.h>

drivers/usb/gadget/g_dnl.c new file +42 -0

+
+static struct usb_device_descriptor device_desc = {
+	.bLength =		sizeof device_desc,
+	.bDescriptorType =	USB_DT_DEVICE,
+
+	.bcdUSB =		__constant_cpu_to_le16(0x0200),
+	.bDeviceClass =		USB_CLASS_COMM,
+	.bDeviceSubClass =      0x02,	/*0x02:CDC-modem , 0x00:CDC-serial*/
+
+	.idVendor =		__constant_cpu_to_le16(DRIVER_VENDOR_NUM),
+	.idProduct =		__constant_cpu_to_le16(DRIVER_PRODUCT_NUM),
+	.iProduct =		STRING_PRODUCT,
+	.bNumConfigurations =	1,
+};
+
+static const struct usb_descriptor_header *otg_desc[] = {
+	(struct usb_descriptor_header *) &(struct usb_otg_descriptor){
+		.bLength =		sizeof(struct usb_otg_descriptor),
+		.bDescriptorType =	USB_DT_OTG,
+		.bmAttributes =		USB_OTG_SRP,
+	},
+	NULL,
+};
+
+/* static strings, in UTF-8 */
+static struct usb_string odin_string_defs[] = {
+	{ 0, manufacturer, },
+	{ 1, product, },
+};
+
+static struct usb_gadget_strings odin_string_tab = {
+	.language	= 0x0409,	/* en-us */
+	.strings	= odin_string_defs,
+};
+
+static struct usb_gadget_strings *g_dnl_composite_strings[] = {
+	&odin_string_tab,
+	NULL,
+};
+
+static int g_dnl_unbind(struct usb_composite_dev *cdev)
+{

drivers/usb/gadget/g_dnl.c new file +36 -0

+
+	return ret;
+}
+
+static int g_dnl_config_register(struct usb_composite_dev *cdev)
+{
+	debug("%s:\n", __func__);
+	static struct usb_configuration config = {
+		.label = "usb_dnload",
+		.bmAttributes =	USB_CONFIG_ATT_ONE | USB_CONFIG_ATT_SELFPOWER,
+		.bConfigurationValue =	CONFIG_USBDOWNLOADER,
+		.iConfiguration =	STRING_USBDOWN,
+
+		.bind = g_dnl_do_config,
+	};
+
+	return usb_add_config(cdev, &config);
+}
+
+static int g_dnl_bind(struct usb_composite_dev *cdev)
+{
+	int			gcnum;
+	int id, ret;
+	struct usb_gadget	*gadget = cdev->gadget;
+
+	debug("%s: gadget: 0x%p cdev: 0x%p\n", __func__, gadget, cdev);
+
+	id = usb_string_id(cdev);
+
+	if (id < 0)
+		return id;
+	odin_string_defs[0].id = id;
+	device_desc.iManufacturer = id;
+
+	id = usb_string_id(cdev);
+	if (id < 0)

drivers/usb/gadget/g_dnl.c new file +20 -0

+	gcnum = usb_gadget_controller_number(gadget);
+
+	debug("gcnum: %d\n", gcnum);
+	if (gcnum >= 0)
+		device_desc.bcdDevice = cpu_to_le16(0x0200 + gcnum);
+	else {
+		debug("%s: controller '%s' not recognized\n",
+			shortname, gadget->name);
+		device_desc.bcdDevice = __constant_cpu_to_le16(0x9999);
+	}
+
+	return 0;
+
+ error:
+	g_dnl_unbind(cdev);
+	return -ENOMEM;
+}
+
+static void g_dnl_suspend(struct usb_composite_dev *cdev)
+{

drivers/usb/gadget/g_dnl.c new file +7 -0

+static struct usb_composite_driver g_dnl_driver = {
+	.name		= NULL,
+	.dev		= &device_desc,
+	.strings	= g_dnl_composite_strings,
+
+	.bind		= g_dnl_bind,
+	.unbind		= g_dnl_unbind,

drivers/usb/gadget/g_dnl.c new file +8 -0

+	memset(str, '\0', sizeof(str));
+
+	strncpy(str, shortname, sizeof(shortname));
+
+	if (!strncmp(s, "dfu", sizeof(s))) {
+		strncat(str, s, sizeof(str));
+	} else {
+		printf("%s: unknown command: %s\n", __func__, s);

drivers/usb/gadget/g_dnl.c new file +10 -0

+		return ret;
+	}
+
+	return 0;
+}
+
+void g_dnl_cleanup(void)
+{
+	usb_composite_unregister(&g_dnl_driver);
+}

include/g_dnl.h new file +33 -0

new file mode 100644
index 0000000..5d0e9a5
@@ -0,0 +1,33 @@
+/*
+ *  Copyright (C) 2012 Samsung Electronics
+ *  Lukasz Majewski <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#ifndef __G_DOWNLOAD_H_
+#define __G_DOWNLOAD_H_
+
+#include <linux/usb/ch9.h>
+#include <usbdescriptors.h>
+#include <linux/usb/gadget.h>
+
+int g_dnl_init(char *s);
+void g_dnl_cleanup(void);
+
+/* USB initialization declaration - board specific*/
+void board_usb_init(void);
+#endif /* __G_DOWNLOAD_H_ */

Dear Lukasz Majewski,

Quoted 72 lines

Composite USB download gadget support (g_dnl) for download functions. This code works on top of composite gadget.

Signed-off-by Lukasz Majewski <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

Cc Marek Vasut <[email protected]>

---
 drivers/usb/gadget/Makefile |    1 +
 drivers/usb/gadget/g_dnl.c  |  235
+++++++++++++++++++++++++++++++++++++++++++
include/g_dnl.h             | 33 ++++++
 3 files changed, 269 insertions(+), 0 deletions(-)
 create mode 100644 drivers/usb/gadget/g_dnl.c
 create mode 100644 include/g_dnl.h

(quoted diff) +0 -0

b/drivers/usb/gadget/Makefile index 87d1918..2c067c8 100644

drivers/usb/gadget/Makefile +1 -0

@@ -29,6 +29,7 @@ LIB	:= $(obj)libusb_gadget.o
 ifdef CONFIG_USB_GADGET
 COBJS-y += epautoconf.o config.o usbstring.o
 COBJS-$(CONFIG_USB_GADGET_S3C_UDC_OTG) += s3c_udc_otg.o
+COBJS-$(CONFIG_USBDOWNLOAD_GADGET) += g_dnl.o
 endif
 ifdef CONFIG_USB_ETHER
 COBJS-y += ether.o epautoconf.o config.o usbstring.o

drivers/usb/gadget/g_dnl.c new file +7 -0

new file mode 100644
index 0000000..7925d0e
@@ -0,0 +1,235 @@
+/*
+ * g_dnl.c -- USB Downloader Gadget
+ *
+ * Copyright (C) 2012 Samsung Electronics
+ * Lukasz Majewski  <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or

modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + */

drivers/usb/gadget/g_dnl.c new file +15 -0

+
+#include <errno.h>
+#include <common.h>
+#include <malloc.h>
+#include <linux/usb/ch9.h>
+#include <usbdescriptors.h>
+#include <linux/usb/gadget.h>
+
+#include <mmc.h>
+#include <part.h>
+
+#include <g_dnl.h>
+#include "f_dfu.h"
+
+#include "gadget_chips.h"

drivers/usb/gadget/g_dnl.c line 40

Quoted 5 lines

drivers/usb/gadget/g_dnl.c new file +5 -0

+#include "composite.c"
+
+/* Samsung's IDs */
+#define DRIVER_VENDOR_NUM 0x04E8
+#define DRIVER_PRODUCT_NUM 0x6601

Is it samsung specific? Or can we use some USB IDs that are not bound to samsung?

drivers/usb/gadget/g_dnl.c line 51

Quoted 11 lines

drivers/usb/gadget/g_dnl.c new file +10 -0

+#define STRING_MANUFACTURER 25
+#define STRING_PRODUCT 2
+#define STRING_USBDOWN 2
+#define CONFIG_USBDOWNLOADER 2
+
+#define DRIVER_VERSION		"usb_dnl 2.0"
+
+static const char shortname[] = "usb_dnl_";
+static const char product[] = "SLP USB download gadget";
+static const char manufacturer[] = "Samsung";

DITTO?

Quoted 52 lines

drivers/usb/gadget/g_dnl.c new file +8 -0

+
+static struct usb_device_descriptor device_desc = {
+	.bLength =		sizeof device_desc,
+	.bDescriptorType =	USB_DT_DEVICE,
+
+	.bcdUSB =		__constant_cpu_to_le16(0x0200),
+	.bDeviceClass =		USB_CLASS_COMM,
+	.bDeviceSubClass =      0x02,	/*0x02:CDC-modem ,

0x00:CDC-serial*/ +

drivers/usb/gadget/g_dnl.c new file +1 -0

+	.idVendor =

__constant_cpu_to_le16(DRIVER_VENDOR_NUM),

drivers/usb/gadget/g_dnl.c new file +1 -0

+	.idProduct =

__constant_cpu_to_le16(DRIVER_PRODUCT_NUM),

drivers/usb/gadget/g_dnl.c new file +6 -0

+	.iProduct =		STRING_PRODUCT,
+	.bNumConfigurations =	1,
+};
+
+static const struct usb_descriptor_header *otg_desc[] = {
+	(struct usb_descriptor_header *) &(struct

usb_otg_descriptor){

drivers/usb/gadget/g_dnl.c new file +1 -0

+		.bLength =		sizeof(struct

usb_otg_descriptor),

drivers/usb/gadget/g_dnl.c new file +29 -0

+		.bDescriptorType =	USB_DT_OTG,
+		.bmAttributes =		USB_OTG_SRP,
+	},
+	NULL,
+};
+
+/* static strings, in UTF-8 */
+static struct usb_string odin_string_defs[] = {
+	{ 0, manufacturer, },
+	{ 1, product, },
+};
+
+static struct usb_gadget_strings odin_string_tab = {
+	.language	= 0x0409,	/* en-us */
+	.strings	= odin_string_defs,
+};
+
+static struct usb_gadget_strings *g_dnl_composite_strings[] = {
+	&odin_string_tab,
+	NULL,
+};
+
+static int g_dnl_unbind(struct usb_composite_dev *cdev)
+{
+	debug("%s\n", __func__);
+	return 0;
+}
+
+static int g_dnl_do_config(struct usb_configuration *c)

drivers/usb/gadget/g_dnl.c line 103

Quoted 5 lines

drivers/usb/gadget/g_dnl.c new file +5 -0

+{
+	int ret = -1;
+	char *s = (char *) c->cdev->driver->name;
+
+	debug("%s: c: 0x%p cdev: 0x%p\n", __func__, c, c->cdev);

Can you elaborate in that debug message more? It's not really useful ;-)

drivers/usb/gadget/g_dnl.c line 113

Quoted 10 lines

drivers/usb/gadget/g_dnl.c new file +9 -0

+	if (gadget_is_otg(c->cdev->gadget)) {
+		c->descriptors = otg_desc;
+		c->bmAttributes |= USB_CONFIG_ATT_WAKEUP;
+	}
+
+	printf("GADGET DRIVER: %s\n", s);
+
+	if (!strcmp(s, "usb_dnl_dfu"))
+		ret = dfu_add(c);

And if this fails, we should set ret= as well, no?

Quoted 42 lines

drivers/usb/gadget/g_dnl.c new file +10 -0

+
+	return ret;
+}
+
+static int g_dnl_config_register(struct usb_composite_dev *cdev)
+{
+	debug("%s:\n", __func__);
+	static struct usb_configuration config = {
+		.label = "usb_dnload",
+		.bmAttributes =	USB_CONFIG_ATT_ONE |

USB_CONFIG_ATT_SELFPOWER,

drivers/usb/gadget/g_dnl.c new file +16 -0

+		.bConfigurationValue =	CONFIG_USBDOWNLOADER,
+		.iConfiguration =	STRING_USBDOWN,
+
+		.bind = g_dnl_do_config,
+	};
+
+	return usb_add_config(cdev, &config);
+}
+
+static int g_dnl_bind(struct usb_composite_dev *cdev)
+{
+	int			gcnum;
+	int id, ret;
+	struct usb_gadget	*gadget = cdev->gadget;
+
+	debug("%s: gadget: 0x%p cdev: 0x%p\n", __func__, gadget,

cdev); +

drivers/usb/gadget/g_dnl.c new file +13 -0

+	id = usb_string_id(cdev);
+
+	if (id < 0)
+		return id;
+	odin_string_defs[0].id = id;
+	device_desc.iManufacturer = id;
+
+	id = usb_string_id(cdev);
+	if (id < 0)
+		return id;
+
+	odin_string_defs[1].id = id;
+	device_desc.iProduct = id;

drivers/usb/gadget/g_dnl.c line 159

Quoted 6 lines

drivers/usb/gadget/g_dnl.c new file +6 -0

+
+	ret = g_dnl_config_register(cdev);
+	if (ret)
+		goto error;
+
+

Two newlines

Quoted 28 lines

drivers/usb/gadget/g_dnl.c new file +5 -0

+	gcnum = usb_gadget_controller_number(gadget);
+
+	debug("gcnum: %d\n", gcnum);
+	if (gcnum >= 0)
+		device_desc.bcdDevice = cpu_to_le16(0x0200 +

gcnum);

drivers/usb/gadget/g_dnl.c new file +4 -0

+	else {
+		debug("%s: controller '%s' not recognized\n",
+			shortname, gadget->name);
+		device_desc.bcdDevice =

__constant_cpu_to_le16(0x9999);

drivers/usb/gadget/g_dnl.c new file +16 -0

+	}
+
+	return 0;
+
+ error:
+	g_dnl_unbind(cdev);
+	return -ENOMEM;
+}
+
+static void g_dnl_suspend(struct usb_composite_dev *cdev)
+{
+	if (cdev->gadget->speed == USB_SPEED_UNKNOWN)
+		return;
+
+	debug("suspend\n");
+}

drivers/usb/gadget/g_dnl.c line 189

Quoted 5 lines

drivers/usb/gadget/g_dnl.c new file +5 -0

+
+static void g_dnl_resume(struct usb_composite_dev *cdev)
+{
+	debug("resume\n");
+}

Maybe you should check if suspend/resume are set at all. Then you won't need the above stubs.

Quoted 12 lines

drivers/usb/gadget/g_dnl.c new file +11 -0

+static struct usb_composite_driver g_dnl_driver = {
+	.name		= NULL,
+	.dev		= &device_desc,
+	.strings	= g_dnl_composite_strings,
+
+	.bind		= g_dnl_bind,
+	.unbind		= g_dnl_unbind,
+
+	.suspend	= g_dnl_suspend,
+	.resume		= g_dnl_resume,
+};

drivers/usb/gadget/g_dnl.c line 207

Quoted 6 lines

drivers/usb/gadget/g_dnl.c new file +6 -0

+
+int g_dnl_init(char *s)
+{
+	int ret;
+	static char str[16];
+

Why use '\0' if you can just use 0 ?

Quoted 14 lines

drivers/usb/gadget/g_dnl.c new file +13 -0

+	memset(str, '\0', sizeof(str));
+
+	strncpy(str, shortname, sizeof(shortname));
+
+	if (!strncmp(s, "dfu", sizeof(s))) {
+		strncat(str, s, sizeof(str));
+	} else {
+		printf("%s: unknown command: %s\n", __func__, s);
+		return -EINVAL;
+	}
+
+	g_dnl_driver.name = str;
+

drivers/usb/gadget/g_dnl.c line 225

Quoted 6 lines

drivers/usb/gadget/g_dnl.c new file +1 -0

+	debug("%s: g_dnl_driver.name: %s\n", __func__,

g_dnl_driver.name);

drivers/usb/gadget/g_dnl.c new file +4 -0

+	ret = usb_composite_register(&g_dnl_driver);
+
+	if (ret) {
+		printf("%s: failed!, error:%d ", __func__, ret);

Missing newline in printf() (...error:%d ").

Quoted 51 lines

drivers/usb/gadget/g_dnl.c new file +10 -0

+		return ret;
+	}
+
+	return 0;
+}
+
+void g_dnl_cleanup(void)
+{
+	usb_composite_unregister(&g_dnl_driver);
+}

include/g_dnl.h new file +15 -0

new file mode 100644
index 0000000..5d0e9a5
@@ -0,0 +1,33 @@
+/*
+ *  Copyright (C) 2012 Samsung Electronics
+ *  Lukasz Majewski <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public

License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */

include/g_dnl.h new file +14 -0

+
+#ifndef __G_DOWNLOAD_H_
+#define __G_DOWNLOAD_H_
+
+#include <linux/usb/ch9.h>
+#include <usbdescriptors.h>
+#include <linux/usb/gadget.h>
+
+int g_dnl_init(char *s);
+void g_dnl_cleanup(void);
+
+/* USB initialization declaration - board specific*/
+void board_usb_init(void);
+#endif /* __G_DOWNLOAD_H_ */

+int g_dnl_init(char *s)

+int g_dnl_init(char *s);

+void g_dnl_cleanup(void);

drivers/usb/gadget/g_dnl.c

Quoted 4 lines

drivers/usb/gadget/g_dnl.c new file +1 -0

 
+static const char shortname[] = "usb_dnl_";

shortname -> gadget_name_prefix

drivers/usb/gadget/g_dnl.c line 189

Quoted 13 lines

drivers/usb/gadget/g_dnl.c new file +12 -0

+static void g_dnl_suspend(struct usb_composite_dev *cdev)
+{
+	if (cdev->gadget->speed == USB_SPEED_UNKNOWN)
+		return;
+
+	debug("suspend\n");
+}
+
+static void g_dnl_resume(struct usb_composite_dev *cdev)
+{
+	debug("resume\n");
+}

do suspend/resume funcs make any sense in u-boot ?

Quoted 2 lines

+int g_dnl_init(char *s)

const char

drivers/usb/gadget/g_dnl.c line 210

Quoted 8 lines

drivers/usb/gadget/g_dnl.c new file +7 -0

+{
+	int ret;
+	static char str[16];
+
+	memset(str, '\0', sizeof(str));
+
+	strncpy(str, shortname, sizeof(shortname));

no need for the memset.

this strncpy looks broken -- the 3rd arg is
for how many bytes are available in the *dest* buffer, not how long the source is.

drivers/usb/gadget/g_dnl.c line 212

Quoted 2 lines

drivers/usb/gadget/g_dnl.c new file +1 -0

+	if (!strncmp(s, "dfu", sizeof(s))) {

sizeof() here is wrong -- that gives you back 4 (the size of a pointer) on 32bit systems

drivers/usb/gadget/g_dnl.c line 213

Quoted 2 lines

drivers/usb/gadget/g_dnl.c new file +1 -0

+		strncat(str, s, sizeof(str));

this is also incorrect. the length given to strncat is how many bytes are left, not the total length.

since this string parsing logic is all just completely broken, i'd suggest replacing it all with:

{
	int ret;
	/* We only allow "dfu" atm, so 3 should be enough */
	static char name[sizeof(shortname) + 3];

	if (strcmp(s, "dfu")) {
		printf("%s: unknown command: %s\n", __func__, s);
		return -EINVAL;
	}

	strcpy(name, shortname);
	strcat(name, s);

drivers/usb/gadget/g_dnl.c line 225

Quoted 2 lines

drivers/usb/gadget/g_dnl.c new file +2 -0

+	if (ret) {
+		printf("%s: failed!, error:%d ", __func__, ret);

needs a space between "error:" and "%d"

include/g_dnl.h

Quoted 7 lines

include/g_dnl.h new file +3 -0

 
+#include <linux/usb/ch9.h>
+#include <usbdescriptors.h>
+#include <linux/usb/gadget.h>

unused -> delete

Quoted 2 lines

+int g_dnl_init(char *s);

int g_dnl_register(const char *type);

this also needs documentation in the header explaining how to use this func

Quoted 2 lines

+void g_dnl_cleanup(void);

void g_dnl_unregister(void);

include/g_dnl.h line 32

Quoted 3 lines

include/g_dnl.h new file +2 -0

+/* USB initialization declaration - board specific*/
+void board_usb_init(void);

not used in these files -> delete

-mike

Dear Mike Frysinger,

Quoted 10 lines · On Tuesday 03 July 2012 05:38:05 Lukasz Majewski wrote:

drivers/usb/gadget/g_dnl.c line 210

Quoted 7 lines

drivers/usb/gadget/g_dnl.c new file +7 -0

+{
+	int ret;
+	static char str[16];
+
+	memset(str, '\0', sizeof(str));
+
+	strncpy(str, shortname, sizeof(shortname));

no need for the memset.

The gadget can be called from many separate commands (e.g. command "dfu" and command "ums") and those commands can be executed without power cycle. Thereof I need to be sure, that str is not polluted by previous name.

Quoted 3 lines

this strncpy looks broken -- the 3rd arg is
for how many bytes are available in the *dest* buffer, not how long the source is.

After looking deeply into the source I admit that providing the upper bound on the dest is more safe.

Quoted 4 lines

drivers/usb/gadget/g_dnl.c line 213

Quoted 1 line

drivers/usb/gadget/g_dnl.c new file +1 -0

+		strncat(str, s, sizeof(str));

this is also incorrect. the length given to strncat is how many bytes are left, not the total length.

I cannot agree. sizeof(str) return 16, which is the destination buffer size.

Quoted 13 lines

since this string parsing logic is all just completely broken, i'd suggest replacing it all with:

{
	int ret;
	/* We only allow "dfu" atm, so 3 should be enough */
	static char name[sizeof(shortname) + 3];
	
	if (strcmp(s, "dfu")) {
		printf("%s: unknown command: %s\n", __func__, s);
		return -EINVAL;
	}

Quoted 2 lines

	strcpy(name, shortname);
	strcat(name, s);

This is a very neat design, but it assumes that there will be only one function ("dfu" in this case). For this particular function +3 applies, but what if another function (like "usb_storage") will be defined?

I'm now working on another function - the USB Mass Storage (named "ums" ;-) ).

Another issue is omitting the strncmp/strncpy functions and depending on
the: static char name[sizeof(shortname) + 3]; definition to prevent buffer overflow.

Support for f_dfu USB function.

Signed-off-by Lukasz Majewski <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

Cc Marek Vasut <[email protected]>

---

+ */

drivers/usb/gadget/f_dfu.c new file +36 -1

+static struct usb_string strings_dfu_generic[] = {
+	[0].s = dfu_name,
+	{  }			/* end of list */
+};
+
+static struct usb_gadget_strings stringtab_dfu_generic = {
+	.language	= 0x0409,	/* en-us */
+	.strings	= strings_dfu_generic,
+};
+
+static struct usb_gadget_strings *dfu_generic_strings[] = {
+	&stringtab_dfu_generic,
+	NULL,
+};
+
+/*
+ * usb_function specific
+ */
+static struct usb_gadget_strings stringtab_dfu = {
+	.language	= 0x0409,	/* en-us */
+	/*
+	 * .strings
+	 *
+	 * assigned during initialization,
+	 * depends on number of flash entities
+	 *
+	 */
+};
+
+static struct usb_gadget_strings *dfu_strings[] = {
+	&stringtab_dfu,
+	NULL,
+};
+
+/*------------------------------------------------------------------------
-*/ +
+static void dnload_request_complete(struct usb_ep *ep, struct usb_request

*req) +{

drivers/usb/gadget/f_dfu.c new file +8 -0

+	struct f_dfu *f_dfu = req->context;
+
+	dfu_write(dfu_get_entity(f_dfu->altsetting), req->buf,
+		  req->length, f_dfu->blk_seq_num);
+
+	if (req->length == 0) {
+		puts("DOWNLOAD ... OK\n");
+		puts("Ctrl+C to exit ...\n");

drivers/usb/gadget/f_dfu.c new file +3 -0

+	req->actual = sizeof(u8);
+}
+

drivers/usb/gadget/f_dfu.c new file +11 -0

+
+
+static int
+dfu_handle(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
+{
+	struct usb_gadget *gadget = f->config->cdev->gadget;
+	struct usb_request *req = f->config->cdev->req;
+	struct f_dfu *f_dfu = f->config->cdev->req->context;
+	u16 len = le16_to_cpu(ctrl->wLength);
+	u16 w_value = le16_to_cpu(ctrl->wValue);
+	int value = 0;

drivers/usb/gadget/f_dfu.c new file +7 -0

+		switch (f_dfu->dfu_state) {
+		case DFU_STATE_appIDLE:
+			switch (ctrl->bRequest) {
+			case USB_REQ_DFU_GETSTATUS:
+				handle_getstatus(req);
+				value = RET_STAT_LEN;
+				break;

drivers/usb/gadget/f_dfu.c new file +18 -0

+	if (!f_dfu->strings)
+		goto enomem;
+
+	for (i = 0; i < n; ++i) {
+		de = dfu_get_entity(i);
+		f_dfu->strings[i].s = de->name;
+	}
+
+	f_dfu->strings[i].id = 0;
+	f_dfu->strings[i].s = NULL;
+
+	return 0;
+
+enomem:
+	while (i)
+		f_dfu->strings[--i].s = NULL;
+
+	kfree(f_dfu->strings);

drivers/usb/gadget/f_dfu.c new file +55 -0

+		if (!d)
+			goto enomem;
+
+		d->bLength =		sizeof(*d);
+		d->bDescriptorType =	USB_DT_INTERFACE;
+		d->bAlternateSetting =	i;
+		d->bNumEndpoints =	0;
+		d->bInterfaceClass =	USB_CLASS_APP_SPEC;
+		d->bInterfaceSubClass =	1;
+		d->bInterfaceProtocol =	2;
+
+		f_dfu->function[i] = (struct usb_descriptor_header *)d;
+	}
+	f_dfu->function[i] = NULL;
+
+	return 0;
+
+enomem:
+	while (i) {
+		kfree(f_dfu->function[--i]);
+		f_dfu->function[i] = NULL;
+	}
+	kfree(f_dfu->function);
+
+	return -ENOMEM;
+}
+
+static int dfu_bind(struct usb_configuration *c, struct usb_function *f)
+{
+	struct usb_composite_dev *cdev = c->cdev;
+	struct f_dfu *f_dfu = func_to_dfu(f);
+	int alt_num = dfu_get_alt_number();
+	int rv, id, i;
+
+	id = usb_interface_id(c, f);
+	if (id < 0)
+		return id;
+	dfu_intf_runtime.bInterfaceNumber = id;
+
+	f_dfu->dfu_state = DFU_STATE_appIDLE;
+	f_dfu->dfu_status = DFU_STATUS_OK;
+
+	rv = dfu_prepare_function(f_dfu, alt_num);
+	if (rv)
+		goto error;
+
+	rv = dfu_prepare_strings(f_dfu, alt_num);
+	if (rv)
+		goto error;
+	for (i = 0; i < alt_num; i++) {
+		id = usb_string_id(cdev);
+		if (id < 0)
+			return id;
+		f_dfu->strings[i].id = id;
+		((struct usb_interface_descriptor *)f_dfu->function[i])

Dear Lukasz Majewski,

Quoted 6 lines

Support for f_dfu USB function.

Signed-off-by Lukasz Majewski <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

Cc Marek Vasut <[email protected]>

---

[...]

drivers/usb/gadget/f_dfu.c line 95

Quoted 6 lines

drivers/usb/gadget/f_dfu.c new file +6 -0

+
+static const char dfu_name[] = "Device Firmware Upgrade";
+
+/* static strings, in UTF-8 */
+/*
+ * dfu_genericiguration specific

generi....what?

Quoted 52 lines

+ */

drivers/usb/gadget/f_dfu.c new file +36 -1

+static struct usb_string strings_dfu_generic[] = {
+	[0].s = dfu_name,
+	{  }			/* end of list */
+};
+
+static struct usb_gadget_strings stringtab_dfu_generic = {
+	.language	= 0x0409,	/* en-us */
+	.strings	= strings_dfu_generic,
+};
+
+static struct usb_gadget_strings *dfu_generic_strings[] = {
+	&stringtab_dfu_generic,
+	NULL,
+};
+
+/*
+ * usb_function specific
+ */
+static struct usb_gadget_strings stringtab_dfu = {
+	.language	= 0x0409,	/* en-us */
+	/*
+	 * .strings
+	 *
+	 * assigned during initialization,
+	 * depends on number of flash entities
+	 *
+	 */
+};
+
+static struct usb_gadget_strings *dfu_strings[] = {
+	&stringtab_dfu,
+	NULL,
+};
+
+/*------------------------------------------------------------------------
-*/ +
+static void dnload_request_complete(struct usb_ep *ep, struct

usb_request *req) +{

drivers/usb/gadget/f_dfu.c new file +12 -0

+	struct f_dfu *f_dfu = req->context;
+
+	dfu_write(dfu_get_entity(f_dfu->altsetting), req->buf,
+		  req->length, f_dfu->blk_seq_num);
+
+	if (req->length == 0) {
+		puts("DOWNLOAD ... OK\n");
+		puts("Ctrl+C to exit ...\n");
+	}
+}
+
+static void handle_getstatus(struct usb_request *req)

drivers/usb/gadget/f_dfu.c line 152

Quoted 6 lines

drivers/usb/gadget/f_dfu.c new file +6 -0

+{
+	struct dfu_status *dstat = (struct dfu_status *)req->buf;
+	struct f_dfu *f_dfu = req->context;
+
+	switch (f_dfu->dfu_state) {
+	case DFU_STATE_dfuDNLOAD_SYNC:

What's this crazy cammel case in here ? :-)

drivers/usb/gadget/f_dfu.c line 164

Quoted 13 lines

drivers/usb/gadget/f_dfu.c new file +12 -0

+	case DFU_STATE_dfuDNBUSY:
+		f_dfu->dfu_state = DFU_STATE_dfuDNLOAD_IDLE;
+		break;
+	case DFU_STATE_dfuMANIFEST_SYNC:
+		break;
+	default:
+		break;
+	}
+
+	/* send status response */
+	dstat->bStatus = f_dfu->dfu_status;
+	/* FIXME: set dstat->bwPollTimeout */

FIXME ... so fix it ;-)

drivers/usb/gadget/f_dfu.c line 173

Quoted 10 lines

drivers/usb/gadget/f_dfu.c new file +9 -0

+	dstat->bState = f_dfu->dfu_state;
+	dstat->iString = 0;
+}
+
+static void handle_getstate(struct usb_request *req)
+{
+	struct f_dfu *f_dfu = req->context;
+
+	((u8 *)req->buf)[0] = f_dfu->dfu_state & 0xff;

Now ... this is ubercrazy ... can't this be made without this typecasting voodoo?

Quoted 4 lines

drivers/usb/gadget/f_dfu.c new file +3 -0

+	req->actual = sizeof(u8);
+}
+

[...]

drivers/usb/gadget/f_dfu.c line 209

Quoted 13 lines

drivers/usb/gadget/f_dfu.c new file +13 -0

+static int handle_dnload(struct usb_gadget *gadget, u16 len)
+{
+	struct usb_composite_dev *cdev = get_gadget_data(gadget);
+	struct usb_request *req = cdev->req;
+	struct f_dfu *f_dfu = req->context;
+
+	if (len == 0)
+		f_dfu->dfu_state = DFU_STATE_dfuMANIFEST_SYNC;
+
+	req->complete = dnload_request_complete;
+
+	return len;
+}

One newline too much below.

Quoted 20 lines

drivers/usb/gadget/f_dfu.c new file +4 -0

+
+
+static int
+dfu_handle(struct usb_function *f, const struct usb_ctrlrequest

*ctrl) +{

drivers/usb/gadget/f_dfu.c new file +10 -0

+	struct usb_gadget *gadget = f->config->cdev->gadget;
+	struct usb_request *req = f->config->cdev->req;
+	struct f_dfu *f_dfu = f->config->cdev->req->context;
+	u16 len = le16_to_cpu(ctrl->wLength);
+	u16 w_value = le16_to_cpu(ctrl->wValue);
+	int value = 0;
+	int standard;
+
+	standard = (ctrl->bRequestType & USB_TYPE_MASK)
+						==

USB_TYPE_STANDARD; +

(quoted diff) +2 -0

+	debug("w_value: 0x%x len: 0x%x\n", w_value, len);
+	debug("standard: 0x%x ctrl->bRequest: 0x%x

f_dfu->dfu_state: 0x%x\n",

drivers/usb/gadget/f_dfu.c line 230

Quoted 3 lines

drivers/usb/gadget/f_dfu.c new file +3 -0

+	       standard, ctrl->bRequest, f_dfu->dfu_state);
+
+	if (!standard) {

This function doesn't fit on my screen ... that means it's waaaay too long and shall be split into more functions ;-)

That's also remove the need for your crazy conditional assignment of standard.

Quoted 8 lines

drivers/usb/gadget/f_dfu.c new file +7 -0

+		switch (f_dfu->dfu_state) {
+		case DFU_STATE_appIDLE:
+			switch (ctrl->bRequest) {
+			case USB_REQ_DFU_GETSTATUS:
+				handle_getstatus(req);
+				value = RET_STAT_LEN;
+				break;

[...]

Quoted 11 lines

drivers/usb/gadget/f_dfu.c new file +9 -1

+
+/*------------------------------------------------------------------------
-*/ +
+static int
+dfu_prepare_strings(struct f_dfu *f_dfu, int n)
+{
+	struct dfu_entity *de = NULL;
+	int i = 0;
+
+	f_dfu->strings = calloc(((n + 1) * sizeof(struct

usb_string)), 1);

calloc(n, 1) ... that's the same as malloc(), isn't it ?

Quoted 24 lines

drivers/usb/gadget/f_dfu.c new file +23 -0

+	if (!f_dfu->strings)
+		goto enomem;
+
+	for (i = 0; i < n; ++i) {
+		de = dfu_get_entity(i);
+		f_dfu->strings[i].s = de->name;
+	}
+
+	f_dfu->strings[i].id = 0;
+	f_dfu->strings[i].s = NULL;
+
+	return 0;
+
+enomem:
+	while (i)
+		f_dfu->strings[--i].s = NULL;
+
+	kfree(f_dfu->strings);
+
+	return -ENOMEM;
+}
+
+static int dfu_prepare_function(struct f_dfu *f_dfu, int n)

Quoted 6 lines

drivers/usb/gadget/f_dfu.c new file +5 -0

+{
+	struct usb_interface_descriptor *d;
+	int i = 0;
+
+	f_dfu->function = calloc(n * sizeof(struct

usb_descriptor_header *), 1);

DITTO

drivers/usb/gadget/f_dfu.c line 511

Quoted 6 lines

drivers/usb/gadget/f_dfu.c new file +5 -0

+	if (!f_dfu->function)
+		goto enomem;
+
+	for (i = 0; i < n; ++i) {
+		d = kzalloc(sizeof(*d), GFP_KERNEL);

Why use the kernel alternatives ? just use malloc()/free() ?

Quoted 62 lines

drivers/usb/gadget/f_dfu.c new file +12 -0

+		if (!d)
+			goto enomem;
+
+		d->bLength =		sizeof(*d);
+		d->bDescriptorType =	USB_DT_INTERFACE;
+		d->bAlternateSetting =	i;
+		d->bNumEndpoints =	0;
+		d->bInterfaceClass =	USB_CLASS_APP_SPEC;
+		d->bInterfaceSubClass =	1;
+		d->bInterfaceProtocol =	2;
+
+		f_dfu->function[i] = (struct usb_descriptor_header

*)d;

drivers/usb/gadget/f_dfu.c new file +16 -0

+	}
+	f_dfu->function[i] = NULL;
+
+	return 0;
+
+enomem:
+	while (i) {
+		kfree(f_dfu->function[--i]);
+		f_dfu->function[i] = NULL;
+	}
+	kfree(f_dfu->function);
+
+	return -ENOMEM;
+}
+
+static int dfu_bind(struct usb_configuration *c, struct

usb_function *f) +{

drivers/usb/gadget/f_dfu.c new file +26 -0

+	struct usb_composite_dev *cdev = c->cdev;
+	struct f_dfu *f_dfu = func_to_dfu(f);
+	int alt_num = dfu_get_alt_number();
+	int rv, id, i;
+
+	id = usb_interface_id(c, f);
+	if (id < 0)
+		return id;
+	dfu_intf_runtime.bInterfaceNumber = id;
+
+	f_dfu->dfu_state = DFU_STATE_appIDLE;
+	f_dfu->dfu_status = DFU_STATUS_OK;
+
+	rv = dfu_prepare_function(f_dfu, alt_num);
+	if (rv)
+		goto error;
+
+	rv = dfu_prepare_strings(f_dfu, alt_num);
+	if (rv)
+		goto error;
+	for (i = 0; i < alt_num; i++) {
+		id = usb_string_id(cdev);
+		if (id < 0)
+			return id;
+		f_dfu->strings[i].id = id;
+		((struct usb_interface_descriptor

*)f_dfu->function[i])

drivers/usb/gadget/f_dfu.c new file +4 -0

+			->iInterface = id;
+	}
+
+	stringtab_dfu.strings = f_dfu->strings;

drivers/usb/gadget/f_dfu.c line 576

Quoted 6 lines

drivers/usb/gadget/f_dfu.c new file +6 -0

+
+	cdev->req->context = f_dfu;
+
+	return 0;
+error:
+	return rv;

So just return the retval ...

Review

Quoted 3 lines

(quoted diff) +2 -0

+}
+

Every time I review patches and find multiple small issues, I feel bad :-(

Quoted 9 lines

drivers/usb/gadget/f_dfu.c line 152

Quoted 8 lines

drivers/usb/gadget/f_dfu.c new file +7 -0

+static void handle_getstatus(struct usb_request *req)
+{
+	struct dfu_status *dstat = (struct dfu_status *)req->buf;
+	struct f_dfu *f_dfu = req->context;
+
+	switch (f_dfu->dfu_state) {
 
+	case DFU_STATE_dfuDNLOAD_SYNC:

What's this crazy cammel case in here ? :-)

I know, that camel case descriptions are not welcome :-), but those are written in this way for purpose.

dfuDNLOAD_SYNC is the exact state name mentioned at DFU specification. Other states - like dfuDNBUSY are exactly the same.

From mine experience it is more readable. Please consider for example the Fig. A1 - Interface state transition diagram from page 28 at DFU_1.1.pdf spec (link below).

http://www.usb.org/developers/devclass_docs/DFU_1.1.pdf

Quoted 6 lines

Quoted 6 lines

drivers/usb/gadget/f_dfu.c new file +6 -0

+	case DFU_STATE_dfuDNBUSY:
+		f_dfu->dfu_state = DFU_STATE_dfuDNLOAD_IDLE;
+		break;
+	case DFU_STATE_dfuMANIFEST_SYNC:
+		break;
+	default:

Quoted 8 lines

drivers/usb/gadget/f_dfu.c line 164

Quoted 6 lines

drivers/usb/gadget/f_dfu.c new file +6 -0

+		break;
+	}
+
+	/* send status response */
+	dstat->bStatus = f_dfu->dfu_status;
+	/* FIXME: set dstat->bwPollTimeout */

FIXME ... so fix it ;-)

This is not u-boot related - will be removed

Quoted 4 lines

Quoted 4 lines

drivers/usb/gadget/f_dfu.c new file +4 -0

+	dstat->bState = f_dfu->dfu_state;
+	dstat->iString = 0;
+}
+

Quoted 8 lines

drivers/usb/gadget/f_dfu.c line 173

Quoted 5 lines

drivers/usb/gadget/f_dfu.c new file +5 -0

+static void handle_getstate(struct usb_request *req)
+{
+	struct f_dfu *f_dfu = req->context;
+
+	((u8 *)req->buf)[0] = f_dfu->dfu_state & 0xff;

Now ... this is ubercrazy ... can't this be made without this typecasting voodoo?

The problem is that req->buf is a void pointer. And the goal is to store dfu state at 8 bits.

Quoted 3 lines

Quoted 3 lines

drivers/usb/gadget/f_dfu.c new file +3 -0

+	req->actual = sizeof(u8);
+}
+

Quoted 10 lines

Quoted 8 lines

drivers/usb/gadget/f_dfu.c new file +7 -0

+static int
+dfu_prepare_strings(struct f_dfu *f_dfu, int n)
+{
+	struct dfu_entity *de = NULL;
+	int i = 0;
+
+	f_dfu->strings = calloc(((n + 1) * sizeof(struct

usb_string)), 1);

calloc(n, 1) ... that's the same as malloc(), isn't it ?

I now wonder how mine mind produced this :-)

Correct version:

f_dfu->strings = calloc(sizeof(struct usb_string), n + 1);

I prefer calloc, since it delivers zeroed memory.

Dear Lukasz Majewski,

[...]

Quoted 14 lines

Quoted 10 lines

drivers/usb/gadget/f_dfu.c line 152

Quoted 9 lines

drivers/usb/gadget/f_dfu.c new file +3 -0

+static void handle_getstatus(struct usb_request *req)
+{
+	struct dfu_status *dstat = (struct dfu_status

*)req->buf;

drivers/usb/gadget/f_dfu.c new file +4 -0

+	struct f_dfu *f_dfu = req->context;
+
+	switch (f_dfu->dfu_state) {
 
+	case DFU_STATE_dfuDNLOAD_SYNC:

What's this crazy cammel case in here ? :-)

I know, that camel case descriptions are not welcome :-), but those are written in this way for purpose.

Quoted 8 lines

dfuDNLOAD_SYNC is the exact state name mentioned at DFU specification. Other states - like dfuDNBUSY are exactly the same.

From mine experience it is more readable. Please consider for example the Fig. A1 - Interface state transition diagram from page 28 at DFU_1.1.pdf spec (link below).

http://www.usb.org/developers/devclass_docs/DFU_1.1.pdf

OK, good.

Quoted 30 lines

Quoted 6 lines

Quoted 6 lines

drivers/usb/gadget/f_dfu.c new file +6 -0

+	case DFU_STATE_dfuDNBUSY:
+		f_dfu->dfu_state = DFU_STATE_dfuDNLOAD_IDLE;
+		break;
+	case DFU_STATE_dfuMANIFEST_SYNC:
+		break;
+	default:

Quoted 8 lines

drivers/usb/gadget/f_dfu.c line 164

Quoted 6 lines

drivers/usb/gadget/f_dfu.c new file +6 -0

+		break;
+	}
+
+	/* send status response */
+	dstat->bStatus = f_dfu->dfu_status;
+	/* FIXME: set dstat->bwPollTimeout */

FIXME ... so fix it ;-)

This is not u-boot related - will be removed

Quoted 12 lines

drivers/usb/gadget/f_dfu.c line 173

Quoted 9 lines

drivers/usb/gadget/f_dfu.c new file +9 -0

+	dstat->bState = f_dfu->dfu_state;
+	dstat->iString = 0;
+}
+
+static void handle_getstate(struct usb_request *req)
+{
+	struct f_dfu *f_dfu = req->context;
+
+	((u8 *)req->buf)[0] = f_dfu->dfu_state & 0xff;

Now ... this is ubercrazy ... can't this be made without this typecasting voodoo?

Quoted 2 lines

The problem is that req->buf is a void pointer. And the goal is to store dfu state at 8 bits.

Sure, but why not make the buffer u8 ?

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

drivers/usb/gadget/f_dfu.c new file +3 -0

+	req->actual = sizeof(u8);
+}
+

[...]

Quoted 11 lines

Quoted 10 lines

Quoted 8 lines

drivers/usb/gadget/f_dfu.c new file +7 -0

+static int
+dfu_prepare_strings(struct f_dfu *f_dfu, int n)
+{
+	struct dfu_entity *de = NULL;
+	int i = 0;
+
+	f_dfu->strings = calloc(((n + 1) * sizeof(struct

usb_string)), 1);

calloc(n, 1) ... that's the same as malloc(), isn't it ?

Quoted 7 lines

I now wonder how mine mind produced this :-)

Correct version:

f_dfu->strings = calloc(sizeof(struct usb_string), n + 1);

I prefer calloc, since it delivers zeroed memory.

ok, that's better :)

[...]

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

Now ... this is ubercrazy ... can't this be made without this typecasting voodoo?

The problem is that req->buf is a void pointer. And the goal is to store dfu state at 8 bits.

Quoted 1 line

Sure, but why not make the buffer u8 ?

The req->buf is a member of usb_request defined at gadget.h.

It represents the request from USB. I cannot guarantee, that we will always regard data pointed by buf as u8. For flexibility of gadget usage it is safer to leave it as void pointer.

New, separate driver at ./drivers/dfu has been added. It allows platform and storage independent operation of DFU.

Signed-off-by Lukasz Majewski <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

Cc Marek Vasut <[email protected]>

---

drivers/dfu/dfu.c new file +19 -0

+#include <common.h>
+#include <malloc.h>
+#include <mmc.h>
+#include <fat.h>
+#include <dfu.h>
+#include <linux/list.h>
+#include <linux/compiler.h>
+
+static LIST_HEAD(dfu_list);
+static int dfu_alt_num;
+
+static int dfu_find_alt_num(char *s)
+{
+	int i = 0;
+
+	for (; *s; s++)
+		if (*s == ';')
+			i++;
+

drivers/dfu/dfu.c new file +5 -0

+
+char *dfu_extract_token(char** e, int *n)
+{
+	char *st = *e;
+

include/dfu.h new file +0 -0

new file mode 100644
index 0000000..f7d823b

Dear Lukasz Majewski,

Quoted 7 lines

New, separate driver at ./drivers/dfu has been added. It allows platform and storage independent operation of DFU.

Signed-off-by Lukasz Majewski <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

Cc Marek Vasut <[email protected]>

---

[...]

Quoted 24 lines

drivers/dfu/dfu.c new file +24 -0

+#include <common.h>
+#include <malloc.h>
+#include <mmc.h>
+#include <fat.h>
+#include <dfu.h>
+#include <linux/list.h>
+#include <linux/compiler.h>
+
+static LIST_HEAD(dfu_list);
+static int dfu_alt_num;
+
+static int dfu_find_alt_num(char *s)
+{
+	int i = 0;
+
+	for (; *s; s++)
+		if (*s == ';')
+			i++;
+
+	return ++i;
+}
+
+static char *dfu_extract_entity(char** env)
+{

drivers/dfu/dfu.c line 51

Quoted 5 lines

drivers/dfu/dfu.c new file +5 -0

+	char *s = *env;
+
+	strsep(env, ";");
+	return s;
+}

Shall we not make these all generic? They seem to be quite helpful components.

Quoted 10 lines

drivers/dfu/dfu.c new file +9 -0

+
+char *dfu_extract_token(char** e, int *n)
+{
+	char *st = *e;
+
+	debug("%s: %s\n", __func__, st);
+
+	strsep(e, " ");
+	*n = *e - st;

drivers/dfu/dfu.c line 66

Quoted 6 lines

drivers/dfu/dfu.c new file +6 -0

+
+	return st;
+}
+
+static unsigned char __aligned(CONFIG_SYS_CACHELINE_SIZE)
+				     dfu_buf[DFU_DATA_BUF_SIZE];

Can we not stack-allocate it with ALLOC_CACHE_ALIGN_BUFFER()?

[...]

Quoted 5 lines

include/dfu.h new file +0 -0

new file mode 100644
index 0000000..f7d823b

[...]

include/dfu.h line 71

Quoted 10 lines

include/dfu.h new file +10 -0

+struct dfu_entity {
+	char			name[DFU_NAME_SIZE];
+	int                     alt;
+	void                    *dev_private;
+	int                     dev_num;
+	enum dfu_device_type    dev_type;
+	enum dfu_layout         layout;
+
+	union {
+		struct mmc_internal_data mmc;

This union seems redundant ;-)

[...]

Hi Marek,

Quoted 36 lines

Dear Lukasz Majewski,

Quoted 7 lines

New, separate driver at ./drivers/dfu has been added. It allows platform and storage independent operation of DFU.

Signed-off-by Lukasz Majewski <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

Cc Marek Vasut <[email protected]>

---

[...]

Quoted 24 lines

drivers/dfu/dfu.c new file +24 -0

+#include <common.h>
+#include <malloc.h>
+#include <mmc.h>
+#include <fat.h>
+#include <dfu.h>
+#include <linux/list.h>
+#include <linux/compiler.h>
+
+static LIST_HEAD(dfu_list);
+static int dfu_alt_num;
+
+static int dfu_find_alt_num(char *s)
+{
+	int i = 0;
+
+	for (; *s; s++)
+		if (*s == ';')
+			i++;
+
+	return ++i;
+}
+
+static char *dfu_extract_entity(char** env)
+{

Quoted 8 lines

drivers/dfu/dfu.c line 51

Quoted 5 lines

drivers/dfu/dfu.c new file +5 -0

+	char *s = *env;
+
+	strsep(env, ";");
+	return s;
+}

Shall we not make these all generic? They seem to be quite helpful components.

It is a good topic for a discussion if those functions shall be moved to ./lib/string.c.
I regarded them as a "little" helper functions for parsing DFU alt setting env variable. Those are very short and build with methods exported from string.c

Quoted 9 lines

Quoted 9 lines

drivers/dfu/dfu.c new file +9 -0

+
+char *dfu_extract_token(char** e, int *n)
+{
+	char *st = *e;
+
+	debug("%s: %s\n", __func__, st);
+
+	strsep(e, " ");
+	*n = *e - st;

Quoted 8 lines

drivers/dfu/dfu.c line 66

Quoted 6 lines

drivers/dfu/dfu.c new file +6 -0

+
+	return st;
+}
+
+static unsigned char __aligned(CONFIG_SYS_CACHELINE_SIZE)
+				     dfu_buf[DFU_DATA_BUF_SIZE];

Can we not stack-allocate it with ALLOC_CACHE_ALIGN_BUFFER()?

The dfu_buf is 4 MiB (this is the size of DFU_DATA_BUF_SIZE). I don't think, that allocating it on the stack (for stack allocation the ALLOC_CACHE_ALIGN_BUFFER() is designed) is a good idea.

Quoted 10 lines

[...]

Quoted 5 lines

include/dfu.h new file +0 -0

new file mode 100644
index 0000000..f7d823b

[...]

Quoted 12 lines

include/dfu.h line 71

Quoted 10 lines

include/dfu.h new file +10 -0

+struct dfu_entity {
+	char			name[DFU_NAME_SIZE];
+	int                     alt;
+	void                    *dev_private;
+	int                     dev_num;
+	enum dfu_device_type    dev_type;
+	enum dfu_layout         layout;
+
+	union {
+		struct mmc_internal_data mmc;

This union seems redundant ;-)

Good point :-), but I predict, that DFU will be used to program other memory types (OneNAND, or NAND). To support those, one needs to extend the union with e.g struct onenand_internal_data onenand.

Since we don't have so many memory types, I think that union usage is acceptable.

Quoted 1 line

[...]

Dear Lukasz Majewski,

Quoted 47 lines

Hi Marek,

Quoted 36 lines

Dear Lukasz Majewski,

Quoted 7 lines

New, separate driver at ./drivers/dfu has been added. It allows platform and storage independent operation of DFU.

Signed-off-by Lukasz Majewski <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

Cc Marek Vasut <[email protected]>

---

[...]

Quoted 24 lines

drivers/dfu/dfu.c new file +24 -0

+#include <common.h>
+#include <malloc.h>
+#include <mmc.h>
+#include <fat.h>
+#include <dfu.h>
+#include <linux/list.h>
+#include <linux/compiler.h>
+
+static LIST_HEAD(dfu_list);
+static int dfu_alt_num;
+
+static int dfu_find_alt_num(char *s)
+{
+	int i = 0;
+
+	for (; *s; s++)
+		if (*s == ';')
+			i++;
+
+	return ++i;
+}
+
+static char *dfu_extract_entity(char** env)
+{

Quoted 8 lines

drivers/dfu/dfu.c line 51

Quoted 5 lines

drivers/dfu/dfu.c new file +5 -0

+	char *s = *env;
+
+	strsep(env, ";");
+	return s;
+}

Shall we not make these all generic? They seem to be quite helpful components.

Quoted 5 lines

It is a good topic for a discussion if those functions shall be moved to ./lib/string.c.
I regarded them as a "little" helper functions for parsing DFU alt setting env variable. Those are very short and build with methods exported from string.c

Good point

Quoted 19 lines

Quoted 10 lines

Quoted 10 lines

drivers/dfu/dfu.c new file +10 -0

+
+char *dfu_extract_token(char** e, int *n)
+{
+	char *st = *e;
+
+	debug("%s: %s\n", __func__, st);
+
+	strsep(e, " ");
+	*n = *e - st;
+

Quoted 8 lines

Quoted 6 lines

drivers/dfu/dfu.c new file +5 -0

+	return st;
+}
+
+static unsigned char __aligned(CONFIG_SYS_CACHELINE_SIZE)
+

dfu_buf[DFU_DATA_BUF_SIZE];

Can we not stack-allocate it with ALLOC_CACHE_ALIGN_BUFFER()?

Quoted 3 lines

The dfu_buf is 4 MiB (this is the size of DFU_DATA_BUF_SIZE). I don't think, that allocating it on the stack (for stack allocation the ALLOC_CACHE_ALIGN_BUFFER() is designed) is a good idea.

Heh, agreed :-)

Quoted 23 lines

Quoted 22 lines

[...]

Quoted 5 lines

include/dfu.h new file +0 -0

new file mode 100644
index 0000000..f7d823b

[...]

include/dfu.h line 71

Quoted 10 lines

include/dfu.h new file +10 -0

+struct dfu_entity {
+	char			name[DFU_NAME_SIZE];
+	int                     alt;
+	void                    *dev_private;
+	int                     dev_num;
+	enum dfu_device_type    dev_type;
+	enum dfu_layout         layout;
+
+	union {
+		struct mmc_internal_data mmc;

This union seems redundant ;-)

Quoted 6 lines

Good point :-), but I predict, that DFU will be used to program other memory types (OneNAND, or NAND). To support those, one needs to extend the union with e.g struct onenand_internal_data onenand.

Since we don't have so many memory types, I think that union usage is acceptable.

And will those pieces be implemented any soon ? :)

Quoted 2 lines

Quoted 2 lines

[...]

Best regards,
Marek Vasut

Quoted 2 lines

And will those pieces be implemented any soon ? :)

:
:-). Since I'm the OneNAND custodian I shall keep in the back of my

head, that support for this memory is important :-)

+static char *dfu_get_dev_type(enum dfu_device_type t)

+static char *dfu_get_layout(enum dfu_device_type l)

drivers/dfu/dfu.c line 148

Quoted 2 lines

drivers/dfu/dfu.c new file +2 -0

+		puts("UPLOAD ... done\n");
+		puts("Ctrl+C to exit ...\n");

combine into a single puts() ?

drivers/dfu/dfu.c line 167

Quoted 12 lines

+static int dfu_fill_entity(struct dfu_entity *dfu, char* s, int alt,

drivers/dfu/dfu.c new file +6 -0

+			    char *interface, int num)
+{
+	char *st = NULL;
+	int n = 0;
+
+	debug("%s: %s interface: %s num: %d\n", __func__, s,

interface, num); +

drivers/dfu/dfu.c new file +2 -0

+	st = dfu_extract_token(&s, &n);
+	strncpy((char *) &dfu->name, st, n);

what's with the pointless cast ? just do:

	strncpy(dfu->name, st, n);

also, "n" here is wrong. it should be sizeof(dfu->name), not (presumably) the length of st. considering this is the 2nd time i noticed this bug in the dfu patchset, you might want to do a grep on your patches to see if your other string related usage is wrong.

drivers/dfu/dfu.c line 209

Quoted 4 lines

drivers/dfu/dfu.c new file +3 -0

+void dfu_free_entities(void)
+{
+	struct dfu_entity *dfu = NULL, *p = NULL;

no point in assigning to NULL here

Quoted 2 lines

+static char *dfu_get_dev_type(enum dfu_device_type t)

static const char *...

Review

Quoted 3 lines

(quoted diff) +2 -0

+{
+	static char *dev_t[] = {NULL, "MMC", "ONENAND", "NAND" };

static const char * const dev_t[] = {...}

Quoted 2 lines

+static char *dfu_get_layout(enum dfu_device_type l)

static const char *...

Quoted 3 lines

(quoted diff) +2 -0

+{
+	static char *dfu_layout[] = {NULL, "RAW_ADDR", "FAT",

"EXT" };

static const char * const dfu_layout[] = {...}

drivers/dfu/dfu.c line 233

Quoted 3 lines

drivers/dfu/dfu.c new file +3 -0

+void dfu_show_entities(void)
+{
+	struct dfu_entity *dfu = NULL;

no point in assigning to NULL

drivers/dfu/dfu.c line 251

Quoted 4 lines

drivers/dfu/dfu.c new file +3 -0

+struct dfu_entity *dfu_get_entity(int alt)
+{
+	struct dfu_entity *dfu = NULL;

no point in assigning to NULL

-mike

Support for MMC storage devices to work with DFU framework.

Signed-off-by Lukasz Majewski <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

Cc Marek Vasut <[email protected]>

---

drivers/dfu/dfu_mmc.c new file +17 -0

+
+#include <common.h>
+#include <malloc.h>
+#include <dfu.h>
+
+int dfu_write_medium_mmc(struct dfu_entity *dfu, void *buf, long *len)
+{
+	ALLOC_CACHE_ALIGN_BUFFER(char, cmd_buf, DFU_CMD_BUF_SIZE);
+
+	memset(cmd_buf, '\0', sizeof(cmd_buf));
+
+	switch (dfu->layout) {
+	case RAW_ADDR:
+		sprintf(cmd_buf, "mmc write 0x%x %x %x", (unsigned int) buf,
+			dfu->data.mmc.lba_start, dfu->data.mmc.lba_size);
+		break;
+	case FAT:

Dear Lukasz Majewski,

Quoted 6 lines

Support for MMC storage devices to work with DFU framework.

Signed-off-by Lukasz Majewski <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

Cc Marek Vasut <[email protected]>

---

[...]

Quoted 22 lines

drivers/dfu/dfu_mmc.c new file +22 -0

+
+#include <common.h>
+#include <malloc.h>
+#include <dfu.h>
+
+int dfu_write_medium_mmc(struct dfu_entity *dfu, void *buf, long *len)
+{
+	ALLOC_CACHE_ALIGN_BUFFER(char, cmd_buf, DFU_CMD_BUF_SIZE);
+
+	memset(cmd_buf, '\0', sizeof(cmd_buf));
+
+	switch (dfu->layout) {
+	case RAW_ADDR:
+		sprintf(cmd_buf, "mmc write 0x%x %x %x", (unsigned int) buf,
+			dfu->data.mmc.lba_start, dfu->data.mmc.lba_size);
+		break;
+	case FAT:
+		sprintf(cmd_buf, "fatwrite mmc %d:%d 0x%x %s %lx",
+			dfu->data.mmc.dev, dfu->data.mmc.part,
+			(unsigned int) buf, dfu->name, *len);
+		break;
+	default:

drivers/dfu/dfu_mmc.c line 48

Quoted 5 lines

drivers/dfu/dfu_mmc.c new file +5 -0

+		printf("%s: Wrong layout!\n", __func__);
+	}
+
+	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
+	run_command(cmd_buf, 0);

Holy Moly ... can we not make this into simple calls to those subsystems ? Instead invoking command is crazy ;-)

Quoted 33 lines

Dear Lukasz Majewski,

Quoted 6 lines

Support for MMC storage devices to work with DFU framework.

Signed-off-by Lukasz Majewski <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

Cc Marek Vasut <[email protected]>

---

[...]

Quoted 22 lines

drivers/dfu/dfu_mmc.c new file +22 -0

+
+#include <common.h>
+#include <malloc.h>
+#include <dfu.h>
+
+int dfu_write_medium_mmc(struct dfu_entity *dfu, void *buf, long *len)
+{
+	ALLOC_CACHE_ALIGN_BUFFER(char, cmd_buf, DFU_CMD_BUF_SIZE);
+
+	memset(cmd_buf, '\0', sizeof(cmd_buf));
+
+	switch (dfu->layout) {
+	case RAW_ADDR:
+		sprintf(cmd_buf, "mmc write 0x%x %x %x", (unsigned int) buf,
+			dfu->data.mmc.lba_start, dfu->data.mmc.lba_size);
+		break;
+	case FAT:
+		sprintf(cmd_buf, "fatwrite mmc %d:%d 0x%x %s %lx",
+			dfu->data.mmc.dev, dfu->data.mmc.part,
+			(unsigned int) buf, dfu->name, *len);
+		break;
+	default:

Quoted 8 lines

drivers/dfu/dfu_mmc.c line 48

Quoted 5 lines

drivers/dfu/dfu_mmc.c new file +5 -0

+		printf("%s: Wrong layout!\n", __func__);
+	}
+
+	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
+	run_command(cmd_buf, 0);

Holy Moly ... can we not make this into simple calls to those subsystems ? Instead invoking command is crazy ;-)

Are they really simple? There's a few other places we do this, and so long as it's documented that DFU depends on CONFIG_FAT_WRITE for writing to fat and so forth.

Dear Tom Rini,

Quoted 43 lines · Marek Vasut

Quoted 41 lines

Dear Lukasz Majewski,

Quoted 6 lines

Support for MMC storage devices to work with DFU framework.

Signed-off-by Lukasz Majewski <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

Cc Marek Vasut <[email protected]>

---

[...]

Quoted 17 lines

drivers/dfu/dfu_mmc.c new file +17 -0

+
+#include <common.h>
+#include <malloc.h>
+#include <dfu.h>
+
+int dfu_write_medium_mmc(struct dfu_entity *dfu, void *buf, long *len)
+{
+	ALLOC_CACHE_ALIGN_BUFFER(char, cmd_buf, DFU_CMD_BUF_SIZE);
+
+	memset(cmd_buf, '\0', sizeof(cmd_buf));
+
+	switch (dfu->layout) {
+	case RAW_ADDR:
+		sprintf(cmd_buf, "mmc write 0x%x %x %x", (unsigned int) buf,
+			dfu->data.mmc.lba_start, dfu->data.mmc.lba_size);
+		break;
+	case FAT:

drivers/dfu/dfu_mmc.c line 48

Quoted 10 lines

drivers/dfu/dfu_mmc.c new file +10 -0

+		sprintf(cmd_buf, "fatwrite mmc %d:%d 0x%x %s %lx",
+			dfu->data.mmc.dev, dfu->data.mmc.part,
+			(unsigned int) buf, dfu->name, *len);
+		break;
+	default:
+		printf("%s: Wrong layout!\n", __func__);
+	}
+
+	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
+	run_command(cmd_buf, 0);

Holy Moly ... can we not make this into simple calls to those subsystems ? Instead invoking command is crazy ;-)

Quoted 3 lines

Are they really simple? There's a few other places we do this, and so long as it's documented that DFU depends on CONFIG_FAT_WRITE for writing to fat and so forth.

Well ain't it easier to call fat_write() or similar?

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

drivers/dfu/dfu_mmc.c line 48

Quoted 2 lines

drivers/dfu/dfu_mmc.c new file +2 -0

+	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
+	run_command(cmd_buf, 0);

Holy Moly ... can we not make this into simple calls to those subsystems ? Instead invoking command is crazy ;-)

Quoted 5 lines

Quoted 3 lines

Are they really simple? There's a few other places we do this, and so long as it's documented that DFU depends on CONFIG_FAT_WRITE for writing to fat and so forth.

Well ain't it easier to call fat_write() or similar?

Assuming that most of the logic in do_fat_fswrite is needed, no. And I think a good portion of it is, at first glance at least.

Dear Tom Rini,

[...]

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Quoted 5 lines

drivers/dfu/dfu_mmc.c line 48

Quoted 2 lines

drivers/dfu/dfu_mmc.c new file +2 -0

+	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
+	run_command(cmd_buf, 0);

Holy Moly ... can we not make this into simple calls to those subsystems ? Instead invoking command is crazy ;-)

Are they really simple? There's a few other places we do this, and so long as it's documented that DFU depends on CONFIG_FAT_WRITE for writing to fat and so forth.

Quoted 4 lines

Quoted 1 line

Well ain't it easier to call fat_write() or similar?

Assuming that most of the logic in do_fat_fswrite is needed, no. And I think a good portion of it is, at first glance at least.

Abstracting it out into a function won't cut it?

Quoted 16 lines · Marek Vasut

Dear Tom Rini,

[...]

Quoted 12 lines

Quoted 11 lines

Quoted 9 lines

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
+	run_command(cmd_buf, 0);

Holy Moly ... can we not make this into simple calls to those subsystems ? Instead invoking command is crazy ;-)

Are they really simple? There's a few other places we do this, and so long as it's documented that DFU depends on CONFIG_FAT_WRITE for writing to fat and so forth.

Well ain't it easier to call fat_write() or similar?

Quoted 4 lines

Quoted 2 lines

Assuming that most of the logic in do_fat_fswrite is needed, no. And I think a good portion of it is, at first glance at least.

Abstracting it out into a function won't cut it?

My inclination would be to say that seems a bit sillier than just using run_command(...) to call the existing command.

Quoted 22 lines · Tom Rini

Quoted 20 lines · Marek Vasut

Dear Tom Rini,

[...]

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
+	run_command(cmd_buf, 0);

Holy Moly ... can we not make this into simple calls to those subsystems ? Instead invoking command is crazy ;-)

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

Are they really simple? There's a few other places we do this, and so long as it's documented that DFU depends on CONFIG_FAT_WRITE for writing to fat and so forth.

Well ain't it easier to call fat_write() or similar?

Assuming that most of the logic in do_fat_fswrite is needed, no. And I think a good portion of it is, at first glance at least.

Abstracting it out into a function won't cut it?

Quoted 2 lines

My inclination would be to say that seems a bit sillier than just using run_command(...) to call the existing command.

Abstracting into a function definitely means the compiler will be able to type-check all the arguments to the function. Is the same true using run_command; I assume everything gets serialized to an array of strings and hence any validation is deferred until run-time then?

Quoted 23 lines · Stephen Warren

Quoted 22 lines · Tom Rini

Quoted 20 lines · Marek Vasut

Dear Tom Rini,

[...]

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
+	run_command(cmd_buf, 0);

Holy Moly ... can we not make this into simple calls to those subsystems ? Instead invoking command is crazy ;-)

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

Are they really simple? There's a few other places we do this, and so long as it's documented that DFU depends on CONFIG_FAT_WRITE for writing to fat and so forth.

Well ain't it easier to call fat_write() or similar?

Assuming that most of the logic in do_fat_fswrite is needed, no. And I think a good portion of it is, at first glance at least.

Abstracting it out into a function won't cut it?

Quoted 7 lines

Quoted 2 lines

My inclination would be to say that seems a bit sillier than just using run_command(...) to call the existing command.

Abstracting into a function definitely means the compiler will be able to type-check all the arguments to the function. Is the same true using run_command; I assume everything gets serialized to an array of strings and hence any validation is deferred until run-time then?

string is constructed with sprintf and that gives us some type checking. Only cast today is buf to an unsigned int. And the information either comes from tools that should/must have done sanity checking or from the 'dfu' command which again should (didn't verify that patch yet) have done sanity checking to get us this far.

Quoted 16 lines

Dear Tom Rini,

[...]

Quoted 12 lines

Quoted 11 lines

Quoted 9 lines

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
+	run_command(cmd_buf, 0);

Holy Moly ... can we not make this into simple calls to those subsystems ? Instead invoking command is crazy ;-)

Are they really simple? There's a few other places we do this, and so long as it's documented that DFU depends on CONFIG_FAT_WRITE for writing to fat and so forth.

Well ain't it easier to call fat_write() or similar?

Quoted 4 lines

Quoted 2 lines

Assuming that most of the logic in do_fat_fswrite is needed, no. And I think a good portion of it is, at first glance at least.

Abstracting it out into a function won't cut it?

My inclination would be to say that seems a bit sillier than just using run_command(...) to call the existing command.

Dear Tom Rini,

Quoted 17 lines · Marek Vasut

Quoted 16 lines

Dear Tom Rini,

Quoted 14 lines

Quoted 11 lines

Quoted 9 lines

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
+	run_command(cmd_buf, 0);

Holy Moly ... can we not make this into simple calls to those subsystems ? Instead invoking command is crazy ;-)

Are they really simple? There's a few other places we do this, and so long as it's documented that DFU depends on CONFIG_FAT_WRITE for writing to fat and so forth.

Well ain't it easier to call fat_write() or similar?

Assuming that most of the logic in do_fat_fswrite is needed, no. And I think a good portion of it is, at first glance at least.

Quoted 4 lines

Quoted 1 line

Abstracting it out into a function won't cut it?

My inclination would be to say that seems a bit sillier than just using run_command(...) to call the existing command.

Whose's syntax will likely soon change, breaking all this :-(