diff mbox series

gpiolib: use __counted_by() for GPIO descriptors

Message ID 20231219125706.23284-1-brgl@bgdev.pl
State New
Headers show
Series gpiolib: use __counted_by() for GPIO descriptors | expand

Commit Message

Bartosz Golaszewski Dec. 19, 2023, 12:57 p.m. UTC
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Pull the array of GPIO descriptors into struct gpio_device as a flexible
array and use __counted_by() to control its size.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c | 14 ++----
 drivers/gpio/gpiolib.h | 98 +++++++++++++++++++++---------------------
 2 files changed, 52 insertions(+), 60 deletions(-)

Comments

Kent Gibson Dec. 19, 2023, 3:06 p.m. UTC | #1
On Tue, Dec 19, 2023 at 01:57:06PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Pull the array of GPIO descriptors into struct gpio_device as a flexible
> array and use __counted_by() to control its size.
>

Looks ok to me, though that might be because it looks a lot like cdev's
struct linereq.

Cheers,
Kent.
Andy Shevchenko Dec. 19, 2023, 3:11 p.m. UTC | #2
On Tue, Dec 19, 2023 at 01:57:06PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Pull the array of GPIO descriptors into struct gpio_device as a flexible
> array and use __counted_by() to control its size.

How big is the struct gpio_device? Unifying like this might provoke subtle
errors on very fragmented memory, where k*alloc() might not find enough free
space. Note, k*alloc() guarantees finding only memory for a single page.
With PAGE_SIZE = 4k, this might be an issue.

I would suggest, if nothing prevents us from switching, to use kvmalloc().
Bartosz Golaszewski Dec. 20, 2023, 8:16 a.m. UTC | #3
On Tue, Dec 19, 2023 at 4:12 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Dec 19, 2023 at 01:57:06PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Pull the array of GPIO descriptors into struct gpio_device as a flexible
> > array and use __counted_by() to control its size.
>
> How big is the struct gpio_device? Unifying like this might provoke subtle
> errors on very fragmented memory, where k*alloc() might not find enough free
> space. Note, k*alloc() guarantees finding only memory for a single page.
> With PAGE_SIZE = 4k, this might be an issue.
>
> I would suggest, if nothing prevents us from switching, to use kvmalloc().
>

That's a good point but there's another thing. We need to call
gpiochip_get_ngpios() before the allocation. I need to revisit this
one.

Bart

> --
> With Best Regards,
> Andy Shevchenko
>
>
Bartosz Golaszewski Dec. 20, 2023, 12:35 p.m. UTC | #4
On Wed, Dec 20, 2023 at 9:16 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Tue, Dec 19, 2023 at 4:12 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Tue, Dec 19, 2023 at 01:57:06PM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > Pull the array of GPIO descriptors into struct gpio_device as a flexible
> > > array and use __counted_by() to control its size.
> >
> > How big is the struct gpio_device? Unifying like this might provoke subtle
> > errors on very fragmented memory, where k*alloc() might not find enough free
> > space. Note, k*alloc() guarantees finding only memory for a single page.
> > With PAGE_SIZE = 4k, this might be an issue.
> >
> > I would suggest, if nothing prevents us from switching, to use kvmalloc().
> >
>
> That's a good point but there's another thing. We need to call
> gpiochip_get_ngpios() before the allocation. I need to revisit this
> one.
>

It's trickier than I thought. We need the struct device to exist (have
its software node assigned) before we check the property but we cannot
allocate gdev (embedding struct device) + descs before we have read
it. Eh, maybe it's not worth it and let's keep the two allocations
separate.

Bart

> Bart
>
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index c9ca809b55de..7ebeb5bb7918 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -18,6 +18,7 @@ 
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
+#include <linux/overflow.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
@@ -669,7 +670,6 @@  static void gpiodev_release(struct device *dev)
 
 	ida_free(&gpio_ida, gdev->id);
 	kfree_const(gdev->label);
-	kfree(gdev->descs);
 	kfree(gdev);
 }
 
@@ -831,7 +831,7 @@  int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 	 * First: allocate and populate the internal stat container, and
 	 * set up the struct device.
 	 */
-	gdev = kzalloc(sizeof(*gdev), GFP_KERNEL);
+	gdev = kzalloc(struct_size(gdev, descs, gc->ngpio), GFP_KERNEL);
 	if (!gdev)
 		return -ENOMEM;
 	gdev->dev.bus = &gpio_bus_type;
@@ -873,16 +873,10 @@  int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 	if (ret)
 		goto err_free_dev_name;
 
-	gdev->descs = kcalloc(gc->ngpio, sizeof(*gdev->descs), GFP_KERNEL);
-	if (!gdev->descs) {
-		ret = -ENOMEM;
-		goto err_free_dev_name;
-	}
-
 	gdev->label = kstrdup_const(gc->label ?: "unknown", GFP_KERNEL);
 	if (!gdev->label) {
 		ret = -ENOMEM;
-		goto err_free_descs;
+		goto err_free_dev_name;
 	}
 
 	gdev->ngpio = gc->ngpio;
@@ -1021,8 +1015,6 @@  int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 		list_del(&gdev->list);
 err_free_label:
 	kfree_const(gdev->label);
-err_free_descs:
-	kfree(gdev->descs);
 err_free_dev_name:
 	kfree(dev_name(&gdev->dev));
 err_free_ida:
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 0ce7451a6b24..d65ee94d88f8 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -21,6 +21,53 @@ 
 
 #define GPIOCHIP_NAME	"gpiochip"
 
+/**
+ * struct gpio_desc - Opaque descriptor for a GPIO
+ *
+ * @gdev:		Pointer to the parent GPIO device
+ * @flags:		Binary descriptor flags
+ * @label:		Name of the consumer
+ * @name:		Line name
+ * @hog:		Pointer to the device node that hogs this line (if any)
+ *
+ * These are obtained using gpiod_get() and are preferable to the old
+ * integer-based handles.
+ *
+ * Contrary to integers, a pointer to a &struct gpio_desc is guaranteed to be
+ * valid until the GPIO is released.
+ */
+struct gpio_desc {
+	struct gpio_device	*gdev;
+	unsigned long		flags;
+/* flag symbols are bit numbers */
+#define FLAG_REQUESTED	0
+#define FLAG_IS_OUT	1
+#define FLAG_EXPORT	2	/* protected by sysfs_lock */
+#define FLAG_SYSFS	3	/* exported via /sys/class/gpio/control */
+#define FLAG_ACTIVE_LOW	6	/* value has active low */
+#define FLAG_OPEN_DRAIN	7	/* Gpio is open drain type */
+#define FLAG_OPEN_SOURCE 8	/* Gpio is open source type */
+#define FLAG_USED_AS_IRQ 9	/* GPIO is connected to an IRQ */
+#define FLAG_IRQ_IS_ENABLED 10	/* GPIO is connected to an enabled IRQ */
+#define FLAG_IS_HOGGED	11	/* GPIO is hogged */
+#define FLAG_TRANSITORY 12	/* GPIO may lose value in sleep or reset */
+#define FLAG_PULL_UP    13	/* GPIO has pull up enabled */
+#define FLAG_PULL_DOWN  14	/* GPIO has pull down enabled */
+#define FLAG_BIAS_DISABLE    15	/* GPIO has pull disabled */
+#define FLAG_EDGE_RISING     16	/* GPIO CDEV detects rising edge events */
+#define FLAG_EDGE_FALLING    17	/* GPIO CDEV detects falling edge events */
+#define FLAG_EVENT_CLOCK_REALTIME	18 /* GPIO CDEV reports REALTIME timestamps in events */
+#define FLAG_EVENT_CLOCK_HTE		19 /* GPIO CDEV reports hardware timestamps in events */
+
+	/* Connection label */
+	const char		*label;
+	/* Name of the GPIO */
+	const char		*name;
+#ifdef CONFIG_OF_DYNAMIC
+	struct device_node	*hog;
+#endif
+};
+
 /**
  * struct gpio_device - internal state container for GPIO devices
  * @dev: the GPIO device struct
@@ -31,7 +78,6 @@ 
  * @owner: helps prevent removal of modules exporting active GPIOs
  * @chip: pointer to the corresponding gpiochip, holding static
  * data for this device
- * @descs: array of ngpio descriptors.
  * @ngpio: the number of GPIO lines on this GPIO device, equal to the size
  * of the @descs array.
  * @base: GPIO base in the DEPRECATED global Linux GPIO numberspace, assigned
@@ -48,6 +94,7 @@ 
  *       user-space operations when the device gets unregistered during
  *       a hot-unplug event
  * @pin_ranges: range of pins served by the GPIO driver
+ * @descs: array of ngpio descriptors.
  *
  * This state container holds most of the runtime variable data
  * for a GPIO device and can hold references and live on after the
@@ -61,7 +108,6 @@  struct gpio_device {
 	struct device		*mockdev;
 	struct module		*owner;
 	struct gpio_chip	*chip;
-	struct gpio_desc	*descs;
 	int			base;
 	u16			ngpio;
 	const char		*label;
@@ -80,6 +126,7 @@  struct gpio_device {
 	 */
 	struct list_head pin_ranges;
 #endif
+	struct gpio_desc	descs[] __counted_by(ngpio);
 };
 
 static inline struct gpio_device *to_gpio_device(struct device *dev)
@@ -141,53 +188,6 @@  extern struct mutex gpio_devices_lock;
 
 void gpiod_line_state_notify(struct gpio_desc *desc, unsigned long action);
 
-/**
- * struct gpio_desc - Opaque descriptor for a GPIO
- *
- * @gdev:		Pointer to the parent GPIO device
- * @flags:		Binary descriptor flags
- * @label:		Name of the consumer
- * @name:		Line name
- * @hog:		Pointer to the device node that hogs this line (if any)
- *
- * These are obtained using gpiod_get() and are preferable to the old
- * integer-based handles.
- *
- * Contrary to integers, a pointer to a &struct gpio_desc is guaranteed to be
- * valid until the GPIO is released.
- */
-struct gpio_desc {
-	struct gpio_device	*gdev;
-	unsigned long		flags;
-/* flag symbols are bit numbers */
-#define FLAG_REQUESTED	0
-#define FLAG_IS_OUT	1
-#define FLAG_EXPORT	2	/* protected by sysfs_lock */
-#define FLAG_SYSFS	3	/* exported via /sys/class/gpio/control */
-#define FLAG_ACTIVE_LOW	6	/* value has active low */
-#define FLAG_OPEN_DRAIN	7	/* Gpio is open drain type */
-#define FLAG_OPEN_SOURCE 8	/* Gpio is open source type */
-#define FLAG_USED_AS_IRQ 9	/* GPIO is connected to an IRQ */
-#define FLAG_IRQ_IS_ENABLED 10	/* GPIO is connected to an enabled IRQ */
-#define FLAG_IS_HOGGED	11	/* GPIO is hogged */
-#define FLAG_TRANSITORY 12	/* GPIO may lose value in sleep or reset */
-#define FLAG_PULL_UP    13	/* GPIO has pull up enabled */
-#define FLAG_PULL_DOWN  14	/* GPIO has pull down enabled */
-#define FLAG_BIAS_DISABLE    15	/* GPIO has pull disabled */
-#define FLAG_EDGE_RISING     16	/* GPIO CDEV detects rising edge events */
-#define FLAG_EDGE_FALLING    17	/* GPIO CDEV detects falling edge events */
-#define FLAG_EVENT_CLOCK_REALTIME	18 /* GPIO CDEV reports REALTIME timestamps in events */
-#define FLAG_EVENT_CLOCK_HTE		19 /* GPIO CDEV reports hardware timestamps in events */
-
-	/* Connection label */
-	const char		*label;
-	/* Name of the GPIO */
-	const char		*name;
-#ifdef CONFIG_OF_DYNAMIC
-	struct device_node	*hog;
-#endif
-};
-
 #define gpiod_not_found(desc)		(IS_ERR(desc) && PTR_ERR(desc) == -ENOENT)
 
 int gpiod_request(struct gpio_desc *desc, const char *label);