Re: [PATCH 02/11] soc/fsl: Introduce DPAA BMan device management driver

[Paul Bolle <pebolle@tiscali.nl>]

On do, 2015-07-09 at 16:21 -0400, Roy Pledge wrote:
> --- /dev/null
> +++ b/drivers/soc/fsl/qbman/Kconfig

> +menuconfig FSL_DPA
> +	bool "Freescale DPAA support"
> +	depends on FSL_SOC || COMPILE_TEST

(I already commented on COMPILE_TEST in a separate mail.)

> +	default n
> +	help
> +		FSL Data-Path Acceleration Architecture drivers
> +
> +		These are not the actual Ethernet driver(s)
> +
> +if FSL_DPA
> +
> +config FSL_DPA_CHECKING
> +	bool "additional driver checking"
> +	default n
> +	help
> +		Compiles in additional checks to sanity-check the drivers and
> +		any use of it by other code. Not recommended for performance

Only recommended for people that are certain none of the 100+ asserts
will ever trigger. See below.

> +config FSL_DPA_CAN_WAIT
> +	bool
> +	default y
> +
> +config FSL_DPA_CAN_WAIT_SYNC
> +	bool
> +	default y

Both these aren't actually used in this patch. The first patch that uses
them is 3/11.

Besides, the way these two symbols are implemented makes them function
as aliases for FSL_DPA. So why are they needed?

> +config FSL_BMAN
> +	tristate "BMan device management"
> +	default n
> +	help
> +		FSL DPAA BMan driver

(Will readers know what BMan means?)

> +endif # FSL_DPA

> --- /dev/null
> +++ b/drivers/soc/fsl/qbman/Makefile

> +obj-$(CONFIG_FSL_BMAN)				+= bman.o

> --- /dev/null
> +++ b/drivers/soc/fsl/qbman/bman.c

> +int bm_pool_set(u32 bpid, const u32 *thresholds)
> +{
> +	if (!bm)
> +		return -ENODEV;
> +	bm_set_pool(bm, bpid, thresholds[0], thresholds[1],
> +		thresholds[2], thresholds[3]);
> +	return 0;
> +}
> +EXPORT_SYMBOL(bm_pool_set);

Nit: the first (caller of this function and) user of this export is
added in 3/11.

I couldn't find a MODULE_LICENSE() in bman.c. So building this as a
module and loading that module will generate a warning and taint the
kernel.

> --- /dev/null
> +++ b/drivers/soc/fsl/qbman/dpaa_sys.h

> +#ifdef CONFIG_FSL_DPA_CHECKING
> +#define DPA_ASSERT(x) \
> +	do { \
> +		if (!(x)) { \
> +			pr_crit("ASSERT: (%s:%d) %s\n", __FILE__, __LINE__, \
> +				__stringify_1(x)); \
> +			dump_stack(); \
> +			panic("assertion failure"); \

Not my call, but why panic() here?

> +		} \
> +	} while (0)
> +#else
> +#define DPA_ASSERT(x)
> +#endif

Thanks,


Paul Bolle