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; > netdev@vger.kernel.org; linux-ker...@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 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
