usage of sparse or other trick for improved type safety

usage of sparse or other trick for improved type safety

[Woodruff, Richard]

Hi Tony,

I am hoping to solicit an opinion from you for OMAP frameworks in general.

In some recent review there was some debate about how it was good practice to form parameters in a way which didn't get misused. Nishanth was having some experience where end users doing custom ports made some hard to find mistakes.

I was wondering if it is useful to create a standard or it's a waste of time.  The knee-jerk reaction to comment is to consider annotations for driver users of api's, then inside the framework trust internals to do the right thing.  Complexity divide between a driver and some frameworks might be similar to user-vs-kernel.

I think example in this case was IVA and other external subsystems commonly got away using stale definitions when managing their own power domains.  Example seemed a little pedantic but it is true that this has broken several times through migrations. At customer fan out it causes a lot of effort which could have been avoided.

Just last week someone was trying to caste away an iomem annotation from external driver based on warning. For me warning was a good thing and forced discussion.

I do recall you pushing what ARM and Linux tress did in this area back into OMAP years back.  Question is if it's worth internalizing more for our ever growing frameworks.

Thanks,
Richard W.

Re: usage of sparse or other trick for improved type safety

[Tony Lindgren]

Hi,

* Woodruff, Richard <r-woodruff2@ti.com> [120524 11:16]:
> Hi Tony,
> 
> I am hoping to solicit an opinion from you for OMAP frameworks in general.
> 
> In some recent review there was some debate about how it was good practice to form parameters in a way which didn't get misused. Nishanth was having some experience where end users doing custom ports made some hard to find mistakes.
> 
> I was wondering if it is useful to create a standard or it's a waste of time.  The knee-jerk reaction to comment is to consider annotations for driver users of api's, then inside the framework trust internals to do the right thing.  Complexity divide between a driver and some frameworks might be similar to user-vs-kernel.
> 
> I think example in this case was IVA and other external subsystems commonly got away using stale definitions when managing their own power domains.  Example seemed a little pedantic but it is true that this has broken several times through migrations. At customer fan out it causes a lot of effort which could have been avoided.
> 
> Just last week someone was trying to caste away an iomem annotation from external driver based on warning. For me warning was a good thing and forced discussion.
> 
> I do recall you pushing what ARM and Linux tress did in this area back into OMAP years back.  Question is if it's worth internalizing more for our ever growing frameworks.

I think we should have frameworks in place to manage clocks and powerdomains
for pretty much all drivers using runtime PM by now. That has helped making the
device drivers more generic and easier to maintain. And it's also less likely
for device drivers to accidentally mess up things for other drivers.

Before we had these frameworks in place it was often hard to debug when something
did not work for some omaps. Things would just not work or would stop working
for some drivers with no ideas what was going on. So yeah, having these kind of
frameworks in place has helped a lot to avoid breakage like you're describing
above.

For some external subsystems it might be possible to let the omap kernel manage
powerdomains and other resource via remote proc interface, assuming these
registers are ioremapped in the omap kernel side and not owned by the external
subsystem. The messaging to do this would add some undesired latencies though,
but maybe those would not be so critical for the external subsystems as they
tend to be full on or completely off type accelerators.

The other alternative of course is to implement similar frameworks for the
external subsystems. Some of this might be possible to implement as simple
macros with some type checks if it's subsystem specific. For doing it for
all omap devices, macros were soon found not to be enough as the various
bits all over need to be linked together for managing various resources.

Regards,

Tony

RE: usage of sparse or other trick for improved type safety

[Woodruff, Richard]

Hi Tony,

> From: Tony Lindgren [mailto:tony@atomide.com]
> Sent: Friday, May 25, 2012 2:53 AM

Thanks for quick input.  Apologies on slow ack of it.

> Before we had these frameworks in place it was often hard to debug when
> something
> did not work for some omaps. Things would just not work or would stop
> working
> for some drivers with no ideas what was going on. So yeah, having these
> kind of
> frameworks in place has helped a lot to avoid breakage like you're
> describing
> above.

Trees which had to hit the lowest power states for full customer boards all employed some form of abstraction of clock, power, voltage. The one today in mainline today is the most clean. Aspects around auto-generation are very good even if a bit big in size.

Perhaps main grump I hear is the number of abstraction layers sometimes makes debugging difficult. It would be nice to find smart tricks for compile to prune away some.

Still one can experience some of the mystery issues, as the PRCM connection to IP-device utilizes a protocol which the device side can mess up. If the device does not shut down its local function and associated clocks cleanly it will show 'stuck in transition' and this gates final global changes. In one of the closed implementation we would bug() drivers who did not shut down their internal clocks properly before allowing global clock release.  Most of the issues seen in field are at drivers/peripheral-ip.


> For some external subsystems it might be possible to let the omap kernel
> manage
> powerdomains and other resource via remote proc interface, assuming these
> registers are ioremapped in the omap kernel side and not owned by the
> external
> subsystem. The messaging to do this would add some undesired latencies
> though,
> but maybe those would not be so critical for the external subsystems as
> they
> tend to be full on or completely off type accelerators.

Humm. Maybe for some. Guess walking what is used and sorting might tell.

The way some subsystems 'ideal' operation is described from designers implies tight control of timing and sequence for things like power state. A RPC doesn't feel like it fits with intent. However... practically speaking from 'full system view' RPC may fit sometimes. A subsystem exporting hooks to save 100uW using its optimal state set against other components running in 500mW range minimizes usefulness.
 
> The other alternative of course is to implement similar frameworks for the
> external subsystems. Some of this might be possible to implement as simple
> macros with some type checks if it's subsystem specific. For doing it for
> all omap devices, macros were soon found not to be enough as the various
> bits all over need to be linked together for managing various resources.

Agree. 

I don't know insides of all subsystems.  Though what I know or hear is kind of a mixed picture.

OMAP has an ultra high level of integration. As such you might find something like DSP-Bios may provide a good hook but quick integration of a previously standalone single purpose piece does not have time to be readapted.

The type checking question kind of grows out of this.  Linux might today offer a clean run-time api which is place where high wall should be built with type check.  But... the driver might not be able to function yet with the API alone given state of evolution of both ends.

Review question was pointing re-hitting of bugs for what could be argued 'ideal' internal framework api. How to fix up what is in use by real drivers or to add/fix external api so its useful should be roadmap.

Regards,
Richard W.

Re: usage of sparse or other trick for improved type safety

[Jean Pihet]

Hi Richard, all,

On Tue, Jun 12, 2012 at 6:34 PM, Woodruff, Richard <r-woodruff2@ti.com> wrote:
> Hi Tony,
>
>> From: Tony Lindgren [mailto:tony@atomide.com]
>> Sent: Friday, May 25, 2012 2:53 AM
>
> Thanks for quick input.  Apologies on slow ack of it.
>
>> Before we had these frameworks in place it was often hard to debug when
>> something
>> did not work for some omaps. Things would just not work or would stop
>> working
>> for some drivers with no ideas what was going on. So yeah, having these
>> kind of
>> frameworks in place has helped a lot to avoid breakage like you're
>> describing
>> above.
>
> Trees which had to hit the lowest power states for full customer boards all employed some form of abstraction of clock, power, voltage. The one today in mainline today is the most clean. Aspects around auto-generation are very good even if a bit big in size.
>
> Perhaps main grump I hear is the number of abstraction layers sometimes makes debugging difficult. It would be nice to find smart tricks for compile to prune away some.
>
> Still one can experience some of the mystery issues, as the PRCM connection to IP-device utilizes a protocol which the device side can mess up. If the device does not shut down its local function and associated clocks cleanly it will show 'stuck in transition' and this gates final global changes. In one of the closed implementation we would bug() drivers who did not shut down their internal clocks properly before allowing global clock release.  Most of the issues seen in field are at drivers/peripheral-ip.
>
>
>> For some external subsystems it might be possible to let the omap kernel
>> manage
>> powerdomains and other resource via remote proc interface, assuming these
>> registers are ioremapped in the omap kernel side and not owned by the
>> external
>> subsystem. The messaging to do this would add some undesired latencies
>> though,
>> but maybe those would not be so critical for the external subsystems as
>> they
>> tend to be full on or completely off type accelerators.
>
> Humm. Maybe for some. Guess walking what is used and sorting might tell.
>
> The way some subsystems 'ideal' operation is described from designers implies tight control of timing and sequence for things like power state. A RPC doesn't feel like it fits with intent. However... practically speaking from 'full system view' RPC may fit sometimes. A subsystem exporting hooks to save 100uW using its optimal state set against other components running in 500mW range minimizes usefulness.
>
>> The other alternative of course is to implement similar frameworks for the
>> external subsystems. Some of this might be possible to implement as simple
>> macros with some type checks if it's subsystem specific. For doing it for
>> all omap devices, macros were soon found not to be enough as the various
>> bits all over need to be linked together for managing various resources.
>
> Agree.
>
> I don't know insides of all subsystems.  Though what I know or hear is kind of a mixed picture.
>
> OMAP has an ultra high level of integration. As such you might find something like DSP-Bios may provide a good hook but quick integration of a previously standalone single purpose piece does not have time to be readapted.
>
> The type checking question kind of grows out of this.  Linux might today offer a clean run-time api which is place where high wall should be built with type check.  But... the driver might not be able to function yet with the API alone given state of evolution of both ends.

Regarding the API and type checking I think the following must be enforced:
1. proper type checking in the API, possibly by the compiler,
2. clean separation of an API into an internal part (used only by the
framework implementation) and an external part (used by the callers of
the API).

About 1: using enum for the parameters did not help much AFACIT. The
compiler cannot tell if the parameter from a variable is in the
allowed range.
Any thought?

About 2: while introducing the functional power states I came across
this problem. Ideally the current states macros (PWRDM_POWER_* and
PWRSTS_*) shall be used by the internal code only in powerdomain*.[ch]
while the callers (pm.c etc) shall use the new macros
(PWRDM_FUNC_PWRST_*) and API (mainly pwrdm_*_func_* and
omap_set_pwrdm_state).

Here below is a patch extract (trimmed for brevity) to illustrate the problem.

From here is see a few possible options:
1. clearly separate the internal and external parts of the API in the
header file using comments (as done in the patch below) or with a new
#ifdef POWERDOMAIN_INTERNAL_API (ugly I know),
2. create a new internal only header file (powerdomain_internal.h) and
include it only from powerdomain*.[ch],
3. move the external API to pm.h and keep the internal API in powerdomain.h

Although I am using the func power states as an example I think this
is a applicable to all PM frameworks APIs.

What do you think?


diff --git a/arch/arm/mach-omap2/powerdomain.h
b/arch/arm/mach-omap2/powerdomain.h
index bab84fc..0404f9f 100644
--- a/arch/arm/mach-omap2/powerdomain.h
+++ b/arch/arm/mach-omap2/powerdomain.h
@@ -26,7 +23,51 @@
-/* Powerdomain basic power states */
+/***************************************************************
+ * External API
+ ***************************************************************/
+
+/* Powerdomain functional power states, used by the external API functions */
+enum pwrdm_func_state {
+	PWRDM_FUNC_PWRST_OFF		= 0x0,
+	PWRDM_FUNC_PWRST_OSWR,
+	PWRDM_FUNC_PWRST_CSWR,
+	PWRDM_FUNC_PWRST_INACTIVE,
+	PWRDM_FUNC_PWRST_ON,
+	PWRDM_MAX_FUNC_PWRSTS		/* Last value, used as the max value */
+};
+
+struct clockdomain;
+struct powerdomain;
+
...
+
+int pwrdm_read_prev_func_pwrst(struct powerdomain *pwrdm);
+int pwrdm_read_func_pwrst(struct powerdomain *pwrdm);
+int pwrdm_read_next_func_pwrst(struct powerdomain *pwrdm);
+extern int omap_set_pwrdm_state(struct powerdomain *pwrdm,
+				enum pwrdm_func_state func_pwrst);
+
+
+/***************************************************************
+ * Internal code, only used in powerdomain*.[ch]
+ ***************************************************************/
+
+/* Powerdomain internal power states, internal use only */
 #define PWRDM_POWER_OFF		0x0
 #define PWRDM_POWER_RET		0x1
 #define PWRDM_POWER_INACTIVE	0x2
@@ -45,7 +86,6 @@
 #define PWRSTS_RET_ON		(PWRSTS_RET | PWRSTS_ON)
 #define PWRSTS_OFF_RET_ON	(PWRSTS_OFF_RET | PWRSTS_ON)

-
 /* Powerdomain flags */
 #define PWRDM_HAS_HDWR_SAR	(1 << 0) /* hardware save-and-restore support */
 #define PWRDM_HAS_MPU_QUIRK	(1 << 1) /* MPU pwr domain has MEM bank 0 bits


> Review question was pointing re-hitting of bugs for what could be argued 'ideal' internal framework api. How to fix up what is in use by real drivers or to add/fix external api so its useful should be roadmap.
>
> Regards,
> Richard W.
>

Thanks for starting the discussion!

Regards,
Jean
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: usage of sparse or other trick for improved type safety

[Jean Pihet]

Hi!

Added Paul in Cc:.

On Thu, Jun 14, 2012 at 10:05 AM, Jean Pihet <jean.pihet@newoldbits.com> wrote:
> Hi Richard, all,
>
> On Tue, Jun 12, 2012 at 6:34 PM, Woodruff, Richard <r-woodruff2@ti.com> wrote:
>> Hi Tony,
>>
>>> From: Tony Lindgren [mailto:tony@atomide.com]
>>> Sent: Friday, May 25, 2012 2:53 AM
>>
>> Thanks for quick input.  Apologies on slow ack of it.
>>
>>> Before we had these frameworks in place it was often hard to debug when
>>> something
>>> did not work for some omaps. Things would just not work or would stop
>>> working
>>> for some drivers with no ideas what was going on. So yeah, having these
>>> kind of
>>> frameworks in place has helped a lot to avoid breakage like you're
>>> describing
>>> above.
>>
>> Trees which had to hit the lowest power states for full customer boards all employed some form of abstraction of clock, power, voltage. The one today in mainline today is the most clean. Aspects around auto-generation are very good even if a bit big in size.
>>
>> Perhaps main grump I hear is the number of abstraction layers sometimes makes debugging difficult. It would be nice to find smart tricks for compile to prune away some.
>>
>> Still one can experience some of the mystery issues, as the PRCM connection to IP-device utilizes a protocol which the device side can mess up. If the device does not shut down its local function and associated clocks cleanly it will show 'stuck in transition' and this gates final global changes. In one of the closed implementation we would bug() drivers who did not shut down their internal clocks properly before allowing global clock release.  Most of the issues seen in field are at drivers/peripheral-ip.
>>
>>
>>> For some external subsystems it might be possible to let the omap kernel
>>> manage
>>> powerdomains and other resource via remote proc interface, assuming these
>>> registers are ioremapped in the omap kernel side and not owned by the
>>> external
>>> subsystem. The messaging to do this would add some undesired latencies
>>> though,
>>> but maybe those would not be so critical for the external subsystems as
>>> they
>>> tend to be full on or completely off type accelerators.
>>
>> Humm. Maybe for some. Guess walking what is used and sorting might tell.
>>
>> The way some subsystems 'ideal' operation is described from designers implies tight control of timing and sequence for things like power state. A RPC doesn't feel like it fits with intent. However... practically speaking from 'full system view' RPC may fit sometimes. A subsystem exporting hooks to save 100uW using its optimal state set against other components running in 500mW range minimizes usefulness.
>>
>>> The other alternative of course is to implement similar frameworks for the
>>> external subsystems. Some of this might be possible to implement as simple
>>> macros with some type checks if it's subsystem specific. For doing it for
>>> all omap devices, macros were soon found not to be enough as the various
>>> bits all over need to be linked together for managing various resources.
>>
>> Agree.
>>
>> I don't know insides of all subsystems.  Though what I know or hear is kind of a mixed picture.
>>
>> OMAP has an ultra high level of integration. As such you might find something like DSP-Bios may provide a good hook but quick integration of a previously standalone single purpose piece does not have time to be readapted.
>>
>> The type checking question kind of grows out of this.  Linux might today offer a clean run-time api which is place where high wall should be built with type check.  But... the driver might not be able to function yet with the API alone given state of evolution of both ends.
>
> Regarding the API and type checking I think the following must be enforced:
> 1. proper type checking in the API, possibly by the compiler,
> 2. clean separation of an API into an internal part (used only by the
> framework implementation) and an external part (used by the callers of
> the API).
>
> About 1: using enum for the parameters did not help much AFACIT. The
> compiler cannot tell if the parameter from a variable is in the
> allowed range.
> Any thought?
>
> About 2: while introducing the functional power states I came across
> this problem. Ideally the current states macros (PWRDM_POWER_* and
> PWRSTS_*) shall be used by the internal code only in powerdomain*.[ch]
> while the callers (pm.c etc) shall use the new macros
> (PWRDM_FUNC_PWRST_*) and API (mainly pwrdm_*_func_* and
> omap_set_pwrdm_state).
>
> Here below is a patch extract (trimmed for brevity) to illustrate the problem.
FYI the full patch is at http://marc.info/?l=linux-omap&m=133968581305050&w=2

>
> From here is see a few possible options:
> 1. clearly separate the internal and external parts of the API in the
> header file using comments (as done in the patch below) or with a new
> #ifdef POWERDOMAIN_INTERNAL_API (ugly I know),
Some more details about that option: the internal values could be
prefixed by '__' or similar which kinds of highlights a bad API usage.

> 2. create a new internal only header file (powerdomain_internal.h) and
> include it only from powerdomain*.[ch],
> 3. move the external API to pm.h and keep the internal API in powerdomain.h
>
> Although I am using the func power states as an example I think this
> is a applicable to all PM frameworks APIs.
>
> What do you think?
Any thought?

> diff --git a/arch/arm/mach-omap2/powerdomain.h
> b/arch/arm/mach-omap2/powerdomain.h
> index bab84fc..0404f9f 100644
> --- a/arch/arm/mach-omap2/powerdomain.h
> +++ b/arch/arm/mach-omap2/powerdomain.h
> @@ -26,7 +23,51 @@
> -/* Powerdomain basic power states */
> +/***************************************************************
> + * External API
> + ***************************************************************/
> +
> +/* Powerdomain functional power states, used by the external API functions */
> +enum pwrdm_func_state {
> +       PWRDM_FUNC_PWRST_OFF            = 0x0,
> +       PWRDM_FUNC_PWRST_OSWR,
> +       PWRDM_FUNC_PWRST_CSWR,
> +       PWRDM_FUNC_PWRST_INACTIVE,
> +       PWRDM_FUNC_PWRST_ON,
> +       PWRDM_MAX_FUNC_PWRSTS           /* Last value, used as the max value */
> +};
> +
> +struct clockdomain;
> +struct powerdomain;
> +
> ...
> +
> +int pwrdm_read_prev_func_pwrst(struct powerdomain *pwrdm);
> +int pwrdm_read_func_pwrst(struct powerdomain *pwrdm);
> +int pwrdm_read_next_func_pwrst(struct powerdomain *pwrdm);
> +extern int omap_set_pwrdm_state(struct powerdomain *pwrdm,
> +                               enum pwrdm_func_state func_pwrst);
> +
> +
> +/***************************************************************
> + * Internal code, only used in powerdomain*.[ch]
> + ***************************************************************/
> +
> +/* Powerdomain internal power states, internal use only */
>  #define PWRDM_POWER_OFF                0x0
>  #define PWRDM_POWER_RET                0x1
>  #define PWRDM_POWER_INACTIVE   0x2
> @@ -45,7 +86,6 @@
>  #define PWRSTS_RET_ON          (PWRSTS_RET | PWRSTS_ON)
>  #define PWRSTS_OFF_RET_ON      (PWRSTS_OFF_RET | PWRSTS_ON)
>
> -
>  /* Powerdomain flags */
>  #define PWRDM_HAS_HDWR_SAR     (1 << 0) /* hardware save-and-restore support */
>  #define PWRDM_HAS_MPU_QUIRK    (1 << 1) /* MPU pwr domain has MEM bank 0 bits
>
>
>> Review question was pointing re-hitting of bugs for what could be argued 'ideal' internal framework api. How to fix up what is in use by real drivers or to add/fix external api so its useful should be roadmap.
>>
>> Regards,
>> Richard W.
>>
>
> Thanks for starting the discussion!
>
> Regards,
> Jean
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html