| Message ID | 20180508140628.f30774c70c4c481bff3f8000@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 8 May 2018 at 21:06, Kim Phillips <kim.phillips@arm.com> 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 <mathieu.poirier@linaro.org> > Cc: Alex Williamson <alex.williamson@redhat.com> > Cc: Eric Auger <eric.auger@redhat.com> > Cc: Russell King <linux@armlinux.org.uk> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Todd Kjos <tkjos@google.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Thierry Reding <treding@nvidia.com> > Cc: Robin Murphy <robin.murphy@arm.com> > Signed-off-by: Kim Phillips <kim.phillips@arm.com> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> [...] Kind regards Uffe
On Tue, 15 May 2018 08:59:02 +0200 Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 8 May 2018 at 21:06, Kim Phillips <kim.phillips@arm.com> 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
On Tue, May 15, 2018 at 08:59:02AM +0200, Ulf Hansson wrote: > On 8 May 2018 at 21:06, Kim Phillips <kim.phillips@arm.com> 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.
On Tue, May 15, 2018 at 08:15:19AM -0500, Kim Phillips wrote: > On Tue, 15 May 2018 08:59:02 +0200 > Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > On 8 May 2018 at 21:06, Kim Phillips <kim.phillips@arm.com> 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.
On Tue, 15 May 2018 14:48:31 +0100 Russell King - ARM Linux <linux@armlinux.org.uk> 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 <ulf.hansson@linaro.org> wrote: > > > > > On 8 May 2018 at 21:06, Kim Phillips <kim.phillips@arm.com> 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
On 15 May 2018 at 08:37, Kim Phillips <kim.phillips@arm.com> wrote: > On Tue, 15 May 2018 14:48:31 +0100 > Russell King - ARM Linux <linux@armlinux.org.uk> 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 <ulf.hansson@linaro.org> wrote: >> > >> > > On 8 May 2018 at 21:06, Kim Phillips <kim.phillips@arm.com> 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
On 15 May 2018 at 15:41, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > On Tue, May 15, 2018 at 08:59:02AM +0200, Ulf Hansson wrote: >> On 8 May 2018 at 21:06, Kim Phillips <kim.phillips@arm.com> 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
On Tue, May 8, 2018 at 10:06 PM, Kim Phillips <kim.phillips@arm.com> 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
On Wed, May 16, 2018 at 12:16:28PM +0300, Andy Shevchenko wrote: > On Tue, May 8, 2018 at 10:06 PM, Kim Phillips <kim.phillips@arm.com> 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?
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 <kim.phillips@arm.com> 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.
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) {
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 <mathieu.poirier@linaro.org> Cc: Alex Williamson <alex.williamson@redhat.com> Cc: Eric Auger <eric.auger@redhat.com> Cc: Russell King <linux@armlinux.org.uk> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Todd Kjos <tkjos@google.com> Cc: Geert Uytterhoeven <geert+renesas@glider.be> Cc: Thierry Reding <treding@nvidia.com> Cc: Robin Murphy <robin.murphy@arm.com> Signed-off-by: Kim Phillips <kim.phillips@arm.com> --- 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(+)