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

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.

Alternatively, revert the possibility for fsg_common_init() to take struct fsg_common as an argument, but that would be more intrusive I think and I would prefer the former.

}; struct fsg_config { @@ -2728,12 +2729,15 @@ static struct fsg_common *fsg_common_init(struct fsg_common *common, /* Allocate? */ if (!common) {

• common = kzalloc(sizeof *common, GFP_KERNEL);
  

• common = kzalloc(sizeof(struct fsg_common) +
  

• 		 sizeof(struct fsg_buffhd) * FSG_NUM_BUFFERS,
  

• 		 GFP_KERNEL);
  

Better yet:

kzalloc(sizeof *common + FSG_NUM_BUFFERS * sizeof *(common->buffhds), GFP_KERNEL);

but like I've said, I would prefer that buffhds is a separate array.

diff --git a/drivers/usb/gadget/file_storage.c b/drivers/usb/gadget/file_storage.c index 639e14a..21d366d 100644 --- a/drivers/usb/gadget/file_storage.c +++ b/drivers/usb/gadget/file_storage.c @@ -461,7 +461,6 @@ struct fsg_dev { struct fsg_buffhd *next_buffhd_to_fill; struct fsg_buffhd *next_buffhd_to_drain;

• struct fsg_buffhd buffhds[FSG_NUM_BUFFERS]; int thread_wakeup_needed; struct completion thread_notifier;

@@ -488,6 +487,8 @@ struct fsg_dev { unsigned int nluns; struct fsg_lun *luns; struct fsg_lun *curlun;

• /* Must be the last entry */
• struct fsg_buffhd buffhds[0];

IIRC, C99's way of doing it is: “struct fsg_buffhd buffhds[];”.

}; typedef void (*fsg_routine_t)(struct fsg_dev *); @@ -3587,7 +3588,9 @@ static int __init fsg_alloc(void) { struct fsg_dev *fsg;

• fsg = kzalloc(sizeof *fsg, GFP_KERNEL);

• fsg = kzalloc(sizeof(struct fsg_dev) +

•       sizeof(struct fsg_buffhd) * FSG_NUM_BUFFERS, GFP_KERNEL);
  

Better yet:

kzalloc(sizeof *fsg + FSG_NUM_BUFFERS * sizeof *(fsg->buffhds), GFP_KERNEL);

if (!fsg) return -ENOMEM; spin_lock_init(&fsg->lock);

diff --git a/drivers/usb/gadget/mass_storage.c b/drivers/usb/gadget/mass_storage.c index d3eb274..67c58d0 100644 --- a/drivers/usb/gadget/mass_storage.c +++ b/drivers/usb/gadget/mass_storage.c @@ -116,9 +116,8 @@ static int __init msg_do_config(struct usb_configuration *c) static const struct fsg_operations ops = { .thread_exits = msg_thread_exits, };

• static struct fsg_common common;

• struct fsg_common *common = NULL;

• struct fsg_common *retp; struct fsg_config config; int ret;

@@ -130,12 +129,12 @@ static int __init msg_do_config(struct usb_configuration *c) fsg_config_from_params(&config, &mod_data); config.ops = &ops;

• retp = fsg_common_init(&common, c->cdev, &config);
• if (IS_ERR(retp))

• return PTR_ERR(retp);
  

• common = fsg_common_init(common, c->cdev, &config);

The first argument should be NULL.

• if (IS_ERR(common))

• return PTR_ERR(common);
  

• ret = fsg_bind_config(c->cdev, c, &common);
• fsg_common_put(&common);

• ret = fsg_bind_config(c->cdev, c, common);
• fsg_common_put(common); return ret;

}

diff --git a/drivers/usb/gadget/storage_common.c b/drivers/usb/gadget/storage_common.c index d3dd227..197eace 100644 --- a/drivers/usb/gadget/storage_common.c +++ b/drivers/usb/gadget/storage_common.c @@ -262,8 +262,24 @@ static struct fsg_lun *fsg_lun_from_dev(struct device *dev) #define EP0_BUFSIZE 256 #define DELAYED_STATUS (EP0_BUFSIZE + 999) /* An impossibly large value */ -/* Number of buffers we will use. 2 is enough for double-buffering */ -#define FSG_NUM_BUFFERS 2 +#ifdef CONFIG_USB_DEBUG

+static unsigned int fsg_num_buffers = CONFIG_USB_GADGET_STORAGE_NUM_BUFFERS; +module_param(fsg_num_buffers, uint, S_IRUGO); +MODULE_PARM_DESC(fsg_num_buffers, "Number of pipeline buffers"); +#define FSG_NUM_BUFFERS fsg_num_buffers

Could we get rid of the macro all together, and just stick to “fsg_num_buffers” variable? Upper case name suggest that it's a compile time constant which is not true.

+#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

Similarly, I'd change the name to lowercase as to reflect that sometimes it may be not a compile-time constant.

+#endif /* CONFIG_USB_DEBUG */

+#define FSG_NUM_BUFFERS_IS_VALID(num) (num >= 2 && num <= 4 ? true : false)

The “?:” is not needed.

#define FSG_NUM_BUFFERS_IS_VALID(num) ((num) >= 2 && (num) <= 4)