From: Bartosz Golaszewski <[email protected]>
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 <[email protected]>
---
drivers/gpio/gpiolib.c | 14 ++----
drivers/gpio/gpiolib.h | 98 +++++++++++++++++++++---------------------
2 files changed, 52 insertions(+), 60 deletions(-)
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);
--
2.40.1
On Tue, Dec 19, 2023 at 01:57:06PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> 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.
On Tue, Dec 19, 2023 at 01:57:06PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> 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().
--
With Best Regards,
Andy Shevchenko
On Tue, Dec 19, 2023 at 4:12 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Tue, Dec 19, 2023 at 01:57:06PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <[email protected]>
> >
> > 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
>
>
On Wed, Dec 20, 2023 at 9:16 AM Bartosz Golaszewski <[email protected]> wrote:
>
> On Tue, Dec 19, 2023 at 4:12 PM Andy Shevchenko
> <[email protected]> wrote:
> >
> > On Tue, Dec 19, 2023 at 01:57:06PM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <[email protected]>
> > >
> > > 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
> >
> >