mbox series

[v3,0/5] amba: minor fix and various cleanups

Message ID 20210126165835.687514-1-u.kleine-koenig@pengutronix.de (mailing list archive)
Headers show
Series amba: minor fix and various cleanups | expand

Message

Uwe Kleine-König Jan. 26, 2021, 4:58 p.m. UTC
From: Uwe Kleine-König <u.kleine-koenig.org@pengutronix.de

Hello,

Changes since v2 sent with Message-Id:
20201124133139.3072124-1-uwe@kleine-koenig.org:

 - Rebase to v5.11-rc1 (which resulted in a few conflicts in
   drivers/hwtracing).
 - Add various Acks.
 - Send to more maintainers directly (which I think is one of the
   reasons why there are so few Acks).

For my taste patch 4 needs some more acks (drivers/char/hw_random,
drivers/dma, drivers/gpu/drm/pl111, drivers/i2c, drivers/mmc,
drivers/vfio, drivers/watchdog and sound/arm have no maintainer feedback
yet).

My suggestion is to let this series go in via Russell King (who cares
for amba). Once enough Acks are there I can also provide a tag for
merging into different trees. Just tell me if you prefer this solution.

Would be great if this could make it for v5.12, but I'm aware it's
already late in the v5.11 cycle so it might have to wait for v5.13.

Best regards
Uwe

Uwe Kleine-König (5):
  amba: Fix resource leak for drivers without .remove
  amba: reorder functions
  vfio: platform: simplify device removal
  amba: Make the remove callback return void
  amba: Make use of bus_type functions

 drivers/amba/bus.c                            | 234 +++++++++---------
 drivers/char/hw_random/nomadik-rng.c          |   3 +-
 drivers/dma/pl330.c                           |   3 +-
 drivers/gpu/drm/pl111/pl111_drv.c             |   4 +-
 drivers/hwtracing/coresight/coresight-catu.c  |   3 +-
 .../hwtracing/coresight/coresight-cpu-debug.c |   4 +-
 .../hwtracing/coresight/coresight-cti-core.c  |   4 +-
 drivers/hwtracing/coresight/coresight-etb10.c |   4 +-
 .../coresight/coresight-etm3x-core.c          |   4 +-
 .../coresight/coresight-etm4x-core.c          |   4 +-
 .../hwtracing/coresight/coresight-funnel.c    |   4 +-
 .../coresight/coresight-replicator.c          |   4 +-
 drivers/hwtracing/coresight/coresight-stm.c   |   4 +-
 .../hwtracing/coresight/coresight-tmc-core.c  |   4 +-
 drivers/hwtracing/coresight/coresight-tpiu.c  |   4 +-
 drivers/i2c/busses/i2c-nomadik.c              |   4 +-
 drivers/input/serio/ambakmi.c                 |   3 +-
 drivers/memory/pl172.c                        |   4 +-
 drivers/memory/pl353-smc.c                    |   4 +-
 drivers/mmc/host/mmci.c                       |   4 +-
 drivers/rtc/rtc-pl030.c                       |   4 +-
 drivers/rtc/rtc-pl031.c                       |   4 +-
 drivers/spi/spi-pl022.c                       |   5 +-
 drivers/tty/serial/amba-pl010.c               |   4 +-
 drivers/tty/serial/amba-pl011.c               |   3 +-
 drivers/vfio/platform/vfio_amba.c             |  15 +-
 drivers/video/fbdev/amba-clcd.c               |   4 +-
 drivers/watchdog/sp805_wdt.c                  |   4 +-
 include/linux/amba/bus.h                      |   2 +-
 sound/arm/aaci.c                              |   4 +-
 30 files changed, 157 insertions(+), 198 deletions(-)


base-commit: 5c8fe583cce542aa0b84adc939ce85293de36e5e

Comments

Vinod Koul Jan. 26, 2021, 5:30 p.m. UTC | #1
On 26-01-21, 17:58, Uwe Kleine-König wrote:
> All amba drivers return 0 in their remove callback. Together with the
> driver core ignoring the return value anyhow, it doesn't make sense to
> return a value here.
> 
> Change the remove prototype to return void, which makes it explicit that
> returning an error value doesn't work as expected. This simplifies changing
> the core remove callback to return void, too.
> 
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Acked-by: Krzysztof Kozlowski <krzk@kernel.org> # for drivers/memory
> Acked-by: Mark Brown <broonie@kernel.org>
> Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/amba/bus.c                                 | 5 ++---
>  drivers/char/hw_random/nomadik-rng.c               | 3 +--
>  drivers/dma/pl330.c                                | 3 +--

For dmaengine:

Acked-By: Vinod Koul <vkoul@kernel.org>
Uwe Kleine-König Jan. 26, 2021, 5:56 p.m. UTC | #2
Hello,

On Tue, Jan 26, 2021 at 05:08:40PM +0000, Suzuki K Poulose wrote:
> On 1/26/21 4:58 PM, Uwe Kleine-König wrote:
> > All amba drivers return 0 in their remove callback. Together with the
> > driver core ignoring the return value anyhow, it doesn't make sense to
> > return a value here.
> > 
> > Change the remove prototype to return void, which makes it explicit that
> > returning an error value doesn't work as expected. This simplifies changing
> > the core remove callback to return void, too.
> > 
> > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> > Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Acked-by: Krzysztof Kozlowski <krzk@kernel.org> # for drivers/memory
> > Acked-by: Mark Brown <broonie@kernel.org>
>  > Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Acked-by: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> 
> >   drivers/hwtracing/coresight/coresight-etm4x-core.c | 4 +---
> 
> You are most likely to have a conflict for the above file, with what is
> in coresight/next. It should be easy to resolve.

I'm surprised to see that the remove callback introduced in 2952ecf5df33
("coresight: etm4x: Refactor probing routine") has an __exit annotation.

With .suppress_bind_attrs = true you don't need a remove callback at
all. (And without .suppress_bind_attrs = true the remove callback must
have no __exit annotation.)

This make me looking at commit 45fe7befe0db ("coresight: remove broken
__exit annotations") by Arnd. Unless I miss something the better change
would have been to remove the unused remove callbacks instead of dropping
their __exit annotation?!

Anyhow, my conflict resolution looks as follows:

diff --cc drivers/hwtracing/coresight/coresight-etm4x-core.c
index 82787cba537d,473ab7480a36..000000000000
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@@ -1703,6 -1903,28 +1903,27 @@@ static int __exit etm4_remove_dev(struc
  	cpus_read_unlock();
  
  	coresight_unregister(drvdata->csdev);
+ 
+ 	return 0;
+ }
+ 
 -static int __exit etm4_remove_amba(struct amba_device *adev)
++static void __exit etm4_remove_amba(struct amba_device *adev)
+ {
+ 	struct etmv4_drvdata *drvdata = dev_get_drvdata(&adev->dev);
+ 
+ 	if (drvdata)
 -		return etm4_remove_dev(drvdata);
 -	return 0;
++		etm4_remove_dev(drvdata);
+ }
+ 
+ static int __exit etm4_remove_platform_dev(struct platform_device *pdev)
+ {
+ 	int ret = 0;
+ 	struct etmv4_drvdata *drvdata = dev_get_drvdata(&pdev->dev);
+ 
+ 	if (drvdata)
+ 		ret = etm4_remove_dev(drvdata);
+ 	pm_runtime_disable(&pdev->dev);
+ 	return ret;
  }
  
  static const struct amba_id etm4_ids[] = {

If this series should make it in for 5.12 we probably need an immutable
branch between hwtracing and amba.

> Otherwise, the changes look good for the drivers/hwtracing/coresight/*
> 
> Acked-by: Suzuki K Poulose <suzuki.poulose@arm.com>

Thanks
Uwe
Russell King (Oracle) Jan. 26, 2021, 7:05 p.m. UTC | #3
On Tue, Jan 26, 2021 at 06:56:52PM +0100, Uwe Kleine-König wrote:
> I'm surprised to see that the remove callback introduced in 2952ecf5df33
> ("coresight: etm4x: Refactor probing routine") has an __exit annotation.

In general, remove callbacks should not have an __exit annotation.
__exit _can_ be discarded at link time for built-in stuff.
Russell King (Oracle) Feb. 2, 2021, 10:49 a.m. UTC | #4
On Tue, Jan 26, 2021 at 05:58:30PM +0100, Uwe Kleine-König wrote:
> From: Uwe Kleine-König <u.kleine-koenig.org@pengutronix.de
> 
> Hello,
> 
> Changes since v2 sent with Message-Id:
> 20201124133139.3072124-1-uwe@kleine-koenig.org:
> 
>  - Rebase to v5.11-rc1 (which resulted in a few conflicts in
>    drivers/hwtracing).
>  - Add various Acks.
>  - Send to more maintainers directly (which I think is one of the
>    reasons why there are so few Acks).
> 
> For my taste patch 4 needs some more acks (drivers/char/hw_random,
> drivers/dma, drivers/gpu/drm/pl111, drivers/i2c, drivers/mmc,
> drivers/vfio, drivers/watchdog and sound/arm have no maintainer feedback
> yet).
> 
> My suggestion is to let this series go in via Russell King (who cares
> for amba). Once enough Acks are there I can also provide a tag for
> merging into different trees. Just tell me if you prefer this solution.
> 
> Would be great if this could make it for v5.12, but I'm aware it's
> already late in the v5.11 cycle so it might have to wait for v5.13.

I think you need to have a 6th patch which moves the
probe/remove/shutdown methods into the bus_type - if you're setting
them for every struct device_driver, then there's no point doing that
and they may as well be in the bus_type.

Apart from that, it looks good.
Greg KH Feb. 2, 2021, 2:06 p.m. UTC | #5
On Tue, Feb 02, 2021 at 02:53:50PM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> the following changes since commit 5c8fe583cce542aa0b84adc939ce85293de36e5e:
> 
>   Linux 5.11-rc1 (2020-12-27 15:30:22 -0800)
> 
> are available in the Git repository at:
> 
>   https://git.pengutronix.de/git/ukl/linux tags/amba-make-remove-return-void
> 
> for you to fetch changes up to f170b59fedd733b92f58c4d7c8357fbf7601d623:
> 
>   amba: Make use of bus_type functions (2021-02-02 14:26:02 +0100)
> 
> I expect this tag to be merged by Russell King as amba maintainer and by
> Mathieu Poirier (or Greg Kroah-Hartman?) for coresight as there are some
> pending conflicting changes. These are not hard to resolve but also
> non-trivial. Tell me if you need assistance for resolving, also if it's only a
> second pair of eyes to judge your resolution.
> 
> Best regards,
> Uwe
> 
> ----------------------------------------------------------------
> Tag for adaptions to struct amba_driver::remove changing prototype
> 
> ----------------------------------------------------------------
> Uwe Kleine-König (5):
>       amba: Fix resource leak for drivers without .remove
>       amba: reorder functions
>       vfio: platform: simplify device removal
>       amba: Make the remove callback return void
>       amba: Make use of bus_type functions
> 
>  drivers/amba/bus.c                                 | 234 +++++++++++++++++++++++++++++++++------------------------------
>  drivers/char/hw_random/nomadik-rng.c               |   3 +-
>  drivers/dma/pl330.c                                |   3 +-
>  drivers/gpu/drm/pl111/pl111_drv.c                  |   4 +-
>  drivers/hwtracing/coresight/coresight-catu.c       |   3 +-
>  drivers/hwtracing/coresight/coresight-cpu-debug.c  |   4 +-
>  drivers/hwtracing/coresight/coresight-cti-core.c   |   4 +-
>  drivers/hwtracing/coresight/coresight-etb10.c      |   4 +-
>  drivers/hwtracing/coresight/coresight-etm3x-core.c |   4 +-
>  drivers/hwtracing/coresight/coresight-etm4x-core.c |   4 +-
>  drivers/hwtracing/coresight/coresight-funnel.c     |   4 +-
>  drivers/hwtracing/coresight/coresight-replicator.c |   4 +-
>  drivers/hwtracing/coresight/coresight-stm.c        |   4 +-
>  drivers/hwtracing/coresight/coresight-tmc-core.c   |   4 +-
>  drivers/hwtracing/coresight/coresight-tpiu.c       |   4 +-
>  drivers/i2c/busses/i2c-nomadik.c                   |   4 +-
>  drivers/input/serio/ambakmi.c                      |   3 +-
>  drivers/memory/pl172.c                             |   4 +-
>  drivers/memory/pl353-smc.c                         |   4 +-
>  drivers/mmc/host/mmci.c                            |   4 +-
>  drivers/rtc/rtc-pl030.c                            |   4 +-
>  drivers/rtc/rtc-pl031.c                            |   4 +-
>  drivers/spi/spi-pl022.c                            |   5 +-
>  drivers/tty/serial/amba-pl010.c                    |   4 +-
>  drivers/tty/serial/amba-pl011.c                    |   3 +-
>  drivers/vfio/platform/vfio_amba.c                  |  15 ++--
>  drivers/video/fbdev/amba-clcd.c                    |   4 +-
>  drivers/watchdog/sp805_wdt.c                       |   4 +-
>  include/linux/amba/bus.h                           |   2 +-
>  sound/arm/aaci.c                                   |   4 +-
>  30 files changed, 157 insertions(+), 198 deletions(-)
> 
> 


I'm glad to take this through my char/misc tree, as that's where the
other coresight changes flow through.  So if no one else objects, I will
do so...

thanks,

greg k-h
Uwe Kleine-König Feb. 3, 2021, 7:45 a.m. UTC | #6
Hello,

we already talked about this via irc, but for the record and the benefit
of others:

On Tue, Feb 02, 2021 at 10:49:15AM +0000, Russell King - ARM Linux admin wrote:
> I think you need to have a 6th patch which moves the
> probe/remove/shutdown methods into the bus_type - if you're setting
> them for every struct device_driver, then there's no point doing that
> and they may as well be in the bus_type.

This is implemented in patch 5 already.

Best regards
Uwe
Russell King (Oracle) Feb. 4, 2021, 4:52 p.m. UTC | #7
On Tue, Feb 02, 2021 at 03:06:05PM +0100, Greg Kroah-Hartman wrote:
> I'm glad to take this through my char/misc tree, as that's where the
> other coresight changes flow through.  So if no one else objects, I will
> do so...

Greg, did you end up pulling this after all? If not, Uwe produced a v2.
I haven't merged v2 yet as I don't know what you've done.

Thanks.
Greg KH Feb. 4, 2021, 4:56 p.m. UTC | #8
On Thu, Feb 04, 2021 at 04:52:24PM +0000, Russell King - ARM Linux admin wrote:
> On Tue, Feb 02, 2021 at 03:06:05PM +0100, Greg Kroah-Hartman wrote:
> > I'm glad to take this through my char/misc tree, as that's where the
> > other coresight changes flow through.  So if no one else objects, I will
> > do so...
> 
> Greg, did you end up pulling this after all? If not, Uwe produced a v2.
> I haven't merged v2 yet as I don't know what you've done.

I thought you merged this?

Did you take v1?

I am totally lost here...

thanks,

greg k-h
Russell King (Oracle) Feb. 4, 2021, 4:59 p.m. UTC | #9
On Thu, Feb 04, 2021 at 05:56:50PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Feb 04, 2021 at 04:52:24PM +0000, Russell King - ARM Linux admin wrote:
> > On Tue, Feb 02, 2021 at 03:06:05PM +0100, Greg Kroah-Hartman wrote:
> > > I'm glad to take this through my char/misc tree, as that's where the
> > > other coresight changes flow through.  So if no one else objects, I will
> > > do so...
> > 
> > Greg, did you end up pulling this after all? If not, Uwe produced a v2.
> > I haven't merged v2 yet as I don't know what you've done.
> 
> I thought you merged this?

I took v1, and put it in a branch I've promised in the past not to
rebase/rewind. Uwe is now asking for me to take a v2 or apply a patch
on top.

The only reason to produce an "immutable" branch is if it's the basis
for some dependent work and you need that branch merged into other
people's trees... so the whole "lets produce a v2" is really odd
workflow... I'm confused about what I should do, and who has to be
informed which option I take.

I'm rather lost here too.
Uwe Kleine-König Feb. 4, 2021, 6:15 p.m. UTC | #10
On Thu, Feb 04, 2021 at 04:59:51PM +0000, Russell King - ARM Linux admin wrote:
> On Thu, Feb 04, 2021 at 05:56:50PM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Feb 04, 2021 at 04:52:24PM +0000, Russell King - ARM Linux admin wrote:
> > > On Tue, Feb 02, 2021 at 03:06:05PM +0100, Greg Kroah-Hartman wrote:
> > > > I'm glad to take this through my char/misc tree, as that's where the
> > > > other coresight changes flow through.  So if no one else objects, I will
> > > > do so...
> > > 
> > > Greg, did you end up pulling this after all? If not, Uwe produced a v2.
> > > I haven't merged v2 yet as I don't know what you've done.
> > 
> > I thought you merged this?
> 
> I took v1, and put it in a branch I've promised in the past not to
> rebase/rewind. Uwe is now asking for me to take a v2 or apply a patch
> on top.
> 
> The only reason to produce an "immutable" branch is if it's the basis
> for some dependent work and you need that branch merged into other
> people's trees... so the whole "lets produce a v2" is really odd
> workflow... I'm confused about what I should do, and who has to be
> informed which option I take.
> 
> I'm rather lost here too.

Sorry to have cause this confusion. After I saw that my initial tag
missed to adapt a driver I wanted to make it easy for you to fix the
situation.
So I created a patch to fix it and created a second tag with the patch
squashed in. Obviously only one of them have to be picked and I hoped
you (= Russell + Greg) would agree which option to pick.

My preference would be if you both pick up v2 of the tag to yield a
history that is bisectable without build problems, but if Russell (who
already picked up the broken tag) considers his tree immutable and so
isn't willing to rebase, then picking up the patch is the way to go.

I suggest that Russell descides which option he wants to pick and tells
Greg to do the same!?

Best regards
Uwe
Uwe Kleine-König Feb. 5, 2021, 9:37 a.m. UTC | #11
Hello Russell, hello Greg,

On Thu, Feb 04, 2021 at 07:15:51PM +0100, Uwe Kleine-König wrote:
> On Thu, Feb 04, 2021 at 04:59:51PM +0000, Russell King - ARM Linux admin wrote:
> > On Thu, Feb 04, 2021 at 05:56:50PM +0100, Greg Kroah-Hartman wrote:
> > > On Thu, Feb 04, 2021 at 04:52:24PM +0000, Russell King - ARM Linux admin wrote:
> > > > On Tue, Feb 02, 2021 at 03:06:05PM +0100, Greg Kroah-Hartman wrote:
> > > > > I'm glad to take this through my char/misc tree, as that's where the
> > > > > other coresight changes flow through.  So if no one else objects, I will
> > > > > do so...
> > > > 
> > > > Greg, did you end up pulling this after all? If not, Uwe produced a v2.
> > > > I haven't merged v2 yet as I don't know what you've done.
> > > 
> > > I thought you merged this?
> > 
> > I took v1, and put it in a branch I've promised in the past not to
> > rebase/rewind. Uwe is now asking for me to take a v2 or apply a patch
> > on top.
> > 
> > The only reason to produce an "immutable" branch is if it's the basis
> > for some dependent work and you need that branch merged into other
> > people's trees... so the whole "lets produce a v2" is really odd
> > workflow... I'm confused about what I should do, and who has to be
> > informed which option I take.
> > 
> > I'm rather lost here too.
> 
> Sorry to have cause this confusion. After I saw that my initial tag
> missed to adapt a driver I wanted to make it easy for you to fix the
> situation.
> So I created a patch to fix it and created a second tag with the patch
> squashed in. Obviously only one of them have to be picked and I hoped
> you (= Russell + Greg) would agree which option to pick.
> 
> My preference would be if you both pick up v2 of the tag to yield a
> history that is bisectable without build problems, but if Russell (who
> already picked up the broken tag) considers his tree immutable and so
> isn't willing to rebase, then picking up the patch is the way to go.

OK, the current state is that Russell applied the patch fixing
drivers/mailbox/arm_mhuv2.c on top of merging my first tag.

So the way forward now is that Greg pulls

	git://git.armlinux.org.uk/~rmk/linux-arm.git devel-stable

which currently points to 

	860660fd829e ("ARM: 9055/1: mailbox: arm_mhuv2: make remove callback return void")

, into his tree that contains the hwtracing changes that conflict with my
changes. @Greg: Is this good enough, or do you require a dedicated tag
to pull that?

I think these conflicting hwtracing changes are not yet in any of Greg's
trees (at least they are not in next).

When I pull

	https://git.kernel.org/pub/scm/linux/kernel/git/coresight/linux.git next

(currently pointing to 4e73ff249184 ("coresight: etm4x: Handle accesses
to TRCSTALLCTLR")) into 860660fd829e, I get a conflict in
drivers/hwtracing/coresight/coresight-etm4x-core.c as expected. My
resolution looks as follows:

diff --cc drivers/hwtracing/coresight/coresight-etm4x-core.c
index 82787cba537d,5017d33ba4f5..000000000000
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@@ -1703,6 -1906,28 +1906,27 @@@ static int __exit etm4_remove_dev(struc
  	cpus_read_unlock();
  
  	coresight_unregister(drvdata->csdev);
+ 
+ 	return 0;
+ }
+ 
 -static int __exit etm4_remove_amba(struct amba_device *adev)
++static void __exit etm4_remove_amba(struct amba_device *adev)
+ {
+ 	struct etmv4_drvdata *drvdata = dev_get_drvdata(&adev->dev);
+ 
+ 	if (drvdata)
 -		return etm4_remove_dev(drvdata);
 -	return 0;
++		etm4_remove_dev(drvdata);
+ }
+ 
+ static int __exit etm4_remove_platform_dev(struct platform_device *pdev)
+ {
+ 	int ret = 0;
+ 	struct etmv4_drvdata *drvdata = dev_get_drvdata(&pdev->dev);
+ 
+ 	if (drvdata)
+ 		ret = etm4_remove_dev(drvdata);
+ 	pm_runtime_disable(&pdev->dev);
+ 	return ret;
  }
  
  static const struct amba_id etm4_ids[] = {

Best regards
Uwe
Greg KH Feb. 5, 2021, 10:18 a.m. UTC | #12
On Fri, Feb 05, 2021 at 10:37:44AM +0100, Uwe Kleine-König wrote:
> Hello Russell, hello Greg,
> 
> On Thu, Feb 04, 2021 at 07:15:51PM +0100, Uwe Kleine-König wrote:
> > On Thu, Feb 04, 2021 at 04:59:51PM +0000, Russell King - ARM Linux admin wrote:
> > > On Thu, Feb 04, 2021 at 05:56:50PM +0100, Greg Kroah-Hartman wrote:
> > > > On Thu, Feb 04, 2021 at 04:52:24PM +0000, Russell King - ARM Linux admin wrote:
> > > > > On Tue, Feb 02, 2021 at 03:06:05PM +0100, Greg Kroah-Hartman wrote:
> > > > > > I'm glad to take this through my char/misc tree, as that's where the
> > > > > > other coresight changes flow through.  So if no one else objects, I will
> > > > > > do so...
> > > > > 
> > > > > Greg, did you end up pulling this after all? If not, Uwe produced a v2.
> > > > > I haven't merged v2 yet as I don't know what you've done.
> > > > 
> > > > I thought you merged this?
> > > 
> > > I took v1, and put it in a branch I've promised in the past not to
> > > rebase/rewind. Uwe is now asking for me to take a v2 or apply a patch
> > > on top.
> > > 
> > > The only reason to produce an "immutable" branch is if it's the basis
> > > for some dependent work and you need that branch merged into other
> > > people's trees... so the whole "lets produce a v2" is really odd
> > > workflow... I'm confused about what I should do, and who has to be
> > > informed which option I take.
> > > 
> > > I'm rather lost here too.
> > 
> > Sorry to have cause this confusion. After I saw that my initial tag
> > missed to adapt a driver I wanted to make it easy for you to fix the
> > situation.
> > So I created a patch to fix it and created a second tag with the patch
> > squashed in. Obviously only one of them have to be picked and I hoped
> > you (= Russell + Greg) would agree which option to pick.
> > 
> > My preference would be if you both pick up v2 of the tag to yield a
> > history that is bisectable without build problems, but if Russell (who
> > already picked up the broken tag) considers his tree immutable and so
> > isn't willing to rebase, then picking up the patch is the way to go.
> 
> OK, the current state is that Russell applied the patch fixing
> drivers/mailbox/arm_mhuv2.c on top of merging my first tag.
> 
> So the way forward now is that Greg pulls
> 
> 	git://git.armlinux.org.uk/~rmk/linux-arm.git devel-stable
> 
> which currently points to 
> 
> 	860660fd829e ("ARM: 9055/1: mailbox: arm_mhuv2: make remove callback return void")
> 
> , into his tree that contains the hwtracing changes that conflict with my
> changes. @Greg: Is this good enough, or do you require a dedicated tag
> to pull that?
> 
> I think these conflicting hwtracing changes are not yet in any of Greg's
> trees (at least they are not in next).
> 
> When I pull
> 
> 	https://git.kernel.org/pub/scm/linux/kernel/git/coresight/linux.git next
> 
> (currently pointing to 4e73ff249184 ("coresight: etm4x: Handle accesses
> to TRCSTALLCTLR")) into 860660fd829e, I get a conflict in
> drivers/hwtracing/coresight/coresight-etm4x-core.c as expected. My
> resolution looks as follows:

Ok, my resolution looked a bit different.

Can you pull my char-misc-testing branch and verify I got this all
pulled in correctly?

thanks,

greg k-h
Uwe Kleine-König Feb. 5, 2021, 10:56 a.m. UTC | #13
On Fri, Feb 05, 2021 at 11:18:17AM +0100, Greg Kroah-Hartman wrote:
> On Fri, Feb 05, 2021 at 10:37:44AM +0100, Uwe Kleine-König wrote:
> > Hello Russell, hello Greg,
> > 
> > On Thu, Feb 04, 2021 at 07:15:51PM +0100, Uwe Kleine-König wrote:
> > > On Thu, Feb 04, 2021 at 04:59:51PM +0000, Russell King - ARM Linux admin wrote:
> > > > On Thu, Feb 04, 2021 at 05:56:50PM +0100, Greg Kroah-Hartman wrote:
> > > > > On Thu, Feb 04, 2021 at 04:52:24PM +0000, Russell King - ARM Linux admin wrote:
> > > > > > On Tue, Feb 02, 2021 at 03:06:05PM +0100, Greg Kroah-Hartman wrote:
> > > > > > > I'm glad to take this through my char/misc tree, as that's where the
> > > > > > > other coresight changes flow through.  So if no one else objects, I will
> > > > > > > do so...
> > > > > > 
> > > > > > Greg, did you end up pulling this after all? If not, Uwe produced a v2.
> > > > > > I haven't merged v2 yet as I don't know what you've done.
> > > > > 
> > > > > I thought you merged this?
> > > > 
> > > > I took v1, and put it in a branch I've promised in the past not to
> > > > rebase/rewind. Uwe is now asking for me to take a v2 or apply a patch
> > > > on top.
> > > > 
> > > > The only reason to produce an "immutable" branch is if it's the basis
> > > > for some dependent work and you need that branch merged into other
> > > > people's trees... so the whole "lets produce a v2" is really odd
> > > > workflow... I'm confused about what I should do, and who has to be
> > > > informed which option I take.
> > > > 
> > > > I'm rather lost here too.
> > > 
> > > Sorry to have cause this confusion. After I saw that my initial tag
> > > missed to adapt a driver I wanted to make it easy for you to fix the
> > > situation.
> > > So I created a patch to fix it and created a second tag with the patch
> > > squashed in. Obviously only one of them have to be picked and I hoped
> > > you (= Russell + Greg) would agree which option to pick.
> > > 
> > > My preference would be if you both pick up v2 of the tag to yield a
> > > history that is bisectable without build problems, but if Russell (who
> > > already picked up the broken tag) considers his tree immutable and so
> > > isn't willing to rebase, then picking up the patch is the way to go.
> > 
> > OK, the current state is that Russell applied the patch fixing
> > drivers/mailbox/arm_mhuv2.c on top of merging my first tag.
> > 
> > So the way forward now is that Greg pulls
> > 
> > 	git://git.armlinux.org.uk/~rmk/linux-arm.git devel-stable
> > 
> > which currently points to 
> > 
> > 	860660fd829e ("ARM: 9055/1: mailbox: arm_mhuv2: make remove callback return void")
> > 
> > , into his tree that contains the hwtracing changes that conflict with my
> > changes. @Greg: Is this good enough, or do you require a dedicated tag
> > to pull that?
> > 
> > I think these conflicting hwtracing changes are not yet in any of Greg's
> > trees (at least they are not in next).
> > 
> > When I pull
> > 
> > 	https://git.kernel.org/pub/scm/linux/kernel/git/coresight/linux.git next
> > 
> > (currently pointing to 4e73ff249184 ("coresight: etm4x: Handle accesses
> > to TRCSTALLCTLR")) into 860660fd829e, I get a conflict in
> > drivers/hwtracing/coresight/coresight-etm4x-core.c as expected. My
> > resolution looks as follows:
> 
> Ok, my resolution looked a bit different.
> 
> Can you pull my char-misc-testing branch and verify I got this all
> pulled in correctly?

minor side-note: mentioning the repo url would have simplified that test.

I looked at

	https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git char-misc-testing

commit 0573d3fa48640f0fa6b105ff92dcb02b94d6c1ab now.

I didn't compile test, but I'm willing to bet your resolution is wrong.
You have no return statement in etm4_remove_dev() but its return type is
int and etm4_remove_amba() still returns int but should return void.

Best regards
Uwe
Greg KH Feb. 5, 2021, 11:07 a.m. UTC | #14
On Fri, Feb 05, 2021 at 11:56:15AM +0100, Uwe Kleine-König wrote:
> On Fri, Feb 05, 2021 at 11:18:17AM +0100, Greg Kroah-Hartman wrote:
> > On Fri, Feb 05, 2021 at 10:37:44AM +0100, Uwe Kleine-König wrote:
> > > Hello Russell, hello Greg,
> > > 
> > > On Thu, Feb 04, 2021 at 07:15:51PM +0100, Uwe Kleine-König wrote:
> > > > On Thu, Feb 04, 2021 at 04:59:51PM +0000, Russell King - ARM Linux admin wrote:
> > > > > On Thu, Feb 04, 2021 at 05:56:50PM +0100, Greg Kroah-Hartman wrote:
> > > > > > On Thu, Feb 04, 2021 at 04:52:24PM +0000, Russell King - ARM Linux admin wrote:
> > > > > > > On Tue, Feb 02, 2021 at 03:06:05PM +0100, Greg Kroah-Hartman wrote:
> > > > > > > > I'm glad to take this through my char/misc tree, as that's where the
> > > > > > > > other coresight changes flow through.  So if no one else objects, I will
> > > > > > > > do so...
> > > > > > > 
> > > > > > > Greg, did you end up pulling this after all? If not, Uwe produced a v2.
> > > > > > > I haven't merged v2 yet as I don't know what you've done.
> > > > > > 
> > > > > > I thought you merged this?
> > > > > 
> > > > > I took v1, and put it in a branch I've promised in the past not to
> > > > > rebase/rewind. Uwe is now asking for me to take a v2 or apply a patch
> > > > > on top.
> > > > > 
> > > > > The only reason to produce an "immutable" branch is if it's the basis
> > > > > for some dependent work and you need that branch merged into other
> > > > > people's trees... so the whole "lets produce a v2" is really odd
> > > > > workflow... I'm confused about what I should do, and who has to be
> > > > > informed which option I take.
> > > > > 
> > > > > I'm rather lost here too.
> > > > 
> > > > Sorry to have cause this confusion. After I saw that my initial tag
> > > > missed to adapt a driver I wanted to make it easy for you to fix the
> > > > situation.
> > > > So I created a patch to fix it and created a second tag with the patch
> > > > squashed in. Obviously only one of them have to be picked and I hoped
> > > > you (= Russell + Greg) would agree which option to pick.
> > > > 
> > > > My preference would be if you both pick up v2 of the tag to yield a
> > > > history that is bisectable without build problems, but if Russell (who
> > > > already picked up the broken tag) considers his tree immutable and so
> > > > isn't willing to rebase, then picking up the patch is the way to go.
> > > 
> > > OK, the current state is that Russell applied the patch fixing
> > > drivers/mailbox/arm_mhuv2.c on top of merging my first tag.
> > > 
> > > So the way forward now is that Greg pulls
> > > 
> > > 	git://git.armlinux.org.uk/~rmk/linux-arm.git devel-stable
> > > 
> > > which currently points to 
> > > 
> > > 	860660fd829e ("ARM: 9055/1: mailbox: arm_mhuv2: make remove callback return void")
> > > 
> > > , into his tree that contains the hwtracing changes that conflict with my
> > > changes. @Greg: Is this good enough, or do you require a dedicated tag
> > > to pull that?
> > > 
> > > I think these conflicting hwtracing changes are not yet in any of Greg's
> > > trees (at least they are not in next).
> > > 
> > > When I pull
> > > 
> > > 	https://git.kernel.org/pub/scm/linux/kernel/git/coresight/linux.git next
> > > 
> > > (currently pointing to 4e73ff249184 ("coresight: etm4x: Handle accesses
> > > to TRCSTALLCTLR")) into 860660fd829e, I get a conflict in
> > > drivers/hwtracing/coresight/coresight-etm4x-core.c as expected. My
> > > resolution looks as follows:
> > 
> > Ok, my resolution looked a bit different.
> > 
> > Can you pull my char-misc-testing branch and verify I got this all
> > pulled in correctly?
> 
> minor side-note: mentioning the repo url would have simplified that test.

Sorry, I thought you had it based on the above info.

> I looked at
> 
> 	https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git char-misc-testing
> 
> commit 0573d3fa48640f0fa6b105ff92dcb02b94d6c1ab now.
> 
> I didn't compile test, but I'm willing to bet your resolution is wrong.
> You have no return statement in etm4_remove_dev() but its return type is
> int and etm4_remove_amba() still returns int but should return void.

Can you send a patch to fix this up?

thanks,

greg k-h