Re: [PATCH v4] usb: gadget: storage_common: make FSG_NUM_BUFFERS variable size

[Per Forlin <per.forlin@linaro.org>]

2011/8/18 Michal Nazarewicz <mina86@mina86.com>:
> On Thu, 18 Aug 2011 11:28:46 +0200, Per Forlin wrote:
>>
>> diff --git a/drivers/usb/gadget/f_mass_storage.c
>> b/drivers/usb/gadget/f_mass_storage.c
>> index 5b93395..3e546d9 100644
>> --- a/drivers/usb/gadget/f_mass_storage.c
>> +++ b/drivers/usb/gadget/f_mass_storage.c
>> @@ -363,7 +363,6 @@ struct fsg_common {
>>        struct fsg_buffhd       *next_buffhd_to_fill;
>>        struct fsg_buffhd       *next_buffhd_to_drain;
>> -       struct fsg_buffhd       buffhds[FSG_NUM_BUFFERS];
>>        int                     cmnd_size;
>>        u8                      cmnd[MAX_COMMAND_SIZE];
>> @@ -407,6 +406,8 @@ struct fsg_common {
>>        char inquiry_string[8 + 16 + 4 + 1];
>>        struct kref             ref;
>> +       /* Must be the last entry */
>> +       struct fsg_buffhd       buffhds[0];
>
> I would rather see it as “struct fsg_buffhd *buffhds;” since this change
> requires both mass_storage.c and multi.c to be changed.
>
If the allocation of buffhds is done separately in fsg_common_init().
mass_storage.c and multi.c doesn't need to be changed. But it's little
tricky to know whether buffhds should be allocated or not.

if (!common->buffhds)
  common->buffhds = kzalloc()
This works fine if the common is declared static since all data is 0
by default. If common is allocated by kmalloc and then passed to
fsg_commin_init() this check isn't reliable.
memset of common will erase buffhds pointer as well. A minor issue,
storing this pointer before running memset will fix it. I would like
to propose a different approach.

+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -363,7 +363,7 @@ struct fsg_common {
-	struct fsg_buffhd	buffhds[FSG_NUM_BUFFERS];
+	struct fsg_buffhd	buffhds[CONFIG_USB_GADGET_STORAGE_NUM_BUFFERS];

+++ b/drivers/usb/gadget/file_storage.c
@@ -461,7 +461,7 @@ struct fsg_dev {
-	struct fsg_buffhd	buffhds[FSG_NUM_BUFFERS];
+	struct fsg_buffhd	buffhds[CONFIG_USB_GADGET_STORAGE_NUM_BUFFERS];

+++ b/drivers/usb/gadget/storage_common.c
@@ -52,6 +52,12 @@
+/*
+ * There is a num_buffers module param when USB_GADGET_DEBUG is defined.
+ * This parameter sets the length of the fsg_buffhds array.
+ * The valid range of num_buffers is:
+ * num >= 2 && num <= CONFIG_USB_GADGET_STORAGE_NUM_BUFFERS.
+ */

+#ifdef CONFIG_USB_GADGET_DEBUG_FILES
I am in favor of #ifdef some Kconfig option. This simplifies for
automated build/tests farms where def_configs are being used to
configure the system.
This option should not affect the performance significantly.

+
+static unsigned int fsg_num_buffers = CONFIG_USB_GADGET_STORAGE_NUM_BUFFERS;
+module_param_named(num_buffers, fsg_num_buffers, uint, S_IRUGO);
+MODULE_PARM_DESC(fsg_num_buffers, "Number of pipeline buffers");
+
+#else
+
+/*
+ * Number of buffers we will use.
+ * 2 is usually enough for good buffering pipeline
+ */
+#define fsg_num_buffers	CONFIG_USB_GADGET_STORAGE_NUM_BUFFERS
+
+#endif /* CONFIG_USB_DEBUG */
+
+#define FSG_NUM_BUFFERS_IS_VALID(num) ((num) >= 2 && \
+		(num) <= CONFIG_USB_GADGET_STORAGE_NUM_BUFFERS)

Keep the length of the buffhds array constant. Use a variable
fsg_num_buffers when iterating that array.
This minimize the code to change. But to the price of using
CONFIG_USB_GADGET_STORAGE_NUM_BUFFERS to declare
and fsg_num_buffers to access.

Is this proposal better or worse?

Thanks,
Per