From: Bartosz Golaszewski <brgl@bgdev.pl>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Andy Shevchenko <andy@kernel.org>,
linux-pwm@vger.kernel.org,
Linus Walleij <linus.walleij@linaro.org>,
linux-gpio@vger.kernel.org,
Thierry Reding <thierry.reding@gmail.com>,
kernel@pengutronix.de
Subject: Re: [PATCH v3 100/108] gpio: mvebu: Make use of devm_pwmchip_alloc() function
Date: Mon, 4 Dec 2023 22:28:15 +0100 [thread overview]
Message-ID: <CAMRc=MeiL7omJhe_gb_UiL_bbJGoujp3Js4=Fe=qXKHv8TjM-Q@mail.gmail.com> (raw)
In-Reply-To: <CAMRc=Md9bRdk5ZaCVu5gmZ4r5JR7eVZDriap32FMrN05PyTENQ@mail.gmail.com>
On Mon, Dec 4, 2023 at 9:27 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Sat, Dec 2, 2023 at 1:43 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > On Fri, Dec 01, 2023 at 11:14:32AM +0100, Bartosz Golaszewski wrote:
> > > On Tue, Nov 28, 2023 at 10:07 AM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > >
> > >
> > > [snip]
> > >
> > > >
> > > > > I see the
> > > > > chip->operational field that is set to false on release. In my
> > > > > version, we just use a NULL-pointer to carry the same information.
> > > >
> > > > Yup, sounds obvious. Your usage of "just" sounds as if your variant was
> > > > better. To give the alternative view where the suggested approach sounds
> > > > better would be:
> > > >
> > > > You need a pointer and I "just" a bool that even has a name implying its
> > > > function. You need to dereference the pointer in several places as the
> > > > needed information is distributed over two structures while it's all
> > > > together in a single struct for the usual foo_alloc() + foo_register()
> > > > approach.
> > > >
> > >
> > > There's another reason we do that. I'm no longer sure if I mentioned
> > > it in my talk (I meant to anyway).
> > >
> > > In GPIO we have API functions that may be called from any context -
> > > thus needing spinlocks for locking - but also driver callbacks that
> > > may use mutexes internally or otherwise sleep. I don't know if this is
> > > the case for PWM too but in GPIO we may end up in a situation where if
> > > we used a spinlock to protect some kind of an "is_operational" field,
> > > we'd end up sleeping with a spinlock taken and if we used a mutex, we
> > > couldn't use API function from atomic contexts.
> > >
> > > This is the reason behind locking being so broken in GPIO at the
> > > moment and why I'm trying to fix it this release cycle.
> > >
> > > Splitting the implementation into two structures and protecting the
> > > pointer to the provider structure with SRCU has the benefit of not
> > > limiting us in what locks we use underneath.
> > >
> > > Every subsystem has its own issues and we need to find something
> > > generic enough to cover them all (or most of them anyway). I don't
> > > think having a single structure cuts it.
> >
> > I'm convinced it works. I introduced a wrapper pwmchip_lock() that for
> > now uses a mutex and once we have fast pwm_chips it uses a mutex for
> > sleeping pwm_chips and a spinlock for the fast ones.
> >
> > That's similar to how struct irq_chip::irq_bus_lock works. For sleeping
> > chips that callback uses a mutex, for fast chips a spinlock.
> >
>
> Fair enough. I'd love to see a benchmark of what's faster one day
> though: two structures with dereferencing and SRCU or one structure
> with mutex/spinlock.
>
Actually there is one thing that - while not technically wrong - makes
the split solution better. In case of your abstracted lock, you find
yourself in a very all-or-nothing locking situation, where all of the
structure is locked or none is. With SRCU protecting just the pointer
to implementation, we can easily factor that part out and leave
whatever fine-grained locking is required to the subsystem.
Additionally: the pointer to implementation has many readers but only
one writer. I believe this to be the same for your "operational"
field. I don't know the PWM code very well but I can only guess that
the situation is similar, where subsystem data structures are read
more often than they are modified and multiple readers could access
the structure at the same time lowering latencies.
Just another 2 cents.
Bart
> By "fair enough" I mean: I still don't like it for the reasons I
> mentioned before but I cannot point out anything technically wrong.
>
> Bart
>
> > Best regards
> > Uwe
> >
> > --
> > Pengutronix e.K. | Uwe Kleine-König |
> > Industrial Linux Solutions | https://www.pengutronix.de/ |
next prev parent reply other threads:[~2023-12-04 21:28 UTC|newest]
Thread overview: 163+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-21 13:49 [PATCH v3 000/108] pwm: Fix lifetime issues for pwm_chips Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 001/108] pwm: cros-ec: Change prototype of helper to prepare further changes Uwe Kleine-König
2023-11-22 8:52 ` Tzung-Bi Shih
2023-11-23 9:24 ` Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 002/108] pwm: Provide a macro to get the parent device of a given chip Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 003/108] pwm: ab8500: Make use of pwmchip_parent() macro Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 004/108] pwm: atmel: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 005/108] pwm: atmel-tcb: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 006/108] pwm: bcm-kona: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 007/108] pwm: crc: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 008/108] pwm: cros-ec: " Uwe Kleine-König
2023-11-22 8:52 ` Tzung-Bi Shih
2023-11-21 13:49 ` [PATCH v3 009/108] pwm: dwc: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 010/108] pwm: ep93xx: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 011/108] pwm: fsl-ftm: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 012/108] pwm: img: Make use of parent device pointer in driver data Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 013/108] pwm: imx27: Make use of pwmchip_parent() macro Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 014/108] pwm: jz4740: " Uwe Kleine-König
2023-11-21 14:13 ` Paul Cercueil
2023-11-21 15:51 ` Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 015/108] pwm: lpc18xx-sct: Make use of parent device pointer in driver data Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 016/108] pwm: lpss: Make use of pwmchip_parent() macro Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 017/108] pwm: mediatek: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 018/108] pwm: meson: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 019/108] pwm: mtk-disp: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 020/108] pwm: omap: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 021/108] pwm: pca9685: Store parent device in driver data Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 022/108] pwm: raspberrypi-poe: Make use of pwmchip_parent() macro Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 023/108] pwm: rcar: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 024/108] pwm: rz-mtu3: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 025/108] pwm: samsung: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 026/108] pwm: sifive: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 027/108] pwm: stm32-lp: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 028/108] pwm: stm32: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 029/108] pwm: stmpe: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 030/108] pwm: sun4i: " Uwe Kleine-König
2023-11-25 18:48 ` Jernej Škrabec
2023-11-21 13:49 ` [PATCH v3 031/108] pwm: tiecap: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 032/108] pwm: tiehrpwm: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 033/108] pwm: twl-led: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 034/108] pwm: twl: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 035/108] pwm: vt8500: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 036/108] staging: greybus: pwm: " Uwe Kleine-König
2023-11-23 12:55 ` Greg Kroah-Hartman
2023-11-21 13:49 ` [PATCH v3 037/108] pwm: Provide devm_pwmchip_alloc() function Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 038/108] pwm: ab8500: Make use of " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 039/108] pwm: apple: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 040/108] pwm: atmel-hlcdc: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 041/108] pwm: atmel: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 042/108] pwm: atmel-tcb: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 043/108] pwm: bcm2835: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 044/108] pwm: bcm-iproc: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 045/108] pwm: bcm-kona: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 046/108] pwm: berlin: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 047/108] pwm: brcmstb: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 048/108] pwm: clk: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 049/108] pwm: clps711x: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 050/108] pwm: crc: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 051/108] pwm: cros-ec: " Uwe Kleine-König
2023-11-22 8:52 ` Tzung-Bi Shih
2023-11-22 23:56 ` Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 052/108] pwm: dwc: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 053/108] pwm: ep93xx: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 054/108] pwm: fsl-ftm: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 055/108] pwm: hibvt: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 056/108] pwm: img: " Uwe Kleine-König
2023-11-21 13:49 ` [PATCH v3 057/108] pwm: imx1: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 058/108] pwm: imx27: " Uwe Kleine-König
2023-12-05 11:49 ` Philipp Zabel
2023-12-05 12:14 ` Uwe Kleine-König
2023-12-05 12:50 ` Philipp Zabel
2023-11-21 13:50 ` [PATCH v3 059/108] pwm: imx-tpm: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 060/108] pwm: intel-lgm: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 061/108] pwm: iqs620a: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 062/108] pwm: jz4740: " Uwe Kleine-König
2023-11-21 14:16 ` Paul Cercueil
2023-11-21 13:50 ` [PATCH v3 063/108] pwm: keembay: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 064/108] pwm: lp3943: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 065/108] pwm: lpc18xx-sct: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 066/108] pwm: lpc32xx: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 067/108] pwm: lpss-*: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 068/108] pwm: mediatek: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 069/108] pwm: meson: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 070/108] pwm: microchip-core: " Uwe Kleine-König
2023-11-22 11:14 ` Conor Dooley
2023-11-22 22:48 ` Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 071/108] pwm: mtk-disp: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 072/108] pwm: mxs: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 073/108] pwm: ntxec: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 074/108] pwm: omap-dmtimer: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 075/108] pwm: pca9685: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 076/108] pwm: pxa: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 077/108] pwm: raspberrypi-poe: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 078/108] pwm: rcar: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 079/108] pwm: renesas-tpu: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 080/108] pwm: rockchip: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 081/108] pwm: rz-mtu3: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 082/108] pwm: samsung: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 083/108] pwm: sifive: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 084/108] pwm: sl28cpld: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 085/108] pwm: spear: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 086/108] pwm: sprd: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 087/108] pwm: sti: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 088/108] pwm: stm32-lp: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 089/108] pwm: stm32: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 090/108] pwm: stmpe: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 091/108] pwm: sun4i: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 092/108] pwm: sunplus: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 093/108] pwm: tegra: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 094/108] pwm: tiecap: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 095/108] pwm: twl-led: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 096/108] pwm: twl: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 097/108] pwm: visconti: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 098/108] pwm: vt8500: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 099/108] pwm: xilinx: " Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 100/108] gpio: mvebu: " Uwe Kleine-König
2023-11-21 14:02 ` Bartosz Golaszewski
2023-11-21 16:11 ` Uwe Kleine-König
2023-11-22 9:05 ` Uwe Kleine-König
2023-11-22 10:36 ` Bartosz Golaszewski
2023-11-22 23:39 ` Uwe Kleine-König
2023-11-24 12:14 ` Thierry Reding
2023-11-24 21:16 ` Bartosz Golaszewski
2023-11-24 21:59 ` Uwe Kleine-König
2023-11-27 10:58 ` Uwe Kleine-König
2023-11-27 20:22 ` Bartosz Golaszewski
2023-11-28 9:07 ` Uwe Kleine-König
2023-12-01 10:14 ` Bartosz Golaszewski
2023-12-02 0:43 ` Uwe Kleine-König
2023-12-04 20:27 ` Bartosz Golaszewski
2023-12-04 21:28 ` Bartosz Golaszewski [this message]
2023-12-05 9:31 ` Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 101/108] drm/bridge: ti-sn65dsi86: " Uwe Kleine-König
2023-11-21 15:15 ` Doug Anderson
2023-11-21 16:05 ` Uwe Kleine-König
2023-11-21 16:14 ` Doug Anderson
2023-11-23 9:17 ` Uwe Kleine-König
2023-11-27 9:32 ` Uwe Kleine-König
2023-11-23 9:46 ` Laurent Pinchart
2023-11-23 10:10 ` Uwe Kleine-König
2023-12-06 12:06 ` Laurent Pinchart
2023-12-06 14:23 ` Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 102/108] leds: qcom-lpg: " Uwe Kleine-König
2023-11-21 15:16 ` Lee Jones
2023-11-21 15:58 ` Uwe Kleine-König
2023-11-22 11:56 ` Lee Jones
2023-11-22 17:15 ` Thierry Reding
2023-11-23 10:44 ` Uwe Kleine-König
2023-11-24 12:27 ` Thierry Reding
2023-11-24 18:22 ` Uwe Kleine-König
2023-11-24 21:21 ` Bartosz Golaszewski
2023-11-22 17:54 ` Uwe Kleine-König
2023-11-23 10:21 ` Lee Jones
2023-11-23 10:54 ` Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 104/108] pwm: Ensure that pwm_chips are allocated using pwmchip_alloc() Uwe Kleine-König
2023-11-22 17:19 ` Thierry Reding
2023-11-22 23:52 ` Uwe Kleine-König
2023-11-24 12:28 ` Thierry Reding
2023-11-21 13:50 ` [PATCH v3 105/108] pwm: Ensure a struct pwm has the same lifetime as its pwm_chip Uwe Kleine-König
2023-11-21 15:53 ` Gustavo A. R. Silva
2023-11-21 13:50 ` [PATCH v3 106/108] pwm: Ensure the memory backing a PWM chip isn't freed while used Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 107/108] pwm: Add more locking Uwe Kleine-König
2023-11-21 13:50 ` [PATCH v3 108/108] WIP: pwm: Add support for pwmchip devices for faster and easier userspace access Uwe Kleine-König
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMRc=MeiL7omJhe_gb_UiL_bbJGoujp3Js4=Fe=qXKHv8TjM-Q@mail.gmail.com' \
--to=brgl@bgdev.pl \
--cc=andy@kernel.org \
--cc=kernel@pengutronix.de \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=thierry.reding@gmail.com \
--cc=u.kleine-koenig@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).