Re: [PATCH 08/12] fsl/fman: Add Frame Manager support
You should note that I started with scanning this patch for basic, say,
build system issues. Which I did find. But then I kept spotting all kind
of oddities. After a while I stopped looking closely. So, to note
something, I haven't yet looked into the 24 symbols this series exports.
There might be a reason for all 24 of them, but I just thought it a bit
suspect.
But, anyhow, my guess is this series needs a close look or two before
the people understanding ethernet drivers (I'm not one of them) will
consider reviewing it for more substantial issues.
On Wed, 2015-06-10 at 18:21 +0300, Madalin Bucur wrote:
> --- a/drivers/net/ethernet/freescale/fman/Kconfig
> +++ b/drivers/net/ethernet/freescale/fman/Kconfig
> +if FSL_FMAN
> +
> +config FSL_FM_MAX_FRAME_SIZE
> + int "Maximum L2 frame size"
> + depends on FSL_FMAN
This dependency is already provided through "if FSL_MAN" above.
> + range 64 9600
> + default "1522"
> + help
> + Configure this in relation to the maximum possible MTU of your
> + network configuration. In particular, one would need to
> + increase this value in order to use jumbo frames.
> + FSL_FM_MAX_FRAME_SIZE must accommodate the Ethernet FCS
> + (4 bytes) and one ETH+VLAN header (18 bytes), to a total of
> + 22 bytes in excess of the desired L3 MTU.
> +
> + Note that having too large a FSL_FM_MAX_FRAME_SIZE (much larger
> + than the actual MTU) may lead to buffer exhaustion, especially
> + in the case of badly fragmented datagrams on the Rx path.
> + Conversely, having a FSL_FM_MAX_FRAME_SIZE smaller than the
> + actual MTU will lead to frames being dropped.
> +
> +config FSL_FM_RX_EXTRA_HEADROOM
> + int "Add extra headroom at beginning of data buffers"
> + depends on FSL_FMAN
Ditto.
> + range 16 384
> + default "64"
> + help
> + Configure this to tell the Frame Manager to reserve some extra
> + space at the beginning of a data buffer on the receive path,
> + before Internal Context fields are copied. This is in addition
> + to the private data area already reserved for driver internal
> + use. The provided value must be a multiple of 16.
> +
> + This option does not affect in any way the layout of
> + transmitted buffers.
> +
> +endif# FSL_FMAN
> --- a/drivers/net/ethernet/freescale/fman/Makefile
> +++ b/drivers/net/ethernet/freescale/fman/Makefile
>
> obj-y+= fsl_fman.o
>
> -fsl_fman-objs:= fman.o fm_muram.o
> +fsl_fman-objs:= fman.o fm_muram.o fm.o fm_drv.o
> --- /dev/null
> +++ b/drivers/net/ethernet/freescale/fman/fm.c
> +/* static functions */
There's no need to tell us that.
> +static int check_fm_parameters(struct fm_t *p_fm)
> +{
> +#ifdef FM_AID_MODE_NO_TNUM_SW005
I think this is always defined (I already deleted that part of the
patch, so perhaps I'm missing some subtle issue).
> + if (p_fm->p_fm_state_struct->rev_info.major_rev >= 6 &&
> + p_fm->p_fm_drv_param->dma_aid_mode !=
> + E_FMAN_DMA_AID_OUT_PORT_ID) {
> + pr_err("dma_aid_mode not supported by this
> integration.\n");
> + return -EDOM;
> + }
> +#endif /* FM_AID_MODE_NO_TNUM_SW005 */
> +#ifdef FM_HAS_TOTAL_DMAS
Ditto.
> + if ((p_fm->p_fm_state_struct->rev_info.major_rev < 6) &&
> + (!p_fm->p_fm_state_struct->max_num_of_open_dmas ||
> + (p_fm->p_fm_state_struct->max_num_of_open_dmas >
> + p_fm->intg->bmi_max_num_of_dmas))) {
> + pr_err("max_num_of_open_dmas number has to be in the range 1 -
> %d\n",
> +p_fm->intg->bmi_max_num_of_dmas);
> + return -EDOM;
> + }
> +#endif /* FM_HAS_TOTAL_DMAS */
> +#ifdef FM_NO_WATCHDOG
Ditto. I'll stop checking for this stuff now. Please clean them up (or
show me that I'm wrong).
> + if ((p_fm->p_fm_state_struct->rev_info.major_rev == 2) &&
> + (p_fm->p_fm_drv_param->dma_watchdog)) {
> + pr_err("watchdog!\n");
> + return -EINVAL;
> + }
> +#endif /* FM_NO_WATCHDOG */
> +/* fm_init
> + *
> + * Initializes the FM module
> + *
> + * @Param[in] p_fm - FM module descriptor
> + *
> + * @Return0 on success; Error code otherwise.
> + */
I know little about kerneldoc, but this is not kerneldoc, right?
> +/* fm_free
> + * Frees all resources that were assigned to FM module.
> + * Calling this routine invalidates the descriptor.
> + * p_fm - FM module descriptor
> + *Return0 on success; Error code otherwise.
> + */
Ditto.
> +void fm_event_isr(struct fm_t *p_fm)
> +{
> +#define FM_M_CALL_MAC_ISR(_id)\
> + (p_fm->intr_mng[(enum fm_inter_module_event)(FM_EV_MAC0 + _id)]. \
> + f_isr(p_fm->intr_mng[(enum fm_inter_module_event)(FM_EV_MAC0
Re: [PATCH 08/12] fsl/fman: Add Frame Manager support
So I couldn't help having yet another look at the code, just to drive
home my point.
On Thu, 2015-06-11 at 10:55 +0200, Paul Bolle wrote:
> > +void *fm_drv_init(void)
>
> static.
>
> > +{
> > + memset(&fm_drvs, 0, sizeof(fm_drvs));
fm_drvs is an external variable. It is guaranteed to be zero, isn't it?
> > + mutex_init(&fm_drv_mutex);
> > +
> > + /* Register to the DTB for basic FM API */
> > + platform_driver_register(&fm_driver);
> > +
> > + return &fm_drvs;
You're returning a pointer to external variable. How's that useful?
And note this is the last time we'll ever see fm_drvs. So I think that
all this variable does for the code is getting initialized to zero,
twice.
> > +}
> > +
> > +int fm_drv_free(void *p_fm_drv)
>
> static.
>
> > +{
> > + platform_driver_unregister(&fm_driver);
> > + mutex_destroy(&fm_drv_mutex);
> > +
> > + return 0;
This function has one caller, which doesn't check the return value. So
this should be a function returning void. Of course, a wrapper of two
lines called only once means you should actually not put this into a
separate function.
> > +}
> > +static void *p_fm_drv;
>
> > +static int __init __cold fm_load(void)
> > +{
> > + p_fm_drv = fm_drv_init();
> > + if (!p_fm_drv) {
fm_drv_init() returns a pointer to an external variable. So how can this
happen?
> > + pr_err("Failed to init FM wrapper!\n");
> > + return -ENODEV;
> > + }
> > +
> > + pr_info("Freescale FM module\n");
> > + return 0;
> > +}
This is all rather basic. It must be, otherwise I wouldn't spot it.
So I keep spotting these basic oddities, with every cup of coffee I
treat myself to while reading through this, wherever I look. By now I'm
sure there's no need for the netdev people to look at this, not yet.
Paul Bolle
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH 08/12] fsl/fman: Add Frame Manager support
Hi Paul,
Thank you very much for your feedback.
I'm planning to address the issues you've raised in the next submission.
Regards,
Igal Liberman.
> -Original Message-
> From: Paul Bolle [mailto:pebo...@tiscali.nl]
> Sent: Thursday, June 11, 2015 12:38 PM
> To: Bucur Madalin-Cristian-B32716
> Cc: net...@vger.kernel.org; linux-ker...@vger.kernel.org; linuxppc-
> d...@lists.ozlabs.org; Wood Scott-B07421; Liberman Igal-B31950
> Subject: Re: [PATCH 08/12] fsl/fman: Add Frame Manager support
>
> So I couldn't help having yet another look at the code, just to drive home my
> point.
>
> On Thu, 2015-06-11 at 10:55 +0200, Paul Bolle wrote:
> > > +void *fm_drv_init(void)
> >
> > static.
> >
> > > +{
> > > + memset(&fm_drvs, 0, sizeof(fm_drvs));
>
> fm_drvs is an external variable. It is guaranteed to be zero, isn't it?
>
> > > + mutex_init(&fm_drv_mutex);
> > > +
> > > + /* Register to the DTB for basic FM API */
> > > + platform_driver_register(&fm_driver);
> > > +
> > > + return &fm_drvs;
>
> You're returning a pointer to external variable. How's that useful?
>
> And note this is the last time we'll ever see fm_drvs. So I think that all
> this
> variable does for the code is getting initialized to zero, twice.
>
> > > +}
> > > +
> > > +int fm_drv_free(void *p_fm_drv)
> >
> > static.
> >
> > > +{
> > > + platform_driver_unregister(&fm_driver);
> > > + mutex_destroy(&fm_drv_mutex);
> > > +
> > > + return 0;
>
> This function has one caller, which doesn't check the return value. So this
> should be a function returning void. Of course, a wrapper of two lines called
> only once means you should actually not put this into a separate function.
>
> > > +}
>
> > > +static void *p_fm_drv;
> >
> > > +static int __init __cold fm_load(void) {
> > > + p_fm_drv = fm_drv_init();
> > > + if (!p_fm_drv) {
>
> fm_drv_init() returns a pointer to an external variable. So how can this
> happen?
>
> > > + pr_err("Failed to init FM wrapper!\n");
> > > + return -ENODEV;
> > > + }
> > > +
> > > + pr_info("Freescale FM module\n");
> > > + return 0;
> > > +}
>
> This is all rather basic. It must be, otherwise I wouldn't spot it.
>
> So I keep spotting these basic oddities, with every cup of coffee I treat
> myself to while reading through this, wherever I look. By now I'm sure
> there's no need for the netdev people to look at this, not yet.
>
>
> Paul Bolle
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 08/12] fsl/fman: Add Frame Manager support
On 06/10/2015 11:21 AM, Madalin Bucur wrote:
From: Igal Liberman
Add Frame Manger Driver support.
This patch adds The FMan configuration, initialization and
runtime control routines.
Signed-off-by: Igal Liberman
---
drivers/net/ethernet/freescale/fman/Kconfig| 37 +
drivers/net/ethernet/freescale/fman/Makefile |2 +-
drivers/net/ethernet/freescale/fman/fm.c | 1478
drivers/net/ethernet/freescale/fman/fm.h | 404 ++
drivers/net/ethernet/freescale/fman/fm_common.h| 367 +
drivers/net/ethernet/freescale/fman/fm_drv.c | 827 +++
drivers/net/ethernet/freescale/fman/fm_drv.h | 123 ++
drivers/net/ethernet/freescale/fman/inc/enet_ext.h | 199 +++
drivers/net/ethernet/freescale/fman/inc/fm_ext.h | 453 ++
.../net/ethernet/freescale/fman/inc/fsl_fman_drv.h | 94 ++
drivers/net/ethernet/freescale/fman/inc/net_ext.h | 534 +++
drivers/net/ethernet/freescale/fman/inc/service.h | 90 ++
12 files changed, 4607 insertions(+), 1 deletion(-)
create mode 100644 drivers/net/ethernet/freescale/fman/fm.c
create mode 100644 drivers/net/ethernet/freescale/fman/fm.h
create mode 100644 drivers/net/ethernet/freescale/fman/fm_common.h
create mode 100644 drivers/net/ethernet/freescale/fman/fm_drv.c
create mode 100644 drivers/net/ethernet/freescale/fman/fm_drv.h
create mode 100644 drivers/net/ethernet/freescale/fman/inc/enet_ext.h
create mode 100644 drivers/net/ethernet/freescale/fman/inc/fm_ext.h
create mode 100644 drivers/net/ethernet/freescale/fman/inc/fsl_fman_drv.h
create mode 100644 drivers/net/ethernet/freescale/fman/inc/net_ext.h
create mode 100644 drivers/net/ethernet/freescale/fman/inc/service.h
-- cut ---
+
+#endif /* __FM_H */
diff --git a/drivers/net/ethernet/freescale/fman/fm_common.h
b/drivers/net/ethernet/freescale/fman/fm_common.h
new file mode 100644
index 000..125c057
--- /dev/null
+++ b/drivers/net/ethernet/freescale/fman/fm_common.h
@@ -0,0 +1,367 @@
+/*
+ * 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.
+ */
+
+/* File fm_common.h
+ * Description FM internal structures and definitions.
+ */
+#ifndef __FM_COMMON_H
+#define __FM_COMMON_H
+
+#include "service.h"
+#include "fm_ext.h"
+
+/* Uniqe defines */
Unique instead of Uniqe?
+#define FM_QMI_NO_ECC_EXCEPTIONS /* P1 */
+#define FM_CSI_CFED_LIMIT /* P1 */
+#define FM_PEDANTIC_DMA/* P1 */
+#define FM_QMI_NO_DEQ_OPTIONS_SUPPORT /* P1 */
+#define FM_HAS_TOTAL_DMAS /* P1-P5 */
+#define FM_DEQ_PIPELINE_PARAMS_FOR_OP /* P1, T/B */
+#define FM_NO_DISPATCH_RAM_ECC /* P2-P5 */
+#define FM_NO_WATCHDOG /* P4 */
+#define FM_NO_TNUM_AGING /* P2-P5 */
+#define FM_NO_BACKUP_POOLS /* P2-P5 */
+#define FM_NO_OP_OBSERVED_POOLS/* P2-P5, T/B */
+#define FM_NO_ADVANCED_RATE_LIMITER/* P2-P5 */
+#define FM_OP_OPEN_DMA_MIN_LIMIT /* T/B */
+#define FM_NO_RESTRICT_ON_ACCESS_RSRC /* T/B */
+#define FM_FRAME_END_PARAMS_FOR_OP
Re: [PATCH 08/12] fsl/fman: Add Frame Manager support
On Mon, 2015-06-15 at 23:42 -0400, Bob Cochran wrote:
> On 06/10/2015 11:21 AM, Madalin Bucur wrote:
> >
> > +#define FM_QMI_NO_ECC_EXCEPTIONS /* P1 */
> > +#define FM_CSI_CFED_LIMIT /* P1 */
> > +#define FM_PEDANTIC_DMA/* P1 */
> > +#define FM_QMI_NO_DEQ_OPTIONS_SUPPORT /* P1 */
> > +#define FM_HAS_TOTAL_DMAS /* P1-P5 */
> > +#define FM_DEQ_PIPELINE_PARAMS_FOR_OP /* P1, T/B */
> > +#define FM_NO_DISPATCH_RAM_ECC /* P2-P5 */
> > +#define FM_NO_WATCHDOG /* P4 */
> > +#define FM_NO_TNUM_AGING /* P2-P5 */
> > +#define FM_NO_BACKUP_POOLS /* P2-P5 */
> > +#define FM_NO_OP_OBSERVED_POOLS/* P2-P5, T/B */
> > +#define FM_NO_ADVANCED_RATE_LIMITER/* P2-P5 */
> > +#define FM_OP_OPEN_DMA_MIN_LIMIT /* T/B */
> > +#define FM_NO_RESTRICT_ON_ACCESS_RSRC /* T/B */
> > +#define FM_FRAME_END_PARAMS_FOR_OP /* T/B */
> > +#define FM_QMI_NO_SINGLE_ECC_EXCEPTION /* T/B */
> > +
> > +/* FMan Errata */
>
>
> Will there be documentation to let the user know how to turn off and
> on these errata (code fixes) ? From reviewing the source for some
> of the errata fixes, I gather it's not always automatic. I saw that
> booleans are sometimes used (e.g, bcb_workaround) along with HW
> block IP version information to either apply the errata or not.
The above defines need to go. The driver should support any supported
chip without compile-time knowledge. If they're meant to give the
*option* to optimize away things on chips that don't have certain
requirements, that should be limited to codepaths where testing shows
it makes a significant difference, and the choice of supported chips
(not a list of fman internal knobs) needs to be exposed to kconfig.
> Also, some of the comments and errata names below refer to
> information that is probably strictly internal to Freescale, so how
> can I be sure whether to apply these errata or not and their
> purpose? My concern is that some of these errata may be applied to
> my SoC by default when they shouldn't be, and I may not know if it's
> a potential problem. I saw this sort of thing in the SDK kernel
> when FMAN V3H errata fixes were applied to FMAN V3L and wrong values
> were set due to V3L having lesser capabilities than V3H.
Yes, the SDK kernel has problems with multiplatform support in the
FMan driver. We don't want to repeat those problems here.
> +
> > +static inline bool TRY_LOCK(spinlock_t *spinlock, volatile bool
> > *p_flag)
> > +{
> > + unsigned long int_flags;
> > +
> > + if (spinlock)
> > + spin_lock_irqsave(spinlock, int_flags);
> > + else
> > + local_irq_save(int_flags);
> > +
> > + if (*p_flag) {
> > + if (spinlock)
> > + spin_unlock_irqrestore(spinlock, int_flags);
> > + else
> > + local_irq_restore(int_flags);
> > + return false;
> > + }
> > + *p_flag = true;
> > +
> > + if (spinlock)
> > + spin_unlock_irqrestore(spinlock, int_flags);
> > + else
> > + local_irq_restore(int_flags);
> > +
> > + return true;
> > +}
> > +
> > +#define RELEASE_LOCK(_flag) (_flag = false)
Even aside from the question of why this wrapper is needed to begin
with, this is not a correct implementation of lock release. There's
nothing stopping either compiler or hardware reordering of the store
to _flag relative to any accesses to lock-protected data.
-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
