On 18/10/2019 22:38, Simon Glass wrote:
Hi Jean-Jacques,
On Tue, 1 Oct 2019 at 05:51, Jean-Jacques Hiblot <jjhib...@ti.com> wrote:
Prepare the way for a managed GPIO API by handling NULL pointers without
crashing nor failing.
VALIDATE_DESC() and validate_desc() come straight from Linux.
Signed-off-by: Jean-Jacques Hiblot <jjhib...@ti.com>
---
drivers/gpio/gpio-uclass.c | 66 ++++++++++++++++++++++++++++++++------
include/asm-generic/gpio.h | 2 +-
2 files changed, 57 insertions(+), 11 deletions(-)
diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
index 01cfa2f788..63c10f438b 100644
--- a/drivers/gpio/gpio-uclass.c
+++ b/drivers/gpio/gpio-uclass.c
@@ -18,6 +18,33 @@
DECLARE_GLOBAL_DATA_PTR;
+/*
+ * This descriptor validation needs to be inserted verbatim into each
+ * function taking a descriptor, so we need to use a preprocessor
+ * macro to avoid endless duplication. If the desc is NULL it is an
+ * optional GPIO and calls should just bail out.
+ */
+static int validate_desc(const struct gpio_desc *desc, const char *func)
+{
+ if (!desc)
+ return 0;
+ if (IS_ERR(desc)) {
+ pr_warn("%s: invalid GPIO (errorpointer)\n", func);
+ return PTR_ERR(desc);
+ }
+ if (!desc->dev) {
+ pr_warn("%s: invalid GPIO (no device)\n", func);
+ return -EINVAL;
+ }
+ return 1;
+}
+
+#define VALIDATE_DESC(desc) do { \
+ int __valid = validate_desc(desc, __func__); \
+ if (__valid <= 0) \
+ return __valid; \
+ } while (0)
This adds to code size so should be behind a CONFIG I think.
I'm not sure we really want to keep this out. Most of the added code
size, will be about the error messages. I would rather remove them (or
use a debug() or warn_non_spl()
+
/**
* gpio_to_device() - Convert global GPIO number to device, number
*
@@ -269,11 +296,14 @@ int gpio_hog_lookup_name(const char *name, struct
gpio_desc **desc)
int dm_gpio_request(struct gpio_desc *desc, const char *label)
{
- struct udevice *dev = desc->dev;
+ struct udevice *dev;
struct gpio_dev_priv *uc_priv;
char *str;
int ret;
+ VALIDATE_DESC(desc);
+ dev = desc->dev;
+
uc_priv = dev_get_uclass_priv(dev);
if (uc_priv->name[desc->offset])
return -EBUSY;
@@ -400,6 +430,8 @@ static int check_reserved(const struct gpio_desc *desc,
const char *func)
{
struct gpio_dev_priv *uc_priv;
+ VALIDATE_DESC(desc);
+
if (!dm_gpio_is_valid(desc))
return -ENOENT;
@@ -468,6 +500,8 @@ int dm_gpio_get_value(const struct gpio_desc *desc)
int value;
int ret;
+ VALIDATE_DESC(desc);
+
ret = check_reserved(desc, "get_value");
if (ret)
return ret;
@@ -481,6 +515,8 @@ int dm_gpio_set_value(const struct gpio_desc *desc, int
value)
{
int ret;
+ VALIDATE_DESC(desc);
+
ret = check_reserved(desc, "set_value");
if (ret)
return ret;
@@ -493,9 +529,12 @@ int dm_gpio_set_value(const struct gpio_desc *desc, int
value)
int dm_gpio_get_open_drain(struct gpio_desc *desc)
{
- struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
+ struct dm_gpio_ops *ops;
int ret;
+ VALIDATE_DESC(desc);
+ ops = gpio_get_ops(desc->dev);
+
ret = check_reserved(desc, "get_open_drain");
if (ret)
return ret;
@@ -508,9 +547,12 @@ int dm_gpio_get_open_drain(struct gpio_desc *desc)
int dm_gpio_set_open_drain(struct gpio_desc *desc, int value)
{
- struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
+ struct dm_gpio_ops *ops;
int ret;
+ VALIDATE_DESC(desc);
+ ops = gpio_get_ops(desc->dev);
+
ret = check_reserved(desc, "set_open_drain");
if (ret)
return ret;
@@ -525,10 +567,14 @@ int dm_gpio_set_open_drain(struct gpio_desc *desc, int
value)
int dm_gpio_set_dir_flags(struct gpio_desc *desc, ulong flags)
{
- struct udevice *dev = desc->dev;
- struct dm_gpio_ops *ops = gpio_get_ops(dev);
+ struct udevice *dev;
+ struct dm_gpio_ops *ops;
int ret;
+ VALIDATE_DESC(desc);
+ dev = desc->dev;
+ ops = gpio_get_ops(dev);
+
ret = check_reserved(desc, "set_dir");
if (ret)
return ret;
@@ -570,7 +616,6 @@ int dm_gpio_set_dir(struct gpio_desc *desc)
int gpio_get_value(unsigned gpio)
{
int ret;
-
unrelated change?
struct gpio_desc desc;
ret = gpio_to_device(gpio, &desc);
@@ -933,6 +978,8 @@ int gpio_get_list_count(struct udevice *dev, const char
*list_name)
int dm_gpio_free(struct udevice *dev, struct gpio_desc *desc)
{
+ VALIDATE_DESC(desc);
+
/* For now, we don't do any checking of dev */
return _dm_gpio_free(desc->dev, desc->offset);
}
@@ -981,12 +1028,11 @@ static int gpio_renumber(struct udevice *removed_dev)
int gpio_get_number(const struct gpio_desc *desc)
{
- struct udevice *dev = desc->dev;
struct gpio_dev_priv *uc_priv;
- if (!dev)
- return -1;
- uc_priv = dev->uclass_priv;
+ VALIDATE_DESC(desc);
I think this is pretty opaque. How about writing the code out in full,
with a helper function to do the check. The helper function can return
the error code perhaps.
ret = validate_desc(desc);
if (ret)
return log_msg_ret("gpio_free", ret);
The helper is there already. I could remove macro usage, or maybe rename
the macro as VALIDATE_DESC_OR_EXIT().
JJ
Regards,
Simon
+
+ uc_priv = dev_get_uclass_priv(desc->dev);
return uc_priv->gpio_base + desc->offset;
}
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index d6cf18744f..ba669b65a9 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -139,7 +139,7 @@ struct gpio_desc {
*/
static inline bool dm_gpio_is_valid(const struct gpio_desc *desc)
{
- return desc->dev != NULL;
+ return desc && desc->dev;
}
/**
--
2.17.1
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
