| Message ID | 20210716210245.13240-1-alexandru.tachici@analog.com |
|---|---|
| Headers | show |
| Series | spi: spi-bcm2835: Fix deadlock | expand |
On Sat, Jul 17, 2021 at 12:02:44AM +0300, alexandru.tachici@analog.com wrote: > From: Alexandru Tachici <alexandru.tachici@analog.com> > > The bcm2835_spi_transfer_one function can create a deadlock > if it is called while another thread already has the > CCF lock. Please don't send cover letters for single patches, if there is anything that needs saying put it in the changelog of the patch or after the --- if it's administrative stuff. This reduces mail volume and ensures that any important information is recorded in the changelog rather than being lost.
On Sat, 17 Jul 2021 00:02:44 +0300, alexandru.tachici@analog.com wrote: > The bcm2835_spi_transfer_one function can create a deadlock > if it is called while another thread already has the > CCF lock. > > This behavior was observed at boot and when trying to > print the clk_summary debugfs. I had registered > at the time multiple clocks of AD9545 through the CCF. > Tested this using an RPi 4 connected to AD9545 through SPI. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/1] spi: spi-bcm2835: Fix deadlock commit: c45c1e82bba130db4f19d9dbc1deefcf4ea994ed All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
Hi all, > From: Mark Brown <broonie@kernel.org> > Sent: Tuesday, July 20, 2021 8:48 PM > To: Tachici, Alexandru <Alexandru.Tachici@analog.com>; linux- > kernel@vger.kernel.org; linux-spi@vger.kernel.org > Cc: Mark Brown <broonie@kernel.org>; nsaenz@kernel.org; > f.fainelli@gmail.com; rjui@broadcom.com; swarren@wwwdotorg.org; > bcm-kernel-feedback-list@broadcom.com; bootc@bootc.net; Sa, > Nuno <Nuno.Sa@analog.com> > Subject: Re: [PATCH 0/1] spi: spi-bcm2835: Fix deadlock > > On Sat, 17 Jul 2021 00:02:44 +0300, alexandru.tachici@analog.com > wrote: > > The bcm2835_spi_transfer_one function can create a deadlock > > if it is called while another thread already has the > > CCF lock. > > > > This behavior was observed at boot and when trying to > > print the clk_summary debugfs. I had registered > > at the time multiple clocks of AD9545 through the CCF. > > Tested this using an RPi 4 connected to AD9545 through SPI. > > > > [...] > > Applied to > > > https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/k > ernel/git/broonie/spi.git__;!!A3Ni8CS0y2Y!sBvE9XdQTgcPnOamJTAcY8 > 6Pjg5Cv-t1aDGASU9IO-JQeIPDBf5TBud6qV26eQ$ for-next > > Thanks! > > [1/1] spi: spi-bcm2835: Fix deadlock > commit: c45c1e82bba130db4f19d9dbc1deefcf4ea994ed > > All being well this means that it will be integrated into the linux-next > tree (usually sometime in the next 24 hours) and sent to Linus during > the next merge window (or sooner if it is a bug fix), however if > problems are discovered then the patch may be dropped or reverted. > > You may get further e-mails resulting from automated or manual > testing > and review of the tree, please engage with people reporting problems > and > send followup patches addressing any issues that are reported if > needed. > > If any updates are required or you are submitting further changes they > should be sent as incremental updates against current git, existing > patches will not be replaced. > > Please add any relevant lists and maintainers to the CCs when replying > to this mail. > > Thanks, > Mark I'm really curious about this one and how should we proceed. Maybe this is not new (just to me) and the way to go is just to "fix" the spi controller when we hit the issue? I'm asking this because there's a more fundamental problem when this pieces align together (CCF + SPI). What I mean is that this can potentially happen in every system that happens to have a spi based clock provider and in which the spi controller tries to access the CCF in the spi transfer function... Doing a quick and short look I can already see that [1], [2], [3] and [4] could hit the same deadlock... Honestly, I'm not sure what is the fix here since when we look individually at the pieces (CCF, SPI, SPI controller) there's nothing really wrong. The problem is when combined together... My naive thinking is that having something like 'spi_sync_nodefer();' would be a way to prevent this (or just changing 'spi_sync()' so that it can never defer the msg to the spi thread). Looking alone to ' __spi_pump_messages()' I can see that this probably not trivial though... [1]: https://elixir.bootlin.com/linux/v5.14-rc2/source/drivers/spi/spi-tegra20-slink.c#L686 [2]: https://elixir.bootlin.com/linux/latest/source/drivers/spi/spi-sun6i.c#L353 [3]: https://elixir.bootlin.com/linux/latest/source/drivers/spi/spi-sun4i.c#L271 [4]: https://elixir.bootlin.com/linux/latest/source/drivers/spi/spi-qcom-qspi.c#L237 - Nuno Sá
On Wed, Jul 21, 2021 at 06:47:01AM +0000, Sa, Nuno wrote: > > To: Tachici, Alexandru <Alexandru.Tachici@analog.com>; linux- > > kernel@vger.kernel.org; linux-spi@vger.kernel.org > > Cc: Mark Brown <broonie@kernel.org>; nsaenz@kernel.org; > > f.fainelli@gmail.com; rjui@broadcom.com; swarren@wwwdotorg.org; > > bcm-kernel-feedback-list@broadcom.com; bootc@bootc.net; Sa, > > Nuno <Nuno.Sa@analog.com> > > Subject: Re: [PATCH 0/1] spi: spi-bcm2835: Fix deadlock Please delete unneeded context from mails when replying. Doing this makes it much easier to find your reply in the message, helping ensure it won't be missed by people scrolling through the irrelevant quoted material. > I'm really curious about this one and how should we proceed. Maybe this is not > new (just to me) and the way to go is just to "fix" the spi controller when we hit the > issue? I'm asking this because there's a more fundamental problem when this pieces > align together (CCF + SPI). What I mean is that this can potentially happen in every > system that happens to have a spi based clock provider and in which the spi controller > tries to access the CCF in the spi transfer function... Doing a quick and short look I can > already see that [1], [2], [3] and [4] could hit the same deadlock... The clock API just doesn't work very well for things on buses that might sleep, I2C is another example - it's a long standing general issue that needs to be addressed in the clock framework for example with finer grained locking but nobody has come up with anything yet.
From: Alexandru Tachici <alexandru.tachici@analog.com> The bcm2835_spi_transfer_one function can create a deadlock if it is called while another thread already has the CCF lock. This behavior was observed at boot and when trying to print the clk_summary debugfs. I had registered at the time multiple clocks of AD9545 through the CCF. Tested this using an RPi 4 connected to AD9545 through SPI. See upstream attempt here: https://lore.kernel.org/lkml/20210614070718.78041-3-alexandru.tachici@analog.com/T/ This can happen to any other clock that needs to read the rate/phase from hardware using the SPI. Because when issuing a clk_get_rate/phase, the requesting thread already holds the CCF lock. If another thread, in this case the one that does the spi transfer tries the same, it will cause a deadlock. This happens by chance because not always every spi request gets deferred to a khthread. Alexandru Tachici (1): spi: spi-bcm2835: Fix deadlock drivers/spi/spi-bcm2835.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) -- 2.25.1