Re: [PATCH v2 08/21] usb/gadget: f_mass_storage: split fsg_common initialization into a number of functions
On Fri, Jul 19 2013, Andrzej Pietrasiewicz wrote:
When configfs is in place, the things related to intialization
of struct fsg_common will be split over a number of places.
This patch adds several functions which together cover the former
intialization routine fsg_common_init.
When configfs is in place, the luns will not be represented in sysfs,
so there will be no struct device associated with a lun.
To prepare for this some debug macros need to be adjusted. Two new
fields are added to struct fsg_lun: name and name_pfx.
The name is for storing a string which is presented to the user
instead of the dev_name. The name_pfx, if non-NULL, is prepended
to the name at printing time.
The name_pfx is for a future lun.0, which will be a default group in
mass_storage.name. By design at USB function configfs group's creation
time its name is not known (but instead set a bit later in
drivers/usb/gadget/configfs.c:function_make) and it is this name that
serves the purpose of the said name prefix. So instead of copying
a yet-unknown string a pointer to it is stored in struct fsg_lun.
Signed-off-by: Andrzej Pietrasiewicz andrze...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
+int fsg_common_set_num_buffers(struct fsg_common *common, unsigned int n)
+{
+ struct fsg_buffhd *bh, *new_buffhds;
+ int i, rc;
+
+ rc = fsg_num_buffers_validate(n);
+ if (rc != 0)
+ return rc;
+
+ new_buffhds = kcalloc(n, sizeof *(new_buffhds), GFP_KERNEL);
+ if (!new_buffhds)
+ return -ENOMEM;
+
+ /* Data buffers cyclic list */
+ bh = new_buffhds;
+ i = n;
+ goto buffhds_first_it;
+ do {
+ bh-next = bh + 1;
+ ++bh;
+buffhds_first_it:
+ bh-buf = kmalloc(FSG_BUFLEN, GFP_KERNEL);
+ if (unlikely(!bh-buf))
+ goto error_release;
+ } while (--i);
+ bh-next = new_buffhds;
+
+ common-fsg_num_buffers = n;
+ kfree(common-buffhds);
Instead of kfree, fsg_common_free_buffers must be called here or
otherwise bh-buf won't get freed. In fact, I'd create an internal
implementation:
static void _fsg_common_free_buffers(struct fsg_buffhd *buffhds,
unsigned n) {
if (buffhds) {
struct fsg_buffhd *bh = buffhds;
while (n--) {
kfree(bh-buf);
++bh;
}
kfree(buffhds);
}
}
which could be used here, in error recovery below (error_release label),
and fsg_common_free_buffers().
+ common-buffhds = new_buffhds;
+
+ return 0;
+
+error_release:
+ bh = new_buffhds;
+ i = n - i;
+ while (i--) {
+ kfree(bh-buf);
+ bh++;
+ };
+
+ kfree(new_buffhds);
+
+ return -ENOMEM;
+}
+
+void fsg_common_free_buffers(struct fsg_common *common)
+{
+ struct fsg_buffhd *bh;
+ int i;
+
+ bh = common-buffhds;
+ i = common-fsg_num_buffers;
+ while (i--) {
+ kfree(bh-buf);
+ bh++;
+ };
+
+ kfree(common-buffhds);
+ common-buffhds = NULL;
+}
+int fsg_common_set_nluns(struct fsg_common *common, int nluns)
+{
+ struct fsg_lun **curlun;
+
+ /* Find out how many LUNs there should be */
+ if (nluns 1 || nluns FSG_MAX_LUNS) {
+ pr_err(invalid number of LUNs: %u\n, nluns);
+ return -EINVAL;
+ }
+
+ curlun = kcalloc(nluns, sizeof(*curlun), GFP_KERNEL);
+ if (unlikely(!curlun))
+ return -ENOMEM;
+
+ common-luns = curlun;
+ common-nluns = nluns;
+
+ pr_info(Number of LUNs=%d\n, common-nluns);
+
+ return 0;
This function is fishy. What if someone calls it after bunch of LUNs is
created? If that happens, those luns are lost. At the very least, it
should check if common-luns is NULL and if it is, return -EBUSY or
something.
+}
+
+void fsg_common_free_luns(struct fsg_common *common)
+{
+ kfree(common-luns);
+ common-luns = NULL;
Hmm... This should at least check if any of the common-luns[i] is not
NULL, no? To be on the safe side, I'd just call fsg_common_remove_luns
at the beginning and change fsg_common_remove_luns to set the pointers
to NULL and check whether they are NULL before removing.
+}
+void fsg_common_remove_luns(struct fsg_common *common)
+{
+ int i;
+
+ for (i = 0; i common-nluns; ++i)
+ fsg_common_remove_lun(common-luns[i], common-sysfs);
+}
+int fsg_common_create_lun(struct fsg_common *common, struct fsg_lun_config
*cfg,
+ unsigned int id, const char *name,
+ const char **name_pfx)
+{
+ struct fsg_lun *lun;
+ char *pathbuf;
+ int rc = -ENOMEM;
+ int name_len;
+
+ if (!common-nluns || !common-luns)
+ return -ENODEV;
+
+ if
[PATCH v2 08/21] usb/gadget: f_mass_storage: split fsg_common initialization into a number of functions
When configfs is in place, the things related to intialization
of struct fsg_common will be split over a number of places.
This patch adds several functions which together cover the former
intialization routine fsg_common_init.
When configfs is in place, the luns will not be represented in sysfs,
so there will be no struct device associated with a lun.
To prepare for this some debug macros need to be adjusted. Two new
fields are added to struct fsg_lun: name and name_pfx.
The name is for storing a string which is presented to the user
instead of the dev_name. The name_pfx, if non-NULL, is prepended
to the name at printing time.
The name_pfx is for a future lun.0, which will be a default group in
mass_storage.name. By design at USB function configfs group's creation
time its name is not known (but instead set a bit later in
drivers/usb/gadget/configfs.c:function_make) and it is this name that
serves the purpose of the said name prefix. So instead of copying
a yet-unknown string a pointer to it is stored in struct fsg_lun.
Signed-off-by: Andrzej Pietrasiewicz andrze...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
drivers/usb/gadget/f_mass_storage.c | 430 +-
drivers/usb/gadget/f_mass_storage.h | 33 +++
drivers/usb/gadget/storage_common.h | 42 +++-
3 files changed, 488 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/gadget/f_mass_storage.c
b/drivers/usb/gadget/f_mass_storage.c
index c56e24b..f1b0da8 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -299,6 +299,7 @@ struct fsg_common {
unsigned intshort_packet_received:1;
unsigned intbad_lun_okay:1;
unsigned intrunning:1;
+ unsigned intsysfs:1;
int thread_wakeup_needed;
struct completion thread_notifier;
@@ -2606,6 +2607,395 @@ static inline int fsg_num_buffers_validate(unsigned int
fsg_num_buffers)
return -EINVAL;
}
+static struct fsg_common *fsg_common_setup(struct fsg_common *common, bool
zero)
+{
+ if (!common) {
+ common = kzalloc(sizeof(*common), GFP_KERNEL);
+ if (!common)
+ return ERR_PTR(-ENOMEM);
+ common-free_storage_on_release = 1;
+ } else {
+ if (zero)
+ memset(common, 0, sizeof(*common));
+ common-free_storage_on_release = 0;
+ }
+ init_rwsem(common-filesem);
+ spin_lock_init(common-lock);
+ kref_init(common-ref);
+ init_completion(common-thread_notifier);
+ init_waitqueue_head(common-fsg_wait);
+ common-state = FSG_STATE_TERMINATED;
+
+ return common;
+}
+
+void fsg_common_set_sysfs(struct fsg_common *common, bool sysfs)
+{
+ common-sysfs = sysfs;
+}
+
+int fsg_common_set_num_buffers(struct fsg_common *common, unsigned int n)
+{
+ struct fsg_buffhd *bh, *new_buffhds;
+ int i, rc;
+
+ rc = fsg_num_buffers_validate(n);
+ if (rc != 0)
+ return rc;
+
+ new_buffhds = kcalloc(n, sizeof *(new_buffhds), GFP_KERNEL);
+ if (!new_buffhds)
+ return -ENOMEM;
+
+ /* Data buffers cyclic list */
+ bh = new_buffhds;
+ i = n;
+ goto buffhds_first_it;
+ do {
+ bh-next = bh + 1;
+ ++bh;
+buffhds_first_it:
+ bh-buf = kmalloc(FSG_BUFLEN, GFP_KERNEL);
+ if (unlikely(!bh-buf))
+ goto error_release;
+ } while (--i);
+ bh-next = new_buffhds;
+
+ common-fsg_num_buffers = n;
+ kfree(common-buffhds);
+ common-buffhds = new_buffhds;
+
+ return 0;
+
+error_release:
+ bh = new_buffhds;
+ i = n - i;
+ while (i--) {
+ kfree(bh-buf);
+ bh++;
+ };
+
+ kfree(new_buffhds);
+
+ return -ENOMEM;
+}
+
+void fsg_common_free_buffers(struct fsg_common *common)
+{
+ struct fsg_buffhd *bh;
+ int i;
+
+ bh = common-buffhds;
+ i = common-fsg_num_buffers;
+ while (i--) {
+ kfree(bh-buf);
+ bh++;
+ };
+
+ kfree(common-buffhds);
+ common-buffhds = NULL;
+}
+
+int fsg_common_set_nluns(struct fsg_common *common, int nluns)
+{
+ struct fsg_lun **curlun;
+
+ /* Find out how many LUNs there should be */
+ if (nluns 1 || nluns FSG_MAX_LUNS) {
+ pr_err(invalid number of LUNs: %u\n, nluns);
+ return -EINVAL;
+ }
+
+ curlun = kcalloc(nluns, sizeof(*curlun), GFP_KERNEL);
+ if (unlikely(!curlun))
+ return -ENOMEM;
+
+ common-luns = curlun;
+ common-nluns = nluns;
+
+ pr_info(Number of LUNs=%d\n, common-nluns);
+
+ return 0;
+}
+
+void fsg_common_free_luns(struct fsg_common *common)
+{
+ kfree(common-luns);
+ common-luns
