* [PATCH 1/2] OMAP: omap_device: add omap_device_is_valid() @ 2010-01-26 19:00 Kevin Hilman 2010-01-26 19:00 ` [PATCH 2/2] OMAP: omap_device: when 'called from invalid state', print state Kevin Hilman 2010-01-28 0:30 ` [PATCH 1/2] OMAP: omap_device: add omap_device_is_valid() Paul Walmsley 0 siblings, 2 replies; 6+ messages in thread From: Kevin Hilman @ 2010-01-26 19:00 UTC (permalink / raw) To: linux-omap The omap_device struct contains a 'struct platform_device'. Normally, converting a platform_device pointer to an omap_device pointer consists of simply doing a container_of(), as is done currently by the to_omap_device() macro. However, if this is attempted when using platform_device that has not been created as part of the omap_device creation, the container_of() will point to a memory location before the platform_device pointer which will contain random data. Therefore, we need a way to detect valid omap_device pointers. This patch solves this by using the simple magic number approach. Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com> --- Changes from original version: - set magic value before omap_device_register() arch/arm/plat-omap/include/plat/omap_device.h | 2 ++ arch/arm/plat-omap/omap_device.c | 16 ++++++++++++++++ 2 files changed, 18 insertions(+), 0 deletions(-) diff --git a/arch/arm/plat-omap/include/plat/omap_device.h b/arch/arm/plat-omap/include/plat/omap_device.h index 76d4917..4677ff7 100644 --- a/arch/arm/plat-omap/include/plat/omap_device.h +++ b/arch/arm/plat-omap/include/plat/omap_device.h @@ -62,6 +62,7 @@ * */ struct omap_device { + u32 magic; struct platform_device pdev; struct omap_hwmod **hwmods; struct omap_device_pm_latency *pm_lats; @@ -81,6 +82,7 @@ int omap_device_shutdown(struct platform_device *pdev); /* Core code interface */ +bool omap_device_is_valid(struct omap_device *od); int omap_device_count_resources(struct omap_device *od); int omap_device_fill_resources(struct omap_device *od, struct resource *res); diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c index d8c75c8..187a08f 100644 --- a/arch/arm/plat-omap/omap_device.c +++ b/arch/arm/plat-omap/omap_device.c @@ -100,6 +100,8 @@ # error Unknown OMAP device #endif +#define OMAP_DEVICE_MAGIC 0xf00dcafe + /* Private functions */ /** @@ -413,6 +415,8 @@ struct omap_device *omap_device_build_ss(const char *pdev_name, int pdev_id, od->pm_lats = pm_lats; od->pm_lats_cnt = pm_lats_cnt; + od->magic = OMAP_DEVICE_MAGIC; + ret = omap_device_register(od); if (ret) goto odbs_exit4; @@ -599,6 +603,18 @@ int omap_device_align_pm_lat(struct platform_device *pdev, } /** + * omap_device_is_valid() + * @od: struct omap_device * + * + * Return whether struct omap_device pointer @od points to a valid + * omap_device. + */ +bool omap_device_is_valid(struct omap_device *od) +{ + return (od && od->magic == OMAP_DEVICE_MAGIC); +} + +/** * omap_device_get_pwrdm - return the powerdomain * associated with @od * @od: struct omap_device * * -- 1.6.6 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] OMAP: omap_device: when 'called from invalid state', print state 2010-01-26 19:00 [PATCH 1/2] OMAP: omap_device: add omap_device_is_valid() Kevin Hilman @ 2010-01-26 19:00 ` Kevin Hilman 2010-02-11 7:17 ` Paul Walmsley 2010-01-28 0:30 ` [PATCH 1/2] OMAP: omap_device: add omap_device_is_valid() Paul Walmsley 1 sibling, 1 reply; 6+ messages in thread From: Kevin Hilman @ 2010-01-26 19:00 UTC (permalink / raw) To: linux-omap The omap_device_[enable|idle|shutdown] functions print a warning when called from an invalid state. Print the invalid state in the warning messages. This also uses __func__ to get the function name. Also, move the entire print string onto a single line to facilitate grepping or error messages. Recent discussions on LKML show strong preference for grep-able code vs. strict 80 column limit. Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com> --- arch/arm/plat-omap/omap_device.c | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-) diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c index 187a08f..d4f34ea 100644 --- a/arch/arm/plat-omap/omap_device.c +++ b/arch/arm/plat-omap/omap_device.c @@ -476,8 +476,8 @@ int omap_device_enable(struct platform_device *pdev) od = _find_by_pdev(pdev); if (od->_state == OMAP_DEVICE_STATE_ENABLED) { - WARN(1, "omap_device: %s.%d: omap_device_enable() called from " - "invalid state\n", od->pdev.name, od->pdev.id); + WARN(1, "omap_device: %s.%d: %s() called from invalid state %d\n", + od->pdev.name, od->pdev.id, __func__, od->_state); return -EINVAL; } @@ -515,8 +515,8 @@ int omap_device_idle(struct platform_device *pdev) od = _find_by_pdev(pdev); if (od->_state != OMAP_DEVICE_STATE_ENABLED) { - WARN(1, "omap_device: %s.%d: omap_device_idle() called from " - "invalid state\n", od->pdev.name, od->pdev.id); + WARN(1, "omap_device: %s.%d: %s() called from invalid state %d\n", + od->pdev.name, od->pdev.id, __func__, od->_state); return -EINVAL; } @@ -548,8 +548,8 @@ int omap_device_shutdown(struct platform_device *pdev) if (od->_state != OMAP_DEVICE_STATE_ENABLED && od->_state != OMAP_DEVICE_STATE_IDLE) { - WARN(1, "omap_device: %s.%d: omap_device_shutdown() called " - "from invalid state\n", od->pdev.name, od->pdev.id); + WARN(1, "omap_device: %s.%d: %s() called from invalid state %d\n", + od->pdev.name, od->pdev.id, __func__, od->_state); return -EINVAL; } -- 1.6.6 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] OMAP: omap_device: when 'called from invalid state', print state 2010-01-26 19:00 ` [PATCH 2/2] OMAP: omap_device: when 'called from invalid state', print state Kevin Hilman @ 2010-02-11 7:17 ` Paul Walmsley 0 siblings, 0 replies; 6+ messages in thread From: Paul Walmsley @ 2010-02-11 7:17 UTC (permalink / raw) To: Kevin Hilman; +Cc: linux-omap On Tue, 26 Jan 2010, Kevin Hilman wrote: > The omap_device_[enable|idle|shutdown] functions print a warning > when called from an invalid state. Print the invalid state in > the warning messages. This also uses __func__ to get the function > name. > > Also, move the entire print string onto a single line to facilitate > grepping or error messages. Recent discussions on LKML show > strong preference for grep-able code vs. strict 80 column limit. > > Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com> Thanks, queued for 2.6.34. - Paul ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] OMAP: omap_device: add omap_device_is_valid() 2010-01-26 19:00 [PATCH 1/2] OMAP: omap_device: add omap_device_is_valid() Kevin Hilman 2010-01-26 19:00 ` [PATCH 2/2] OMAP: omap_device: when 'called from invalid state', print state Kevin Hilman @ 2010-01-28 0:30 ` Paul Walmsley 2010-01-28 17:19 ` Kevin Hilman 1 sibling, 1 reply; 6+ messages in thread From: Paul Walmsley @ 2010-01-28 0:30 UTC (permalink / raw) To: Kevin Hilman; +Cc: linux-omap Hi Kevin, this looks fine. Two quick comments: On Tue, 26 Jan 2010, Kevin Hilman wrote: > The omap_device struct contains a 'struct platform_device'. Normally, > converting a platform_device pointer to an omap_device pointer > consists of simply doing a container_of(), as is done currently by the > to_omap_device() macro. > > However, if this is attempted when using platform_device that has not > been created as part of the omap_device creation, the container_of() > will point to a memory location before the platform_device pointer > which will contain random data. > > Therefore, we need a way to detect valid omap_device pointers. This > patch solves this by using the simple magic number approach. Sounds okay; just FYI at some point in the future, it would be good for one of us to take a whack at converting omap_device to contain platform_device, the same way that platform_device contains struct device, etc.. > > Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com> > --- > Changes from original version: > - set magic value before omap_device_register() > > arch/arm/plat-omap/include/plat/omap_device.h | 2 ++ > arch/arm/plat-omap/omap_device.c | 16 ++++++++++++++++ > 2 files changed, 18 insertions(+), 0 deletions(-) > > diff --git a/arch/arm/plat-omap/include/plat/omap_device.h b/arch/arm/plat-omap/include/plat/omap_device.h > index 76d4917..4677ff7 100644 > --- a/arch/arm/plat-omap/include/plat/omap_device.h > +++ b/arch/arm/plat-omap/include/plat/omap_device.h > @@ -62,6 +62,7 @@ > * > */ > struct omap_device { > + u32 magic; > struct platform_device pdev; > struct omap_hwmod **hwmods; > struct omap_device_pm_latency *pm_lats; > @@ -81,6 +82,7 @@ int omap_device_shutdown(struct platform_device *pdev); > > /* Core code interface */ > > +bool omap_device_is_valid(struct omap_device *od); > int omap_device_count_resources(struct omap_device *od); > int omap_device_fill_resources(struct omap_device *od, struct resource *res); > > diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c > index d8c75c8..187a08f 100644 > --- a/arch/arm/plat-omap/omap_device.c > +++ b/arch/arm/plat-omap/omap_device.c > @@ -100,6 +100,8 @@ > # error Unknown OMAP device > #endif > > +#define OMAP_DEVICE_MAGIC 0xf00dcafe > + > /* Private functions */ > > /** > @@ -413,6 +415,8 @@ struct omap_device *omap_device_build_ss(const char *pdev_name, int pdev_id, > od->pm_lats = pm_lats; > od->pm_lats_cnt = pm_lats_cnt; > > + od->magic = OMAP_DEVICE_MAGIC; > + > ret = omap_device_register(od); > if (ret) > goto odbs_exit4; > @@ -599,6 +603,18 @@ int omap_device_align_pm_lat(struct platform_device *pdev, > } > > /** > + * omap_device_is_valid() I think kerneldoc will want a short description after the function name. Can plug this in here during the merge process if you want... > + * @od: struct omap_device * > + * > + * Return whether struct omap_device pointer @od points to a valid > + * omap_device. > + */ > +bool omap_device_is_valid(struct omap_device *od) > +{ > + return (od && od->magic == OMAP_DEVICE_MAGIC); > +} > + > +/** > * omap_device_get_pwrdm - return the powerdomain * associated with @od > * @od: struct omap_device * > * > -- > 1.6.6 > > -- > 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 > - Paul ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] OMAP: omap_device: add omap_device_is_valid() 2010-01-28 0:30 ` [PATCH 1/2] OMAP: omap_device: add omap_device_is_valid() Paul Walmsley @ 2010-01-28 17:19 ` Kevin Hilman 2010-02-21 8:35 ` Paul Walmsley 0 siblings, 1 reply; 6+ messages in thread From: Kevin Hilman @ 2010-01-28 17:19 UTC (permalink / raw) To: Paul Walmsley; +Cc: linux-omap Paul Walmsley <paul@pwsan.com> writes: > Hi Kevin, > > this looks fine. Two quick comments: > > On Tue, 26 Jan 2010, Kevin Hilman wrote: > >> The omap_device struct contains a 'struct platform_device'. Normally, >> converting a platform_device pointer to an omap_device pointer >> consists of simply doing a container_of(), as is done currently by the >> to_omap_device() macro. >> >> However, if this is attempted when using platform_device that has not >> been created as part of the omap_device creation, the container_of() >> will point to a memory location before the platform_device pointer >> which will contain random data. >> >> Therefore, we need a way to detect valid omap_device pointers. This >> patch solves this by using the simple magic number approach. > > Sounds okay; just FYI at some point in the future, it would be good for > one of us to take a whack at converting omap_device to contain > platform_device, the same way that platform_device contains struct device, > etc.. Hmm, not sure I follow... struct omap_device already contains a struct platform_device, which in turn contains a struct device, so to get an omap_device from a struct device, there are two container of operaions: to_platform_device() and to_omap_device() from omap_device.h: struct omap_device { u32 magic; struct platform_device pdev; [...] } #define to_omap_device(x) container_of((x), struct omap_device, pdev) from platform_device.h: struct platform_device { const char * name; int id; struct device dev; [...] } #define to_platform_device(x) container_of((x), struct platform_device, dev) >> >> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com> >> --- >> Changes from original version: >> - set magic value before omap_device_register() >> >> arch/arm/plat-omap/include/plat/omap_device.h | 2 ++ >> arch/arm/plat-omap/omap_device.c | 16 ++++++++++++++++ >> 2 files changed, 18 insertions(+), 0 deletions(-) >> >> diff --git a/arch/arm/plat-omap/include/plat/omap_device.h b/arch/arm/plat-omap/include/plat/omap_device.h >> index 76d4917..4677ff7 100644 >> --- a/arch/arm/plat-omap/include/plat/omap_device.h >> +++ b/arch/arm/plat-omap/include/plat/omap_device.h >> @@ -62,6 +62,7 @@ >> * >> */ >> struct omap_device { >> + u32 magic; >> struct platform_device pdev; >> struct omap_hwmod **hwmods; >> struct omap_device_pm_latency *pm_lats; >> @@ -81,6 +82,7 @@ int omap_device_shutdown(struct platform_device *pdev); >> >> /* Core code interface */ >> >> +bool omap_device_is_valid(struct omap_device *od); >> int omap_device_count_resources(struct omap_device *od); >> int omap_device_fill_resources(struct omap_device *od, struct resource *res); >> >> diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c >> index d8c75c8..187a08f 100644 >> --- a/arch/arm/plat-omap/omap_device.c >> +++ b/arch/arm/plat-omap/omap_device.c >> @@ -100,6 +100,8 @@ >> # error Unknown OMAP device >> #endif >> >> +#define OMAP_DEVICE_MAGIC 0xf00dcafe >> + >> /* Private functions */ >> >> /** >> @@ -413,6 +415,8 @@ struct omap_device *omap_device_build_ss(const char *pdev_name, int pdev_id, >> od->pm_lats = pm_lats; >> od->pm_lats_cnt = pm_lats_cnt; >> >> + od->magic = OMAP_DEVICE_MAGIC; >> + >> ret = omap_device_register(od); >> if (ret) >> goto odbs_exit4; >> @@ -599,6 +603,18 @@ int omap_device_align_pm_lat(struct platform_device *pdev, >> } >> >> /** >> + * omap_device_is_valid() > > I think kerneldoc will want a short description after the function name. > Can plug this in here during the merge process if you want... done, updated version below. Thanks for comments, Kevin >From cd8cfaf6663da00645eca7bcf8f2faf1d5b96cf2 Mon Sep 17 00:00:00 2001 From: Kevin Hilman <khilman@deeprootsystems.com> Date: Tue, 12 Jan 2010 16:37:47 -0800 Subject: [PATCH 1/2] OMAP: omap_device: add omap_device_is_valid() The omap_device struct contains a 'struct platform_device'. Normally, converting a platform_device pointer to an omap_device pointer consists of simply doing a container_of(), as is done currently by the to_omap_device() macro. However, if this is attempted when using platform_device that has not been created as part of the omap_device creation, the container_of() will point to a memory location before the platform_device pointer which will contain random data. Therefore, we need a way to detect valid omap_device pointers. This patch solves this by using the simple magic number approach. Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com> --- arch/arm/plat-omap/include/plat/omap_device.h | 2 ++ arch/arm/plat-omap/omap_device.c | 16 ++++++++++++++++ 2 files changed, 18 insertions(+), 0 deletions(-) diff --git a/arch/arm/plat-omap/include/plat/omap_device.h b/arch/arm/plat-omap/include/plat/omap_device.h index 76d4917..4677ff7 100644 --- a/arch/arm/plat-omap/include/plat/omap_device.h +++ b/arch/arm/plat-omap/include/plat/omap_device.h @@ -62,6 +62,7 @@ * */ struct omap_device { + u32 magic; struct platform_device pdev; struct omap_hwmod **hwmods; struct omap_device_pm_latency *pm_lats; @@ -81,6 +82,7 @@ int omap_device_shutdown(struct platform_device *pdev); /* Core code interface */ +bool omap_device_is_valid(struct omap_device *od); int omap_device_count_resources(struct omap_device *od); int omap_device_fill_resources(struct omap_device *od, struct resource *res); diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c index d6b593d..ec0bcca 100644 --- a/arch/arm/plat-omap/omap_device.c +++ b/arch/arm/plat-omap/omap_device.c @@ -100,6 +100,8 @@ # error Unknown OMAP device #endif +#define OMAP_DEVICE_MAGIC 0xf00dcafe + /* Private functions */ /** @@ -413,6 +415,8 @@ struct omap_device *omap_device_build_ss(const char *pdev_name, int pdev_id, od->pm_lats = pm_lats; od->pm_lats_cnt = pm_lats_cnt; + od->magic = OMAP_DEVICE_MAGIC; + ret = omap_device_register(od); if (ret) goto odbs_exit4; @@ -599,6 +603,18 @@ int omap_device_align_pm_lat(struct platform_device *pdev, } /** + * omap_device_is_valid - Check if pointer is a valid omap_device + * @od: struct omap_device * + * + * Return whether struct omap_device pointer @od points to a valid + * omap_device. + */ +bool omap_device_is_valid(struct omap_device *od) +{ + return (od && od->magic == OMAP_DEVICE_MAGIC); +} + +/** * omap_device_get_pwrdm - return the powerdomain * associated with @od * @od: struct omap_device * * -- 1.6.6 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] OMAP: omap_device: add omap_device_is_valid() 2010-01-28 17:19 ` Kevin Hilman @ 2010-02-21 8:35 ` Paul Walmsley 0 siblings, 0 replies; 6+ messages in thread From: Paul Walmsley @ 2010-02-21 8:35 UTC (permalink / raw) To: Kevin Hilman; +Cc: linux-omap Hi Kevin, On Thu, 28 Jan 2010, Kevin Hilman wrote: > Hmm, not sure I follow... > > struct omap_device already contains a struct platform_device, which in > turn contains a struct device, so to get an omap_device from a struct > device, there are two container of operaions: to_platform_device() and > to_omap_device() > > > from omap_device.h: > > struct omap_device { > u32 magic; > struct platform_device pdev; > [...] > } > #define to_omap_device(x) container_of((x), struct omap_device, pdev) > > from platform_device.h: > struct platform_device { > const char * name; > int id; > struct device dev; > [...] > } > #define to_platform_device(x) container_of((x), struct platform_device, dev) Sorry - my E-mail was too hasty and unclear. What I had in mind was the lifecycle aspect of omap_device - i.e., ensuring that omap_devices are automatically freed by the device core, similar to the way that platform_device_alloc() does it. - Paul ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-02-21 8:35 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-01-26 19:00 [PATCH 1/2] OMAP: omap_device: add omap_device_is_valid() Kevin Hilman 2010-01-26 19:00 ` [PATCH 2/2] OMAP: omap_device: when 'called from invalid state', print state Kevin Hilman 2010-02-11 7:17 ` Paul Walmsley 2010-01-28 0:30 ` [PATCH 1/2] OMAP: omap_device: add omap_device_is_valid() Paul Walmsley 2010-01-28 17:19 ` Kevin Hilman 2010-02-21 8:35 ` Paul Walmsley
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox