2018-05-08 19:15:45

by Kim Phillips

[permalink] [raw]
Subject: [PATCH 1/4] amba: Export amba_bustype

This patch is provided in the context of allowing the Coresight driver
subsystem to be loaded as modules. Coresight uses amba_bus in its call
to bus_find_device() in of_coresight_get_endpoint_device() when
searching for a configurable endpoint device. This patch allows
Coresight to reference amba_bustype when built as a module.

Cc: Mathieu Poirier <[email protected]>
Cc: Alex Williamson <[email protected]>
Cc: Eric Auger <[email protected]>
Cc: Russell King <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Todd Kjos <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Thierry Reding <[email protected]>
Cc: Robin Murphy <[email protected]>
Signed-off-by: Kim Phillips <[email protected]>
---
There was a prior patch submitted by Alex W. here:

https://lkml.org/lkml/2017/6/19/811

But I can't tell its fate - presume simply delayed?

Coresight uses amba_bus in its call to bus_find_device() here:

https://lxr.missinglinkelectronics.com/linux/drivers/hwtracing/coresight/of_coresight.c#L51

Grepping for bus_type and EXPORT shows other busses exporting their
type, so I don't think this is the wrong approach. If, OTOH, Coresight
needs to do something differently, please comment.

drivers/amba/bus.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 594c228d2f02..12283bd06733 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -197,6 +197,7 @@ struct bus_type amba_bustype = {
.pm = &amba_pm,
.force_dma = true,
};
+EXPORT_SYMBOL_GPL(amba_bustype);

static int __init amba_init(void)
{
--
2.16.2



2018-05-09 13:41:31

by Robin Murphy

[permalink] [raw]
Subject: Re: [PATCH 1/4] amba: Export amba_bustype

Hi Kim,

On 08/05/18 20:06, Kim Phillips wrote:
> This patch is provided in the context of allowing the Coresight driver
> subsystem to be loaded as modules. Coresight uses amba_bus in its call
> to bus_find_device() in of_coresight_get_endpoint_device() when
> searching for a configurable endpoint device. This patch allows
> Coresight to reference amba_bustype when built as a module.
>
> Cc: Mathieu Poirier <[email protected]>
> Cc: Alex Williamson <[email protected]>
> Cc: Eric Auger <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Todd Kjos <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Thierry Reding <[email protected]>
> Cc: Robin Murphy <[email protected]>
> Signed-off-by: Kim Phillips <[email protected]>
> ---
> There was a prior patch submitted by Alex W. here:
>
> https://lkml.org/lkml/2017/6/19/811
>
> But I can't tell its fate - presume simply delayed?
>
> Coresight uses amba_bus in its call to bus_find_device() here:
>
> https://lxr.missinglinkelectronics.com/linux/drivers/hwtracing/coresight/of_coresight.c#L51
>
> Grepping for bus_type and EXPORT shows other busses exporting their
> type, so I don't think this is the wrong approach. If, OTOH, Coresight
> needs to do something differently, please comment.

Exposing raw bus_types is pretty ugly, but it is indeed the status quo,
so this probably is the reasonable thing to do. I suppose an amba_bus
equivalent of of_find_device_by_node() could be implemented, but for
only a single potential user that doesn't seem particularly worthwhile,
since unless some massive shake-up of how buses work comes along the
bus_type will inevitably end up being exported for other reasons anyway.
So, in the context of this series;

Reviewed-by: Robin Murphy <[email protected]>

However, as a wild idea for sidestepping the issue completely (or at
least keeping it within the CoreSight framework), at first glance it
appears something like the below might be feasible, although I may well
be missing some obvious reason why not.

Thanks,
Robin.

----->8-----
diff --git a/drivers/hwtracing/coresight/of_coresight.c
b/drivers/hwtracing/coresight/of_coresight.c
index 7c375443ede6..2c3fdc9b63e6 100644
--- a/drivers/hwtracing/coresight/of_coresight.c
+++ b/drivers/hwtracing/coresight/of_coresight.c
@@ -27,28 +27,13 @@

static int of_dev_node_match(struct device *dev, void *data)
{
- return dev->of_node == data;
+ return dev->parent->of_node == data;
}

static struct device *
of_coresight_get_endpoint_device(struct device_node *endpoint)
{
- struct device *dev = NULL;
-
- /*
- * If we have a non-configurable replicator, it will be found on the
- * platform bus.
- */
- dev = bus_find_device(&platform_bus_type, NULL,
- endpoint, of_dev_node_match);
- if (dev)
- return dev;
-
- /*
- * We have a configurable component - circle through the AMBA bus
- * looking for the device that matches the endpoint node.
- */
- return bus_find_device(&amba_bustype, NULL,
+ return bus_find_device(&coresight_bustype, NULL,
endpoint, of_dev_node_match);
}


2018-05-09 15:42:03

by Mathieu Poirier

[permalink] [raw]
Subject: Re: [PATCH 1/4] amba: Export amba_bustype

On Wed, May 09, 2018 at 02:38:32PM +0100, Robin Murphy wrote:
> Hi Kim,
>
> On 08/05/18 20:06, Kim Phillips wrote:
> >This patch is provided in the context of allowing the Coresight driver
> >subsystem to be loaded as modules. Coresight uses amba_bus in its call
> >to bus_find_device() in of_coresight_get_endpoint_device() when
> >searching for a configurable endpoint device. This patch allows
> >Coresight to reference amba_bustype when built as a module.
> >
> >Cc: Mathieu Poirier <[email protected]>
> >Cc: Alex Williamson <[email protected]>
> >Cc: Eric Auger <[email protected]>
> >Cc: Russell King <[email protected]>
> >Cc: Greg Kroah-Hartman <[email protected]>
> >Cc: Todd Kjos <[email protected]>
> >Cc: Geert Uytterhoeven <[email protected]>
> >Cc: Thierry Reding <[email protected]>
> >Cc: Robin Murphy <[email protected]>
> >Signed-off-by: Kim Phillips <[email protected]>
> >---
> >There was a prior patch submitted by Alex W. here:
> >
> >https://lkml.org/lkml/2017/6/19/811
> >
> >But I can't tell its fate - presume simply delayed?
> >
> >Coresight uses amba_bus in its call to bus_find_device() here:
> >
> >https://lxr.missinglinkelectronics.com/linux/drivers/hwtracing/coresight/of_coresight.c#L51
> >
> >Grepping for bus_type and EXPORT shows other busses exporting their
> >type, so I don't think this is the wrong approach. If, OTOH, Coresight
> >needs to do something differently, please comment.
>
> Exposing raw bus_types is pretty ugly, but it is indeed the status quo, so
> this probably is the reasonable thing to do. I suppose an amba_bus
> equivalent of of_find_device_by_node() could be implemented, but for only a
> single potential user that doesn't seem particularly worthwhile, since
> unless some massive shake-up of how buses work comes along the bus_type will
> inevitably end up being exported for other reasons anyway. So, in the
> context of this series;
>
> Reviewed-by: Robin Murphy <[email protected]>
>
> However, as a wild idea for sidestepping the issue completely (or at least
> keeping it within the CoreSight framework), at first glance it appears
> something like the below might be feasible, although I may well be missing
> some obvious reason why not.
>
> Thanks,
> Robin.
>
> ----->8-----
> diff --git a/drivers/hwtracing/coresight/of_coresight.c
> b/drivers/hwtracing/coresight/of_coresight.c
> index 7c375443ede6..2c3fdc9b63e6 100644
> --- a/drivers/hwtracing/coresight/of_coresight.c
> +++ b/drivers/hwtracing/coresight/of_coresight.c
> @@ -27,28 +27,13 @@
>
> static int of_dev_node_match(struct device *dev, void *data)
> {
> - return dev->of_node == data;
> + return dev->parent->of_node == data;
> }
>
> static struct device *
> of_coresight_get_endpoint_device(struct device_node *endpoint)
> {
> - struct device *dev = NULL;
> -
> - /*
> - * If we have a non-configurable replicator, it will be found on the
> - * platform bus.
> - */
> - dev = bus_find_device(&platform_bus_type, NULL,
> - endpoint, of_dev_node_match);
> - if (dev)
> - return dev;
> -
> - /*
> - * We have a configurable component - circle through the AMBA bus
> - * looking for the device that matches the endpoint node.
> - */
> - return bus_find_device(&amba_bustype, NULL,
> + return bus_find_device(&coresight_bustype, NULL,
> endpoint, of_dev_node_match);
> }

Hi Robin and thanks for the input.

Your approach would work if all CS devices would be on the CS bus, which is not
the case at discovery time when of_coresight_get_endpoint_device() is called.

Mathieu

>

2018-05-09 16:04:40

by Robin Murphy

[permalink] [raw]
Subject: Re: [PATCH 1/4] amba: Export amba_bustype

On 09/05/18 16:40, Mathieu Poirier wrote:
> On Wed, May 09, 2018 at 02:38:32PM +0100, Robin Murphy wrote:
>> Hi Kim,
>>
>> On 08/05/18 20:06, Kim Phillips wrote:
>>> This patch is provided in the context of allowing the Coresight driver
>>> subsystem to be loaded as modules. Coresight uses amba_bus in its call
>>> to bus_find_device() in of_coresight_get_endpoint_device() when
>>> searching for a configurable endpoint device. This patch allows
>>> Coresight to reference amba_bustype when built as a module.
>>>
>>> Cc: Mathieu Poirier <[email protected]>
>>> Cc: Alex Williamson <[email protected]>
>>> Cc: Eric Auger <[email protected]>
>>> Cc: Russell King <[email protected]>
>>> Cc: Greg Kroah-Hartman <[email protected]>
>>> Cc: Todd Kjos <[email protected]>
>>> Cc: Geert Uytterhoeven <[email protected]>
>>> Cc: Thierry Reding <[email protected]>
>>> Cc: Robin Murphy <[email protected]>
>>> Signed-off-by: Kim Phillips <[email protected]>
>>> ---
>>> There was a prior patch submitted by Alex W. here:
>>>
>>> https://lkml.org/lkml/2017/6/19/811
>>>
>>> But I can't tell its fate - presume simply delayed?
>>>
>>> Coresight uses amba_bus in its call to bus_find_device() here:
>>>
>>> https://lxr.missinglinkelectronics.com/linux/drivers/hwtracing/coresight/of_coresight.c#L51
>>>
>>> Grepping for bus_type and EXPORT shows other busses exporting their
>>> type, so I don't think this is the wrong approach. If, OTOH, Coresight
>>> needs to do something differently, please comment.
>>
>> Exposing raw bus_types is pretty ugly, but it is indeed the status quo, so
>> this probably is the reasonable thing to do. I suppose an amba_bus
>> equivalent of of_find_device_by_node() could be implemented, but for only a
>> single potential user that doesn't seem particularly worthwhile, since
>> unless some massive shake-up of how buses work comes along the bus_type will
>> inevitably end up being exported for other reasons anyway. So, in the
>> context of this series;
>>
>> Reviewed-by: Robin Murphy <[email protected]>
>>
>> However, as a wild idea for sidestepping the issue completely (or at least
>> keeping it within the CoreSight framework), at first glance it appears
>> something like the below might be feasible, although I may well be missing
>> some obvious reason why not.
>>
>> Thanks,
>> Robin.
>>
>> ----->8-----
>> diff --git a/drivers/hwtracing/coresight/of_coresight.c
>> b/drivers/hwtracing/coresight/of_coresight.c
>> index 7c375443ede6..2c3fdc9b63e6 100644
>> --- a/drivers/hwtracing/coresight/of_coresight.c
>> +++ b/drivers/hwtracing/coresight/of_coresight.c
>> @@ -27,28 +27,13 @@
>>
>> static int of_dev_node_match(struct device *dev, void *data)
>> {
>> - return dev->of_node == data;
>> + return dev->parent->of_node == data;
>> }
>>
>> static struct device *
>> of_coresight_get_endpoint_device(struct device_node *endpoint)
>> {
>> - struct device *dev = NULL;
>> -
>> - /*
>> - * If we have a non-configurable replicator, it will be found on the
>> - * platform bus.
>> - */
>> - dev = bus_find_device(&platform_bus_type, NULL,
>> - endpoint, of_dev_node_match);
>> - if (dev)
>> - return dev;
>> -
>> - /*
>> - * We have a configurable component - circle through the AMBA bus
>> - * looking for the device that matches the endpoint node.
>> - */
>> - return bus_find_device(&amba_bustype, NULL,
>> + return bus_find_device(&coresight_bustype, NULL,
>> endpoint, of_dev_node_match);
>> }
>
> Hi Robin and thanks for the input.
>
> Your approach would work if all CS devices would be on the CS bus, which is not
> the case at discovery time when of_coresight_get_endpoint_device() is called.

Ah, now I see that obvious thing I was indeed missing - I got mixed up
and started thinking bus_add_device() only got called as part of driver
probe, but of course it's actually much earlier in
{platform,amba}_device_add(). That means my idea cannot in fact work at
all, since both ends of any link would defer waiting for the other to
probe (and thus call coresight_register()) first. Oh well, I guess
poking amba_bustype really does remain the only reasonable answer.

Thanks,
Robin.

2018-05-14 21:50:16

by Kim Phillips

[permalink] [raw]
Subject: Re: [PATCH 1/4] amba: Export amba_bustype

On Wed, 9 May 2018 17:02:34 +0100
Robin Murphy <[email protected]> wrote:

> probe (and thus call coresight_register()) first. Oh well, I guess
> poking amba_bustype really does remain the only reasonable answer.

Thanks all, I've added Robin's Reviewed-by, and submitted this patch to
RMK's patch tracking system, here:

http://www.armlinux.org.uk/developer/patches/viewpatch.php?id=8773/1

Cheers,

Kim

2018-05-15 06:59:49

by Ulf Hansson

[permalink] [raw]
Subject: Re: [PATCH 1/4] amba: Export amba_bustype

On 8 May 2018 at 21:06, Kim Phillips <[email protected]> wrote:
> This patch is provided in the context of allowing the Coresight driver
> subsystem to be loaded as modules. Coresight uses amba_bus in its call
> to bus_find_device() in of_coresight_get_endpoint_device() when
> searching for a configurable endpoint device. This patch allows
> Coresight to reference amba_bustype when built as a module.

Sounds like you are fixing a bug, don't your want this to go for
stable and then also add a fixes tag?

>
> Cc: Mathieu Poirier <[email protected]>
> Cc: Alex Williamson <[email protected]>
> Cc: Eric Auger <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Todd Kjos <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Thierry Reding <[email protected]>
> Cc: Robin Murphy <[email protected]>
> Signed-off-by: Kim Phillips <[email protected]>

Reviewed-by: Ulf Hansson <[email protected]>

[...]

Kind regards
Uffe

2018-05-15 13:51:15

by Kim Phillips

[permalink] [raw]
Subject: Re: [PATCH 1/4] amba: Export amba_bustype

On Tue, 15 May 2018 08:59:02 +0200
Ulf Hansson <[email protected]> wrote:

> On 8 May 2018 at 21:06, Kim Phillips <[email protected]> wrote:
> > This patch is provided in the context of allowing the Coresight driver
> > subsystem to be loaded as modules. Coresight uses amba_bus in its call
> > to bus_find_device() in of_coresight_get_endpoint_device() when
> > searching for a configurable endpoint device. This patch allows
> > Coresight to reference amba_bustype when built as a module.
>
> Sounds like you are fixing a bug, don't your want this to go for
> stable and then also add a fixes tag?

How do you consider this a bug fix? What commit would the fixes tag
reference? The introduction of the amba bus? Not only aren't busses
required to export their bus_type, but that commit predates git.

Kim

2018-05-15 13:59:06

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH 1/4] amba: Export amba_bustype

On Tue, May 15, 2018 at 08:59:02AM +0200, Ulf Hansson wrote:
> On 8 May 2018 at 21:06, Kim Phillips <[email protected]> wrote:
> > This patch is provided in the context of allowing the Coresight driver
> > subsystem to be loaded as modules. Coresight uses amba_bus in its call
> > to bus_find_device() in of_coresight_get_endpoint_device() when
> > searching for a configurable endpoint device. This patch allows
> > Coresight to reference amba_bustype when built as a module.
>
> Sounds like you are fixing a bug, don't your want this to go for
> stable and then also add a fixes tag?

What bug is this fixing exactly that would qualify it for stable
backporting?

The lack of an export is never a bug unless there is some existing
user which requires it. This is not the case here.

What Kim is doing in his new patch series is making Coresight - which
is currently only available as either disabled or built-in - possible
to be loaded as a module. This is a new feature, and in the process
of creating this new feature, Kim needs a symbol that wasn't previously
needed to be exported.

I think it would be hard to argue that Coresight not being available
as a module is a bug worthy of backporting to older kernels.

Therefore, it is not a bug, and it certainly does not qualify for
backporting to stable trees:

- It must be obviously correct and tested.

Probably.

- It cannot be bigger than 100 lines, with context.

Is.

- It must fix only one thing.

Does.

- It must fix a real bug that bothers people (not a, "This could be a
problem..." type thing).

Nope.

- It must fix a problem that causes a build error (but not for things
marked CONFIG_BROKEN), an oops, a hang, data corruption, a real
security issue, or some "oh, that's not good" issue. In short, something
critical.

Nope, not in any stable tree.

- Serious issues as reported by a user of a distribution kernel may also
be considered if they fix a notable performance or interactivity issue.
As these fixes are not as obvious and have a higher risk of a subtle
regression they should only be submitted by a distribution kernel
maintainer and include an addendum linking to a bugzilla entry if it
exists and additional information on the user-visible impact.

Hasn't been.

- New device IDs and quirks are also accepted.

Is not that.

- No "theoretical race condition" issues, unless an explanation of how the
race can be exploited is also provided.

Is not that.

- It cannot contain any "trivial" fixes in it (spelling changes,
whitespace cleanups, etc).

Doesn't (so okay.)

- It must follow the
:ref:`Documentation/process/submitting-patches.rst <submittingpatches>`
rules.

Does.

- It or an equivalent fix must already exist in Linus' tree (upstream).

Eventually.

--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

2018-05-15 14:06:26

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH 1/4] amba: Export amba_bustype

On Tue, May 15, 2018 at 08:15:19AM -0500, Kim Phillips wrote:
> On Tue, 15 May 2018 08:59:02 +0200
> Ulf Hansson <[email protected]> wrote:
>
> > On 8 May 2018 at 21:06, Kim Phillips <[email protected]> wrote:
> > > This patch is provided in the context of allowing the Coresight driver
> > > subsystem to be loaded as modules. Coresight uses amba_bus in its call
> > > to bus_find_device() in of_coresight_get_endpoint_device() when
> > > searching for a configurable endpoint device. This patch allows
> > > Coresight to reference amba_bustype when built as a module.
> >
> > Sounds like you are fixing a bug, don't your want this to go for
> > stable and then also add a fixes tag?
>
> How do you consider this a bug fix? What commit would the fixes tag
> reference? The introduction of the amba bus? Not only aren't busses
> required to export their bus_type, but that commit predates git.

I do not consider it a bug fix (see my reply to Ulf) and I certainly
do not think it should qualify for backporting to *stable* kernels.

While the impact on stable kernels of just this patch should be low,
that's not really the point: one of the requirements for stable kernels
is that patches should be _real_ bug fixes - stuff that affects people
using the kernel.

That is not the case in this instance - there is no problem with any
of the existing kernels with not having this symbol exported.

The only problem which we're aware of is with Coresight, and only then
when your patches to allow Coresight to be modular are merged. That's
a new feature, and this new feature now requires a symbol that was not
previously required to be exported to now be exported.

So, the need for this export comes from your new feature, not from a
bug report that is affecting people. As long as your new feature is
not backported (does it even qualify for backporting to stable kernels?)
then there is no problem with any existing stable kernel, and so there
is no requirement for it to be backported.

Hence, there's no need to Cc stable, and no need for a fixes tag. It's
not a fix, it's a feature enhancement to permit modular code to use
bus_find_device() with this bus type which wasn't previously required.

--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

2018-05-15 14:45:38

by Kim Phillips

[permalink] [raw]
Subject: Re: [PATCH 1/4] amba: Export amba_bustype

On Tue, 15 May 2018 14:48:31 +0100
Russell King - ARM Linux <[email protected]> wrote:

> On Tue, May 15, 2018 at 08:15:19AM -0500, Kim Phillips wrote:
> > On Tue, 15 May 2018 08:59:02 +0200
> > Ulf Hansson <[email protected]> wrote:
> >
> > > On 8 May 2018 at 21:06, Kim Phillips <[email protected]> wrote:
> > > > This patch is provided in the context of allowing the Coresight driver
> > > > subsystem to be loaded as modules. Coresight uses amba_bus in its call
> > > > to bus_find_device() in of_coresight_get_endpoint_device() when
> > > > searching for a configurable endpoint device. This patch allows
> > > > Coresight to reference amba_bustype when built as a module.
> > >
> > > Sounds like you are fixing a bug, don't your want this to go for
> > > stable and then also add a fixes tag?
> >
> > How do you consider this a bug fix? What commit would the fixes tag
> > reference? The introduction of the amba bus? Not only aren't busses
> > required to export their bus_type, but that commit predates git.
>
> I do not consider it a bug fix (see my reply to Ulf) and I certainly

I agree this isn't a bug fix.

> The only problem which we're aware of is with Coresight, and only then
> when your patches to allow Coresight to be modular are merged. That's

Just to clarify: the Coresight modularization patch depends on this
patch, so this patch is to be merged first.

Cheers,

Kim

2018-05-15 15:50:38

by Mathieu Poirier

[permalink] [raw]
Subject: Re: [PATCH 1/4] amba: Export amba_bustype

On 15 May 2018 at 08:37, Kim Phillips <[email protected]> wrote:
> On Tue, 15 May 2018 14:48:31 +0100
> Russell King - ARM Linux <[email protected]> wrote:
>
>> On Tue, May 15, 2018 at 08:15:19AM -0500, Kim Phillips wrote:
>> > On Tue, 15 May 2018 08:59:02 +0200
>> > Ulf Hansson <[email protected]> wrote:
>> >
>> > > On 8 May 2018 at 21:06, Kim Phillips <[email protected]> wrote:
>> > > > This patch is provided in the context of allowing the Coresight driver
>> > > > subsystem to be loaded as modules. Coresight uses amba_bus in its call
>> > > > to bus_find_device() in of_coresight_get_endpoint_device() when
>> > > > searching for a configurable endpoint device. This patch allows
>> > > > Coresight to reference amba_bustype when built as a module.
>> > >
>> > > Sounds like you are fixing a bug, don't your want this to go for
>> > > stable and then also add a fixes tag?
>> >
>> > How do you consider this a bug fix? What commit would the fixes tag
>> > reference? The introduction of the amba bus? Not only aren't busses
>> > required to export their bus_type, but that commit predates git.
>>
>> I do not consider it a bug fix (see my reply to Ulf) and I certainly
>
> I agree this isn't a bug fix.
>
>> The only problem which we're aware of is with Coresight, and only then
>> when your patches to allow Coresight to be modular are merged. That's
>
> Just to clarify: the Coresight modularization patch depends on this
> patch, so this patch is to be merged first.

I think the whole feature should be merge at the same time and (with
Russell's ACK) probably easier through my tree.

>
> Cheers,
>
> Kim

2018-05-16 08:57:55

by Ulf Hansson

[permalink] [raw]
Subject: Re: [PATCH 1/4] amba: Export amba_bustype

On 15 May 2018 at 15:41, Russell King - ARM Linux <[email protected]> wrote:
> On Tue, May 15, 2018 at 08:59:02AM +0200, Ulf Hansson wrote:
>> On 8 May 2018 at 21:06, Kim Phillips <[email protected]> wrote:
>> > This patch is provided in the context of allowing the Coresight driver
>> > subsystem to be loaded as modules. Coresight uses amba_bus in its call
>> > to bus_find_device() in of_coresight_get_endpoint_device() when
>> > searching for a configurable endpoint device. This patch allows
>> > Coresight to reference amba_bustype when built as a module.
>>
>> Sounds like you are fixing a bug, don't your want this to go for
>> stable and then also add a fixes tag?
>
> What bug is this fixing exactly that would qualify it for stable
> backporting?

That was my question, as when reading the changelog of $subject patch,
this isn't clear to me.

>
> The lack of an export is never a bug unless there is some existing
> user which requires it. This is not the case here.
>
> What Kim is doing in his new patch series is making Coresight - which
> is currently only available as either disabled or built-in - possible
> to be loaded as a module. This is a new feature, and in the process
> of creating this new feature, Kim needs a symbol that wasn't previously
> needed to be exported.

Thanks for clarifying, I did not review the entire series. The change
make perfect sense, no objections - and of course I agree it's not a
bug fix.

[...]

Kind regards
Uffe

2018-05-16 09:17:50

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH 1/4] amba: Export amba_bustype

On Tue, May 8, 2018 at 10:06 PM, Kim Phillips <[email protected]> wrote:
> This patch is provided in the context of allowing the Coresight driver
> subsystem to be loaded as modules. Coresight uses amba_bus in its call
> to bus_find_device() in of_coresight_get_endpoint_device() when
> searching for a configurable endpoint device. This patch allows
> Coresight to reference amba_bustype when built as a module.

> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -197,6 +197,7 @@ struct bus_type amba_bustype = {
> .pm = &amba_pm,
> .force_dma = true,
> };
> +EXPORT_SYMBOL_GPL(amba_bustype);

Oh,

What wrong with the approach let's say similar to PCI bus?

Whenever you have a struct device you may use two helpers:

dev_is_pci() -> is the device of PCI bus type?
to_pci_dev() -> get's container of struct device for PCI bus case

--
With Best Regards,
Andy Shevchenko

2018-05-16 09:22:17

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH 1/4] amba: Export amba_bustype

On Wed, May 16, 2018 at 12:16:28PM +0300, Andy Shevchenko wrote:
> On Tue, May 8, 2018 at 10:06 PM, Kim Phillips <[email protected]> wrote:
> > This patch is provided in the context of allowing the Coresight driver
> > subsystem to be loaded as modules. Coresight uses amba_bus in its call
> > to bus_find_device() in of_coresight_get_endpoint_device() when
> > searching for a configurable endpoint device. This patch allows
> > Coresight to reference amba_bustype when built as a module.
>
> > --- a/drivers/amba/bus.c
> > +++ b/drivers/amba/bus.c
> > @@ -197,6 +197,7 @@ struct bus_type amba_bustype = {
> > .pm = &amba_pm,
> > .force_dma = true,
> > };
> > +EXPORT_SYMBOL_GPL(amba_bustype);
>
> Oh,
>
> What wrong with the approach let's say similar to PCI bus?
>
> Whenever you have a struct device you may use two helpers:
>
> dev_is_pci() -> is the device of PCI bus type?
> to_pci_dev() -> get's container of struct device for PCI bus case

How does that help with bus_find_device() which requires the bus_type
structure for the type of devices to be searched?

--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

2018-05-16 11:12:56

by Robin Murphy

[permalink] [raw]
Subject: Re: [PATCH 1/4] amba: Export amba_bustype

On 16/05/18 10:18, Russell King - ARM Linux wrote:
> On Wed, May 16, 2018 at 12:16:28PM +0300, Andy Shevchenko wrote:
>> On Tue, May 8, 2018 at 10:06 PM, Kim Phillips <[email protected]> wrote:
>>> This patch is provided in the context of allowing the Coresight driver
>>> subsystem to be loaded as modules. Coresight uses amba_bus in its call
>>> to bus_find_device() in of_coresight_get_endpoint_device() when
>>> searching for a configurable endpoint device. This patch allows
>>> Coresight to reference amba_bustype when built as a module.
>>
>>> --- a/drivers/amba/bus.c
>>> +++ b/drivers/amba/bus.c
>>> @@ -197,6 +197,7 @@ struct bus_type amba_bustype = {
>>> .pm = &amba_pm,
>>> .force_dma = true,
>>> };
>>> +EXPORT_SYMBOL_GPL(amba_bustype);
>>
>> Oh,
>>
>> What wrong with the approach let's say similar to PCI bus?
>>
>> Whenever you have a struct device you may use two helpers:
>>
>> dev_is_pci() -> is the device of PCI bus type?
>> to_pci_dev() -> get's container of struct device for PCI bus case
>
> How does that help with bus_find_device() which requires the bus_type
> structure for the type of devices to be searched?
Not to mention that dev_is_pci() still relies on pci_bus_type itself
being exported anyway.

Robin.