Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1616189 > unrolled thread

[PATCH 1/2] [media] rc-core: Add inlined stubs for core rc_* functions

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] [media] rc-core: Add inlined stubs for core rc_* functions Lee Jones <lee.jones@linaro.org> - 2017-04-04 18:20 +0200
    [PATCH 2/2] [media] cec: Handle RC capability more elegantly Lee Jones <lee.jones@linaro.org> - 2017-04-04 18:20 +0200
      Re: [PATCH 2/2] [media] cec: Handle RC capability more elegantly Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-04-04 18:40 +0200
        Re: [PATCH 2/2] [media] cec: Handle RC capability more elegantly Lee Jones <lee.jones@linaro.org> - 2017-04-05 11:20 +0200

#1616189 — [PATCH 1/2] [media] rc-core: Add inlined stubs for core rc_* functions

Currently users have to use all sorts of ugly #ifery within
their drivers in order to avoid linking issues at build time.
This patch allows users to safely call these functions when
!CONFIG_RC_CORE and make decisions based on the return value
instead.  This is a much more common and clean way of doing
things within the Linux kernel.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 include/media/rc-core.h | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/include/media/rc-core.h b/include/media/rc-core.h
index 73ddd721..1f2043d 100644
--- a/include/media/rc-core.h
+++ b/include/media/rc-core.h
@@ -209,7 +209,14 @@ struct rc_dev {
  * @rc_driver_type: specifies the type of the RC output to be allocated
  * returns a pointer to struct rc_dev.
  */
+#ifdef CONFIG_RC_CORE
 struct rc_dev *rc_allocate_device(enum rc_driver_type);
+#else
+static inline struct rc_dev *rc_allocate_device(int unused)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+#endif
 
 /**
  * devm_rc_allocate_device - Managed RC device allocation
@@ -218,21 +225,42 @@ struct rc_dev *rc_allocate_device(enum rc_driver_type);
  * @rc_driver_type: specifies the type of the RC output to be allocated
  * returns a pointer to struct rc_dev.
  */
+#ifdef CONFIG_RC_CORE
 struct rc_dev *devm_rc_allocate_device(struct device *dev, enum rc_driver_type);
+#else
+static inline struct rc_dev *devm_rc_allocate_device(struct device *dev, int unused)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+#endif
 
 /**
  * rc_free_device - Frees a RC device
  *
  * @dev: pointer to struct rc_dev.
  */
+#ifdef CONFIG_RC_CORE
 void rc_free_device(struct rc_dev *dev);
+#else
+static inline void rc_free_device(struct rc_dev *dev)
+{
+	return;
+}
+#endif
 
 /**
  * rc_register_device - Registers a RC device
  *
  * @dev: pointer to struct rc_dev.
  */
+#ifdef CONFIG_RC_CORE
 int rc_register_device(struct rc_dev *dev);
+#else
+static inline int rc_register_device(struct rc_dev *dev)
+{
+	return -EOPNOTSUPP;
+}
+#endif
 
 /**
  * devm_rc_register_device - Manageded registering of a RC device
@@ -240,14 +268,28 @@ int rc_register_device(struct rc_dev *dev);
  * @parent: pointer to struct device.
  * @dev: pointer to struct rc_dev.
  */
+#ifdef CONFIG_RC_CORE
 int devm_rc_register_device(struct device *parent, struct rc_dev *dev);
+#else
+static inline int devm_rc_register_device(struct device *parent, struct rc_dev *dev)
+{
+	return -EOPNOTSUPP;
+}
+#endif
 
 /**
  * rc_unregister_device - Unregisters a RC device
  *
  * @dev: pointer to struct rc_dev.
  */
+#ifdef CONFIG_RC_CORE
 void rc_unregister_device(struct rc_dev *dev);
+#else
+static inline void rc_unregister_device(struct rc_dev *dev)
+{
+	return;
+}
+#endif
 
 /**
  * rc_open - Opens a RC device
-- 
2.9.3

[toc] | [next] | [standalone]

#1616190 — [PATCH 2/2] [media] cec: Handle RC capability more elegantly

If a user specifies the use of RC as a capability, they should
really be enabling RC Core code.  If they do not we WARN() them
of this and disable the capability for them.

Once we know RC Core code has not been enabled, we can update
the user's capabilities and use them as a term of reference for
other RC-only calls.  This is preferable to having ugly #ifery
scattered throughout C code.

Most of the functions are actually safe to call, since they
sensibly check for a NULL RC pointer before they attempt to
deference it.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/media/cec/cec-core.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
index cfe414a..51be8d6 100644
--- a/drivers/media/cec/cec-core.c
+++ b/drivers/media/cec/cec-core.c
@@ -208,9 +208,13 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
 		return ERR_PTR(-EINVAL);
 	if (WARN_ON(!available_las || available_las > CEC_MAX_LOG_ADDRS))
 		return ERR_PTR(-EINVAL);
+	if (WARN_ON(caps & CEC_CAP_RC && !IS_REACHABLE(CONFIG_RC_CORE)))
+		caps &= ~CEC_CAP_RC;
+
 	adap = kzalloc(sizeof(*adap), GFP_KERNEL);
 	if (!adap)
 		return ERR_PTR(-ENOMEM);
+
 	strlcpy(adap->name, name, sizeof(adap->name));
 	adap->phys_addr = CEC_PHYS_ADDR_INVALID;
 	adap->log_addrs.cec_version = CEC_OP_CEC_VERSION_2_0;
@@ -237,7 +241,6 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
 	if (!(caps & CEC_CAP_RC))
 		return adap;
 
-#if IS_REACHABLE(CONFIG_RC_CORE)
 	/* Prepare the RC input device */
 	adap->rc = rc_allocate_device(RC_DRIVER_SCANCODE);
 	if (!adap->rc) {
@@ -264,9 +267,7 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
 	adap->rc->priv = adap;
 	adap->rc->map_name = RC_MAP_CEC;
 	adap->rc->timeout = MS_TO_NS(100);
-#else
-	adap->capabilities &= ~CEC_CAP_RC;
-#endif
+
 	return adap;
 }
 EXPORT_SYMBOL_GPL(cec_allocate_adapter);
@@ -285,7 +286,6 @@ int cec_register_adapter(struct cec_adapter *adap,
 	adap->owner = parent->driver->owner;
 	adap->devnode.dev.parent = parent;
 
-#if IS_REACHABLE(CONFIG_RC_CORE)
 	if (adap->capabilities & CEC_CAP_RC) {
 		adap->rc->dev.parent = parent;
 		res = rc_register_device(adap->rc);
@@ -298,15 +298,13 @@ int cec_register_adapter(struct cec_adapter *adap,
 			return res;
 		}
 	}
-#endif
 
 	res = cec_devnode_register(&adap->devnode, adap->owner);
 	if (res) {
-#if IS_REACHABLE(CONFIG_RC_CORE)
 		/* Note: rc_unregister also calls rc_free */
 		rc_unregister_device(adap->rc);
 		adap->rc = NULL;
-#endif
+
 		return res;
 	}
 
@@ -337,11 +335,10 @@ void cec_unregister_adapter(struct cec_adapter *adap)
 	if (IS_ERR_OR_NULL(adap))
 		return;
 
-#if IS_REACHABLE(CONFIG_RC_CORE)
 	/* Note: rc_unregister also calls rc_free */
 	rc_unregister_device(adap->rc);
 	adap->rc = NULL;
-#endif
+
 	debugfs_remove_recursive(adap->cec_dir);
 	cec_devnode_unregister(&adap->devnode);
 }
@@ -357,9 +354,7 @@ void cec_delete_adapter(struct cec_adapter *adap)
 	kthread_stop(adap->kthread);
 	if (adap->kthread_config)
 		kthread_stop(adap->kthread_config);
-#if IS_REACHABLE(CONFIG_RC_CORE)
 	rc_free_device(adap->rc);
-#endif
 	kfree(adap);
 }
 EXPORT_SYMBOL_GPL(cec_delete_adapter);
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1616201 — Re: [PATCH 2/2] [media] cec: Handle RC capability more elegantly

On Tue, Apr 04, 2017 at 05:10:05PM +0100, Lee Jones wrote:
> @@ -237,7 +241,6 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
>  	if (!(caps & CEC_CAP_RC))
>  		return adap;
>  
> -#if IS_REACHABLE(CONFIG_RC_CORE)
>  	/* Prepare the RC input device */
>  	adap->rc = rc_allocate_device(RC_DRIVER_SCANCODE);
>  	if (!adap->rc) {

The above, coupled with patch 1:

+#ifdef CONFIG_RC_CORE
 struct rc_dev *rc_allocate_device(enum rc_driver_type);
+#else
+static inline struct rc_dev *rc_allocate_device(int unused)
+{
+       return ERR_PTR(-EOPNOTSUPP);
+}
+#endif

is really not nice.  You claim that this is how stuff is done elsewhere
in the kernel, but no, it isn't.  Look at debugfs.

You're right that debugfs returns an error pointer when it's not
configured.  However, the debugfs dentry is only ever passed back into
the debugfs APIs, it is never dereferenced by the caller.

That is not the case here.  The effect if your change is that the
following dereferences will oops the kernel.  This is unacceptable for
a feature that is deconfigured.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1616751 — Re: [PATCH 2/2] [media] cec: Handle RC capability more elegantly

On Tue, 04 Apr 2017, Russell King - ARM Linux wrote:

> On Tue, Apr 04, 2017 at 05:10:05PM +0100, Lee Jones wrote:
> > @@ -237,7 +241,6 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
> >  	if (!(caps & CEC_CAP_RC))
> >  		return adap;
> >  
> > -#if IS_REACHABLE(CONFIG_RC_CORE)
> >  	/* Prepare the RC input device */
> >  	adap->rc = rc_allocate_device(RC_DRIVER_SCANCODE);
> >  	if (!adap->rc) {
> 
> The above, coupled with patch 1:
> 
> +#ifdef CONFIG_RC_CORE
>  struct rc_dev *rc_allocate_device(enum rc_driver_type);
> +#else
> +static inline struct rc_dev *rc_allocate_device(int unused)
> +{
> +       return ERR_PTR(-EOPNOTSUPP);
> +}
> +#endif
> 
> is really not nice.  You claim that this is how stuff is done elsewhere
> in the kernel, but no, it isn't.  Look at debugfs.

I'm afraid you have entered half way through a conversation, which as
caused a misunderstanding.  Apologies for not being clear in my commit
log.

When I say "this is how it's done else where", that is in reference to
offering stubs which can be tucked away in a header file, rather than
being forced to #if out any functionality which is not available.

> You're right that debugfs returns an error pointer when it's not
> configured.  However, the debugfs dentry is only ever passed back into
> the debugfs APIs, it is never dereferenced by the caller.

Continued on from my last point:

What I do not mean is that this solution is perfect and does not
require a review.  You are completely correct in what you say, the
return values I have used are not suitable.  I failed to see how
callers were treating the return value.  I will carry out due
diligence on that point and re-submit as per your request.

> That is not the case here.  The effect if your change is that the
> following dereferences will oops the kernel.  This is unacceptable for
> a feature that is deconfigured.

Fair point Russell.  Thanks, will fix.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web