Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1203060 > unrolled thread

Re: [v4, 0/9] Freescale DPAA FMan

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [v4, 0/9] Freescale DPAA FMan David Miller <davem@davemloft.net> - 2015-08-08 00:40 +0200
    RE: [v4, 0/9] Freescale DPAA FMan Liberman Igal <Igal.Liberman@freescale.com> - 2015-08-10 22:00 +0200

#1203060 — Re: [v4, 0/9] Freescale DPAA FMan

From: <igal.liberman@freescale.com>
Date: Wed, 5 Aug 2015 12:25:16 +0300

> The Freescale Data Path Acceleration Architecture (DPAA)
> is a set of hardware components on specific QorIQ multicore processors.
> This architecture provides the infrastructure to support simplified
> sharing of networking interfaces and accelerators by multiple CPU cores
> and the accelerators.

I think the directory and code structure of this new driver is
quite excessive.

Because you've split things up _so_ much, you have to have
all of these directories, and even worse and much more important
to me you have to export so many functions from one source file
to another.

I think this is way too much.

For example, in one file you have a bunch of initialization routines.
init_a(), init_b(), init_c(), and you export them all.  Then they
are always called in sequence:

	init_a();
	init_b();
	init_c();

This is completely pointless.  You just needed to export one
function which calls all three functions.

The namespace pollution of this driver is out of control.

You really need to completely rework the architecture and
layout of this driver before I will even begin to review it
again.

And the lack of review interest by other developers should be an
indication to you how undesirable this code submission is to read.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1204518

Hello David,

Thank you for your feedback.

I understand your concerns regarding the FMan driver, we've come a long way from where we started but still there are issues.
The community support is critical for getting the code to the desired quality level and I appreciate the support I receive from you and from the other previous reviewers.

In order to reduce the code scattering I plan to put together all the code for a certain IP block in one file.
For example FMan port in his current state in /drivers/net/freescale/fman/:
        flib (directory)
              ---- fsl_fman_port.h
        inc (directory)
              ---- fm_port_ext.h (API for other drivers/modules)
        port (directory)
              ---- fman_port.c (flib)
              ---- fm_port.c
              ---- fm_port.h
              ---- Makefile
        fm_port_drv.c (file)

New proposed structure in /drivers/net/freescale/fman/:
        fman_port_drv.c (includes simplified code from fm_port.c, fman_port.c and fm_port_drv.c)
        fman_port_drv.h (exported structures and API, minimal)

Of-course, I'll do the same for other modules (MAC, FMan itself).

After this structure change we get:
- Subdirectories completely removed
- Layering reduced, each module becomes much flatter, with one source and header file
- Fewer number of files (sources and headers)
- Namespace pollution drastically reduced
- General complexity of the driver reduced.

I would appreciate your comments about the steps described above.

Regards,
Igal

> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Saturday, August 08, 2015 1:31 AM
> To: Liberman Igal-B31950 <Igal.Liberman@freescale.com>
> Cc: netdev@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-
> kernel@vger.kernel.org; Wood Scott-B07421 <scottwood@freescale.com>;
> Bucur Madalin-Cristian-B32716 <madalin.bucur@freescale.com>;
> pebolle@tiscali.nl; joakim.tjernlund@transmode.se; ppc@mindchasers.com;
> stephen@networkplumber.org
> Subject: Re: [v4, 0/9] Freescale DPAA FMan
> 
> From: <igal.liberman@freescale.com>
> Date: Wed, 5 Aug 2015 12:25:16 +0300
> 
> > The Freescale Data Path Acceleration Architecture (DPAA) is a set of
> > hardware components on specific QorIQ multicore processors.
> > This architecture provides the infrastructure to support simplified
> > sharing of networking interfaces and accelerators by multiple CPU
> > cores and the accelerators.
> 
> I think the directory and code structure of this new driver is quite excessive.
> 
> Because you've split things up _so_ much, you have to have all of these
> directories, and even worse and much more important to me you have to
> export so many functions from one source file to another.
> 
> I think this is way too much.
> 
> For example, in one file you have a bunch of initialization routines.
> init_a(), init_b(), init_c(), and you export them all.  Then they are always
> called in sequence:
> 
> 	init_a();
> 	init_b();
> 	init_c();
> 
> This is completely pointless.  You just needed to export one function which
> calls all three functions.
> 
> The namespace pollution of this driver is out of control.
> 
> You really need to completely rework the architecture and layout of this
> driver before I will even begin to review it again.
> 
> And the lack of review interest by other developers should be an indication
> to you how undesirable this code submission is to read.
> 
> Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web