On 10/26/07, Greg KH <greg@kroah.com> wrote:
quoted text
> On Thu, Oct 25, 2007 at 05:06:59PM +0800, Dave Young wrote:
> > On 10/19/07, Greg KH <greg@kroah.com> wrote:
> > >On Wed, Oct 17, 2007 at 10:48:52AM -0400, Alan Stern wrote:
> > >> On Tue, 16 Oct 2007, Matthew Dharm wrote:
> > >>
> > >> > On Tue, Oct 16, 2007 at 02:04:43PM -0400, Alan Stern wrote:
> > >> > > On Tue, 16 Oct 2007, Matthew Dharm wrote:
> > >> > >
> > >> > > > I haven't looked at this code at all, but neither approach feels
> > >> > > > right to
> > >> > > > me.
> > >> > > >
> > >> > > > How does this work at all?  Even if you load a driver later,
> > >> > > > wouldn't it
> > >> > > > call usb_set_interface(), which would call
> > >> > > > usb_create_sysfs_intf_files()
> > >> > > > and hit the same issue?
> > >> > >
> > >> > > usb_set_interface() is smart enough to remove the old interface
> > >> > > files
> > >> > > before creating new ones, since it expects them to exist already.
> > >> > > Hence there's no problem in that scenario.
> > >> > >
> > >> > > But usb_set_configuration doesn't expect there to be any
> > >> > > pre-existing
> > >> > > interface files, because there isn't even an interface until the
> > >> > > registration is performed.
> > >> >
> > >> > And I'm guessing that you can't call usb_create_sysfs_intf_files()
> > >> > until
> > >> > registration is performed, right?
> > >>
> > >> Right.
> > >>
> > >> > > The most important reason has to do with the endpoint
> > >> > > pseudo-devices.
> > >> > > Different altsettings can have different endpoints, so those have
> > >> > > to be
> > >> > > removed and re-created whenever the altsetting changes.
> > >> >
> > >> > Right, altsettings.  I forgot about those.  I only ever think in
> > >> > terms of
> > >> > multiple configurations.
> > >> >
> > >> > *grumble*
> > >> >
> > >> > If usb_set_interface() has to be smart enough to remove existing
> > >> > files
> > >> > first already, then I guess it's reasonably symmetric to have
> > >> > usb_set_configuration() have the same smarts.  Maybe they can share
> > >> > some
> > >> > common code, even.
> > >>
> > >> It's not a big deal to remove the files first.  In fact, here's a
> > >> patch
> > >> to do it.  Dave, see if this doesn't fix your problem.  I don't like
> > >> it
> > >> much because it does an unnecessary remove/create cycle, but that's
> > >> better than doing something wrong.
> > >>
> > >> It's slightly odd that the sysfs core logs an error when you try to
> > >> create the same file twice but it doesn't when you try to remove a
> > >> non-existent file (or try to remove an existing file twice).  Oh
> > >> well...
> > >
> > >I used to have the 'remove a non-existant file' warning, but that just
> > >triggered _way_ too many responses :)
> > >
> > >
> > >> Index: usb-2.6/drivers/usb/core/message.c
> > >> ===================================================================
> > >> --- usb-2.6.orig/drivers/usb/core/message.c
> > >> +++ usb-2.6/drivers/usb/core/message.c
> > >> @@ -1643,7 +1643,13 @@ free_interfaces:
> > >>                               intf->dev.bus_id, ret);
> > >>                       continue;
> > >>               }
> > >> -             usb_create_sysfs_intf_files (intf);
> > >> +
> > >> +             /* The driver's probe method can call
> > >> usb_set_interface(),
> > >> +              * which would mean the interface's sysfs files are
> > >> already
> > >> +              * created.  Just in case, we'll remove them first.
> > >> +              */
> > >> +             usb_remove_sysfs_intf_files(intf);
> > >> +             usb_create_sysfs_intf_files(intf);
> > >>       }
> > >
> > >If this fixes the problem, care to resend it with a signed-off-by:?
> > >
> > >Yeah, it's not the nicest solution, but I can't think of any other one
> > >either right now :(
> > Hi, greg
> >
> > How about this patch (based on 2.6.24-rc1):
> >
> > diff -upr linux/drivers/usb/core/message.c linux.new/drivers/usb/core/message.c
> > --- linux/drivers/usb/core/message.c  2007-10-25 16:41:32.000000000 +0800
> > +++ linux.new/drivers/usb/core/message.c      2007-10-25 16:39:38.000000000 +0800
> > @@ -1641,7 +1641,8 @@ free_interfaces:
> >                               intf->dev.bus_id, ret);
> >                       continue;
> >               }
> > -             usb_create_sysfs_intf_files (intf);
> > +             if(!usb_sysfs_intf_exist(intf))
> > +                     usb_create_sysfs_intf_files (intf);
> >       }
> >
> >       usb_autosuspend_device(dev);
> > diff -upr linux/drivers/usb/core/sysfs.c linux.new/drivers/usb/core/sysfs.c
> > --- linux/drivers/usb/core/sysfs.c    2007-10-25 16:40:16.000000000 +0800
> > +++ linux.new/drivers/usb/core/sysfs.c        2007-10-25 16:39:32.000000000 +0800
> > @@ -728,6 +728,13 @@ static inline void usb_remove_intf_ep_fi
> >               usb_remove_ep_files(&iface_desc->endpoint[i]);
> >  }
> >
> > +int usb_sysfs_intf_exist(struct usb_interface *intf)
> > +{
> > +     struct device *dev = &intf->dev;
> > +
> > +     return sysfs_dirent_exist(&dev->kobj, intf_attrs[0]->name);
>
> The issue is that you can't just test for the first file.  If you look
> at the logic in the usb_create_sysfs_intf_file() code, we do create
> different files based on the current interface.  So this might not
> always end up with the proper files in userspace, from what I can tell.
>
Yes, I know this is not good, it just fixed the bug for me. It's hard
to test all files simply.

The duplicate file issue is still there,  what to do then?
Alan, could you send the "remove before create" patch with your signed-off?

Anyway the sysfs_dirent_exist is useful for extern use, How about add
and export this function? Greg, If you agree, I would send it as
another patch.


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