driver callback naming

driver callback naming

[Andres Salomon]

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

Hi,

Slightly bikesheddy, but.. The ofono_modem_driver callback naming is
pretty confusing.  Currently, it's:


static struct ofono_modem_driver g1_driver = {
        .name = "HTC G1",
        .probe = g1_probe,
        .enable = g1_enable,
        .disable = g1_disable,
        .remove = g1_remove,
        .populate = g1_populate,
};


I'm used to probe/remove in the kernel being used for device detection
and allocating related resources (as well as powering up or down pci
devices and such), but the enable/disable names tell me nothing.
What's being enabled?  Is 'probe' just a thin layer that detects
whether a device is present and then defers to 'enable' to set it
up (and if so, should we it called 'detect' or something that
doesn't imply a kernel-like 'probe' function)?  Denkenz informs me that
it's for setting power to devices; if that's the case, a better name
might be powerup/powerdown.

Of course, I'm also wondering why there needs to be two separate layers
of calls in the first place.  Why not have drivers register everything
from within probe, call ofono_set_powered(modem, TRUE) once the device
is ready, and be done with it?

This question comes up because sending SIGTERM to the
daemon ends up calling g1_exit, which (in my case) was freeing the
ofono_modem->private_data first, and then calling ofono_modem_remove.
This in turn was calling the 'disable' callback, which expected
ofono_modem->private_data to exist.

The only reason why this doesn't blow up in the generic_at plugin is
because the driver_data is leaked.  If one were to free it from
generic_at_exit in the wrong place (since it's allocated from
generic_at_init, it would make sense to free it in generic_at_exit),
one would see the same SEGV/SIGBUS/SIGILL errors upon ctrl-c.

Re: driver callback naming

[Denis Kenzior]

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

Hi Andres,

> static struct ofono_modem_driver g1_driver = {
>         .name = "HTC G1",
>         .probe = g1_probe,
>         .enable = g1_enable,
>         .disable = g1_disable,
>         .remove = g1_remove,
>         .populate = g1_populate,
> };
>

So the current intention:
.probe - Detect whether device is really supported by the plugin, initialize 
any data structures specific to the device
.remove - Destroy data structures
.enable - Perform power up
.disable - Perform power down
.populate - Populate the atoms supported by this device (e.g. netreg, 
voicecall, etc)  This is called by the core after every power cycle, when the 
device is brought up.

>
> Of course, I'm also wondering why there needs to be two separate layers
> of calls in the first place.  Why not have drivers register everything
> from within probe, call ofono_set_powered(modem, TRUE) once the device
> is ready, and be done with it?

The reason for this is e.g. airplane mode, where you physically want to turn 
off the device.  Another case is for battery / power reasons, e.g. a netbook 
with a USB modem that is not being used.

> The only reason why this doesn't blow up in the generic_at plugin is
> because the driver_data is leaked.  If one were to free it from
> generic_at_exit in the wrong place (since it's allocated from
> generic_at_init, it would make sense to free it in generic_at_exit),
> one would see the same SEGV/SIGBUS/SIGILL errors upon ctrl-c.

So the leak has now been fixed.

I think you're being unnecessarily harsh here.  To be fair, the generic_at 
driver does something like this at init:

create generic_data
create modem
set modem data
register modem

If you were to reverse the steps, e.g.:
remove_modem
destroy generic_data

everything would work just fine.

Regards,
-Denis

Re: driver callback naming

[Andres Salomon]

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

On Sun, 30 Aug 2009 13:45:45 -0500
Denis Kenzior <denkenz@gmail.com> wrote:

> Hi Andres,
> 
> > static struct ofono_modem_driver g1_driver = {
> >         .name = "HTC G1",
> >         .probe = g1_probe,
> >         .enable = g1_enable,
> >         .disable = g1_disable,
> >         .remove = g1_remove,
> >         .populate = g1_populate,
> > };
> >
> 
> So the current intention:
> .probe - Detect whether device is really supported by the plugin,
> initialize any data structures specific to the device
> .remove - Destroy data structures
> .enable - Perform power up
> .disable - Perform power down
> .populate - Populate the atoms supported by this device (e.g. netreg, 
> voicecall, etc)  This is called by the core after every power cycle,
> when the device is brought up.
> 

Thanks!  See patch below.


> >
> > Of course, I'm also wondering why there needs to be two separate
> > layers of calls in the first place.  Why not have drivers register
> > everything from within probe, call ofono_set_powered(modem, TRUE)
> > once the device is ready, and be done with it?
> 
> The reason for this is e.g. airplane mode, where you physically want
> to turn off the device.  Another case is for battery / power reasons,
> e.g. a netbook with a USB modem that is not being used.
> 

Fair enough.  In the kernel, we have callbacks named suspend/resume
to handle that. 

> > The only reason why this doesn't blow up in the generic_at plugin is
> > because the driver_data is leaked.  If one were to free it from
> > generic_at_exit in the wrong place (since it's allocated from
> > generic_at_init, it would make sense to free it in generic_at_exit),
> > one would see the same SEGV/SIGBUS/SIGILL errors upon ctrl-c.
> 
> So the leak has now been fixed.
> 
> I think you're being unnecessarily harsh here.  To be fair, the
> generic_at driver does something like this at init:

My criticism is simply w/ the naming.  'enable'/'disable' doesn't imply
anything about power.  powerup/powerdown, poweron/poweroff,
suspend/resume would all imply power state changes (at least the latter
would be familiar to those who do kernel stuff).  Having comments that
describe what the callbacks do would also work, though.




From 80a7b54d52201dfd7d8b590457450ae0a4f72888 Mon Sep 17 00:00:00 2001
From: Andres Salomon <dilinger@collabora.co.uk>
Date: Sun, 30 Aug 2009 15:56:16 -0400
Subject: [PATCH] Add comments to ofono_modem_driver struct

Document what all the callbacks do.
---
 include/modem.h |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/include/modem.h b/include/modem.h
index 0a5ef60..4303024 100644
--- a/include/modem.h
+++ b/include/modem.h
@@ -50,10 +50,19 @@ ofono_bool_t ofono_modem_get_powered(struct ofono_modem *modem);
 
 struct ofono_modem_driver {
 	const char *name;
+
+	/* probe - Detect existence of device and initialize any
+	 * device-specific data structures */
 	int (*probe)(struct ofono_modem *modem);
+	/* remove - Destroy data structures allocated during probe */
 	int (*remove)(struct ofono_modem *modem);
+
+	/* enable - Power up device */
 	int (*enable)(struct ofono_modem *modem);
+	/* disable - Power down device */
 	int (*disable)(struct ofono_modem *modem);
+
+	/* populate - Populate the atoms supported by this device */
 	int (*populate)(struct ofono_modem *modem);
 };
 
-- 
1.6.3.3

Re: driver callback naming

[Denis Kenzior]

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

Hi Andres,

> > > Of course, I'm also wondering why there needs to be two separate
> > > layers of calls in the first place.  Why not have drivers register
> > > everything from within probe, call ofono_set_powered(modem, TRUE)
> > > once the device is ready, and be done with it?
> >
> > The reason for this is e.g. airplane mode, where you physically want
> > to turn off the device.  Another case is for battery / power reasons,
> > e.g. a netbook with a USB modem that is not being used.
>
> Fair enough.  In the kernel, we have callbacks named suspend/resume
> to handle that.

Power down is different from suspend / resume though.  Suspend implies a 
different usecase, particularly on embedded devices.  In fact, I'm already 
considering adding suspend and resume to the driver API...

> My criticism is simply w/ the naming.  'enable'/'disable' doesn't imply
> anything about power.  powerup/powerdown, poweron/poweroff,
> suspend/resume would all imply power state changes (at least the latter
> would be familiar to those who do kernel stuff).  Having comments that
> describe what the callbacks do would also work, though.
>

Fair enough.

Regards,
-Denis

Re: driver callback naming

[Aki Niemi]

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

2009/8/30 Denis Kenzior <denkenz@gmail.com>:
> The reason for this is e.g. airplane mode, where you physically want to turn
> off the device.  Another case is for battery / power reasons, e.g. a netbook
> with a USB modem that is not being used.

For airplane mode, you want to turn off the radios. I'm also a bit
unclear where in the Nokia modem API the enable/disable callbacks
would map. Offline mode can be set via the modem API, but completely
powering the modem off is done in HW.

On a somewhat related note, I don't quite see what to do with
deregister in the netreg driver. Is the intended end state of the
device, in fact, similar to airplane mode?

Cheers,
Aki

Re: driver callback naming

[Denis Kenzior]

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

Hi Aki,

> 2009/8/30 Denis Kenzior <denkenz@gmail.com>:
> > The reason for this is e.g. airplane mode, where you physically want to
> > turn off the device.  Another case is for battery / power reasons, e.g. a
> > netbook with a USB modem that is not being used.
>
> For airplane mode, you want to turn off the radios. I'm also a bit
> unclear where in the Nokia modem API the enable/disable callbacks
> would map. Offline mode can be set via the modem API, but completely
> powering the modem off is done in HW.

Turning off the radio is the intention.  However, oFono is not only to be used 
with proper modems, but with Bluetooth HFP, SAP, etc, where poweroff might have 
different meaning.  Some hardware supports even more drastic powerdown 
procedures than simply turning off the radio.

>
> On a somewhat related note, I don't quite see what to do with
> deregister in the netreg driver. Is the intended end state of the
> device, in fact, similar to airplane mode?

This maps directly to AT+COPS=2, which means completely deregister from 
network, but still be in full-capability mode.  The usefulness of this is of 
course questionable, like many other areas of the spec.  I'm actually willing 
to drop this from the driver, however there might be some use for this 
functionality when using manual operator selection.

Regards,
-Denis

Re: driver callback naming

[Aki Niemi]

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

Hi Denis,

2009/8/30 Denis Kenzior <denkenz@gmail.com>:
>> On a somewhat related note, I don't quite see what to do with
>> deregister in the netreg driver. Is the intended end state of the
>> device, in fact, similar to airplane mode?
>
> This maps directly to AT+COPS=2, which means completely deregister from
> network, but still be in full-capability mode.

You mean the device ends up in limited service mode, where only
emergency calls are possible? AFAIK, that can only be triggered with
the Nokia modem API as a side effect of deliberately making a manual
registration to a forbidden network. Yuk. :)

> The usefulness of this is of
> course questionable, like many other areas of the spec.  I'm actually willing
> to drop this from the driver, however there might be some use for this
> functionality when using manual operator selection.

I'd vote for dropping it. Unless we want to have real poweron/poweroff
in the modem interface, then the netreg driver could handle RF on/off.

Cheers,
Aki