On 07/02/2015 09:29 PM, Michal Nazarewicz wrote:
On Thu, Jul 02 2015, Krzysztof Opasiak wrote:
This patch replace dynamicly allocated luns array with static one.
This simplifies the code of mass storage function and modules.
It also fix issue with reporting wrong number of LUNs in GET_MAX_LUN
request.
Also change the nluns to max_luns which is better name for value
stored in that place as we no longer need to store size of luns
array.
Reported-by: David Fisher <david.fisher1@xxxxxxxxxxxx>
Signed-off-by: Krzysztof Opasiak <k.opasiak@xxxxxxxxxxx>
---
drivers/usb/gadget/function/f_mass_storage.c | 125 ++++++++++----------------
drivers/usb/gadget/function/f_mass_storage.h | 4 -
drivers/usb/gadget/legacy/acm_ms.c | 6 --
drivers/usb/gadget/legacy/mass_storage.c | 6 --
drivers/usb/gadget/legacy/multi.c | 6 --
5 files changed, 48 insertions(+), 99 deletions(-)
diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index ad0f69b..befe251 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -279,9 +279,9 @@ struct fsg_common {
int cmnd_size;
u8 cmnd[MAX_COMMAND_SIZE];
- unsigned int nluns;
+ int max_lun;
To be honest, I don’t like this change. It is more idiomatic to store
number of elements in an array rather than index of the last element.
Sooner or later someone will do for (i = 0; i < common->max_lun; ++i)
out of habit.
The reason why I did this was allowing not contiguous LUNs ids. As you
suggested in earlier discussion [1] we should allow user space shoot
themselves in the foot and tis is one of the consequence. What we should
really return in GET_MAX_LUN is last valid LUN id in our array, not
number of LUNs. Windows is paying attention to this value and for
examnple for luns: 0 5 6 it makes a different if you reply with nluns -1
or max_lun. In first option windows will see only one LUN while in
second all three of them.
As we increment max_lun only when adding a LUN currently it is not
possible to get out of bounds.
unsigned int lun;
- struct fsg_lun **luns;
+ struct fsg_lun *luns[FSG_MAX_LUNS];
struct fsg_lun *curlun;
unsigned int bulk_out_maxpacket;
@@ -2760,48 +2767,16 @@ static void _fsg_common_remove_luns(struct fsg_common *common, int n)
fsg_common_remove_lun(common->luns[i], common->sysfs);
common->luns[i] = NULL;
}
+
+ _fsg_common_reduce_max_lun(common);
}
void fsg_common_remove_luns(struct fsg_common *common)
{
- _fsg_common_remove_luns(common, common->nluns);
+ _fsg_common_remove_luns(common, common->max_lun);
Shouldn’t this be:
+ _fsg_common_remove_luns(common, common->max_lun + 1);
agree, should be fixed.
}
EXPORT_SYMBOL_GPL(fsg_common_remove_luns);
@@ -2954,7 +2931,7 @@ error_lun:
if (common->sysfs)
device_unregister(&lun->dev);
fsg_lun_close(lun);
- common->luns[id] = NULL;
You need to keep this line.
This one is also true. Sorry, my mistake...
+ _fsg_common_reduce_max_lun(common);
error_sysfs:
kfree(lun);
return rc;
@@ -3305,11 +3282,9 @@ static struct config_group *fsg_lun_make(struct config_group *group,
return ERR_PTR(ret);
fsg_opts = to_fsg_opts(&group->cg_item);
- if (num >= FSG_MAX_LUNS)
- return ERR_PTR(-ERANGE);
Going through all the memory allocation and mutex locking just to find
out that num is to big seems wasteful. I’d keep this check here.
Ok I will leave it as it was before
.
mutex_lock(&fsg_opts->lock);
- if (fsg_opts->refcnt || fsg_opts->common->luns[num]) {
+ if (fsg_opts->refcnt) {
ret = -EBUSY;
goto out;
}
1 - http://marc.info/?l=linux-usb&m=143498348620786&w=2
--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
