Re: [PATCH 03/11] soc/fsl: Introduce the DPAA BMan portal driver
Hi Roy, On vr, 2015-07-10 at 15:19 +, Roy Pledge wrote: > Thanks you for your valuable feedback so far. You're welcome. Please note that I just scan for, well, common build issues. Ie, stuff that requires no domain specific knowledge. > Let me try to address a general issue you mention below: unused > exported APIs. Good timing. I was pondering how to handle 04/11, a big patch that exports 79 (!) symbols. Many of those appear to have no users. > The QMan and BMan drivers provide a base layer for other blocks built > on top of them, for instance an Ethernet Driver, an Encrypt/Decrypt > Engine, a pattern matcher, a compress/decompress engine, etc... > Some of these drivers will be presented for review in the near future, > but in order to try and layer the review/up streaming process we're > presenting each layer as independently as possible. > If you really were to start dissecting which APIs are called you would > come to a very small set of pieces that merely initialize the hardware > but don't provide any opportunity for other users to invoke that HW. > > I hope that this helps you understand our goals in trying to upstream > these drivers. At the end of the day, what matters is what the people that need to sign off on these drivers (ppc? netdev?) are comfortable with. I know that some maintainers won't even bother looking at code that has no callers. In the mean time I'll skip the exports for the remainder of this series. Thanks, Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 03/11] soc/fsl: Introduce the DPAA BMan portal driver
Paul,
Thanks you for your valuable feedback so far. Let me try to address a general
issue you mention below: unused exported APIs.
The QMan and BMan drivers provide a base layer for other blocks built on top of
them, for instance an Ethernet Driver, an Encrypt/Decrypt Engine, a pattern
matcher, a compress/decompress engine, etc...
Some of these drivers will be presented for review in the near future, but in
order to try and layer the review/up streaming process we're presenting each
layer as independently as possible.
If you really were to start dissecting which APIs are called you would come to
a very small set of pieces that merely initialize the hardware but don't
provide any opportunity for other users to invoke that HW.
I hope that this helps you understand our goals in trying to upstream these
drivers.
Roy
> -Original Message-
> From: Paul Bolle [mailto:pebo...@tiscali.nl]
> Sent: Friday, July 10, 2015 9:33 AM
> To: Pledge Roy-R01356
> Cc: linuxppc-...@lists.ozlabs.org; Wood Scott-B07421;
> net...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 03/11] soc/fsl: Introduce the DPAA BMan portal driver
>
> On do, 2015-07-09 at 16:21 -0400, Roy Pledge wrote:
> > --- a/drivers/soc/fsl/qbman/Kconfig
> > +++ b/drivers/soc/fsl/qbman/Kconfig
>
> > +config FSL_DPA_PIRQ_FAST
> > + bool
> > + default y
>
> First used in 04/11.
>
> > +config FSL_DPA_PIRQ_SLOW
> > + bool
> > + default y
> > +
> > +config FSL_DPA_PORTAL_SHARE
> > + bool
> > + default y
>
> As in 02/11: these three symbols function as aliases for FSL_DPA. Why are
> they needed?
>
> > config FSL_BMAN
> > tristate "BMan device management"
> > default n
> > help
> > FSL DPAA BMan driver
> >
> > +config FSL_BMAN_PORTAL
> > + tristate "BMan portal(s)"
> > + default n
> > + help
> > + FSL BMan portal driver
>
> > --- /dev/null
> > +++ b/drivers/soc/fsl/qbman/bman_api.c
>
> > +struct bman_portal {
> > + [...]
> > +#ifdef CONFIG_FSL_DPA_CAN_WAIT_SYNC
>
> This check will always evaluate to true, right? (I'll only report this
> once.)
>
> > + struct bman_pool *rcri_owned; /* only 1 release WAIT_SYNC at
> > a time */
> > +#endif
> > +#ifdef CONFIG_FSL_DPA_PORTAL_SHARE
>
> Ditto.
>
> > + raw_spinlock_t sharing_lock; /* only used if is_shared */
> > + int is_shared;
> > + struct bman_portal *sharing_redirect; #endif
> > + [...]
> > +};
>
> > +const struct bman_portal_config *bman_get_portal_config(void) {
> > + [...]
> > +}
>
> I couldn't find callers of this function.
>
> > +EXPORT_SYMBOL(bman_get_portal_config);
>
> Nor users of this export, obviously.
>
> > +
> > +u32 bman_irqsource_get(void)
> > +{
> > + [...]
> > +}
>
> Ditto.
>
> > +EXPORT_SYMBOL(bman_irqsource_get);
>
> Ditto.
>
> > +int bman_p_irqsource_add(struct bman_portal *p, __maybe_unused u32
> > +bits) {
> > + [...]
> > +}
> > +EXPORT_SYMBOL(bman_p_irqsource_add);
>
> There seem to be no users of this export.
>
> > +int bman_irqsource_add(__maybe_unused u32 bits) {
> > + [...]
> > +}
>
> Unused.
>
> > +EXPORT_SYMBOL(bman_irqsource_add);
>
> Ditto.
>
> > +int bman_irqsource_remove(u32 bits)
> > +{
> > + [...]
> > +}
>
> Ditto.
>
> > +EXPORT_SYMBOL(bman_irqsource_remove);
>
> Ditto.
>
> > +u32 bman_poll_slow(void)
> > +{
> > + [...]
> > +}
>
> Ditto.
>
> > +EXPORT_SYMBOL(bman_poll_slow);
>
> Ditto.
>
> > +void bman_poll(void)
> > +{
> > + [...]
> > +}
>
> Ditto.
>
> > +EXPORT_SYMBOL(bman_poll);
>
> Ditto.
>
> > +static inline struct bm_rcr_entry *try_rel_start(struct bman_portal
> > +**p, #ifdef CONFIG_FSL_DPA_CAN_WAIT
>
> Always true, right?
>
> > + __maybe_unused struct bman_pool
> *pool, #endif
> > + __maybe_unused unsigned long
> *irqflags,
> > + __maybe_unused u32 flags)
>
> And this is a, well, novel way to declare a function.
>
> > +{
> > + [...]
> > +}
>
> > +int bman_flush_stockpile(struct bman_pool *pool, u32 flags) {
> > + [...]
> > +}
>
> Unused function.
>
> > +EXPORT_SYMBOL(bman_flush_stockpile);
>
> So unused export too.
>
> > +#ifdef CONF
Re: [PATCH 03/11] soc/fsl: Introduce the DPAA BMan portal driver
On do, 2015-07-09 at 16:21 -0400, Roy Pledge wrote:
> --- a/drivers/soc/fsl/qbman/Kconfig
> +++ b/drivers/soc/fsl/qbman/Kconfig
> +config FSL_DPA_PIRQ_FAST
> + bool
> + default y
First used in 04/11.
> +config FSL_DPA_PIRQ_SLOW
> + bool
> + default y
> +
> +config FSL_DPA_PORTAL_SHARE
> + bool
> + default y
As in 02/11: these three symbols function as aliases for FSL_DPA. Why
are they needed?
> config FSL_BMAN
> tristate "BMan device management"
> default n
> help
> FSL DPAA BMan driver
>
> +config FSL_BMAN_PORTAL
> + tristate "BMan portal(s)"
> + default n
> + help
> + FSL BMan portal driver
> --- /dev/null
> +++ b/drivers/soc/fsl/qbman/bman_api.c
> +struct bman_portal {
> + [...]
> +#ifdef CONFIG_FSL_DPA_CAN_WAIT_SYNC
This check will always evaluate to true, right? (I'll only report this
once.)
> + struct bman_pool *rcri_owned; /* only 1 release WAIT_SYNC at
> a time */
> +#endif
> +#ifdef CONFIG_FSL_DPA_PORTAL_SHARE
Ditto.
> + raw_spinlock_t sharing_lock; /* only used if is_shared */
> + int is_shared;
> + struct bman_portal *sharing_redirect;
> +#endif
> + [...]
> +};
> +const struct bman_portal_config *bman_get_portal_config(void)
> +{
> + [...]
> +}
I couldn't find callers of this function.
> +EXPORT_SYMBOL(bman_get_portal_config);
Nor users of this export, obviously.
> +
> +u32 bman_irqsource_get(void)
> +{
> + [...]
> +}
Ditto.
> +EXPORT_SYMBOL(bman_irqsource_get);
Ditto.
> +int bman_p_irqsource_add(struct bman_portal *p, __maybe_unused u32 bits)
> +{
> + [...]
> +}
> +EXPORT_SYMBOL(bman_p_irqsource_add);
There seem to be no users of this export.
> +int bman_irqsource_add(__maybe_unused u32 bits)
> +{
> + [...]
> +}
Unused.
> +EXPORT_SYMBOL(bman_irqsource_add);
Ditto.
> +int bman_irqsource_remove(u32 bits)
> +{
> + [...]
> +}
Ditto.
> +EXPORT_SYMBOL(bman_irqsource_remove);
Ditto.
> +u32 bman_poll_slow(void)
> +{
> + [...]
> +}
Ditto.
> +EXPORT_SYMBOL(bman_poll_slow);
Ditto.
> +void bman_poll(void)
> +{
> + [...]
> +}
Ditto.
> +EXPORT_SYMBOL(bman_poll);
Ditto.
> +static inline struct bm_rcr_entry *try_rel_start(struct bman_portal **p,
> +#ifdef CONFIG_FSL_DPA_CAN_WAIT
Always true, right?
> + __maybe_unused struct bman_pool *pool,
> +#endif
> + __maybe_unused unsigned long *irqflags,
> + __maybe_unused u32 flags)
And this is a, well, novel way to declare a function.
> +{
> + [...]
> +}
> +int bman_flush_stockpile(struct bman_pool *pool, u32 flags)
> +{
> + [...]
> +}
Unused function.
> +EXPORT_SYMBOL(bman_flush_stockpile);
So unused export too.
> +#ifdef CONFIG_FSL_BMAN
> +u32 bman_query_free_buffers(struct bman_pool *pool)
> +{
> + return bm_pool_free_buffers(pool->params.bpid);
> +}
> +EXPORT_SYMBOL(bman_query_free_buffers);
> +
> +int bman_update_pool_thresholds(struct bman_pool *pool, const u32
> *thresholds)
> +{
> + u32 bpid;
> +
> + bpid = bman_get_params(pool)->bpid;
> +
> + return bm_pool_set(bpid, thresholds);
> +}
> +EXPORT_SYMBOL(bman_update_pool_thresholds);
More of the same.
> +#endif
> --- /dev/null
> +++ b/drivers/soc/fsl/qbman/bman_portal.c
>
> +module_driver(bman_portal_driver,
> +bman_portal_driver_register, platform_driver_unregister);
No MODULE_LICENSE() here, nor in the other files that make up this
module. So loading this module will trigger a warning and taint the
kernel.
> --- /dev/null
> +++ b/drivers/soc/fsl/qbman/bman_utils.c
> +EXPORT_SYMBOL(bman_alloc_bpid_range);
Unused export.
> +EXPORT_SYMBOL(bman_release_bpid_range);
Ditto.
> +EXPORT_SYMBOL(bman_seed_bpid_range);
Ditto.
> +int bman_reserve_bpid_range(u32 bpid, u32 count)
> +{
> + return dpaa_resource_reserve(&bpalloc, bpid, count);
> +}
> +EXPORT_SYMBOL(bman_reserve_bpid_range);
Because bman_reserve_bpid() is unused, see below, this function and this
export are unused too.
> --- /dev/null
> +++ b/drivers/soc/fsl/qbman/dpaa_resource.c
>
> +#if defined(CONFIG_FSL_BMAN_PORTAL) || defined(CONFIG_FSL_BMAN_PORTAL_MODULE)
#if IS_ENABLED(CONFIG_FSL_BMAN_PORTAL)
> +#ifdef DPAA_RESOURCE_DEBUG
Never defined. So DUMP() is dead code.
> --- a/drivers/soc/fsl/qbman/dpaa_sys.h
> +++ b/drivers/soc/fsl/qbman/dpaa_sys.h
>
> +#define CONFIG_TRY_BETTER_MEMCPY
Please replace the CONFIG_ prefix with something else.
> +#ifdef CONFIG_TRY_BETTER_MEMCPY
This will always be true, right?
> [...]
> +#else
> +#define copy_words memcpy
> +#define copy_shorts memcpy
> +#define copy_bytes memcpy
> +#endif
> --- /dev/null
> +++ b/include/soc/fsl/bman.h
> +static inline int bman_reserve_bpid(u32 bpid)
> +{
> + return bman_reserve_bpid_range(bpid, 1);
> +}
Unused.
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord.
[PATCH 03/11] soc/fsl: Introduce the DPAA BMan portal driver
From: Geoff Thorpe
Add support for Freescale DPAA 1.0 Buffer Manager portals. These portals
allow software drivers for accelerators connected to the datapath to
manage the hardware buffer pools.
Signed-off-by: Geoff Thorpe
Signed-off-by: Emil Medve
Signed-off-by: Roy Pledge
---
drivers/soc/fsl/qbman/Kconfig | 18 +
drivers/soc/fsl/qbman/Makefile|6 +
drivers/soc/fsl/qbman/bman.c |1 +
drivers/soc/fsl/qbman/bman.h | 542 +
drivers/soc/fsl/qbman/bman_api.c | 1048 +
drivers/soc/fsl/qbman/bman_portal.c | 351 +++
drivers/soc/fsl/qbman/bman_priv.h | 81 +++
drivers/soc/fsl/qbman/bman_utils.c| 72 +++
drivers/soc/fsl/qbman/dpaa_resource.c | 356 +++
drivers/soc/fsl/qbman/dpaa_sys.h | 187 ++
include/soc/fsl/bman.h| 514
11 files changed, 3176 insertions(+)
create mode 100644 drivers/soc/fsl/qbman/bman.h
create mode 100644 drivers/soc/fsl/qbman/bman_api.c
create mode 100644 drivers/soc/fsl/qbman/bman_portal.c
create mode 100644 drivers/soc/fsl/qbman/bman_utils.c
create mode 100644 drivers/soc/fsl/qbman/dpaa_resource.c
create mode 100644 include/soc/fsl/bman.h
diff --git a/drivers/soc/fsl/qbman/Kconfig b/drivers/soc/fsl/qbman/Kconfig
index a40ba7a..342a05e 100644
--- a/drivers/soc/fsl/qbman/Kconfig
+++ b/drivers/soc/fsl/qbman/Kconfig
@@ -24,10 +24,28 @@ config FSL_DPA_CAN_WAIT_SYNC
bool
default y
+config FSL_DPA_PIRQ_FAST
+ bool
+ default y
+
+config FSL_DPA_PIRQ_SLOW
+ bool
+ default y
+
+config FSL_DPA_PORTAL_SHARE
+ bool
+ default y
+
config FSL_BMAN
tristate "BMan device management"
default n
help
FSL DPAA BMan driver
+config FSL_BMAN_PORTAL
+ tristate "BMan portal(s)"
+ default n
+ help
+ FSL BMan portal driver
+
endif # FSL_DPA
diff --git a/drivers/soc/fsl/qbman/Makefile b/drivers/soc/fsl/qbman/Makefile
index 02014d9..d5a595d 100644
--- a/drivers/soc/fsl/qbman/Makefile
+++ b/drivers/soc/fsl/qbman/Makefile
@@ -1 +1,7 @@
+# Common
+obj-$(CONFIG_FSL_DPA) += dpaa_resource.o
+
obj-$(CONFIG_FSL_BMAN) += bman.o
+obj-$(CONFIG_FSL_BMAN_PORTAL) += bman-portal.o
+bman-portal-y = bman_portal.o bman_api.o
\
+ bman_utils.o
diff --git a/drivers/soc/fsl/qbman/bman.c b/drivers/soc/fsl/qbman/bman.c
index 9a500ce..4fcabb7 100644
--- a/drivers/soc/fsl/qbman/bman.c
+++ b/drivers/soc/fsl/qbman/bman.c
@@ -341,6 +341,7 @@ u32 bm_pool_free_buffers(u32 bpid)
{
return bm_in(POOL_CONTENT(bpid));
}
+EXPORT_SYMBOL(bm_pool_free_buffers);
static ssize_t show_fbpr_fpc(struct device *dev,
struct device_attribute *dev_attr, char *buf)
diff --git a/drivers/soc/fsl/qbman/bman.h b/drivers/soc/fsl/qbman/bman.h
new file mode 100644
index 000..c987938
--- /dev/null
+++ b/drivers/soc/fsl/qbman/bman.h
@@ -0,0 +1,542 @@
+/* Copyright 2008 - 2015 Freescale Semiconductor, Inc.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ * * Neither the name of Freescale Semiconductor nor the
+ * names of its contributors may be used to endorse or promote products
+ * derived from this software without specific prior written permission.
+ *
+ * ALTERNATIVELY, this software may be distributed under the terms of the
+ * GNU General Public License ("GPL") as published by the Free Software
+ * Foundation, either version 2 of that License or (at your option) any
+ * later version.
+ *
+ * THIS SOFTWARE IS PROVIDED BY Freescale Semiconductor ``AS IS'' AND ANY
+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+ * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+ * DISCLAIMED. IN NO EVENT SHALL Freescale Semiconductor BE LIABLE FOR ANY
+ * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+ * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
THIS
+ * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include "bman_priv.h"
+
+extern u16 bman_pool
