Re: [PATCH v2] staging: intel-gwdpa: gswip: Introduce Gigabit Ethernet Switch (GSWIP) device driver

[Greg KH]

On Wed, Dec 11, 2019 at 04:57:28PM +0800, Jack Ping CHNG wrote:
> This driver enables the Intel's LGM SoC GSWIP block.
> GSWIP is a core module tailored for L2/L3/L4+ data plane and QoS functions.
> It allows CPUs and other accelerators connected to the SoC datapath
> to enqueue and dequeue packets through DMAs.
> Most configuration values are stored in tables such as
> Parsing and Classification Engine tables, Buffer Manager tables and
> Pseudo MAC tables.

Odd line wrapping :(

> Signed-off-by: Jack Ping CHNG <jack.ping.chng@xxxxxxxxx>
> Signed-off-by: Amireddy Mallikarjuna reddy <mallikarjunax.reddy@xxxxxxxxxxxxxxx>
> ---
> Changes on v2:
> - Renamed intel-dpa to intel-gwdpa
> - Added intel-gwdpa.txt(Intel Gateway Datapath Architecture)
> - Added TODO (upstream plan)

I don't see a real plan here.

>  drivers/staging/intel-gwdpa/TODO               |  52 ++

Good, a TODO file!  Let's look at it:

> --- /dev/null
> +++ b/drivers/staging/intel-gwdpa/TODO
> @@ -0,0 +1,52 @@
> +Intel gateway datapath architecture framework (gwdpa)
> +=====================================================
> +
> +Drivers for gwdpa
> +-----------------
> +1. drivers/staging/intel-gwdpa/gswip
> +        patch: switch driver (GSWIP)
> +
> +2. drivers/staging/intel-gwdpa/cqm
> +        patch: queue manager (CQM)
> +
> +3. drivers/staging/intel-gwdpa/pp
> +        patch: packet processor (pp)
> +
> +4. drivers/staging/intel-gwdpa/dpm
> +        patch: datapath manager (DPM)
> +        dependencies: GSWIP, CQM, PP
> +
> +5. driver/net/ethernet/intel
> +        patch: ethernet driver
> +        dependencies: DPM
> +
> +6. drivers/staging/intel-gwdpa/dcdp
> +        patch: direct connect datapath (DCDP)
> +        dependencies: DPM
> +
> +7.1 drivers/net/wireless
> +7.2 drivers/net/wan
> +        patch: wireless driver and DSL driver
> +        dependencies: DCDP

I don't understand any of the above at all.  What does it mean?  Why is
it here?

If I can't understand it, how can someone new to the kernel know what
this is for?

> +Upstream plan
> +--------------
> +
> +      GSWIP  CQM  PP  DPM     DCDP
> +        |     |    |   |        |
> +        |     |    |   |        |
> +        V     V    V   V        V
> +        -------------------------------------( drivers/staging/intel-gwdpa/* )
> +                            |  (move to soc folder)
> +                            V
> +                    -------------------------( drivers/soc/intel/gwdpa-*/* )
> +
> +                            Eth driver  Wireless/
> +                                |       WAN driver
> +                                |         |
> +                                V         V
> +                             ----------------( drivers/net/ethernet/intel )
> +                                             ( drivers/net/wireless )
> +                                             ( drivers/net/wan)
> +
> +* Each driver will have a TODO list.

Again, what kind of plan is this?  It's just a "these files need to be
moved to this location" plan?

Why not do that today?

What is keeping this code from being accepted in the "correct" place
today?  And why do you want it in staging?  You know it takes even more
work to do things here, right?  Are you ready to sign up for that work
(hint, you didn't add your names to the MAINTAINER file, so I worry
about that...)

> diff --git a/drivers/staging/intel-gwdpa/gswip/TODO b/drivers/staging/intel-gwdpa/gswip/TODO
> new file mode 100644
> index 000000000000..fa46170c8260
> --- /dev/null
> +++ b/drivers/staging/intel-gwdpa/gswip/TODO
> @@ -0,0 +1,4 @@
> +- Add debugfs for core and mac.

Why is this a requirement for upstream support?  No code functionality
should ever depend on debugfs for working properly, it's just there for
debugging at random times.

> +- Expand APIs for PCE, BM and PMAC tables to support
> +  QoS, OMCI.

What does this mean?

> +- Add ops for core.

What does this mean?

Please provide much better descriptions here of exactly what needs to be
done, that explains why this code needs to go in this part of the kernel
now instead of the "real" part.

As it is, I can not take this patch now.

thanks,

greg k-h