| Message ID | 20220417165208.39754-1-schspa@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,01/15] gpio: use raw spinlock for gpio chip shadowed data | expand |
Andy Shevchenko <andy.shevchenko@gmail.com> writes: > > Changelog: > v1 -> v2: > - Reduce the useless stacktrace. > - Split to series of patches > > Changelog usually goes after cutter ‘--- ‘ line. Besides that you haven’t compiled your tree just after this patch. > Yes, thanks for reminding me, I will fix it by upload another version of patch if there is no other problems. > Link: https://lore.kernel.org/all/20220415165505.30383-1-schspa@gmail.com/ > > Signed-off-by: Schspa Shi <schspa@gmail.com> > --- > drivers/gpio/gpio-mmio.c | 22 +++++++++++----------- > include/linux/gpio/driver.h | 2 +- > > You can’t do it for one driver only. As I told it will require too much of additional churn to make this to be series. > It seems I have misunderstood your "too much of additional churn". Can you explain it? The gpio-mmio.c and driver.h here are the basics of other gpio drivers. In my opinion, these two files belong to the basic code of gpio, and functions such as bgpio_init are declared in include/linux/gpio/driver.h and implemented in drivers/gpio/gpio-mmio.c. So there is no churn. -- Schspa Shi BRs
On Mon, Apr 18, 2022 at 6:07 AM Schspa Shi <schspa@gmail.com> wrote: > Andy Shevchenko <andy.shevchenko@gmail.com> writes: ... > > drivers/gpio/gpio-mmio.c | 22 +++++++++++----------- > > include/linux/gpio/driver.h | 2 +- > > > > You can’t do it for one driver only. As I told it will require too much of additional churn to make this to be series. > > > > It seems I have misunderstood your "too much of additional churn". Can > you explain it? > The gpio-mmio.c and driver.h here are the basics of other gpio > drivers. In my opinion, these two files > belong to the basic code of gpio, and functions such as bgpio_init are > declared in > include/linux/gpio/driver.h and implemented in > drivers/gpio/gpio-mmio.c. So there is no churn. When you change the member of the data structure, you have to change all its users. You can't change only one at a time because it will be a (compile-time) bisectability issue.
On Mon, Apr 18, 2022 at 2:38 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Apr 18, 2022 at 6:07 AM Schspa Shi <schspa@gmail.com> wrote: > > Andy Shevchenko <andy.shevchenko@gmail.com> writes: > > ... > > > > drivers/gpio/gpio-mmio.c | 22 +++++++++++----------- > > > include/linux/gpio/driver.h | 2 +- > > > > > > You can’t do it for one driver only. As I told it will require too much of additional churn to make this to be series. > > > > > > > It seems I have misunderstood your "too much of additional churn". Can > > you explain it? > > The gpio-mmio.c and driver.h here are the basics of other gpio > > drivers. In my opinion, these two files > > belong to the basic code of gpio, and functions such as bgpio_init are > > declared in > > include/linux/gpio/driver.h and implemented in > > drivers/gpio/gpio-mmio.c. So there is no churn. > > When you change the member of the data structure, you have to change > all its users. You can't change only one at a time because it will be > a (compile-time) bisectability issue. Answering your question here, it will require moving to union with an additional member and corresponding core changes, convert all drivers one-by-one, and remove the old type. It's not worth doing it, but as I said let maintainers decide.
Andy Shevchenko <andy.shevchenko@gmail.com> writes: > On Mon, Apr 18, 2022 at 2:38 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: >> On Mon, Apr 18, 2022 at 6:07 AM Schspa Shi <schspa@gmail.com> wrote: >> > Andy Shevchenko <andy.shevchenko@gmail.com> writes: >> >> ... >> >> > > drivers/gpio/gpio-mmio.c | 22 +++++++++++----------- >> > > include/linux/gpio/driver.h | 2 +- >> > > >> > > You can’t do it for one driver only. As I told it will require too much of additional churn to make this to be series. >> > > >> > >> > It seems I have misunderstood your "too much of additional churn". Can >> > you explain it? >> > The gpio-mmio.c and driver.h here are the basics of other gpio >> > drivers. In my opinion, these two files >> > belong to the basic code of gpio, and functions such as bgpio_init are >> > declared in >> > include/linux/gpio/driver.h and implemented in >> > drivers/gpio/gpio-mmio.c. So there is no churn. >> >> When you change the member of the data structure, you have to change >> all its users. You can't change only one at a time because it will be >> a (compile-time) bisectability issue. Yes, I understand and will take for bisectability use case for the next time. > > Answering your question here, it will require moving to union with an > additional member and corresponding core changes, convert all drivers > one-by-one, and remove the old type. It's not worth doing it, but as I > said let maintainers decide. Okay, sorry for my misunderstanding, I thought you were saying it's bad to modify too many different files in one patch, so I split the patch into a series of patchsets. So, let Linus Walleij or Bartosz Golaszewski to decide for it ? I have the same options as you, it's a small change, and no need to trouble everyone for it. Because this structure will be used as the same instance in multiple files, even if we change this variable to union first, it can be compiled, but the program will still not work properly. This is because bgpio_init is initialized with the type of raw_spinlock_t, but is still accessed as spinlock_t in other drivers, which is a serious abnormal initialization.
On Mon, Apr 18, 2022 at 5:43 PM Schspa Shi <schspa@gmail.com> wrote: > > Andy Shevchenko <andy.shevchenko@gmail.com> writes: > > > On Mon, Apr 18, 2022 at 2:38 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > >> On Mon, Apr 18, 2022 at 6:07 AM Schspa Shi <schspa@gmail.com> wrote: > >> > Andy Shevchenko <andy.shevchenko@gmail.com> writes: > >> > >> ... > >> > >> > > drivers/gpio/gpio-mmio.c | 22 +++++++++++----------- > >> > > include/linux/gpio/driver.h | 2 +- > >> > > > >> > > You can’t do it for one driver only. As I told it will require too much of additional churn to make this to be series. > >> > > > >> > > >> > It seems I have misunderstood your "too much of additional churn". Can > >> > you explain it? > >> > The gpio-mmio.c and driver.h here are the basics of other gpio > >> > drivers. In my opinion, these two files > >> > belong to the basic code of gpio, and functions such as bgpio_init are > >> > declared in > >> > include/linux/gpio/driver.h and implemented in > >> > drivers/gpio/gpio-mmio.c. So there is no churn. > >> > >> When you change the member of the data structure, you have to change > >> all its users. You can't change only one at a time because it will be > >> a (compile-time) bisectability issue. > > > Yes, I understand and will take for bisectability use case for the next time. > > > > > Answering your question here, it will require moving to union with an > > additional member and corresponding core changes, convert all drivers > > one-by-one, and remove the old type. It's not worth doing it, but as I > > said let maintainers decide. > > Okay, sorry for my misunderstanding, I thought you were saying it's > bad to modify too many different files in one patch, so I split the > patch into a series of patchsets. > > So, let Linus Walleij or Bartosz Golaszewski to decide for it ? > I have the same options as you, it's a small change, and no need to > trouble everyone for it. > I prefer a single patch in this case, we can apply it closer to the next merge window. Bart > Because this structure will be used as the same instance in multiple > files, even if we change this variable to union first, it can be > compiled, but the program will still not work properly. This is > because bgpio_init is initialized with the type of raw_spinlock_t, > but is still accessed as spinlock_t in other drivers, which is a > serious abnormal initialization.
diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c index c335a0309ba3..d9dff3dc92ae 100644 --- a/drivers/gpio/gpio-mmio.c +++ b/drivers/gpio/gpio-mmio.c @@ -220,7 +220,7 @@ static void bgpio_set(struct gpio_chip *gc, unsigned int gpio, int val) unsigned long mask = bgpio_line2mask(gc, gpio); unsigned long flags; - spin_lock_irqsave(&gc->bgpio_lock, flags); + raw_spin_lock_irqsave(&gc->bgpio_lock, flags); if (val) gc->bgpio_data |= mask; @@ -229,7 +229,7 @@ static void bgpio_set(struct gpio_chip *gc, unsigned int gpio, int val) gc->write_reg(gc->reg_dat, gc->bgpio_data); - spin_unlock_irqrestore(&gc->bgpio_lock, flags); + raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags); } static void bgpio_set_with_clear(struct gpio_chip *gc, unsigned int gpio, @@ -248,7 +248,7 @@ static void bgpio_set_set(struct gpio_chip *gc, unsigned int gpio, int val) unsigned long mask = bgpio_line2mask(gc, gpio); unsigned long flags; - spin_lock_irqsave(&gc->bgpio_lock, flags); + raw_spin_lock_irqsave(&gc->bgpio_lock, flags); if (val) gc->bgpio_data |= mask; @@ -257,7 +257,7 @@ static void bgpio_set_set(struct gpio_chip *gc, unsigned int gpio, int val) gc->write_reg(gc->reg_set, gc->bgpio_data); - spin_unlock_irqrestore(&gc->bgpio_lock, flags); + raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags); } static void bgpio_multiple_get_masks(struct gpio_chip *gc, @@ -286,7 +286,7 @@ static void bgpio_set_multiple_single_reg(struct gpio_chip *gc, unsigned long flags; unsigned long set_mask, clear_mask; - spin_lock_irqsave(&gc->bgpio_lock, flags); + raw_spin_lock_irqsave(&gc->bgpio_lock, flags); bgpio_multiple_get_masks(gc, mask, bits, &set_mask, &clear_mask); @@ -295,7 +295,7 @@ static void bgpio_set_multiple_single_reg(struct gpio_chip *gc, gc->write_reg(reg, gc->bgpio_data); - spin_unlock_irqrestore(&gc->bgpio_lock, flags); + raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags); } static void bgpio_set_multiple(struct gpio_chip *gc, unsigned long *mask, @@ -347,7 +347,7 @@ static int bgpio_dir_in(struct gpio_chip *gc, unsigned int gpio) { unsigned long flags; - spin_lock_irqsave(&gc->bgpio_lock, flags); + raw_spin_lock_irqsave(&gc->bgpio_lock, flags); gc->bgpio_dir &= ~bgpio_line2mask(gc, gpio); @@ -356,7 +356,7 @@ static int bgpio_dir_in(struct gpio_chip *gc, unsigned int gpio) if (gc->reg_dir_out) gc->write_reg(gc->reg_dir_out, gc->bgpio_dir); - spin_unlock_irqrestore(&gc->bgpio_lock, flags); + raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags); return 0; } @@ -387,7 +387,7 @@ static void bgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val) { unsigned long flags; - spin_lock_irqsave(&gc->bgpio_lock, flags); + raw_spin_lock_irqsave(&gc->bgpio_lock, flags); gc->bgpio_dir |= bgpio_line2mask(gc, gpio); @@ -396,7 +396,7 @@ static void bgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val) if (gc->reg_dir_out) gc->write_reg(gc->reg_dir_out, gc->bgpio_dir); - spin_unlock_irqrestore(&gc->bgpio_lock, flags); + raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags); } static int bgpio_dir_out_dir_first(struct gpio_chip *gc, unsigned int gpio, @@ -610,7 +610,7 @@ int bgpio_init(struct gpio_chip *gc, struct device *dev, if (gc->bgpio_bits > BITS_PER_LONG) return -EINVAL; - spin_lock_init(&gc->bgpio_lock); + raw_spin_lock_init(&gc->bgpio_lock); gc->parent = dev; gc->label = dev_name(dev); gc->base = -1; diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index 874aabd270c9..ff8247a19f57 100644 --- a/include/linux/gpio/driver.h +++ b/include/linux/gpio/driver.h @@ -445,7 +445,7 @@ struct gpio_chip { void __iomem *reg_dir_in; bool bgpio_dir_unreadable; int bgpio_bits; - spinlock_t bgpio_lock; + raw_spinlock_t bgpio_lock; unsigned long bgpio_data; unsigned long bgpio_dir; #endif /* CONFIG_GPIO_GENERIC */
In case of PREEMPT_RT, there is a raw_spinlock -> spinlock dependency as the lockdep report shows. __irq_set_handler irq_get_desc_buslock __irq_get_desc_lock raw_spin_lock_irqsave(&desc->lock, *flags); // raw spinlock get here __irq_do_set_handler mask_ack_irq dwapb_irq_ack spin_lock_irqsave(&gc->bgpio_lock, flags); // sleep able spinlock irq_put_desc_busunlock Replace with a raw lock to avoid BUGs. This lock is only used to access registers, and It's safe to replace with the raw lock without bad influence. [ 15.090359][ T1] ============================= [ 15.090365][ T1] [ BUG: Invalid wait context ] [ 15.090373][ T1] 5.10.59-rt52-00983-g186a6841c682-dirty #3 Not tainted [ 15.090386][ T1] ----------------------------- [ 15.090392][ T1] swapper/0/1 is trying to lock: [ 15.090402][ T1] 70ff00018507c188 (&gc->bgpio_lock){....}-{3:3}, at: _raw_spin_lock_irqsave+0x1c/0x28 [ 15.090470][ T1] other info that might help us debug this: [ 15.090477][ T1] context-{5:5} [ 15.090485][ T1] 3 locks held by swapper/0/1: [ 15.090497][ T1] #0: c2ff0001816de1a0 (&dev->mutex){....}-{4:4}, at: __device_driver_lock+0x98/0x104 [ 15.090553][ T1] #1: ffff90001485b4b8 (irq_domain_mutex){+.+.}-{4:4}, at: irq_domain_associate+0xbc/0x6d4 [ 15.090606][ T1] #2: 4bff000185d7a8e0 (lock_class){....}-{2:2}, at: _raw_spin_lock_irqsave+0x1c/0x28 [ 15.090654][ T1] stack backtrace: [ 15.090661][ T1] CPU: 4 PID: 1 Comm: swapper/0 Not tainted 5.10.59-rt52-00983-g186a6841c682-dirty #3 [ 15.090682][ T1] Hardware name: Horizon Robotics Journey 5 DVB (DT) [ 15.090692][ T1] Call trace: ...... [ 15.090811][ T1] _raw_spin_lock_irqsave+0x1c/0x28 [ 15.090828][ T1] dwapb_irq_ack+0xb4/0x300 [ 15.090846][ T1] __irq_do_set_handler+0x494/0xb2c [ 15.090864][ T1] __irq_set_handler+0x74/0x114 [ 15.090881][ T1] irq_set_chip_and_handler_name+0x44/0x58 [ 15.090900][ T1] gpiochip_irq_map+0x210/0x644 ...... Changelog: v1 -> v2: - Reduce the useless stacktrace. - Split to series of patches Link: https://lore.kernel.org/all/20220415165505.30383-1-schspa@gmail.com/ Signed-off-by: Schspa Shi <schspa@gmail.com> --- drivers/gpio/gpio-mmio.c | 22 +++++++++++----------- include/linux/gpio/driver.h | 2 +- 2 files changed, 12 insertions(+), 12 deletions(-)