Hello Sebastian,
As far as many of your comments are concerned, I agree and do not reply
to them. For others I agree and reply, and for yet others I reply ;)
Please see inline.
On Thursday, November 22, 2012 5:57 PM Sebastian Andrzej Siewior wrote:
> * Andrzej Pietrasiewicz | 2012-11-22 13:06:56 [+0100]:
>
<snip>
> >echo <some file>.img >
> >/cfg/usb-function-gadget/G1/C1/F1/MassStorage/lun0/file
> >
> >Do the similar thing to other functions, then
> >
> >echo 1 > /cfg/usb-function-gadget/G1/ready
>
> The upper part (left out) sounds okay. I don't comment on the names. One
> thing you miss: Lets say you have C1 and C2. How do you configure the same
> F1 in C1 and C2 _and_ how do you configure a different F1 in F1 and
> F2 in C2. I guess the latter will work just now but the former example
> won't. The former example is used by the nokia gadget, the latter by
> g_serial.
The general idea is that:
echo "some_name" > /cfg/usb-function-gadget/G1/C1/F1/name
echo "some_name" > /cfg/usb-function-gadget/G1/C2/F1/name
creates two separate instances of F1 in C1 and C2. Did you mean
the same instance of F1 used both in C1 and C2?
Anyway, since Michał proposed some alternative this scheme is
likely to be revised.
>
> >to actually probe and bind the gadget.
> >
> >In this case, under F1 there will be automatically created interface00
> >directory with the contents similar to this:
> >
> >/cfg/usb-function-gadget/G1/C1/F1/interface00
> >/cfg/usb-function-gadget/G1/C1/F1/interface00/altsetting
> >/cfg/usb-function-gadget/G1/C1/F1/interface00/interface_class
> >/cfg/usb-function-gadget/G1/C1/F1/interface00/interface_number
> >/cfg/usb-function-gadget/G1/C1/F1/interface00/interface_protocol
> >/cfg/usb-function-gadget/G1/C1/F1/interface00/interface_subclass
> >/cfg/usb-function-gadget/G1/C1/F1/interface00/n_endpoints
> >/cfg/usb-function-gadget/G1/C1/F1/interface00/endpoint02
> >/cfg/usb-function-gadget/G1/C1/F1/interface00/endpoint02/attributes
> >/cfg/usb-function-gadget/G1/C1/F1/interface00/endpoint02/endpoint_addre
> >ss /cfg/usb-function-gadget/G1/C1/F1/interface00/endpoint02/interval
> >/cfg/usb-function-gadget/G1/C1/F1/interface00/endpoint02/max_packet_siz
> >e
> >/cfg/usb-function-gadget/G1/C1/F1/interface00/endpoint81
> >/cfg/usb-function-gadget/G1/C1/F1/interface00/endpoint81/attributes
> >/cfg/usb-function-gadget/G1/C1/F1/interface00/endpoint81/endpoint_addre
> >ss /cfg/usb-function-gadget/G1/C1/F1/interface00/endpoint81/interval
> >/cfg/usb-function-gadget/G1/C1/F1/interface00/endpoint81/max_packet_siz
> >e
>
> Interresting. I have no idea if I do like or I don't. Some minor things:
> You do have endpoint02 and you have endpoint_address. I guess
> endpoint_address here reads 2. So I think this attribute is read only.
> Same goes for max_packet_size since this is usually protocol specific.
>
Once upon a time Felipe expressed interest in having all these accessible
from userspace (e.g. for debugging purposes) so here it is.
Meant to be read-only.
<snip>
> >
> >+config USB_FG
> >+ tristate "USB Functions Gadget (EXPERIMENTAL)"
> >+ select USB_LIBCOMPOSITE
> >+ depends on EXPERIMENTAL && CONFIGFS_FS
>
> EXPERIMENTAL is going away. It is no longer under drivers/usb.
>
Anything else instead perhaps?
> >+ help
> >+ USB Functions Gadget is a device which aggregates a number of
> >+ USB functions. The gadget is composed by userspace through a
> >+ configfs interface, which enables specifying what USB
> >+ configurations the gadget is composed of, what USB functions
> >+ a USB configuration is composed of and enabling/disabling
> >+ the gadget.
>
> It would be nice to point to Documentation/usb/file-with-examples.txt
You can expect it in the next version.
>
> >diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
> >index fef41f5..09faa34 100644
> >--- a/drivers/usb/gadget/Makefile
> >+++ b/drivers/usb/gadget/Makefile
> >@@ -38,6 +38,7 @@ obj-$(CONFIG_USB_MV_U3D) += mv_u3d_core.o
> > #
> > # USB gadget drivers
> > #
> >+g_usb_functions-y := usb_functions.o
> > g_zero-y := zero.o
> > g_audio-y := audio.o
> > g_ether-y := ether.o
> >@@ -56,6 +57,7 @@ g_ncm-y := ncm.o
> > g_acm_ms-y := acm_ms.o
> > g_tcm_usb_gadget-y := tcm_usb_gadget.o
> >
> >+obj-$(CONFIG_USB_FG) += g_usb_functions.o
>
> Don't use this indirection and I don't see a reason why you shouldn't drop
> the g_ at the beginning. After all this will not be a gadget but an
> interface to create gadgets.
>
This calls for a discussion: where actually this kind of code should be
located? usb_functions.c or similar? composite.c?
<snip>
>
> >+
> >+USB_GADGET_COMPOSITE_OPTIONS();
> No, no modules parameters please. This will be configsfs only.
You are right.
<snip>
> >+static struct config_group *ufg_make_function_subgroup(
> >+ struct config_group *group, const char *name) {
> >+ struct ufg_function_grp *f_grp;
> >+ struct config_group *result;
> >+
> >+ /*
> >+ * TODO:
> >+ *
> >+ * This makes "interface" an illegal name for a function.
> >+ */
> >+ if (!strncmp(name, "interface", 9)) {
>
> Can't you use something else to match between function & interface?
> After all you know that gadget's parent has to be a "config"-folder,
> followed by an interface, followed by the function.
>
I could have used another level of nesting, where by default there are
/cfg...../C1/F1/name
/cfg...../C1/F1/function/
/cfg...../C1/F1/interfaces/
and
echo "some_name" > /cfg...../C1/F1/name
creates
/cfg...../C1/F1/function/some_name
/cfg...../C1/F1/function/some_name/function-specific-stuff
and later binding the gadget creates
/cfg...../C1/F1/interfaces/interface00
In such a situation the distinction between a function and
an interface is made by construction: a subdirectory of
/cfg...../C1/F1/interfaces must be an interface
While a subdirectory of
/cfg...../C1/F1/function must be a function
This can be done. However, I was not sure whether it was
desirable to have one more directory level.
Now it is
/cfg...../C1/F1/name
/cfg...../C1/F1/some_name
/cfg...../C1/F1/some_name/function-specific-stuff
/cfg...../C1/F1/interface00
<snip>
> >+static ssize_t ufg_function_grp_store_name(struct ufg_function_grp *grp,
> >+ const char *buf, size_t count) {
> >+ int rc;
> >+ struct dentry *parent, *new;
> >+ struct qstr name;
> >+ char name_buf[UFG_STR_LEN];
>
<snip>
> >+ return -ENOMEM;
> >+ d_add(new, NULL);
> >+ rc = ufg_mkdir(parent, new);
> >+ if (rc) {
> >+ d_drop(new);
> >+ dput(new);
> >+
> >+ return rc;
> >+ }
> >+ dput(new); /* make the refcount 1 */
>
> Looking at this: full_name_hash(), d_alloc(), d_add(), d_drop(), dput().
> Do you write a filesystem? I don't see any one of this functions beeing
> use in drivers/target? Why do you need them? I think you do something more
> complicated than it needs to be because target made it without it.
>
This again calls for a discussion about configfs in general.
There is no interface to configfs to operate on the directories
and files programmatically from the kernel, and this is why I use
all these filesystem-specific stuff here: only operation from
userspace through syscalls is supported in configfs so I need to
simulate it.
It is becoming more and more evident that we _will_ need a way
to create/manipulate configfs entries programmatically from the kernel.
The way I implement it now in ufg_mkdir is something which eventually
evaluates to a line of this kind:
grp->group.cg_item.ci_dentry->d_inode->i_op->mkdir()
where grp is a pointer to the parent's config_group container.
When mkdir() operation is called, it calls back to configfs's
make_group from config_group_operations. An important note:
Once make_group is called there is no way to tell whether the
call originates from userspace/syscall, or was initiated
programmatically.
@Joel: can you suggest something else for programmatically creating
configfs directories and reading/writing configfs files?
<snip>
>
> I would keep them all empty. If the serial number has not been specified
> there should be none, same goes for product. So the user has to set them
> if he wants it to be set.
>
Agreed.
<snip>
> >+static int ufg_add_f(struct ufg_dev *ufg_dev, struct usb_configuration
> *u_cfg,
> >+ struct config_item *f, ushort /*out*/ *num_interfaces)
{
> >+ struct config_group *f_grp = to_config_group(f);
> >+ struct ufg_function_grp *function_grp =
> >+ container_of(f_grp, struct ufg_function_grp, group);
> >+ struct usb_function *fn = function_grp->f;
> >+
> >+ if (IS_ERR_OR_NULL(fn))
> >+ return PTR_ERR(fn);
<snip>
> >+ for (d = descriptors; *d; d++)
> >+ switch ((*d)->bDescriptorType) {
> >+ case USB_DT_INTERFACE:
> >+ i_grp = ufg_process_interface_desc(*d,
f_grp);
> >+ if (IS_ERR_OR_NULL(i_grp))
> >+ return -1;
> >+ /* keep gcc quiet about a value not being
used*/
>
> If it is not used, why do we have it?
Ok, it should be
(*num_interfaces)++;
>
> >+ *num_interfaces = *num_interfaces + 1;
<snip>
> >+static void ufg_fill_ufg_driver(struct usb_composite_driver *ufgd,
> >+ struct ufg_gadget_grp *g_grp)
> >+{
> >+ struct usb_device_descriptor *desc;
> >+
> >+ desc = &container_of(ufgd, struct ufg_dev, ufg_driver)-
> >ufg_device_desc;
> >+ desc->bLength = sizeof(desc); /* TODO: *desc ? */
> >+ desc->bDescriptorType = USB_DT_DEVICE;
> >+ desc->bcdUSB = cpu_to_le16(0x0200);
> >+ desc->bDeviceClass = USB_CLASS_PER_INTERFACE;
> >+ desc->bNumConfigurations = 1;
>
> You had config items for those. Where did they go? And what about multi
> configs?
>
Thanks for pointing that, this should be addressed.
I think it is kind of similar to removing
USB_GADGET_COMPOSITE_OPTIONS();
<snip>
<snip>
> >+MODULE_DESCRIPTION(DRIVER_DESC);
> >+MODULE_AUTHOR("Andrzej Pietrasiewicz"); MODULE_ALIAS("usb_functions");
> >+MODULE_LICENSE("GPL");
> Please keep this things (MODULE_*) at the bottom. Why an ALIAS?
>
ALIAS() to have a string which can be referred to in adapters, which
provide the old modprobe + parameters (+ e.g. sysfs) interface and
should be able to request_module("usb_functions");
The string could be defined in some header file included both by
this module and adapter modules. Simpler solutions welcome.
Andrzej
--
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
