Message-Id: <1241295698.4412.15.camel@localhost.localdomain>
Date:	Sat, 02 May 2009 13:21:38 -0700
From:	Marcel Holtmann <marcel@...tmann.org>
To:	Justin Mattock <justinmattock@...il.com>
Cc:	Dave Young <hidave.darkstar@...il.com>,
	Marc Pignat <Marc.Pignat@...s.ch>, jkosina@...e.cz,
	ext-roger.quadros@...ia.com, linux-bluetooth@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [BUG] 2.6.30-rc4 hid bluetooth not working

Hi Justin,

> >> >>> > Subject: bluetooth: Fix serialization when adding/deleting connections in hci_sysfs
> >> >>> >
> >> >>> > add_conn and del_conn should be serialized, but flush_workqueue can't be used
> >> >>> > by the worker thread on it's own queue, so use flush_work to serialize add_conn
> >> >>> > and del_conn against each other.
> >> >>> >
> >> >>> > Signed-off-by: Marc Pignat <marc.pignat@...s.ch>
> >> >>>
> >> >>> Acked-by: Jiri Kosina <jkosina@...e.cz>
> >> >>>
> >> >>> FWIW.
> >> >>
> >> >>nak from my side since I think it is the wrong fix. We really wanna wait
> >> >>for all works to finish here. This includes work from other connection
> >> >>attempts or terminations.
> >> >
> >> > IMHO, there is no need to wait for work currently running, since this is a
> >> > singlethread workqueue.
> >>
> >> Yes, sounds right.
> >>
> >> >
> >> > But it is perhaps simpler to use a lock (mutex or watherver locking primitive).
> >>
> >> I'm here a little bit late. Marcel, I'm quite busy recently, I just
> >> see the commit and then this thread.
> >>
> >> Let me explain why I add two workqueue originally,  because workqueue
> >> will be defered, so   we must guarantee "connection deletion" finished
> >> before "connection adding with same bt addr",  or the "connection
> >> adding" will fail.
> >>
> >> On the other hand flush "adding" workqueue in "connection deletion"
> >> function is not necessary.
> >>
> >> To fix this bug, I think we can just use the two work struct for
> >> add/del, at the same time keeping the original two workqueue.
> >>
> >> Please see following patch for this, (building-test only, I have no
> >> bluetooth device at hand, I can test this the day after tommorrow)
> >
> > so I spent the whole day figuring out what is going on here and we keep
> > making the wrong assumptions over and over again.
> >
> > First of all, we only add the sysfs device when we have a successful
> > connection. And we identify it with the handle. This means that we can
> > NOT have any name clashes anymore since the controller has to make sure
> > a handle is only assigned once. Previously we did this on the BD_ADDR
> > value and that lead to it. That is no longer the case.
> >
> > Second of all the two work queues introduces way too much complexity for
> > a really simple task of adding and removing a sysfs device entry.
> >
> > The real problem we have right now are that we are not initializing the
> > sysfs device when creating the hci_conn. This is just wrong and can lead
> > to all kinds of weird invalid data access. And as a result the adding of
> > the sysfs device should only set the name and add it.
> >
> > We also check device_registered before making sure that device_add has
> > been run. And instead of adding more locking or crazy work queue
> > dependencies, we should use the single thread work queue to ensure the
> > correct order of things.
> >
> > The attached patch introduces a hci_conn_init_sysfs step to make sure we
> > setup the sysfs device correctly. I left the flush_work calls, but I
> > think they are not needed since a del_conn before add_conn is no longer
> > possible now.
> 
> well it seems your not the only one
> with a broken bluetooth(latest git pull
> this morning)

can you try the patch that I attached to the previous email. It should
fix exactly this.

Regards

Marcel


--
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