Hi,
On Mon, Dec 10, 2012 at 03:17:34PM +0100, Andrzej Pietrasiewicz wrote:
> @Joel in particular: please see my comment in the bottom.
>
> On Monday, December 10, 2012 12:57 PM I wrote:
> > Subject: RE: [RFC][PATCH] fs: configfs: programmatically create config
> > groups
> >
> > Hello Joel,
> >
> > So you are alive, I'm glad to hear from you ;) Thank you for your
> response.
> >
> > On Saturday, December 08, 2012 12:18 AM Joel Becker wrote:
> > > Subject: Re: [RFC][PATCH] fs: configfs: programmatically create config
> > > groups
> > >
> > > Hey Guys,
> > > Sorry I missed this for a while. I'll make a couple of inline
> > > comments, and then I'll summarize my (incomplete) thoughts at the
> bottom.
> > >
> > > On Wed, Nov 28, 2012 at 02:50:13PM +0100, Sebastian Andrzej Siewior
> > wrote:
> > > > On 11/28/2012 02:05 PM, Michal Nazarewicz wrote:
> > > > >>On 11/27/2012 05:23 PM, Michal Nazarewicz wrote:
> > > > >>>How should a generic tool know what kind of actions are needed for
> > > > >>>given function to be removed? If you ask me, there should be a way
> > > > >>>to unbind gadget and unload all modules without any specific
> > > > >>>knowledge of the functions. If there is no such mechanism, then
> > > > >>>it's a bad user interface.
> > >
> > > Please remember that configfs is not a "user" interface, it's a
> > > userspace<->kernelspace interface. Like sysfs, it's not required to be
> > > convenient for someone at a bash prompt. My goal is that it is *usable*
> > > from a bash prompt. So it must be that you can create/destroy/configure
> > > objects via mkdir/rmkdir/cat/echo, but you might have a lot of those
> > > mkdir/echo combos to configure something.
> > > When it comes to the "user" interface, a wrapper script or library
> > should
> > > be converting a user intention into all that boilerplate.
> > >
> >
> > If you say so there is little we can do, is there? :O
> >
> > <snip>
> >
> > >
> > > Yeah, user tools are expected (and should be).
> > >
> >
> > ditto
> >
> > > > Anyway, for this to work we have to go through Joel.
> > > >
> > > > > rmdir udcs/* # unload all UDCs
> > > >
> > > > No, for this you still have to rmmod :)
> > > >
> > > >
> > > > >>>I think the question is of information flow direction. If user
> > > > >>>gives some information to the kernel, she should be the one
> > > > >>>creating any necessary directories. But if the information comes
> > > > >>>from kernel to the user, the kernel should create the structure.
> > >
> > > This last paragraph actually describes the distinction between
> > > configfs and sysfs. More specifically, if userspace wants to create an
> > > object in the kernel, it should use configfs. If the kernel has created
> > > an object on its own, it exposes it via sysfs. It is a deliberate non-
> > > goal for configfs to replicate sysfs behavior.
> >
> > So if the lifetime of some object is controlled by the user, it belongs
> > to configfs. If, on the other hand, the lifetime is controlled by the
> > kernel, it belongs to sysfs. I think that the trouble with e.g. luns
> > is that they might want to behave like both (at least in the "more
> > automated"
> > approach where they are programmatically created): they are created
> > by the kernel (=> sysfs) but their lifetime is controlled by the user
> > (=>configfs).
> >
> > >
> > > [General Thoughts]
> > >
> > > First let me restate your problem to see if I understand it.
> > > You want to expose e.g. five LUNs. They should eventually appear in
> > > configfs as five items to configure (.../{lun0,lun1,...lun4}/. The
> > > current configfs way to do this is to have your setup script do:
> > >
> > > cd /cfg/.../mass_storage
> > > mkdir lun0
> > > echo blah >lun0/attr1
> > > echo blahh >lun0/attr2
> > > mkdir lun1
> > > echo blag >lun1/attr1
> > > echo blagg >lun1/attr1
> > > ...
> > >
> > > I think the primary concern expressed by Andrzej is that a random user
> > > could come along and say "mkdir lun8", even though the gadget cannot
> > > support it. A secondary concern from Michal is that userspace has to
> > run
> > > all of those mkdirs. The thread has described varying solutions.
> > > If these original directories were default_groups, you could
> > > disallow any child creation and not require the setup script to create
> > > directories. Here I will point out that you *can* create default_groups
> > > programmatically. The default_groups pointer on each configfs_group can
> > > be different. ->make_group() can attach whatever groups it wants.
> > > If this would work, it would fit both of your concerns, but you do not
> > > have a priori knowledge of the LUN count.
> > > Your original email proposed that the max lun be set like so:
> > >
> > > cd /cfg/.../mass_storage
> > > echo 5 >luns_allowed
> > >
> > > There are then a couple of proposed ways to enforce the limit. Your
> > > ->create_group() idea wants luns_allowed to be able to create subdirs in
> > > the ->store() function. No ->mkdir() is provided, so a lunN cannot be
> > > created by hand.
> > > The ->create_group() idea has three challenges. First, it creates
> > > "default" groups *after* the object is created. This makes no sense if
> > > they are attributes of the toplevel object. Second, it volates the
> > > configfs mantra that user-generated objects are explicitly created.
> > > The kicker, however, is the technical problem. configfs explicitly
> > > forbids tree changes inside attribute operations for deadlock reasons.
> > > Trying to create new tree parts in luns_allowed->store() is exactly
> that.
> > > Another suggestion (I think I read this in the thread) would be to
> > > have ->mkdir() function parse the name and check that it is lunN, where
> > N
> > > is less than the limit. I think that this was shot down because of
> > > parsing the LUN name. I don't think that's too terrible, but there's
> > > certainly room for argument.
> > >
> > > [Possible Solutions]
> > >
> > > Before coming back to the proposed patch, let's look at what I might
> > > do if I were fitting this into configfs.
> > > I would actually do what I described at first:
> > >
> > > cd /cfg/.../mass_storage
> > > mkdir lun0
> > > echo blah >lun0/attr1
> > > ...
> > >
> > > This is idiomatic configfs usage. Sebastian noted this earlier in the
> > > thread. Michal thought the repeated mkdirs were a problem ("I think
> > > that's suboptimal, and we can do better"). This is Michal's objection
> > to
> > > making the user do work. This is where I point out we are not designing
> > > configfs interfaces for ease of end-user use. We're defining them for
> > > transparent and discoverable interfaces for tools to create things in
> > the
> > > kernel. So the repeated mkdir()s are actually a good thing from my
> > > perspective; userspace is telling the kernel to create LUN objects.
> > > But this doesn't alleviate Andrzej's primary concern, nor does it
> > > answer Michal's concerns about corner cases. Let's see if we can solve
> > > those.
> > > First, what about preventing extraneous LUNs? There are two
> > > approaches: the prevent approach, and the ignore approach. In the
> > Target
> > > module, they take the ignore approach. If you create a LUN that is
> > > outside the scope, they just ignore it. So a user can make 10 LUNs, but
> > > if only five are allowed, the other five are ignored. In the prevent
> > > approach, we try to fail the mkdir() when we go over the limit.
> >
> > I think I like the prevent approach better, because I don't know what to
> > do with the unused luns in the ignore approach. I also think that it would
> > be nice if configfs described the actual state of the system instead of
> > presenting all the possible user-created garbage (i.e. something which is
> > ignored).
> >
> > > So you've set a limit with "echo 5 >luns_allowed". How do you
> > > restrict? If you don't care about contiguous LUN numbering, you could
> > > literally do this in your make_group():
> > >
> > > if (count_luns(parent) >= parent->luns_allowed)
> > > return -ENOSPC;
> > >
> > > Note that we don't check the LUN number; SCSI does not require
> > contiguous
> > > numbering. It does require LUN 0, which should probably be a default
> > > group. So your lun count starts at 1.
> > > What if you decide you really want contiguous LUN numbers, which is
> > > considered nice to have? Then we get to Michal's concern about parsing
> > > the name. If you "mkdir lun1", the code has to confirm that it
> > > a) starts with "lun", b) finishes with a parsable number, c) that number
> > > is < luns_allowed.
> > > But you need to do the parsing anyway! If the LUN is created via
> > > mkdir(), somehow the gadget system needs to know what LUN number to
> > > advertise. I would actually decouple it from the name. The name should
> > > be free-form; if it happens to be comprehensible, that's good. Imagine
> > > this:
> > > cd /cfg/.../mass_storage
> > > echo 4 >max_lun_id
> > > mkdir lun100
> > > echo 1 >lun100/lun_id
> > >
> > > The LUN actually has the ID of 1. The fact that the directory name is
> > > wrong is irrelevant to the gadget processing; non-hackers use the
> > tooling,
> > > and the tooling should make sane names. Then your
> > > ->make_group() function can choose either approach to avoiding LUN IDs
> > > that are too large. It can prevent:
> > >
> > > lun_id->store() {
> > > if (lun_id_exists(parent, value))
> > > return ERR_PTR(-EINVAL);
> > > if (value > parent->max_lun_id)
> > > return ERR_PTR(-ENOSPC) /* or EINVAL */
> > > setup_lun();
> > > make_live();
> > > }
> > >
> > > Or it can ignore:
> > >
> > > lun_id->store() {
> > > if (lun_id_exists(parent, value))
> > > return ERR_PTR(-EINVAL);
> > > if (value <= parent->max_lun_id) {
> > > setup_lun();
> > > make_live();
> > > }
> > > /* Do nothing for the ignored sucker */
> > > }
> > >
> > > I think that I, personally, would go with this <name>/lun_id scheme.
> > > Given a requirement for contiguous LUNs and the max_lun_id restriction,
> > > I'd go with the "prevent" option myself.
> >
> > Taking into account what you say about the expected userspace tooling
> > using a free-form name plus the lun_id attribute could be attractive,
> > because the lun_id attribute's value can be analyzed at ->store() and,
> > if it doesn't fit, an appropriate error can be easily returned using
> > just what we already have in configfs. Prevent seems better to me, too.
> >
> > > But is max LUNs a requirement? Why not just have the LUN count be
> > > the number of LUNs created? In that case, you can allow or disallow
> > > discontiguous LUN IDs without worrying about a max ID.
> > >
> >
> > I am not sure, but I think that Michal would say that he wouldn't like
> > things to change once the gadget is up and running, so the number of
> > luns should be fixed up front. On the other hand, after the gadget is
> > set up, we can fail in ->make_group() to prevent the user from creating
> > additional luns with which we don't know what to do. So what you suggest
> > seems fine, but I would ask Michal to be on a safe side.
> >
> > > [Really Hate Mkdir]
> > >
> > > If you considered it appropriate for the LUN objects to be created
> > > by the kernel, the standard way would to have "echo 5 >luns_allowed"
> > > create LUN objects in sysfs. Yes, sysfs.
> > > There's no reason you can't have things in configfs *and* sysfs. My
> > > understanding of your code says to me that the mkdir approach is the way
> > > to go, but if you hate it, this works.
> > >
> >
> > I guess you are right, but see my point about luns' lifetime being
> > controlled by the user and their creation being performed by the kernel.
> >
> > > [What About the Patch?]
> > >
> > > In general, attribute setting that turns into created configfs
> > > objects is against the theory of configfs.
> >
> > This looks like an authoritative answer.
> >
> > > In practice, it's a nightmare
> > > locking change. Coming into this discussion, I though you were doing
> > > dymanic things at ->make_group() time, but that is already supported.
> > > But I want to hear your thoughts. I've dumped a bunch of alternate
> > > approaches here. Do any seem to fit? What other challenges do you see?
> > >
> > > Joel
> > >
> >
> > Once upon a time, Felipe expressed interest in having the information
> > about endpoints and interfaces (mostly read-only stuff, but not all of it)
> > accessible in the gadget subsystem in configfs. This kind of information
> > is fully available only after the gadget has been bound, that is, after
> > the user created all the groups/items and filled all the attributes
> > and told the gadget to start (with e.g. filling some attribute or
> > creating a symlink). So this looks like a programmatic creation of
> > configfs directories. On the other hand, Felipe wanted it for debugging
> > purposes, so the user could just as well create the required directories
> > by hand when they need to, as Sebastian once suggested. Again, this
> > requires
> > ->make_group() to fail if all the information is not available yet.
> > But isn't it kind of similar to the luns' case? It is a bunch of objects
> > whose lifetime is controlled by the user (bind the gadget, unbind the
> > gadget),
> > but which are created by the kernel.
> > Another problem with this is that the user must at some point _guess_
> > what name to use in mkdir. Example:
> >
> > For each interface there should be a folder. So the user must know how
> > many
> > interfaces' folders to create and how to name them, like:
> >
> > $ mkdir ...../some_usb_function/interface00
> > $ mkdir ...../some_usb_function/interface01
> > ...
> > ...
> > ...
> >
> > In each interface folder, there should be some endpoint folders and the
> > user
> > must know how many endpoints there are and how to name them:
> >
> > $ mkdir ...../some_usb_function/interface00/endpoint02
> > $ mkdir ...../some_usb_function/interface00/endpoint81
> >
> > So probably there should be some attribute in some_usb_function,
> > which contains a list of available interface names, and some attribute in
> > interface#, which contains a list of available endpoint names.
> > Workable, but not so convenient. And there could be a problem, too,
> > if the gadget is unbound: there is no way to remove the user-created
> > directories other than by the user themselves, so there will be times
> > (i.e. between gadget unbind and manual rmdir) when these
> > interface/endpoint
> > directories will not correspond to anything present in the system.
> >
> > So maybe this kind of information should live in sysfs? But I don't
> > know now if it would be any easier. Joel? Felipe?
> >
>
> I forgot to mention, representing udcs (USB Device Controllers) in
> configfs is similar to interfaces/endpoints: the user needs to guess
> what name to use in mkdir, e.g. in:
>
> $ mkdir ....../udcs/s3c-hsotgwhy would you even require a UDC directory to be created ? You *register* the UDC with the udc-class and that should be enough to expose all UDCs under /sys/kernel/config/..../udcs/* The fact is that in most cases you will have multiple instances of the *same IP* not different UDCs with different names as you seem to think. While that's possible, it'll be a rare situation. So what we will have in 95% of the cases will be: /...../udcs/dwc3.0 /...../udcs/dwc3.1 ... /...../udcs/dwc3.N The only thing the poor user needs to know is that *.0 is the first controller, *.1 is the second controller and so on. Since that's hardcoded in HW anyway (every instance has its own IRQ, address space, etc, and each of them is physically attached to a single port), you can kinda make that assumption in code too. -- balbi
signature.asc
Description: Digital signature
