793809 - Take FM radio initialization off main thread

Status: RESOLVED FIXED

(Core :: Hardware Abstraction Layer (HAL), defect)

Description

[Michael Wu [:mwu]]

Attachment: Take FM radio initialization off main thread

Comment 1

[Justin Lebar (not reading bugmail)]

>diff --git a/hal/gonk/GonkFMRadio.cpp b/hal/gonk/GonkFMRadio.cpp
> class RadioUpdate : public nsRunnable {
>   hal::FMRadioOperation mOp;
>+  hal::FMRadioOperationStatus mStatus;
> public:
>-  RadioUpdate(hal::FMRadioOperation op)
>+  RadioUpdate(hal::FMRadioOperation op,
>+              hal::FMRadioOperationStatus status = hal::FM_RADIO_OPERATION_STATUS_SUCCESS)

I almost think this would be cleaner if we didn't have the default param here,
but up to you.

>     : mOp(op)
>+    , mStatus(status)
>   {}
> 
>   NS_IMETHOD Run() {
>     hal::FMRadioOperationInformation info;
>     info.operation() = mOp;
>-    info.status() = hal::FM_RADIO_OPERATION_STATUS_SUCCESS;
>+    info.status() = mStatus;
>     info.frequency() = GetFMRadioFrequency();
>     hal::NotifyFMRadioStatus(info);
>     return NS_OK;
>   }
> };
> 

>+static void
>+initTavaruaRadio(hal::FMRadioSettings &aInfo)
> {

>+  mozilla::ScopedClose fd(sRadioFD);
>   char command[64];
>-  snprintf(command, sizeof(command), "/system/bin/fm_qsoc_patches %d 0", cap.version);
>-  rc = system(command);
>+  snprintf(command, sizeof(command), "/system/bin/fm_qsoc_patches %d 0", sTavaruaVersion);
>+  int rc = system(command);

I'd feel a lot safer if this was 

    int rc = system(nsPrintfCString("/system/bin/fm_qsoc_patches %d 0", sTavaruaVersion).get());

because we can't overflow that.  But this is safe as-is, so you don't need to
change it.

>@@ -201,39 +143,116 @@ EnableFMRadio(const hal::FMRadioSettings& aInfo)

>+static void *
>+runTavaruaRadio(void *)

Please indicate either with a comment or an assertion what thread this is
running on.  Same for initTavaruaRadio.

>+void
>+EnableFMRadio(const hal::FMRadioSettings& aInfo)
>+{

ibid.

>+  sRadioFD = fd;
>+  sRadioSettings = aInfo;
>+  fd.forget();

I think you can do |sRadioFD = fd.forget();|.

r=me with nits picked.

Comment 2

[Michael Wu [:mwu]]

Attachment: Take FM radio initialization off main thread, v2

Nits addressed except for the nsPrintfCString part. That line just looks weird to me..

Comment 3

[Michael Wu [:mwu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a25086637d1f

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a25086637d1f