This series contains some tidying up of gpiolib-cdev following the
recent adoption of guard().
The first couple of patches are minor fixes inspired by recent
submissions and reviews for gpiolib.c.
Patch 1 adds a missing include.
Patch 2 switches allocation of struct linereq from kzalloc() to
kvzalloc() as it can be larger than one page - even more so after the
recent relocation of debounce_period_us.
Patch 3 tidies up the functions that use a guard on the linereq
config_mutex.
Patch 4 tidies up the functions that use a guard on the gpio_device.
The new guard macro definitions probably should be relocated into
gpiolib.h, and I'll do that for v2 if there is a general consensus.
I also note that gpio_ioctl() is NOT covered by a guard to inhibit
removal of the gpio_device. This looks like a bug to me, unless there
is some higher level locking at play that I am unaware of?
Cheers,
Kent.
Kent Gibson (4):
gpiolib: cdev: include overflow.h
gpiolib: cdev: allocate linereq using kvzalloc()
gpiolib: cdev: replace locking wrappers for config_mutex with guards
gpiolib: cdev: replace locking wrappers for gpio_device with guards
drivers/gpio/gpiolib-cdev.c | 263 +++++++++++-------------------------
1 file changed, 79 insertions(+), 184 deletions(-)
--
2.39.2
struct_size() is used to calculate struct linereq size, so explicitly
include overflow.h.
Signed-off-by: Kent Gibson <[email protected]>
---
drivers/gpio/gpiolib-cdev.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 744734405912..44d864f63130 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -20,6 +20,7 @@
#include <linux/kfifo.h>
#include <linux/module.h>
#include <linux/mutex.h>
+#include <linux/overflow.h>
#include <linux/pinctrl/consumer.h>
#include <linux/poll.h>
#include <linux/rbtree.h>
--
2.39.2
The size of struct linereq may exceed a page, so allocate space for
it using kvzalloc() instead of kzalloc().
Signed-off-by: Kent Gibson <[email protected]>
---
drivers/gpio/gpiolib-cdev.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 44d864f63130..6fec793f5513 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1723,7 +1723,7 @@ static void linereq_free(struct linereq *lr)
kfifo_free(&lr->events);
kfree(lr->label);
gpio_device_put(lr->gdev);
- kfree(lr);
+ kvfree(lr);
}
static int linereq_release(struct inode *inode, struct file *file)
@@ -1788,7 +1788,7 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
if (ret)
return ret;
- lr = kzalloc(struct_size(lr, lines, ulr.num_lines), GFP_KERNEL);
+ lr = kvzalloc(struct_size(lr, lines, ulr.num_lines), GFP_KERNEL);
if (!lr)
return -ENOMEM;
lr->num_lines = ulr.num_lines;
--
2.39.2
After the adoption of guard(), the locking wrappers that hold the
config_mutex for linereq_set_values() and linereq_set_config() no
longer add value, so combine them into the functions they wrap.
Signed-off-by: Kent Gibson <[email protected]>
---
drivers/gpio/gpiolib-cdev.c | 63 ++++++++++++++-----------------------
1 file changed, 24 insertions(+), 39 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 6fec793f5513..5b07578e3bfa 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1454,14 +1454,19 @@ static long linereq_get_values(struct linereq *lr, void __user *ip)
return 0;
}
-static long linereq_set_values_unlocked(struct linereq *lr,
- struct gpio_v2_line_values *lv)
+static long linereq_set_values(struct linereq *lr, void __user *ip)
{
DECLARE_BITMAP(vals, GPIO_V2_LINES_MAX);
+ struct gpio_v2_line_values lv;
struct gpio_desc **descs;
unsigned int i, didx, num_set;
int ret;
+ if (copy_from_user(&lv, ip, sizeof(lv)))
+ return -EFAULT;
+
+ guard(mutex)(&lr->config_mutex);
+
/*
* gpiod_set_array_value_complex() requires compacted desc and val
* arrays, rather than the sparse ones in lv.
@@ -1472,12 +1477,12 @@ static long linereq_set_values_unlocked(struct linereq *lr,
bitmap_zero(vals, GPIO_V2_LINES_MAX);
/* scan requested lines to determine the subset to be set */
for (num_set = 0, i = 0; i < lr->num_lines; i++) {
- if (lv->mask & BIT_ULL(i)) {
+ if (lv.mask & BIT_ULL(i)) {
/* setting inputs is not allowed */
if (!test_bit(FLAG_IS_OUT, &lr->lines[i].desc->flags))
return -EPERM;
/* add to compacted values */
- if (lv->bits & BIT_ULL(i))
+ if (lv.bits & BIT_ULL(i))
__set_bit(num_set, vals);
num_set++;
/* capture desc for the num_set == 1 case */
@@ -1493,7 +1498,7 @@ static long linereq_set_values_unlocked(struct linereq *lr,
if (!descs)
return -ENOMEM;
for (didx = 0, i = 0; i < lr->num_lines; i++) {
- if (lv->mask & BIT_ULL(i)) {
+ if (lv.mask & BIT_ULL(i)) {
descs[didx] = lr->lines[i].desc;
didx++;
}
@@ -1507,31 +1512,28 @@ static long linereq_set_values_unlocked(struct linereq *lr,
return ret;
}
-static long linereq_set_values(struct linereq *lr, void __user *ip)
-{
- struct gpio_v2_line_values lv;
-
- if (copy_from_user(&lv, ip, sizeof(lv)))
- return -EFAULT;
-
- guard(mutex)(&lr->config_mutex);
-
- return linereq_set_values_unlocked(lr, &lv);
-}
-
-static long linereq_set_config_unlocked(struct linereq *lr,
- struct gpio_v2_line_config *lc)
+static long linereq_set_config(struct linereq *lr, void __user *ip)
{
+ struct gpio_v2_line_config lc;
struct gpio_desc *desc;
struct line *line;
unsigned int i;
u64 flags, edflags;
int ret;
+ if (copy_from_user(&lc, ip, sizeof(lc)))
+ return -EFAULT;
+
+ ret = gpio_v2_line_config_validate(&lc, lr->num_lines);
+ if (ret)
+ return ret;
+
+ guard(mutex)(&lr->config_mutex);
+
for (i = 0; i < lr->num_lines; i++) {
line = &lr->lines[i];
desc = lr->lines[i].desc;
- flags = gpio_v2_line_config_flags(lc, i);
+ flags = gpio_v2_line_config_flags(&lc, i);
gpio_v2_line_config_flags_to_desc_flags(flags, &desc->flags);
edflags = flags & GPIO_V2_LINE_EDGE_DETECTOR_FLAGS;
/*
@@ -1539,7 +1541,7 @@ static long linereq_set_config_unlocked(struct linereq *lr,
* or output, else the line will be treated "as is".
*/
if (flags & GPIO_V2_LINE_FLAG_OUTPUT) {
- int val = gpio_v2_line_config_output_value(lc, i);
+ int val = gpio_v2_line_config_output_value(&lc, i);
edge_detector_stop(line);
ret = gpiod_direction_output(desc, val);
@@ -1550,7 +1552,7 @@ static long linereq_set_config_unlocked(struct linereq *lr,
if (ret)
return ret;
- ret = edge_detector_update(line, lc, i, edflags);
+ ret = edge_detector_update(line, &lc, i, edflags);
if (ret)
return ret;
}
@@ -1562,23 +1564,6 @@ static long linereq_set_config_unlocked(struct linereq *lr,
return 0;
}
-static long linereq_set_config(struct linereq *lr, void __user *ip)
-{
- struct gpio_v2_line_config lc;
- int ret;
-
- if (copy_from_user(&lc, ip, sizeof(lc)))
- return -EFAULT;
-
- ret = gpio_v2_line_config_validate(&lc, lr->num_lines);
- if (ret)
- return ret;
-
- guard(mutex)(&lr->config_mutex);
-
- return linereq_set_config_unlocked(lr, &lc);
-}
-
static long linereq_ioctl_unlocked(struct file *file, unsigned int cmd,
unsigned long arg)
{
--
2.39.2
Replace the wrapping functions that inhibit removal of the gpio_device
with equivalent guard macros.
Signed-off-by: Kent Gibson <[email protected]>
---
drivers/gpio/gpiolib-cdev.c | 195 ++++++++++--------------------------
1 file changed, 52 insertions(+), 143 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 5b07578e3bfa..77ecf308ef39 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -65,44 +65,20 @@ typedef long (*ioctl_fn)(struct file *, unsigned int, unsigned long);
typedef ssize_t (*read_fn)(struct file *, char __user *,
size_t count, loff_t *);
-static __poll_t call_poll_locked(struct file *file,
- struct poll_table_struct *wait,
- struct gpio_device *gdev, poll_fn func)
-{
- __poll_t ret;
-
- down_read(&gdev->sem);
- ret = func(file, wait);
- up_read(&gdev->sem);
-
- return ret;
-}
-
-static long call_ioctl_locked(struct file *file, unsigned int cmd,
- unsigned long arg, struct gpio_device *gdev,
- ioctl_fn func)
-{
- long ret;
+DEFINE_CLASS(_read_sem_guard,
+ struct rw_semaphore *,
+ up_read(_T),
+ ({
+ down_read(sem);
+ sem;
+ }),
+ struct rw_semaphore *sem);
- down_read(&gdev->sem);
- ret = func(file, cmd, arg);
- up_read(&gdev->sem);
+/* guard that downs a rw_semaphore while in scope */
+#define read_sem_guard(sem) CLASS(_read_sem_guard, _sem)(sem)
- return ret;
-}
-
-static ssize_t call_read_locked(struct file *file, char __user *buf,
- size_t count, loff_t *f_ps,
- struct gpio_device *gdev, read_fn func)
-{
- ssize_t ret;
-
- down_read(&gdev->sem);
- ret = func(file, buf, count, f_ps);
- up_read(&gdev->sem);
-
- return ret;
-}
+/* guard on the gpio_device sem to inhibit device removal while in use */
+#define gdev_guard(gdev) read_sem_guard(gdev->sem)
/*
* GPIO line handle management
@@ -238,8 +214,8 @@ static long linehandle_set_config(struct linehandle_state *lh,
return 0;
}
-static long linehandle_ioctl_unlocked(struct file *file, unsigned int cmd,
- unsigned long arg)
+static long linehandle_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
{
struct linehandle_state *lh = file->private_data;
void __user *ip = (void __user *)arg;
@@ -248,6 +224,8 @@ static long linehandle_ioctl_unlocked(struct file *file, unsigned int cmd,
unsigned int i;
int ret;
+ gdev_guard(&lh->gdev);
+
if (!lh->gdev->chip)
return -ENODEV;
@@ -297,15 +275,6 @@ static long linehandle_ioctl_unlocked(struct file *file, unsigned int cmd,
}
}
-static long linehandle_ioctl(struct file *file, unsigned int cmd,
- unsigned long arg)
-{
- struct linehandle_state *lh = file->private_data;
-
- return call_ioctl_locked(file, cmd, arg, lh->gdev,
- linehandle_ioctl_unlocked);
-}
-
#ifdef CONFIG_COMPAT
static long linehandle_ioctl_compat(struct file *file, unsigned int cmd,
unsigned long arg)
@@ -1564,12 +1533,14 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
return 0;
}
-static long linereq_ioctl_unlocked(struct file *file, unsigned int cmd,
- unsigned long arg)
+static long linereq_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
{
struct linereq *lr = file->private_data;
void __user *ip = (void __user *)arg;
+ gdev_guard(&lr->gdev);
+
if (!lr->gdev->chip)
return -ENODEV;
@@ -1585,15 +1556,6 @@ static long linereq_ioctl_unlocked(struct file *file, unsigned int cmd,
}
}
-static long linereq_ioctl(struct file *file, unsigned int cmd,
- unsigned long arg)
-{
- struct linereq *lr = file->private_data;
-
- return call_ioctl_locked(file, cmd, arg, lr->gdev,
- linereq_ioctl_unlocked);
-}
-
#ifdef CONFIG_COMPAT
static long linereq_ioctl_compat(struct file *file, unsigned int cmd,
unsigned long arg)
@@ -1602,12 +1564,14 @@ static long linereq_ioctl_compat(struct file *file, unsigned int cmd,
}
#endif
-static __poll_t linereq_poll_unlocked(struct file *file,
- struct poll_table_struct *wait)
+static __poll_t linereq_poll(struct file *file,
+ struct poll_table_struct *wait)
{
struct linereq *lr = file->private_data;
__poll_t events = 0;
+ gdev_guard(&lr->gdev);
+
if (!lr->gdev->chip)
return EPOLLHUP | EPOLLERR;
@@ -1620,22 +1584,16 @@ static __poll_t linereq_poll_unlocked(struct file *file,
return events;
}
-static __poll_t linereq_poll(struct file *file,
- struct poll_table_struct *wait)
-{
- struct linereq *lr = file->private_data;
-
- return call_poll_locked(file, wait, lr->gdev, linereq_poll_unlocked);
-}
-
-static ssize_t linereq_read_unlocked(struct file *file, char __user *buf,
- size_t count, loff_t *f_ps)
+static ssize_t linereq_read(struct file *file, char __user *buf,
+ size_t count, loff_t *f_ps)
{
struct linereq *lr = file->private_data;
struct gpio_v2_line_event le;
ssize_t bytes_read = 0;
int ret;
+ gdev_guard(&lr->gdev);
+
if (!lr->gdev->chip)
return -ENODEV;
@@ -1677,15 +1635,6 @@ static ssize_t linereq_read_unlocked(struct file *file, char __user *buf,
return bytes_read;
}
-static ssize_t linereq_read(struct file *file, char __user *buf,
- size_t count, loff_t *f_ps)
-{
- struct linereq *lr = file->private_data;
-
- return call_read_locked(file, buf, count, f_ps, lr->gdev,
- linereq_read_unlocked);
-}
-
static void linereq_free(struct linereq *lr)
{
struct line *line;
@@ -1938,12 +1887,14 @@ struct lineevent_state {
(GPIOEVENT_REQUEST_RISING_EDGE | \
GPIOEVENT_REQUEST_FALLING_EDGE)
-static __poll_t lineevent_poll_unlocked(struct file *file,
- struct poll_table_struct *wait)
+static __poll_t lineevent_poll(struct file *file,
+ struct poll_table_struct *wait)
{
struct lineevent_state *le = file->private_data;
__poll_t events = 0;
+ gdev_guard(&le->gdev);
+
if (!le->gdev->chip)
return EPOLLHUP | EPOLLERR;
@@ -1955,14 +1906,6 @@ static __poll_t lineevent_poll_unlocked(struct file *file,
return events;
}
-static __poll_t lineevent_poll(struct file *file,
- struct poll_table_struct *wait)
-{
- struct lineevent_state *le = file->private_data;
-
- return call_poll_locked(file, wait, le->gdev, lineevent_poll_unlocked);
-}
-
static int lineevent_unregistered_notify(struct notifier_block *nb,
unsigned long action, void *data)
{
@@ -1979,8 +1922,8 @@ struct compat_gpioeevent_data {
u32 id;
};
-static ssize_t lineevent_read_unlocked(struct file *file, char __user *buf,
- size_t count, loff_t *f_ps)
+static ssize_t lineevent_read(struct file *file, char __user *buf,
+ size_t count, loff_t *f_ps)
{
struct lineevent_state *le = file->private_data;
struct gpioevent_data ge;
@@ -1988,6 +1931,8 @@ static ssize_t lineevent_read_unlocked(struct file *file, char __user *buf,
ssize_t ge_size;
int ret;
+ gdev_guard(&le->gdev);
+
if (!le->gdev->chip)
return -ENODEV;
@@ -2042,15 +1987,6 @@ static ssize_t lineevent_read_unlocked(struct file *file, char __user *buf,
return bytes_read;
}
-static ssize_t lineevent_read(struct file *file, char __user *buf,
- size_t count, loff_t *f_ps)
-{
- struct lineevent_state *le = file->private_data;
-
- return call_read_locked(file, buf, count, f_ps, le->gdev,
- lineevent_read_unlocked);
-}
-
static void lineevent_free(struct lineevent_state *le)
{
if (le->device_unregistered_nb.notifier_call)
@@ -2071,13 +2007,15 @@ static int lineevent_release(struct inode *inode, struct file *file)
return 0;
}
-static long lineevent_ioctl_unlocked(struct file *file, unsigned int cmd,
- unsigned long arg)
+static long lineevent_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
{
struct lineevent_state *le = file->private_data;
void __user *ip = (void __user *)arg;
struct gpiohandle_data ghd;
+ gdev_guard(&le->gdev);
+
if (!le->gdev->chip)
return -ENODEV;
@@ -2103,15 +2041,6 @@ static long lineevent_ioctl_unlocked(struct file *file, unsigned int cmd,
return -EINVAL;
}
-static long lineevent_ioctl(struct file *file, unsigned int cmd,
- unsigned long arg)
-{
- struct lineevent_state *le = file->private_data;
-
- return call_ioctl_locked(file, cmd, arg, le->gdev,
- lineevent_ioctl_unlocked);
-}
-
#ifdef CONFIG_COMPAT
static long lineevent_ioctl_compat(struct file *file, unsigned int cmd,
unsigned long arg)
@@ -2671,12 +2600,14 @@ static int gpio_device_unregistered_notify(struct notifier_block *nb,
return NOTIFY_OK;
}
-static __poll_t lineinfo_watch_poll_unlocked(struct file *file,
- struct poll_table_struct *pollt)
+static __poll_t lineinfo_watch_poll(struct file *file,
+ struct poll_table_struct *pollt)
{
struct gpio_chardev_data *cdev = file->private_data;
__poll_t events = 0;
+ gdev_guard(&cdev->gdev);
+
if (!cdev->gdev->chip)
return EPOLLHUP | EPOLLERR;
@@ -2689,17 +2620,8 @@ static __poll_t lineinfo_watch_poll_unlocked(struct file *file,
return events;
}
-static __poll_t lineinfo_watch_poll(struct file *file,
- struct poll_table_struct *pollt)
-{
- struct gpio_chardev_data *cdev = file->private_data;
-
- return call_poll_locked(file, pollt, cdev->gdev,
- lineinfo_watch_poll_unlocked);
-}
-
-static ssize_t lineinfo_watch_read_unlocked(struct file *file, char __user *buf,
- size_t count, loff_t *off)
+static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
+ size_t count, loff_t *off)
{
struct gpio_chardev_data *cdev = file->private_data;
struct gpio_v2_line_info_changed event;
@@ -2707,6 +2629,8 @@ static ssize_t lineinfo_watch_read_unlocked(struct file *file, char __user *buf,
int ret;
size_t event_size;
+ gdev_guard(&cdev->gdev);
+
if (!cdev->gdev->chip)
return -ENODEV;
@@ -2769,15 +2693,6 @@ static ssize_t lineinfo_watch_read_unlocked(struct file *file, char __user *buf,
return bytes_read;
}
-static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
- size_t count, loff_t *off)
-{
- struct gpio_chardev_data *cdev = file->private_data;
-
- return call_read_locked(file, buf, count, off, cdev->gdev,
- lineinfo_watch_read_unlocked);
-}
-
/**
* gpio_chrdev_open() - open the chardev for ioctl operations
* @inode: inode for this chardev
@@ -2791,17 +2706,15 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file)
struct gpio_chardev_data *cdev;
int ret = -ENOMEM;
- down_read(&gdev->sem);
+ gdev_guard(&gdev);
/* Fail on open if the backing gpiochip is gone */
- if (!gdev->chip) {
- ret = -ENODEV;
- goto out_unlock;
- }
+ if (!gdev->chip)
+ return -ENODEV;
cdev = kzalloc(sizeof(*cdev), GFP_KERNEL);
if (!cdev)
- goto out_unlock;
+ return -ENODEV;
cdev->watched_lines = bitmap_zalloc(gdev->chip->ngpio, GFP_KERNEL);
if (!cdev->watched_lines)
@@ -2830,8 +2743,6 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file)
if (ret)
goto out_unregister_device_notifier;
- up_read(&gdev->sem);
-
return ret;
out_unregister_device_notifier:
@@ -2845,8 +2756,6 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file)
bitmap_free(cdev->watched_lines);
out_free_cdev:
kfree(cdev);
-out_unlock:
- up_read(&gdev->sem);
return ret;
}
--
2.39.2
(+PeterZ)
On Wed, Dec 20, 2023 at 2:52 AM Kent Gibson <[email protected]> wrote:
> Replace the wrapping functions that inhibit removal of the gpio_device
> with equivalent guard macros.
>
> Signed-off-by: Kent Gibson <[email protected]>
(...)
> +DEFINE_CLASS(_read_sem_guard,
> + struct rw_semaphore *,
> + up_read(_T),
> + ({
> + down_read(sem);
> + sem;
> + }),
> + struct rw_semaphore *sem);
Isn't this so generic that it should be in <linux/cleanup.h>?
Otherwise all the patches look good to me.
Yours,
Linus Walleij
On Wed, Dec 20, 2023 at 12:56 PM Linus Walleij <[email protected]> wrote:
>
> (+PeterZ)
>
> On Wed, Dec 20, 2023 at 2:52 AM Kent Gibson <[email protected]> wrote:
>
> > Replace the wrapping functions that inhibit removal of the gpio_device
> > with equivalent guard macros.
> >
> > Signed-off-by: Kent Gibson <[email protected]>
> (...)
> > +DEFINE_CLASS(_read_sem_guard,
> > + struct rw_semaphore *,
> > + up_read(_T),
> > + ({
> > + down_read(sem);
> > + sem;
> > + }),
> > + struct rw_semaphore *sem);
>
> Isn't this so generic that it should be in <linux/cleanup.h>?
>
> Otherwise all the patches look good to me.
>
We already have this:
DEFINE_GUARD(rwsem_read, struct rw_semaphore *, down_read(_T), up_read(_T))
DEFINE_GUARD(rwsem_write, struct rw_semaphore *, down_write(_T), up_write(_T))
DEFINE_FREE(up_read, struct rw_semaphore *, if (_T) up_read(_T))
DEFINE_FREE(up_write, struct rw_semaphore *, if (_T) up_write(_T))
This can surely be used here, right?
Bart
> Yours,
> Linus Walleij
On Wed, Dec 20, 2023 at 01:05:35PM +0100, Bartosz Golaszewski wrote:
> On Wed, Dec 20, 2023 at 12:56 PM Linus Walleij <[email protected]> wrote:
> >
> > (+PeterZ)
> >
> > On Wed, Dec 20, 2023 at 2:52 AM Kent Gibson <[email protected]> wrote:
> >
> > > Replace the wrapping functions that inhibit removal of the gpio_device
> > > with equivalent guard macros.
> > >
> > > Signed-off-by: Kent Gibson <[email protected]>
> > (...)
> > > +DEFINE_CLASS(_read_sem_guard,
> > > + struct rw_semaphore *,
> > > + up_read(_T),
> > > + ({
> > > + down_read(sem);
> > > + sem;
> > > + }),
> > > + struct rw_semaphore *sem);
> >
> > Isn't this so generic that it should be in <linux/cleanup.h>?
> >
> > Otherwise all the patches look good to me.
> >
>
> We already have this:
>
> DEFINE_GUARD(rwsem_read, struct rw_semaphore *, down_read(_T), up_read(_T))
> DEFINE_GUARD(rwsem_write, struct rw_semaphore *, down_write(_T), up_write(_T))
>
> DEFINE_FREE(up_read, struct rw_semaphore *, if (_T) up_read(_T))
> DEFINE_FREE(up_write, struct rw_semaphore *, if (_T) up_write(_T))
>
Ah - in rwsem.h - I missed that.
> This can surely be used here, right?
>
Don't see why not.
I would still like to move the gpio_device specific macros to gpiolib.h,
as they apply to the struct gpio_device defined there.
The naming probably needs some reworking, so open to suggestions on
that.
Cheers,
Kent.
On Wed, Dec 20, 2023 at 1:13 PM Kent Gibson <[email protected]> wrote:
>
> On Wed, Dec 20, 2023 at 01:05:35PM +0100, Bartosz Golaszewski wrote:
> > On Wed, Dec 20, 2023 at 12:56 PM Linus Walleij <[email protected]> wrote:
> > >
> > > (+PeterZ)
> > >
> > > On Wed, Dec 20, 2023 at 2:52 AM Kent Gibson <[email protected]> wrote:
> > >
> > > > Replace the wrapping functions that inhibit removal of the gpio_device
> > > > with equivalent guard macros.
> > > >
> > > > Signed-off-by: Kent Gibson <[email protected]>
> > > (...)
> > > > +DEFINE_CLASS(_read_sem_guard,
> > > > + struct rw_semaphore *,
> > > > + up_read(_T),
> > > > + ({
> > > > + down_read(sem);
> > > > + sem;
> > > > + }),
> > > > + struct rw_semaphore *sem);
> > >
> > > Isn't this so generic that it should be in <linux/cleanup.h>?
> > >
> > > Otherwise all the patches look good to me.
> > >
> >
> > We already have this:
> >
> > DEFINE_GUARD(rwsem_read, struct rw_semaphore *, down_read(_T), up_read(_T))
> > DEFINE_GUARD(rwsem_write, struct rw_semaphore *, down_write(_T), up_write(_T))
> >
> > DEFINE_FREE(up_read, struct rw_semaphore *, if (_T) up_read(_T))
> > DEFINE_FREE(up_write, struct rw_semaphore *, if (_T) up_write(_T))
> >
>
> Ah - in rwsem.h - I missed that.
>
> > This can surely be used here, right?
> >
>
> Don't see why not.
>
> I would still like to move the gpio_device specific macros to gpiolib.h,
> as they apply to the struct gpio_device defined there.
Which ones? Because I'd rather use guard(rwsem_read)(&gdev->sem); than
some custom wrapper as this one's purpose is clearer.
Bart
> The naming probably needs some reworking, so open to suggestions on
> that.
>
> Cheers,
> Kent.
On Wed, Dec 20, 2023 at 01:16:00PM +0100, Bartosz Golaszewski wrote:
> On Wed, Dec 20, 2023 at 1:13 PM Kent Gibson <[email protected]> wrote:
> >
> > On Wed, Dec 20, 2023 at 01:05:35PM +0100, Bartosz Golaszewski wrote:
> > > On Wed, Dec 20, 2023 at 12:56 PM Linus Walleij <[email protected]> wrote:
> > > >
> > > > (+PeterZ)
> > > >
> > > > On Wed, Dec 20, 2023 at 2:52 AM Kent Gibson <[email protected]> wrote:
> > > >
> > > > > Replace the wrapping functions that inhibit removal of the gpio_device
> > > > > with equivalent guard macros.
> > > > >
> > > > > Signed-off-by: Kent Gibson <[email protected]>
> > > > (...)
> > > > > +DEFINE_CLASS(_read_sem_guard,
> > > > > + struct rw_semaphore *,
> > > > > + up_read(_T),
> > > > > + ({
> > > > > + down_read(sem);
> > > > > + sem;
> > > > > + }),
> > > > > + struct rw_semaphore *sem);
> > > >
> > > > Isn't this so generic that it should be in <linux/cleanup.h>?
> > > >
> > > > Otherwise all the patches look good to me.
> > > >
> > >
> > > We already have this:
> > >
> > > DEFINE_GUARD(rwsem_read, struct rw_semaphore *, down_read(_T), up_read(_T))
> > > DEFINE_GUARD(rwsem_write, struct rw_semaphore *, down_write(_T), up_write(_T))
> > >
> > > DEFINE_FREE(up_read, struct rw_semaphore *, if (_T) up_read(_T))
> > > DEFINE_FREE(up_write, struct rw_semaphore *, if (_T) up_write(_T))
> > >
> >
> > Ah - in rwsem.h - I missed that.
> >
> > > This can surely be used here, right?
> > >
> >
> > Don't see why not.
> >
> > I would still like to move the gpio_device specific macros to gpiolib.h,
> > as they apply to the struct gpio_device defined there.
>
> Which ones? Because I'd rather use guard(rwsem_read)(&gdev->sem); than
> some custom wrapper as this one's purpose is clearer.
>
It would be read and write guards for the gpio_device.
cdev would only be using the read flavour.
And possibly named something other than read/write as the purpose is to
prevent (read) or allow (write) object removal.
I though that would be clearer than having to reference gpiolib.h to see
what gdev->sem covers, and allow you to change the locking
mechanism later and not have to update cdev.
Cheers,
Kent.
On Wed, Dec 20, 2023 at 1:23 PM Kent Gibson <[email protected]> wrote:
>
> On Wed, Dec 20, 2023 at 01:16:00PM +0100, Bartosz Golaszewski wrote:
> > On Wed, Dec 20, 2023 at 1:13 PM Kent Gibson <[email protected]> wrote:
> > >
> > > On Wed, Dec 20, 2023 at 01:05:35PM +0100, Bartosz Golaszewski wrote:
> > > > On Wed, Dec 20, 2023 at 12:56 PM Linus Walleij <[email protected]> wrote:
> > > > >
> > > > > (+PeterZ)
> > > > >
> > > > > On Wed, Dec 20, 2023 at 2:52 AM Kent Gibson <[email protected]> wrote:
> > > > >
> > > > > > Replace the wrapping functions that inhibit removal of the gpio_device
> > > > > > with equivalent guard macros.
> > > > > >
> > > > > > Signed-off-by: Kent Gibson <[email protected]>
> > > > > (...)
> > > > > > +DEFINE_CLASS(_read_sem_guard,
> > > > > > + struct rw_semaphore *,
> > > > > > + up_read(_T),
> > > > > > + ({
> > > > > > + down_read(sem);
> > > > > > + sem;
> > > > > > + }),
> > > > > > + struct rw_semaphore *sem);
> > > > >
> > > > > Isn't this so generic that it should be in <linux/cleanup.h>?
> > > > >
> > > > > Otherwise all the patches look good to me.
> > > > >
> > > >
> > > > We already have this:
> > > >
> > > > DEFINE_GUARD(rwsem_read, struct rw_semaphore *, down_read(_T), up_read(_T))
> > > > DEFINE_GUARD(rwsem_write, struct rw_semaphore *, down_write(_T), up_write(_T))
> > > >
> > > > DEFINE_FREE(up_read, struct rw_semaphore *, if (_T) up_read(_T))
> > > > DEFINE_FREE(up_write, struct rw_semaphore *, if (_T) up_write(_T))
> > > >
> > >
> > > Ah - in rwsem.h - I missed that.
> > >
> > > > This can surely be used here, right?
> > > >
> > >
> > > Don't see why not.
> > >
> > > I would still like to move the gpio_device specific macros to gpiolib.h,
> > > as they apply to the struct gpio_device defined there.
> >
> > Which ones? Because I'd rather use guard(rwsem_read)(&gdev->sem); than
> > some custom wrapper as this one's purpose is clearer.
> >
>
> It would be read and write guards for the gpio_device.
> cdev would only be using the read flavour.
> And possibly named something other than read/write as the purpose is to
> prevent (read) or allow (write) object removal.
>
> I though that would be clearer than having to reference gpiolib.h to see
> what gdev->sem covers, and allow you to change the locking
> mechanism later and not have to update cdev.
>
I still prefer open-coded guards here for clarity. I hope that with
SRCU in gpiolib.c, we'll get rid of locking in cdev entirely anyway.
Bart
> Cheers,
> Kent.
>
On Wed, Dec 20, 2023 at 01:30:57PM +0100, Bartosz Golaszewski wrote:
> On Wed, Dec 20, 2023 at 1:23 PM Kent Gibson <[email protected]> wrote:
> >
> > >
> >
> > It would be read and write guards for the gpio_device.
> > cdev would only be using the read flavour.
> > And possibly named something other than read/write as the purpose is to
> > prevent (read) or allow (write) object removal.
> >
> > I though that would be clearer than having to reference gpiolib.h to see
> > what gdev->sem covers, and allow you to change the locking
> > mechanism later and not have to update cdev.
> >
>
> I still prefer open-coded guards here for clarity. I hope that with
> SRCU in gpiolib.c, we'll get rid of locking in cdev entirely anyway.
>
Ok, it is your object so I should use it the way you want it used.
Btw, before I go pushing out a v2, do you have an answer on whether
gpio_ioctl() requires a guard, as mentioned in the cover letter?
Is the fact there is an active ioctl on the chardev sufficient in
itself to keep the gpio_device alive?
Cheers,
Kent.
On Wed, Dec 20, 2023 at 1:53 PM Kent Gibson <[email protected]> wrote:
>
> On Wed, Dec 20, 2023 at 01:30:57PM +0100, Bartosz Golaszewski wrote:
> > On Wed, Dec 20, 2023 at 1:23 PM Kent Gibson <[email protected]> wrote:
> > >
> > > >
> > >
> > > It would be read and write guards for the gpio_device.
> > > cdev would only be using the read flavour.
> > > And possibly named something other than read/write as the purpose is to
> > > prevent (read) or allow (write) object removal.
> > >
> > > I though that would be clearer than having to reference gpiolib.h to see
> > > what gdev->sem covers, and allow you to change the locking
> > > mechanism later and not have to update cdev.
> > >
> >
> > I still prefer open-coded guards here for clarity. I hope that with
> > SRCU in gpiolib.c, we'll get rid of locking in cdev entirely anyway.
> >
>
> Ok, it is your object so I should use it the way you want it used.
>
> Btw, before I go pushing out a v2, do you have an answer on whether
> gpio_ioctl() requires a guard, as mentioned in the cover letter?
> Is the fact there is an active ioctl on the chardev sufficient in
> itself to keep the gpio_device alive?
>
AFAICT: no. I think it's a bug (good catch!). Can you extend your
series with a backportable bugfix that would come first?
Bartosz
> Cheers,
> Kent.
On Wed, Dec 20, 2023 at 02:19:37PM +0100, Bartosz Golaszewski wrote:
> On Wed, Dec 20, 2023 at 1:53 PM Kent Gibson <[email protected]> wrote:
> >
> > On Wed, Dec 20, 2023 at 01:30:57PM +0100, Bartosz Golaszewski wrote:
> > > On Wed, Dec 20, 2023 at 1:23 PM Kent Gibson <[email protected]> wrote:
> > > >
> > > > >
> > > >
> > > > It would be read and write guards for the gpio_device.
> > > > cdev would only be using the read flavour.
> > > > And possibly named something other than read/write as the purpose is to
> > > > prevent (read) or allow (write) object removal.
> > > >
> > > > I though that would be clearer than having to reference gpiolib.h to see
> > > > what gdev->sem covers, and allow you to change the locking
> > > > mechanism later and not have to update cdev.
> > > >
> > >
> > > I still prefer open-coded guards here for clarity. I hope that with
> > > SRCU in gpiolib.c, we'll get rid of locking in cdev entirely anyway.
> > >
> >
> > Ok, it is your object so I should use it the way you want it used.
> >
> > Btw, before I go pushing out a v2, do you have an answer on whether
> > gpio_ioctl() requires a guard, as mentioned in the cover letter?
> > Is the fact there is an active ioctl on the chardev sufficient in
> > itself to keep the gpio_device alive?
> >
>
> AFAICT: no. I think it's a bug (good catch!).
The wrappers made that harder to pick up.
It kind of stood out as the exception after changing the other ioctls
over to guards - where was the guard for that one?
> Can you extend your
> series with a backportable bugfix that would come first?
>
Sure. That would still use the guard(rwsem_read)?
I mean you don't to go adding a wrapper for the fix, just to
subsequently remove it, right?
Cheers,
Kent.
On Wed, Dec 20, 2023 at 2:28 PM Kent Gibson <[email protected]> wrote:
>
> On Wed, Dec 20, 2023 at 02:19:37PM +0100, Bartosz Golaszewski wrote:
> > On Wed, Dec 20, 2023 at 1:53 PM Kent Gibson <[email protected]> wrote:
> > >
> > > On Wed, Dec 20, 2023 at 01:30:57PM +0100, Bartosz Golaszewski wrote:
> > > > On Wed, Dec 20, 2023 at 1:23 PM Kent Gibson <[email protected]> wrote:
> > > > >
> > > > > >
> > > > >
> > > > > It would be read and write guards for the gpio_device.
> > > > > cdev would only be using the read flavour.
> > > > > And possibly named something other than read/write as the purpose is to
> > > > > prevent (read) or allow (write) object removal.
> > > > >
> > > > > I though that would be clearer than having to reference gpiolib.h to see
> > > > > what gdev->sem covers, and allow you to change the locking
> > > > > mechanism later and not have to update cdev.
> > > > >
> > > >
> > > > I still prefer open-coded guards here for clarity. I hope that with
> > > > SRCU in gpiolib.c, we'll get rid of locking in cdev entirely anyway.
> > > >
> > >
> > > Ok, it is your object so I should use it the way you want it used.
> > >
> > > Btw, before I go pushing out a v2, do you have an answer on whether
> > > gpio_ioctl() requires a guard, as mentioned in the cover letter?
> > > Is the fact there is an active ioctl on the chardev sufficient in
> > > itself to keep the gpio_device alive?
> > >
> >
> > AFAICT: no. I think it's a bug (good catch!).
>
> The wrappers made that harder to pick up.
> It kind of stood out as the exception after changing the other ioctls
> over to guards - where was the guard for that one?
>
Yeah, it makes sense. This is precisely why guards are so much better
than hand-coding locks.
> > Can you extend your
> > series with a backportable bugfix that would come first?
> >
>
> Sure. That would still use the guard(rwsem_read)?
> I mean you don't to go adding a wrapper for the fix, just to
> subsequently remove it, right?
>
In master - sure. But we definitely do want to backport that to stable
branches and for that we need to use the old wrapper.
Bart
> Cheers,
> Kent.
On Wed, Dec 20, 2023 at 02:47:45PM +0100, Bartosz Golaszewski wrote:
> On Wed, Dec 20, 2023 at 2:28 PM Kent Gibson <[email protected]> wrote:
> >
> > On Wed, Dec 20, 2023 at 02:19:37PM +0100, Bartosz Golaszewski wrote:
> > > On Wed, Dec 20, 2023 at 1:53 PM Kent Gibson <[email protected]> wrote:
> > > >
> > > > On Wed, Dec 20, 2023 at 01:30:57PM +0100, Bartosz Golaszewski wrote:
> > > > > On Wed, Dec 20, 2023 at 1:23 PM Kent Gibson <[email protected]> wrote:
> > > > > >
> > > > > > >
> > > > > >
> > > > > > It would be read and write guards for the gpio_device.
> > > > > > cdev would only be using the read flavour.
> > > > > > And possibly named something other than read/write as the purpose is to
> > > > > > prevent (read) or allow (write) object removal.
> > > > > >
> > > > > > I though that would be clearer than having to reference gpiolib.h to see
> > > > > > what gdev->sem covers, and allow you to change the locking
> > > > > > mechanism later and not have to update cdev.
> > > > > >
> > > > >
> > > > > I still prefer open-coded guards here for clarity. I hope that with
> > > > > SRCU in gpiolib.c, we'll get rid of locking in cdev entirely anyway.
> > > > >
> > > >
> > > > Ok, it is your object so I should use it the way you want it used.
> > > >
> > > > Btw, before I go pushing out a v2, do you have an answer on whether
> > > > gpio_ioctl() requires a guard, as mentioned in the cover letter?
> > > > Is the fact there is an active ioctl on the chardev sufficient in
> > > > itself to keep the gpio_device alive?
> > > >
> > >
> > > AFAICT: no. I think it's a bug (good catch!).
> >
> > The wrappers made that harder to pick up.
> > It kind of stood out as the exception after changing the other ioctls
> > over to guards - where was the guard for that one?
> >
>
> Yeah, it makes sense. This is precisely why guards are so much better
> than hand-coding locks.
>
> > > Can you extend your
> > > series with a backportable bugfix that would come first?
> > >
> >
> > Sure. That would still use the guard(rwsem_read)?
> > I mean you don't to go adding a wrapper for the fix, just to
> > subsequently remove it, right?
> >
>
> In master - sure. But we definitely do want to backport that to stable
> branches and for that we need to use the old wrapper.
>
Ok, so cleanup.h is too recent for backporting.
Adding and then removing a wrapper it is then.
Cheers,
Kent.
On Wed, Dec 20, 2023 at 09:51:04AM +0800, Kent Gibson wrote:
> The size of struct linereq may exceed a page, so allocate space for
> it using kvzalloc() instead of kzalloc().
It might be this needs a bit of elaboration. The kmalloc() tries to allocate
a contiguous (in physical address space) chunk of memory and with fragmented
memory it might be not possible. So the above issue might (rarely) happen.
In most cases the call to kmalloc() will succeed.
--
With Best Regards,
Andy Shevchenko
On Wed, Dec 20, 2023 at 04:30:24PM +0200, Andy Shevchenko wrote:
> On Wed, Dec 20, 2023 at 09:51:04AM +0800, Kent Gibson wrote:
> > The size of struct linereq may exceed a page, so allocate space for
> > it using kvzalloc() instead of kzalloc().
>
> It might be this needs a bit of elaboration. The kmalloc() tries to allocate
> a contiguous (in physical address space) chunk of memory and with fragmented
> memory it might be not possible. So the above issue might (rarely) happen.
> In most cases the call to kmalloc() will succeed.
>
For sure, the kzalloc() generally works - or we wouldn't've gotten this
far as tests with MAX_LINES would've been failing.
We are targetting a very niche failure mode here.
The size allocated can only be determined at runtime, may be more or
less than a page, and we don't care whether the physical memory allocated
is contiguous.
As such kvzalloc() is the appropriate allocator.
Are you suggesting repeating the relevant sections of the
kmalloc/vmalloc() documentation or Memory Allocation Guide as part of the
checkin comment?
Cheers,
Kent.
On Wed, Dec 20, 2023 at 10:53:07PM +0800, Kent Gibson wrote:
> On Wed, Dec 20, 2023 at 04:30:24PM +0200, Andy Shevchenko wrote:
> > On Wed, Dec 20, 2023 at 09:51:04AM +0800, Kent Gibson wrote:
> > > The size of struct linereq may exceed a page, so allocate space for
> > > it using kvzalloc() instead of kzalloc().
> >
> > It might be this needs a bit of elaboration. The kmalloc() tries to allocate
> > a contiguous (in physical address space) chunk of memory and with fragmented
> > memory it might be not possible. So the above issue might (rarely) happen.
> > In most cases the call to kmalloc() will succeed.
>
> For sure, the kzalloc() generally works - or we wouldn't've gotten this
> far as tests with MAX_LINES would've been failing.
> We are targetting a very niche failure mode here.
>
> The size allocated can only be determined at runtime, may be more or
> less than a page, and we don't care whether the physical memory allocated
> is contiguous.
> As such kvzalloc() is the appropriate allocator.
>
> Are you suggesting repeating the relevant sections of the
> kmalloc/vmalloc() documentation or Memory Allocation Guide as part of the
> checkin comment?
I suggesting to make clear in the commit message that:
- there is no bug per se with the code logic (i.o.w.
there is no issue to have allocations bigger than one page)
- this is very rarely case when it might be a problem
You can also put a reference to the documentation, if you wish.
This should be harmless and adding not more than a line into
the commit message (or even as a Link: tag to the HTML variant of it).
--
With Best Regards,
Andy Shevchenko