On Thu, 22 Dec 2011 02:21:17 +0100, Benoit Goby <benoit@xxxxxxxxxxx> wrote:
+static ssize_t rndis_ethaddr_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t size)
+{
+ struct android_usb_function *f = dev_get_drvdata(dev);
+ struct rndis_function_config *rndis = f->config;
+
+ if (sscanf(buf, "%02x:%02x:%02x:%02x:%02x:%02x\n",
+ (int *)&rndis->ethaddr[0], (int *)&rndis->ethaddr[1],
+ (int *)&rndis->ethaddr[2], (int *)&rndis->ethaddr[3],
+ (int *)&rndis->ethaddr[4], (int *)&rndis->ethaddr[5]) == 6)
Uhm, no.
First of all, if only some of the values are provided or valid only part of
the MAC address gets set (even though function returns -EINVAL). Second of
all rndis->ethaddr is of type u8 and casting u8* to int* is a very bad idea
here since setting one of the fields will overwrite 3 bytes around.
What you need is:
unsigned char tmp[6];
unsigned i;
if (sscanf(buf, "%hhx:%hhx:%hhx:%hhx:%hhx:%hhx",
tmp + 0, tmp + 1, tmp + 2, tmp + 3, tmp + 4, tmp + 5) !=
ETH_ALEN)
return -EINVAL;
for (i = 0; i < ETH_ALEN; ++i)
rndis->ethaddr[i] = tmp[i]
return 0;
(You can get away with memcpy as well.)
+ return size;
+ return -EINVAL;
+}
+struct mass_storage_function_config {
+ struct fsg_config fsg;
+ struct fsg_common *common;
+};
You should be able to throw “struct fsg_config” when “fsg_common” is created,
so this looks like a waste of space to me.
+static int mass_storage_function_init(struct android_usb_function *f,
+ struct usb_composite_dev *cdev)
+{
+ struct mass_storage_function_config *config;
+ struct fsg_common *common;
+ int err;
+
+ config = kzalloc(sizeof(struct mass_storage_function_config),
+ GFP_KERNEL);
How about “config = kzalloc(sizeof *config, GFP_KERNEL)”?
+ if (!config)
+ return -ENOMEM;
+
+ config->fsg.nluns = 1;
+ config->fsg.luns[0].removable = 1;
+
+ common = fsg_common_init(NULL, cdev, &config->fsg);
+ if (IS_ERR(common)) {
+ kfree(config);
+ return PTR_ERR(common);
+ }
+
+ err = sysfs_create_link(&f->dev->kobj,
+ &common->luns[0].dev.kobj,
+ "lun");
+ if (err) {
+ kfree(config);
common is not freed, fsg_common_put(common) is needed here.
+ return err;
+ }
+
+ config->common = common;
+ f->config = config;
Anyway, as described above, I'd do “f->config = common” here and get rid of the
“struct mass_storage_function_config” all together. Arguably, “struct fsg_config”
could be created on stack to further simplify the code.
+ return 0;
+}
+
+static void mass_storage_function_cleanup(struct android_usb_function *f)
+{
It looks like it is missing fsg_common_put(f->config->common). Or am I missing something?
+ kfree(f->config);
+ f->config = NULL;
+}
+static ssize_t mass_storage_inquiry_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct android_usb_function *f = dev_get_drvdata(dev);
+ struct mass_storage_function_config *config = f->config;
+ return sprintf(buf, "%s\n", config->common->inquiry_string);
+}
+
+static ssize_t mass_storage_inquiry_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t size)
+{
+ struct android_usb_function *f = dev_get_drvdata(dev);
+ struct mass_storage_function_config *config = f->config;
+ if (size >= sizeof(config->common->inquiry_string))
+ return -EINVAL;
+ if (sscanf(buf, "%s", config->common->inquiry_string) != 1)
+ return -EINVAL;
+ return size;
+}
+
+static DEVICE_ATTR(inquiry_string, S_IRUGO | S_IWUSR,
+ mass_storage_inquiry_show,
+ mass_storage_inquiry_store);
Have you thought about adding that to f_mass_storage.c so that everyone can use it?
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +----<email/xmpp: mpn@xxxxxxxxxx>--------------ooO--(_)--Ooo--
--
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
