All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 2/2] rfkill: Set device powered even adapter is not created
@ 2012-01-16  9:02 Yu A Wang
  2012-04-19  7:45 ` Johan Hedberg
  0 siblings, 1 reply; 6+ messages in thread
From: Yu A Wang @ 2012-01-16  9:02 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Yu A Wang

---
 src/rfkill.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/src/rfkill.c b/src/rfkill.c
index b40c6e7..5d32fac 100644
--- a/src/rfkill.c
+++ b/src/rfkill.c
@@ -128,11 +128,18 @@ static gboolean rfkill_event(GIOChannel *chan,
 	if (id < 0)
 		return TRUE;
 
+	DBG("RFKILL unblock for hci%d", id);
+
 	adapter = manager_find_adapter_by_id(id);
-	if (!adapter)
+	if (!adapter) {
+		/*
+		 * If device is rfkilled, the initialize operation
+		 * may failed and adapter is not created, then we
+		 * need to set the device powered directly.
+		 */
+		adapter_ops_set_powered(id, TRUE);
 		return TRUE;
-
-	DBG("RFKILL unblock for hci%d", id);
+	}
 
 	btd_adapter_restore_powered(adapter);
 
-- 
1.7.2.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] rfkill: Set device powered even adapter is not created
  2012-01-16  9:02 [PATCH 2/2] rfkill: Set device powered even adapter is not created Yu A Wang
@ 2012-04-19  7:45 ` Johan Hedberg
  2012-04-19  8:57   ` Wang, Arron
  0 siblings, 1 reply; 6+ messages in thread
From: Johan Hedberg @ 2012-04-19  7:45 UTC (permalink / raw)
  To: Yu A Wang; +Cc: linux-bluetooth

Hi,

On Mon, Jan 16, 2012, Yu A Wang wrote:
> ---
>  src/rfkill.c |   13 ++++++++++---
>  1 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/src/rfkill.c b/src/rfkill.c
> index b40c6e7..5d32fac 100644
> --- a/src/rfkill.c
> +++ b/src/rfkill.c
> @@ -128,11 +128,18 @@ static gboolean rfkill_event(GIOChannel *chan,
>  	if (id < 0)
>  		return TRUE;
>  
> +	DBG("RFKILL unblock for hci%d", id);
> +
>  	adapter = manager_find_adapter_by_id(id);
> -	if (!adapter)
> +	if (!adapter) {
> +		/*
> +		 * If device is rfkilled, the initialize operation
> +		 * may failed and adapter is not created, then we
> +		 * need to set the device powered directly.
> +		 */
> +		adapter_ops_set_powered(id, TRUE);
>  		return TRUE;
> -
> -	DBG("RFKILL unblock for hci%d", id);
> +	}
>  
>  	btd_adapter_restore_powered(adapter);

This looks more like a workaround to another issue: if the kernel is
aware of the adapter but user space isn't it means that something has
gone wrong during the initialization process and *that* should be fixed
instead of blindly attempting to power on the adapter id anyway.

Johan

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH 2/2] rfkill: Set device powered even adapter is not created
  2012-04-19  7:45 ` Johan Hedberg
@ 2012-04-19  8:57   ` Wang, Arron
  2012-04-19 12:31     ` Johan Hedberg
  0 siblings, 1 reply; 6+ messages in thread
From: Wang, Arron @ 2012-04-19  8:57 UTC (permalink / raw)
  To: Hedberg, Johan; +Cc: linux-bluetooth

[-- Attachment #1: Type: text/plain, Size: 1930 bytes --]

Hi Johan,

>-----Original Message-----
>From: Johan Hedberg [mailto:johan.hedberg@intel.com]
>Sent: Thursday, April 19, 2012 3:46 PM
>To: Wang, Arron
>Cc: linux-bluetooth@vger.kernel.org
>Subject: Re: [PATCH 2/2] rfkill: Set device powered even adapter is not
created
>
>Hi,
>
>On Mon, Jan 16, 2012, Yu A Wang wrote:
>> ---
>>  src/rfkill.c |   13 ++++++++++---
>>  1 files changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/rfkill.c b/src/rfkill.c index b40c6e7..5d32fac 100644
>> --- a/src/rfkill.c
>> +++ b/src/rfkill.c
>> @@ -128,11 +128,18 @@ static gboolean rfkill_event(GIOChannel *chan,
>>  	if (id < 0)
>>  		return TRUE;
>>
>> +	DBG("RFKILL unblock for hci%d", id);
>> +
>>  	adapter = manager_find_adapter_by_id(id);
>> -	if (!adapter)
>> +	if (!adapter) {
>> +		/*
>> +		 * If device is rfkilled, the initialize operation
>> +		 * may failed and adapter is not created, then we
>> +		 * need to set the device powered directly.
>> +		 */
>> +		adapter_ops_set_powered(id, TRUE);
>>  		return TRUE;
>> -
>> -	DBG("RFKILL unblock for hci%d", id);
>> +	}
>>
>>  	btd_adapter_restore_powered(adapter);
>
>This looks more like a workaround to another issue: if the kernel is aware
of the
>adapter but user space isn't it means that something has gone wrong during
the
>initialization process and *that* should be fixed instead of blindly
attempting to
>power on the adapter id anyway.
Kernel works well and can detect the adapter, Bluetooth initialize failed
due to when we init HCI device, we try to start the device, however the
device is hardware/software rfkilled. From the code, we only init the
adapter when the device is up, this result in the current code in rfkill.c
can't bring up the device because adapter is NULL. Also only the kernel
detected the device we can get the rfkill event, the code in rfkill.c also
checked the device, then it is safe to power on the device in rfkill.c

Thanks
Arron


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 8614 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] rfkill: Set device powered even adapter is not created
  2012-04-19  8:57   ` Wang, Arron
@ 2012-04-19 12:31     ` Johan Hedberg
  2012-04-19 12:41       ` Marcel Holtmann
  0 siblings, 1 reply; 6+ messages in thread
From: Johan Hedberg @ 2012-04-19 12:31 UTC (permalink / raw)
  To: Wang, Arron; +Cc: linux-bluetooth

Hi Arron,

On Thu, Apr 19, 2012, Wang, Arron wrote:
> >> --- a/src/rfkill.c
> >> +++ b/src/rfkill.c
> >> @@ -128,11 +128,18 @@ static gboolean rfkill_event(GIOChannel *chan,
> >>  	if (id < 0)
> >>  		return TRUE;
> >>
> >> +	DBG("RFKILL unblock for hci%d", id);
> >> +
> >>  	adapter = manager_find_adapter_by_id(id);
> >> -	if (!adapter)
> >> +	if (!adapter) {
> >> +		/*
> >> +		 * If device is rfkilled, the initialize operation
> >> +		 * may failed and adapter is not created, then we
> >> +		 * need to set the device powered directly.
> >> +		 */
> >> +		adapter_ops_set_powered(id, TRUE);
> >>  		return TRUE;
> >> -
> >> -	DBG("RFKILL unblock for hci%d", id);
> >> +	}
> >>
> >>  	btd_adapter_restore_powered(adapter);
> >
> >This looks more like a workaround to another issue: if the kernel is
> >aware of the adapter but user space isn't it means that something has
> >gone wrong during the initialization process and *that* should be
> >fixed instead of blindly attempting to power on the adapter id
> >anyway.
> 
> Kernel works well and can detect the adapter, Bluetooth initialize failed
> due to when we init HCI device, we try to start the device, however the
> device is hardware/software rfkilled. From the code, we only init the
> adapter when the device is up, this result in the current code in rfkill.c
> can't bring up the device because adapter is NULL. Also only the kernel
> detected the device we can get the rfkill event, the code in rfkill.c also
> checked the device, then it is safe to power on the device in rfkill.c

Ok, now I understand the issue better. Unfortunately your approach wont
work with the management interface since the HCI index is invalid until
the kernel has been able to send the first basic HCI commands. The
expectation there is also that the first mgmt command to be sent for a
new index is read_info.

I had a brief chat with Marcel and one potential solution would be to
have a kernel side timer for rfkilled devices to let the basic HCI
commands be sent before bringing the adapter down. This would allow user
space to have a properly initialized adapter object and your patches
wouldn't be needed.

Johan

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] rfkill: Set device powered even adapter is not created
  2012-04-19 12:31     ` Johan Hedberg
@ 2012-04-19 12:41       ` Marcel Holtmann
  2012-04-19 14:13         ` Johan Hedberg
  0 siblings, 1 reply; 6+ messages in thread
From: Marcel Holtmann @ 2012-04-19 12:41 UTC (permalink / raw)
  To: Johan Hedberg; +Cc: Wang, Arron, linux-bluetooth

Hi Johan,

> > >> --- a/src/rfkill.c
> > >> +++ b/src/rfkill.c
> > >> @@ -128,11 +128,18 @@ static gboolean rfkill_event(GIOChannel *chan,
> > >>  	if (id < 0)
> > >>  		return TRUE;
> > >>
> > >> +	DBG("RFKILL unblock for hci%d", id);
> > >> +
> > >>  	adapter = manager_find_adapter_by_id(id);
> > >> -	if (!adapter)
> > >> +	if (!adapter) {
> > >> +		/*
> > >> +		 * If device is rfkilled, the initialize operation
> > >> +		 * may failed and adapter is not created, then we
> > >> +		 * need to set the device powered directly.
> > >> +		 */
> > >> +		adapter_ops_set_powered(id, TRUE);
> > >>  		return TRUE;
> > >> -
> > >> -	DBG("RFKILL unblock for hci%d", id);
> > >> +	}
> > >>
> > >>  	btd_adapter_restore_powered(adapter);
> > >
> > >This looks more like a workaround to another issue: if the kernel is
> > >aware of the adapter but user space isn't it means that something has
> > >gone wrong during the initialization process and *that* should be
> > >fixed instead of blindly attempting to power on the adapter id
> > >anyway.
> > 
> > Kernel works well and can detect the adapter, Bluetooth initialize failed
> > due to when we init HCI device, we try to start the device, however the
> > device is hardware/software rfkilled. From the code, we only init the
> > adapter when the device is up, this result in the current code in rfkill.c
> > can't bring up the device because adapter is NULL. Also only the kernel
> > detected the device we can get the rfkill event, the code in rfkill.c also
> > checked the device, then it is safe to power on the device in rfkill.c
> 
> Ok, now I understand the issue better. Unfortunately your approach wont
> work with the management interface since the HCI index is invalid until
> the kernel has been able to send the first basic HCI commands. The
> expectation there is also that the first mgmt command to be sent for a
> new index is read_info.
> 
> I had a brief chat with Marcel and one potential solution would be to
> have a kernel side timer for rfkilled devices to let the basic HCI
> commands be sent before bringing the adapter down. This would allow user
> space to have a properly initialized adapter object and your patches
> wouldn't be needed.

I actually meant to have the pending RFKILL events to be handled in
userspace.

But sure for the RFKILL switches that are tight to a Bluetooth
controller and are part of hci_dev, we could be even smarter inside the
kernel.

Regards

Marcel



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] rfkill: Set device powered even adapter is not created
  2012-04-19 12:41       ` Marcel Holtmann
@ 2012-04-19 14:13         ` Johan Hedberg
  0 siblings, 0 replies; 6+ messages in thread
From: Johan Hedberg @ 2012-04-19 14:13 UTC (permalink / raw)
  To: Marcel Holtmann; +Cc: Wang, Arron, linux-bluetooth

Hi Marcel,

On Thu, Apr 19, 2012, Marcel Holtmann wrote:
> > > >> --- a/src/rfkill.c
> > > >> +++ b/src/rfkill.c
> > > >> @@ -128,11 +128,18 @@ static gboolean rfkill_event(GIOChannel *chan,
> > > >>  	if (id < 0)
> > > >>  		return TRUE;
> > > >>
> > > >> +	DBG("RFKILL unblock for hci%d", id);
> > > >> +
> > > >>  	adapter = manager_find_adapter_by_id(id);
> > > >> -	if (!adapter)
> > > >> +	if (!adapter) {
> > > >> +		/*
> > > >> +		 * If device is rfkilled, the initialize operation
> > > >> +		 * may failed and adapter is not created, then we
> > > >> +		 * need to set the device powered directly.
> > > >> +		 */
> > > >> +		adapter_ops_set_powered(id, TRUE);
> > > >>  		return TRUE;
> > > >> -
> > > >> -	DBG("RFKILL unblock for hci%d", id);
> > > >> +	}
> > > >>
> > > >>  	btd_adapter_restore_powered(adapter);
> > > >
> > > >This looks more like a workaround to another issue: if the kernel is
> > > >aware of the adapter but user space isn't it means that something has
> > > >gone wrong during the initialization process and *that* should be
> > > >fixed instead of blindly attempting to power on the adapter id
> > > >anyway.
> > > 
> > > Kernel works well and can detect the adapter, Bluetooth initialize failed
> > > due to when we init HCI device, we try to start the device, however the
> > > device is hardware/software rfkilled. From the code, we only init the
> > > adapter when the device is up, this result in the current code in rfkill.c
> > > can't bring up the device because adapter is NULL. Also only the kernel
> > > detected the device we can get the rfkill event, the code in rfkill.c also
> > > checked the device, then it is safe to power on the device in rfkill.c
> > 
> > Ok, now I understand the issue better. Unfortunately your approach wont
> > work with the management interface since the HCI index is invalid until
> > the kernel has been able to send the first basic HCI commands. The
> > expectation there is also that the first mgmt command to be sent for a
> > new index is read_info.
> > 
> > I had a brief chat with Marcel and one potential solution would be to
> > have a kernel side timer for rfkilled devices to let the basic HCI
> > commands be sent before bringing the adapter down. This would allow user
> > space to have a properly initialized adapter object and your patches
> > wouldn't be needed.
> 
> I actually meant to have the pending RFKILL events to be handled in
> userspace.
> 
> But sure for the RFKILL switches that are tight to a Bluetooth
> controller and are part of hci_dev, we could be even smarter inside the
> kernel.

I assumed you meant kernel side since that's where the rfkill enabling
is handled right now (user space only takes action for unrfkilling
adapters). I.e. I'd add some extra logic to hci_rfkill_set_block
(hci_core.c) to check if the adapter has been fully initialized and if
not set a timer instead of directly calling hci_dev_do_close. And if
hci_rfkill_set_block gets called again with blocked == 0 then the timer
would be removed.

Johan

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2012-04-19 14:13 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-01-16  9:02 [PATCH 2/2] rfkill: Set device powered even adapter is not created Yu A Wang
2012-04-19  7:45 ` Johan Hedberg
2012-04-19  8:57   ` Wang, Arron
2012-04-19 12:31     ` Johan Hedberg
2012-04-19 12:41       ` Marcel Holtmann
2012-04-19 14:13         ` Johan Hedberg

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.