Message-ID: <Pine.LNX.4.44L0.1406041100500.922-100000@iolanthe.rowland.org>
Date:	Wed, 4 Jun 2014 11:26:13 -0400 (EDT)
From:	Alan Stern <stern@...land.harvard.edu>
To:	Andrzej Pietrasiewicz <andrzej.p@...sung.com>
cc:	Wei.Yang@...driver.com, Michal Nazarewicz <mina86@...a86.com>,
	Felipe Balbi <balbi@...com>, <gregkh@...uxfoundation.org>,
	USB list <linux-usb@...r.kernel.org>,
	Kernel development list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1] USB:gadget: Fix a warning while loading g_mass_storage

On Wed, 4 Jun 2014, Andrzej Pietrasiewicz wrote:

> When Sebastian introduced the function registration interface I didn't
> specially like the naming: struct usb_function_instance is something
> different than an instance of struct usb_function.

What is the difference in purpose between usb_function and
usb_function_instance?  I can't tell just by reading the header file.  
Does one of them get created dynamically when the host sets the
appropriate config?

It's quite noticeable that composite.h does not contain nearly enough
documentation.  Only four of the structures defined there have any
kerneldoc, and none of the functions do.

Also, there seems to be some confusion between structures that 
represent drivers and those that represent devices (or parts of a 
device).  For example, struct usb_function contains instance data as 
well as driver callbacks.

> The purpose of fsg_alloc_inst() is to create a usb_function_instance
> whose container_of is struct fsg_opts. In fact it is struct fsg_opts
> which is actually allocated; one of its members is struct fsg_common
> which is also allocated - individually for each struct usb_function_instance.
> 
> Among traditional gadgets there is no gadget which uses mass storage function
> more than once. On the other hand, when gadgets are created with configfs
> it is forbidden to link a given function more than once into a given
> config,

What is the reason for this restriction?

>  that is a struct usb_function_instance can be referenced by more
> than one config, but can be referenced only once in a given config;
> each symbolic link corresponds to an instance of struct usb_function
> (don't confuse with struct usb_function_instance).

It's extremely easy to confuse them, since I don't understand the 
differences between them.  It even seems like you confused them in this 
description: You mentioned "link a given function", "link corresponds 
to an instance of struct usb_function", and "struct 
usb_function_instance can be referenced by more than one config".  
What's the difference between linking a usb_function and referencing a 
usb_function_instance?  Normally "linking" and "referencing" mean more 
or less the same thing.

> So yes, an fsg_common can be shared among instances of struct usb_function,
> but neither with traditional gadgets as they are now nor with configfs
> is it possible to have the same fsg_common referenced more than once
> in a given config.

That's a relief.  But it still seems like a bad design.  If there can 
be only one struct fsg_dev associated with struct fsg_common, why have 
separate structures?  And if there can be multiple fsg_dev structures 
associated with struct fsg_common, why does struct fsg_common contain a 
pointer to an fsg_dev (in fact, two of them)?

The issue that started these thoughts was the way fsg_common.new_fsg 
gets used, as modified by the patch in the thread's original email.  
It's not clear why new_fsg is needed at all.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists