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: netdev@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
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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
