| Message ID | b0fb1c8837b69d56de2004dce945d0aa33d88357.1607257456.git.lukas@wunner.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [4.19-stable,1/5] spi: Introduce device-managed SPI controller allocation | expand |
On Sun, Dec 06, 2020 at 01:31:03PM +0100, Lukas Wunner wrote: > [ Upstream commit e13ee6cc4781edaf8c7321bee19217e3702ed481 ] > > bcm2835aux_spi_remove() accesses the driver's private data after calling > spi_unregister_master() even though that function releases the last > reference on the spi_master and thereby frees the private data. > > Fix by switching over to the new devm_spi_alloc_master() helper which > keeps the private data accessible until the driver has unbound. > > Fixes: b9dd3f6d4172 ("spi: bcm2835aux: Fix controller unregister order") > Signed-off-by: Lukas Wunner <lukas@wunner.de> > Cc: <stable@vger.kernel.org> # v4.4+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation > Cc: <stable@vger.kernel.org> # v4.4+: b9dd3f6d4172: spi: bcm2835aux: Fix controller unregister order > Cc: <stable@vger.kernel.org> # v4.4+ > Link: https://lore.kernel.org/r/b290b06357d0c0bdee9cecc539b840a90630f101.1605121038.git.lukas@wunner.de > Signed-off-by: Mark Brown <broonie@kernel.org> Please ensure that commit d853b3406903 ("spi: bcm2835aux: Restore err assignment in bcm2835aux_spi_probe") is picked up with this patch in all of the stable trees that it is applied to. Cheers, Nathan
On Mon, Dec 07, 2020 at 05:49:01PM -0700, Nathan Chancellor wrote: > On Sun, Dec 06, 2020 at 01:31:03PM +0100, Lukas Wunner wrote: > > [ Upstream commit e13ee6cc4781edaf8c7321bee19217e3702ed481 ] > > > > bcm2835aux_spi_remove() accesses the driver's private data after calling > > spi_unregister_master() even though that function releases the last > > reference on the spi_master and thereby frees the private data. > > > > Fix by switching over to the new devm_spi_alloc_master() helper which > > keeps the private data accessible until the driver has unbound. > > > > Fixes: b9dd3f6d4172 ("spi: bcm2835aux: Fix controller unregister order") > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > > Cc: <stable@vger.kernel.org> # v4.4+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation > > Cc: <stable@vger.kernel.org> # v4.4+: b9dd3f6d4172: spi: bcm2835aux: Fix controller unregister order > > Cc: <stable@vger.kernel.org> # v4.4+ > > Link: https://lore.kernel.org/r/b290b06357d0c0bdee9cecc539b840a90630f101.1605121038.git.lukas@wunner.de > > Signed-off-by: Mark Brown <broonie@kernel.org> > > Please ensure that commit d853b3406903 ("spi: bcm2835aux: Restore err > assignment in bcm2835aux_spi_probe") is picked up with this patch in all > of the stable trees that it is applied to. That shouldn't be necessary as I've made sure that the backports to 4.19 and earlier do not exhibit the issue fixed by d853b3406903. However, nobody is perfect, so if I've missed anything, please let me know. Thanks! Lukas
On Tue, Dec 08, 2020 at 08:32:41AM +0100, Lukas Wunner wrote: >On Mon, Dec 07, 2020 at 05:49:01PM -0700, Nathan Chancellor wrote: >> On Sun, Dec 06, 2020 at 01:31:03PM +0100, Lukas Wunner wrote: >> > [ Upstream commit e13ee6cc4781edaf8c7321bee19217e3702ed481 ] >> > >> > bcm2835aux_spi_remove() accesses the driver's private data after calling >> > spi_unregister_master() even though that function releases the last >> > reference on the spi_master and thereby frees the private data. >> > >> > Fix by switching over to the new devm_spi_alloc_master() helper which >> > keeps the private data accessible until the driver has unbound. >> > >> > Fixes: b9dd3f6d4172 ("spi: bcm2835aux: Fix controller unregister order") >> > Signed-off-by: Lukas Wunner <lukas@wunner.de> >> > Cc: <stable@vger.kernel.org> # v4.4+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation >> > Cc: <stable@vger.kernel.org> # v4.4+: b9dd3f6d4172: spi: bcm2835aux: Fix controller unregister order >> > Cc: <stable@vger.kernel.org> # v4.4+ >> > Link: https://lore.kernel.org/r/b290b06357d0c0bdee9cecc539b840a90630f101.1605121038.git.lukas@wunner.de >> > Signed-off-by: Mark Brown <broonie@kernel.org> >> >> Please ensure that commit d853b3406903 ("spi: bcm2835aux: Restore err >> assignment in bcm2835aux_spi_probe") is picked up with this patch in all >> of the stable trees that it is applied to. > >That shouldn't be necessary as I've made sure that the backports to >4.19 and earlier do not exhibit the issue fixed by d853b3406903. > >However, nobody is perfect, so if I've missed anything, please let >me know. Could we instead have the backports exhibit the issue (like they did upstream) and then take d853b3406903 on top? -- Thanks, Sasha
On Tue, Dec 08, 2020 at 08:47:39AM -0500, Sasha Levin wrote: > On Tue, Dec 08, 2020 at 08:32:41AM +0100, Lukas Wunner wrote: > > On Mon, Dec 07, 2020 at 05:49:01PM -0700, Nathan Chancellor wrote: > > > On Sun, Dec 06, 2020 at 01:31:03PM +0100, Lukas Wunner wrote: > > > > [ Upstream commit e13ee6cc4781edaf8c7321bee19217e3702ed481 ] > > > > > > > > bcm2835aux_spi_remove() accesses the driver's private data after calling > > > > spi_unregister_master() even though that function releases the last > > > > reference on the spi_master and thereby frees the private data. > > > > > > > > Fix by switching over to the new devm_spi_alloc_master() helper which > > > > keeps the private data accessible until the driver has unbound. > > > > > > > > Fixes: b9dd3f6d4172 ("spi: bcm2835aux: Fix controller unregister order") > > > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > > > > Cc: <stable@vger.kernel.org> # v4.4+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation > > > > Cc: <stable@vger.kernel.org> # v4.4+: b9dd3f6d4172: spi: bcm2835aux: Fix controller unregister order > > > > Cc: <stable@vger.kernel.org> # v4.4+ > > > > Link: https://lore.kernel.org/r/b290b06357d0c0bdee9cecc539b840a90630f101.1605121038.git.lukas@wunner.de > > > > Signed-off-by: Mark Brown <broonie@kernel.org> > > > > > > Please ensure that commit d853b3406903 ("spi: bcm2835aux: Restore err > > > assignment in bcm2835aux_spi_probe") is picked up with this patch in all > > > of the stable trees that it is applied to. > > > > That shouldn't be necessary as I've made sure that the backports to > > 4.19 and earlier do not exhibit the issue fixed by d853b3406903. > > > > However, nobody is perfect, so if I've missed anything, please let > > me know. > > Could we instead have the backports exhibit the issue (like they did > upstream) and then take d853b3406903 on top? The upstream commit e13ee6cc4781 did not apply cleanly to 4.19 and earlier, several adjustments were required. Could I have made it so that the fixup d853b3406903 would have still been required? Probably, but it seems a little silly to submit a known-bad patch. Thanks, Lukas
On Tue, Dec 08, 2020 at 06:11:45PM +0100, Lukas Wunner wrote: > On Tue, Dec 08, 2020 at 08:47:39AM -0500, Sasha Levin wrote: > > On Tue, Dec 08, 2020 at 08:32:41AM +0100, Lukas Wunner wrote: > > > On Mon, Dec 07, 2020 at 05:49:01PM -0700, Nathan Chancellor wrote: > > > > On Sun, Dec 06, 2020 at 01:31:03PM +0100, Lukas Wunner wrote: > > > > > [ Upstream commit e13ee6cc4781edaf8c7321bee19217e3702ed481 ] > > > > > > > > > > bcm2835aux_spi_remove() accesses the driver's private data after calling > > > > > spi_unregister_master() even though that function releases the last > > > > > reference on the spi_master and thereby frees the private data. > > > > > > > > > > Fix by switching over to the new devm_spi_alloc_master() helper which > > > > > keeps the private data accessible until the driver has unbound. > > > > > > > > > > Fixes: b9dd3f6d4172 ("spi: bcm2835aux: Fix controller unregister order") > > > > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > > > > > Cc: <stable@vger.kernel.org> # v4.4+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation > > > > > Cc: <stable@vger.kernel.org> # v4.4+: b9dd3f6d4172: spi: bcm2835aux: Fix controller unregister order > > > > > Cc: <stable@vger.kernel.org> # v4.4+ > > > > > Link: https://lore.kernel.org/r/b290b06357d0c0bdee9cecc539b840a90630f101.1605121038.git.lukas@wunner.de > > > > > Signed-off-by: Mark Brown <broonie@kernel.org> > > > > > > > > Please ensure that commit d853b3406903 ("spi: bcm2835aux: Restore err > > > > assignment in bcm2835aux_spi_probe") is picked up with this patch in all > > > > of the stable trees that it is applied to. > > > > > > That shouldn't be necessary as I've made sure that the backports to > > > 4.19 and earlier do not exhibit the issue fixed by d853b3406903. > > > > > > However, nobody is perfect, so if I've missed anything, please let > > > me know. > > > > Could we instead have the backports exhibit the issue (like they did > > upstream) and then take d853b3406903 on top? > > The upstream commit e13ee6cc4781 did not apply cleanly to 4.19 and earlier, > several adjustments were required. Could I have made it so that the fixup > d853b3406903 would have still been required? Probably, but it seems a > little silly to submit a known-bad patch. > > Thanks, > > Lukas I did not really look at the actual patches themselves, just the fact that I saw the commit title without my patch as a follow up in the series. If your backport avoids the issue entirely, that is fine by me. Cheers, Nathan
On Tue, Dec 08, 2020 at 06:11:45PM +0100, Lukas Wunner wrote: >On Tue, Dec 08, 2020 at 08:47:39AM -0500, Sasha Levin wrote: >> On Tue, Dec 08, 2020 at 08:32:41AM +0100, Lukas Wunner wrote: >> > On Mon, Dec 07, 2020 at 05:49:01PM -0700, Nathan Chancellor wrote: >> > > On Sun, Dec 06, 2020 at 01:31:03PM +0100, Lukas Wunner wrote: >> > > > [ Upstream commit e13ee6cc4781edaf8c7321bee19217e3702ed481 ] >> > > > >> > > > bcm2835aux_spi_remove() accesses the driver's private data after calling >> > > > spi_unregister_master() even though that function releases the last >> > > > reference on the spi_master and thereby frees the private data. >> > > > >> > > > Fix by switching over to the new devm_spi_alloc_master() helper which >> > > > keeps the private data accessible until the driver has unbound. >> > > > >> > > > Fixes: b9dd3f6d4172 ("spi: bcm2835aux: Fix controller unregister order") >> > > > Signed-off-by: Lukas Wunner <lukas@wunner.de> >> > > > Cc: <stable@vger.kernel.org> # v4.4+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation >> > > > Cc: <stable@vger.kernel.org> # v4.4+: b9dd3f6d4172: spi: bcm2835aux: Fix controller unregister order >> > > > Cc: <stable@vger.kernel.org> # v4.4+ >> > > > Link: https://lore.kernel.org/r/b290b06357d0c0bdee9cecc539b840a90630f101.1605121038.git.lukas@wunner.de >> > > > Signed-off-by: Mark Brown <broonie@kernel.org> >> > > >> > > Please ensure that commit d853b3406903 ("spi: bcm2835aux: Restore err >> > > assignment in bcm2835aux_spi_probe") is picked up with this patch in all >> > > of the stable trees that it is applied to. >> > >> > That shouldn't be necessary as I've made sure that the backports to >> > 4.19 and earlier do not exhibit the issue fixed by d853b3406903. >> > >> > However, nobody is perfect, so if I've missed anything, please let >> > me know. >> >> Could we instead have the backports exhibit the issue (like they did >> upstream) and then take d853b3406903 on top? > >The upstream commit e13ee6cc4781 did not apply cleanly to 4.19 and earlier, >several adjustments were required. Could I have made it so that the fixup >d853b3406903 would have still been required? Probably, but it seems a >little silly to submit a known-bad patch. Not silly, there are two motives here: 1. We don't want to diverge too much with backports, which means that they need to be minimal. 2. It'll make auditing later easier. What will happen now is that after this patch is merges, we'll trigger a warning saying that there's a fix upstream for one of these patches, and we'll end up wasting the time (of probably a few folks) figuring this out. Note I'm not asking to submit a broken patch, but I'm asking to submit a minimal backport followed by the upstream fix to that upstream bug :) -- Thanks, Sasha
On Tue, Dec 08, 2020 at 04:17:45PM -0500, Sasha Levin wrote: > On Tue, Dec 08, 2020 at 06:11:45PM +0100, Lukas Wunner wrote: > > On Tue, Dec 08, 2020 at 08:47:39AM -0500, Sasha Levin wrote: > > > Could we instead have the backports exhibit the issue (like they did > > > upstream) and then take d853b3406903 on top? > > > > The upstream commit e13ee6cc4781 did not apply cleanly to 4.19 and earlier, > > several adjustments were required. Could I have made it so that the fixup > > d853b3406903 would have still been required? Probably, but it seems a > > little silly to submit a known-bad patch. [...] > 2. It'll make auditing later easier. What will happen now is that after > this patch is merges, we'll trigger a warning saying that there's a fix > upstream for one of these patches, and we'll end up wasting the time (of > probably a few folks) figuring this out. Would it be possible to amend the tooling such that multiple "[ Upstream commit ... ]" lines are supported at the top of the commit message, signifying that the backport patch subsumes all cited upstream commits? Could the extra work for stable maintainers be avoided that way? I imagine there might be more cases where a "clean" backport is not possible, requiring multiple upstream patches to be combined. > Note I'm not asking to submit a broken patch, but I'm asking to submit a > minimal backport followed by the upstream fix to that upstream bug :) Then please apply the series sans bcm2835aux patch and I'll follow up with a two-patch series specifically for that driver. Alternatively, please consider whether multiple "[ Upstream commit ... ]" lines would be a viable solution and if it is, add a line as follows when applying the bcm2835aux patch: [ Upstream commit d853b3406903a7dc5b14eb5bada3e8cd677f66a2 ] Thanks, Lukas
On Wed, Dec 09, 2020 at 09:37:47AM +0100, Lukas Wunner wrote: > On Tue, Dec 08, 2020 at 04:17:45PM -0500, Sasha Levin wrote: > > On Tue, Dec 08, 2020 at 06:11:45PM +0100, Lukas Wunner wrote: > > > On Tue, Dec 08, 2020 at 08:47:39AM -0500, Sasha Levin wrote: > > > > Could we instead have the backports exhibit the issue (like they did > > > > upstream) and then take d853b3406903 on top? > > > > > > The upstream commit e13ee6cc4781 did not apply cleanly to 4.19 and earlier, > > > several adjustments were required. Could I have made it so that the fixup > > > d853b3406903 would have still been required? Probably, but it seems a > > > little silly to submit a known-bad patch. > [...] > > 2. It'll make auditing later easier. What will happen now is that after > > this patch is merges, we'll trigger a warning saying that there's a fix > > upstream for one of these patches, and we'll end up wasting the time (of > > probably a few folks) figuring this out. > > Would it be possible to amend the tooling such that multiple > "[ Upstream commit ... ]" lines are supported at the top of > the commit message, signifying that the backport patch > subsumes all cited upstream commits? You could, but that's extra work, which I think you are trying to avoid when you say: > Could the extra work for stable maintainers be avoided that way? this :) > I imagine there might be more cases where a "clean" backport is > not possible, requiring multiple upstream patches to be combined. It is quite rare. And again, it's almost always better to just take all of the patches involved, as individual patches, that way we "know" we did it right, and it's easier to track and audit and review that way. > > Note I'm not asking to submit a broken patch, but I'm asking to submit a > > minimal backport followed by the upstream fix to that upstream bug :) > > Then please apply the series sans bcm2835aux patch and I'll follow up > with a two-patch series specifically for that driver. Can you just resend the whole series so we know we got it correct? thanks, greg k-h
On Wed, Dec 09, 2020 at 10:36:55AM +0100, Greg KH wrote: > On Wed, Dec 09, 2020 at 09:37:47AM +0100, Lukas Wunner wrote: > > Then please apply the series sans bcm2835aux patch and I'll follow up > > with a two-patch series specifically for that driver. > > Can you just resend the whole series so we know we got it correct? The other patches in the series do not depend on the bcm2835aux patch, so you can apply them independently. Thanks, Lukas
On Wed, Dec 09, 2020 at 10:38:18AM +0100, Lukas Wunner wrote: > On Wed, Dec 09, 2020 at 10:36:55AM +0100, Greg KH wrote: > > On Wed, Dec 09, 2020 at 09:37:47AM +0100, Lukas Wunner wrote: > > > Then please apply the series sans bcm2835aux patch and I'll follow up > > > with a two-patch series specifically for that driver. > > > > Can you just resend the whole series so we know we got it correct? > > The other patches in the series do not depend on the bcm2835aux patch, > so you can apply them independently. Ok, so I need to drop this patch from all of the other series you sent out? You can see how this is getting messy from my side :) thanks, greg k-h
On Wed, Dec 09, 2020 at 10:44:59AM +0100, Greg KH wrote: > On Wed, Dec 09, 2020 at 10:38:18AM +0100, Lukas Wunner wrote: > > On Wed, Dec 09, 2020 at 10:36:55AM +0100, Greg KH wrote: > > > On Wed, Dec 09, 2020 at 09:37:47AM +0100, Lukas Wunner wrote: > > > > Then please apply the series sans bcm2835aux patch and I'll follow up > > > > with a two-patch series specifically for that driver. > > > > > > Can you just resend the whole series so we know we got it correct? > > > > The other patches in the series do not depend on the bcm2835aux patch, > > so you can apply them independently. > > Ok, so I need to drop this patch from all of the other series you sent > out? You can see how this is getting messy from my side :) Is this workflow description still up-to-date? http://kroah.com/log/blog/2019/08/14/patch-workflow-with-mutt-2019/ So you just select all patches in Mutt sans the bcm2835aux one and apply them? No I don't see how this is getting messy. Thanks, Lukas
On Thu, Dec 10, 2020 at 08:01:42AM +0100, Lukas Wunner wrote: > On Wed, Dec 09, 2020 at 10:44:59AM +0100, Greg KH wrote: > > On Wed, Dec 09, 2020 at 10:38:18AM +0100, Lukas Wunner wrote: > > > On Wed, Dec 09, 2020 at 10:36:55AM +0100, Greg KH wrote: > > > > On Wed, Dec 09, 2020 at 09:37:47AM +0100, Lukas Wunner wrote: > > > > > Then please apply the series sans bcm2835aux patch and I'll follow up > > > > > with a two-patch series specifically for that driver. > > > > > > > > Can you just resend the whole series so we know we got it correct? > > > > > > The other patches in the series do not depend on the bcm2835aux patch, > > > so you can apply them independently. > > > > Ok, so I need to drop this patch from all of the other series you sent > > out? You can see how this is getting messy from my side :) > > Is this workflow description still up-to-date? > > http://kroah.com/log/blog/2019/08/14/patch-workflow-with-mutt-2019/ > > So you just select all patches in Mutt sans the bcm2835aux one > and apply them? > > No I don't see how this is getting messy. The less "special" steps I have to make, the less chance I mess something up :) I've queued these up now, please check that I got it right. thanks, greg k-h
On Thu, Dec 10, 2020 at 01:45:19PM +0100, Greg KH wrote: > The less "special" steps I have to make, the less chance I mess > something up :) > > I've queued these up now, please check that I got it right. Perfect, thank you. I'll follow up with a respin of the bcm2835aux patch. Lukas
On Thu, Dec 10, 2020 at 05:13:36PM +0100, Lukas Wunner wrote: > On Thu, Dec 10, 2020 at 01:45:19PM +0100, Greg KH wrote: > > The less "special" steps I have to make, the less chance I mess > > something up :) > > > > I've queued these up now, please check that I got it right. > > Perfect, thank you. I'll follow up with a respin of the bcm2835aux patch. Okay I've just sent out a series consisting of 2 patches. That series applies cleanly to all of 4.19, 4.14, 4.9 and 4.4-stable. Thanks, Lukas
diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c index 11895c98aae3..8ea7e31b8c2f 100644 --- a/drivers/spi/spi-bcm2835aux.c +++ b/drivers/spi/spi-bcm2835aux.c @@ -407,7 +407,7 @@ static int bcm2835aux_spi_probe(struct platform_device *pdev) unsigned long clk_hz; int err; - master = spi_alloc_master(&pdev->dev, sizeof(*bs)); + master = devm_spi_alloc_master(&pdev->dev, sizeof(*bs)); if (!master) { dev_err(&pdev->dev, "spi_alloc_master() failed\n"); return -ENOMEM; @@ -439,30 +439,27 @@ static int bcm2835aux_spi_probe(struct platform_device *pdev) /* the main area */ res = platform_get_resource(pdev, IORESOURCE_MEM, 0); bs->regs = devm_ioremap_resource(&pdev->dev, res); - if (IS_ERR(bs->regs)) { - err = PTR_ERR(bs->regs); - goto out_master_put; - } + if (IS_ERR(bs->regs)) + return PTR_ERR(bs->regs); bs->clk = devm_clk_get(&pdev->dev, NULL); if ((!bs->clk) || (IS_ERR(bs->clk))) { err = PTR_ERR(bs->clk); dev_err(&pdev->dev, "could not get clk: %d\n", err); - goto out_master_put; + return err; } bs->irq = platform_get_irq(pdev, 0); if (bs->irq <= 0) { dev_err(&pdev->dev, "could not get IRQ: %d\n", bs->irq); - err = bs->irq ? bs->irq : -ENODEV; - goto out_master_put; + return bs->irq ? bs->irq : -ENODEV; } /* this also enables the HW block */ err = clk_prepare_enable(bs->clk); if (err) { dev_err(&pdev->dev, "could not prepare clock: %d\n", err); - goto out_master_put; + return err; } /* just checking if the clock returns a sane value */ @@ -495,8 +492,6 @@ static int bcm2835aux_spi_probe(struct platform_device *pdev) out_clk_disable: clk_disable_unprepare(bs->clk); -out_master_put: - spi_master_put(master); return err; }
[ Upstream commit e13ee6cc4781edaf8c7321bee19217e3702ed481 ] bcm2835aux_spi_remove() accesses the driver's private data after calling spi_unregister_master() even though that function releases the last reference on the spi_master and thereby frees the private data. Fix by switching over to the new devm_spi_alloc_master() helper which keeps the private data accessible until the driver has unbound. Fixes: b9dd3f6d4172 ("spi: bcm2835aux: Fix controller unregister order") Signed-off-by: Lukas Wunner <lukas@wunner.de> Cc: <stable@vger.kernel.org> # v4.4+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation Cc: <stable@vger.kernel.org> # v4.4+: b9dd3f6d4172: spi: bcm2835aux: Fix controller unregister order Cc: <stable@vger.kernel.org> # v4.4+ Link: https://lore.kernel.org/r/b290b06357d0c0bdee9cecc539b840a90630f101.1605121038.git.lukas@wunner.de Signed-off-by: Mark Brown <broonie@kernel.org> --- drivers/spi/spi-bcm2835aux.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)