2017-02-01 17:14:31

by Carlo Caione

[permalink] [raw]
Subject: [PATCH] tools/btattach: Add detach option

From: Carlo Caione <[email protected]>

Signed-off-by: Carlo Caione <[email protected]>
---
tools/btattach.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/tools/btattach.c b/tools/btattach.c
index 5adbc8d..f1f115d 100644
--- a/tools/btattach.c
+++ b/tools/btattach.c
@@ -194,6 +194,7 @@ static void usage(void)
"\t-P, --protocol <proto> Specify protocol type\n"
"\t-S, --speed <baudrate> Specify which baudrate to use\n"
"\t-N, --noflowctl Disable flow control\n"
+ "\t-D, --detach Open device and then fork\n"
"\t-h, --help Show help options\n");
}

@@ -203,6 +204,7 @@ static const struct option main_options[] = {
{ "protocol", required_argument, NULL, 'P' },
{ "speed", required_argument, NULL, 'S' },
{ "noflowctl",no_argument, NULL, 'N' },
+ { "detach", no_argument, NULL, 'D' },
{ "version", no_argument, NULL, 'v' },
{ "help", no_argument, NULL, 'h' },
{ }
@@ -230,7 +232,7 @@ static const struct {
int main(int argc, char *argv[])
{
const char *bredr_path = NULL, *amp_path = NULL, *proto = NULL;
- bool flowctl = true, raw_device = false;
+ bool flowctl = true, raw_device = false, detach = false;
sigset_t mask;
int exit_status, count = 0, proto_id = HCI_UART_H4;
unsigned int speed = B115200;
@@ -263,6 +265,9 @@ int main(int argc, char *argv[])
case 'N':
flowctl = false;
break;
+ case 'D':
+ detach = true;
+ break;
case 'R':
raw_device = true;
break;
@@ -348,6 +353,9 @@ int main(int argc, char *argv[])
return EXIT_FAILURE;
}

+ if (detach && fork())
+ return 0;
+
exit_status = mainloop_run();

return exit_status;
--
2.9.3



2017-02-02 10:28:29

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH] tools/btattach: Add detach option

Hi Carlo,

> >>> ---
> >>> tools/btattach.c | 10 +++++++++-
> >>> 1 file changed, 9 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/tools/btattach.c b/tools/btattach.c
> >>> index 5adbc8d..f1f115d 100644
> >>> --- a/tools/btattach.c
> >>> +++ b/tools/btattach.c
> >>> @@ -194,6 +194,7 @@ static void usage(void)
> >>> "\t-P, --protocol <proto> Specify protocol type\n"
> >>> "\t-S, --speed <baudrate> Specify which baudrate to use\n"
> >>> "\t-N, --noflowctl Disable flow control\n"
> >>> + "\t-D, --detach Open device and then fork\n"
> >>> "\t-h, --help Show help options\n”);
> >>
> >> And is this really a good idea. I think that I had removed all the fork calls from the standard tools. Mainly since we fail to maintain the signals and more important child signals correctly. So we quickly end up with zombies or orphaned processes. My thinking instead was to leave this to systemd to handle.
> >
> > Interesting. Any pointer how to achieve that?
> > The problem is that I have btattach called by a systemd unit file to
> > download the firmware to the bluetooth transceiver. I also have
> > another systemd unit that must be called strictly after btattach has
> > done uploading the firmware. Since btattach is hanging on the port
> > AFAIU there is no way I can tell exactly when it is done uploading the
> > firmware. That's why I was adding this detach option, to mark the unit
> > as 'Type=forking' and enforcing the ordering with
> > 'Before=next.service`.
> > Anything I am missing?
>
> why do you need to know when the firmware uploading has completed? What hardware is this?
>
> The hardware is a custom ARM board based on an Amlogic SOC.
> More specifically I need btattach systemd unit to run before systemd-rfkill. This is because the rfkill driver is actually driving the GPIO enable of the transceiver, so if systemd-rfkill runs before btattach has completed telling to disable the radio (because we rebooted with the BT off) this just interrupts the firmware downloading leaving the transceiver in a non-working state.

this whole design mess with RFKILL switches for GPIO reset lines is a problem. We start fixing this for Intel and Broadcom based Bluetooth chips. So I would prefer if we can get this fixed inside the kernel. Also serdev bus is coming that will make btattach obsolete for hardwired Bluetooth UART chips.

And frankly, I would prefer to add RFKILL handling to btattach then trying to introduce a fork() which only existence to is to do some syncing with systemd-rfkill.

Regards

Marcel


2017-02-02 09:40:25

by Carlo Caione

[permalink] [raw]
Subject: Re: [PATCH] tools/btattach: Add detach option

On Thu, 2 Feb 2017 at 04:25, Marcel Holtmann <[email protected]> wrote:

> Hi Carlo,
>
> >>> ---
> >>> tools/btattach.c | 10 +++++++++-
> >>> 1 file changed, 9 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/tools/btattach.c b/tools/btattach.c
> >>> index 5adbc8d..f1f115d 100644
> >>> --- a/tools/btattach.c
> >>> +++ b/tools/btattach.c
> >>> @@ -194,6 +194,7 @@ static void usage(void)
> >>> "\t-P, --protocol <proto> Specify protocol type\n"
> >>> "\t-S, --speed <baudrate> Specify which baudrate to use\n"
> >>> "\t-N, --noflowctl Disable flow control\n"
> >>> + "\t-D, --detach Open device and then fork\n"
> >>> "\t-h, --help Show help options\n”);
> >>
> >> And is this really a good idea. I think that I had removed all the fork
> calls from the standard tools. Mainly since we fail to maintain the signals
> and more important child signals correctly. So we quickly end up with
> zombies or orphaned processes. My thinking instead was to leave this to
> systemd to handle.
> >
> > Interesting. Any pointer how to achieve that?
> > The problem is that I have btattach called by a systemd unit file to
> > download the firmware to the bluetooth transceiver. I also have
> > another systemd unit that must be called strictly after btattach has
> > done uploading the firmware. Since btattach is hanging on the port
> > AFAIU there is no way I can tell exactly when it is done uploading the
> > firmware. That's why I was adding this detach option, to mark the unit
> > as 'Type=forking' and enforcing the ordering with
> > 'Before=next.service`.
> > Anything I am missing?
>
> why do you need to know when the firmware uploading has completed? What
> hardware is this?


The hardware is a custom ARM board based on an Amlogic SOC.
More specifically I need btattach systemd unit to run before
systemd-rfkill. This is because the rfkill driver is actually driving the
GPIO enable of the transceiver, so if systemd-rfkill runs before btattach
has completed telling to disable the radio (because we rebooted with the BT
off) this just interrupts the firmware downloading leaving the transceiver
in a non-working state.

> --
Carlo Caione | +39.340.80.30.096 | Endless

2017-02-02 03:25:46

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH] tools/btattach: Add detach option

Hi Carlo,

>>> ---
>>> tools/btattach.c | 10 +++++++++-
>>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/btattach.c b/tools/btattach.c
>>> index 5adbc8d..f1f115d 100644
>>> --- a/tools/btattach.c
>>> +++ b/tools/btattach.c
>>> @@ -194,6 +194,7 @@ static void usage(void)
>>> "\t-P, --protocol <proto> Specify protocol type\n"
>>> "\t-S, --speed <baudrate> Specify which baudrate to use\n"
>>> "\t-N, --noflowctl Disable flow control\n"
>>> + "\t-D, --detach Open device and then fork\n"
>>> "\t-h, --help Show help options\n”);
>>
>> And is this really a good idea. I think that I had removed all the fork calls from the standard tools. Mainly since we fail to maintain the signals and more important child signals correctly. So we quickly end up with zombies or orphaned processes. My thinking instead was to leave this to systemd to handle.
>
> Interesting. Any pointer how to achieve that?
> The problem is that I have btattach called by a systemd unit file to
> download the firmware to the bluetooth transceiver. I also have
> another systemd unit that must be called strictly after btattach has
> done uploading the firmware. Since btattach is hanging on the port
> AFAIU there is no way I can tell exactly when it is done uploading the
> firmware. That's why I was adding this detach option, to mark the unit
> as 'Type=forking' and enforcing the ordering with
> 'Before=next.service`.
> Anything I am missing?

why do you need to know when the firmware uploading has completed? What hardware is this?

Regards

Marcel


2017-02-01 22:19:19

by Carlo Caione

[permalink] [raw]
Subject: Re: [PATCH] tools/btattach: Add detach option

On Wed, Feb 1, 2017 at 8:41 PM, Marcel Holtmann <[email protected]> wrote=
:
> Hi Carl,
>
>> Signed-off-by: Carlo Caione <[email protected]>
>
> user space does not use signed off by statements.
>
>> ---
>> tools/btattach.c | 10 +++++++++-
>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/btattach.c b/tools/btattach.c
>> index 5adbc8d..f1f115d 100644
>> --- a/tools/btattach.c
>> +++ b/tools/btattach.c
>> @@ -194,6 +194,7 @@ static void usage(void)
>> "\t-P, --protocol <proto> Specify protocol type\n"
>> "\t-S, --speed <baudrate> Specify which baudrate to use\n"
>> "\t-N, --noflowctl Disable flow control\n"
>> + "\t-D, --detach Open device and then fork\n"
>> "\t-h, --help Show help options\n=E2=80=9D);
>
> And is this really a good idea. I think that I had removed all the fork c=
alls from the standard tools. Mainly since we fail to maintain the signals =
and more important child signals correctly. So we quickly end up with zombi=
es or orphaned processes. My thinking instead was to leave this to systemd =
to handle.

Interesting. Any pointer how to achieve that?
The problem is that I have btattach called by a systemd unit file to
download the firmware to the bluetooth transceiver. I also have
another systemd unit that must be called strictly after btattach has
done uploading the firmware. Since btattach is hanging on the port
AFAIU there is no way I can tell exactly when it is done uploading the
firmware. That's why I was adding this detach option, to mark the unit
as 'Type=3Dforking' and enforcing the ordering with
'Before=3Dnext.service`.
Anything I am missing?


--=20
Carlo Caione | +39.340.80.30.096 | Endless

2017-02-01 19:41:14

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH] tools/btattach: Add detach option

Hi Carl,

> Signed-off-by: Carlo Caione <[email protected]>

user space does not use signed off by statements.

> ---
> tools/btattach.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/tools/btattach.c b/tools/btattach.c
> index 5adbc8d..f1f115d 100644
> --- a/tools/btattach.c
> +++ b/tools/btattach.c
> @@ -194,6 +194,7 @@ static void usage(void)
> "\t-P, --protocol <proto> Specify protocol type\n"
> "\t-S, --speed <baudrate> Specify which baudrate to use\n"
> "\t-N, --noflowctl Disable flow control\n"
> + "\t-D, --detach Open device and then fork\n"
> "\t-h, --help Show help options\n”);

And is this really a good idea. I think that I had removed all the fork calls from the standard tools. Mainly since we fail to maintain the signals and more important child signals correctly. So we quickly end up with zombies or orphaned processes. My thinking instead was to leave this to systemd to handle.

In addition, we are close to getting serdev bus which means btattach will be something of the past really soon.

Regards

Marcel