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

Groups > linux.kernel > #1422596 > unrolled thread

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

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: [PATCH net-next] net: ena: Add a driver for Amazon Elastic  Network Adapters (ENA) David Miller <davem@davemloft.net> - 2016-06-15 07:30 +0200
    Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic  Network Adapters (ENA) Matt Wilson <msw@amzn.com> - 2016-06-15 08:30 +0200
      Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic  Network Adapters (ENA) David Miller <davem@davemloft.net> - 2016-06-15 08:30 +0200
        Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic  Network Adapters (ENA) Matt Wilson <msw@amzn.com> - 2016-06-15 08:50 +0200

#1422596 — Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

From: Netanel Belgazal <netanel@annapurnalabs.com>
Date: Mon, 13 Jun 2016 11:46:13 +0300

> +#define ena_trc_dbg(format, arg...) \
> +	pr_debug("[ENA_COM: %s] " format, __func__, ##arg)
> +#define ena_trc_info(format, arg...) \
> +	pr_info("[ENA_COM: %s] " format, __func__, ##arg)
> +#define ena_trc_warn(format, arg...) \
> +	pr_warn("[ENA_COM: %s] " format, __func__, ##arg)
> +#define ena_trc_err(format, arg...) \
> +	pr_err("[ENA_COM: %s] " format, __func__, ##arg)

These custom tracing macros are quite inappropriate.

We have the function tracer in the kernel when that is needed.  So spitting
out __func__ all over the place is not something that should be found in
drivers these days.

And one can modify pr_fmt do make pr_debug et al. have whatever prefix
one wants.

I suspect there will be several rounds of review to weed out things
like this.  You can preempt a lot of that by removing as much in your
driver that the kernel has existing facilities for.

Thanks.

[toc] | [next] | [standalone]

#1422649

On Tue, Jun 14, 2016 at 10:25:16PM -0700, David Miller wrote:
> From: Netanel Belgazal <netanel@annapurnalabs.com>
> Date: Mon, 13 Jun 2016 11:46:13 +0300
> 
> > +#define ena_trc_dbg(format, arg...) \
> > +	pr_debug("[ENA_COM: %s] " format, __func__, ##arg)
> > +#define ena_trc_info(format, arg...) \
> > +	pr_info("[ENA_COM: %s] " format, __func__, ##arg)
> > +#define ena_trc_warn(format, arg...) \
> > +	pr_warn("[ENA_COM: %s] " format, __func__, ##arg)
> > +#define ena_trc_err(format, arg...) \
> > +	pr_err("[ENA_COM: %s] " format, __func__, ##arg)
> 
> These custom tracing macros are quite inappropriate.
> 
> We have the function tracer in the kernel when that is needed.  So spitting
> out __func__ all over the place is not something that should be found in
> drivers these days.

Point taken, though existing drivers (even fairly popular ones) also
aren't as clean as you might like. A quick look around...

msw@carbon:~/git/upstream/linux$ git grep -B1 '__func__' drivers/net/ethernet/ | grep -A1 '#define' 
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h-#define BNX2X_ERROR(fmt, ...)	    			    	     \
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h:	    pr_err("[%s:%d]" fmt, __func__, __LINE__, ##__VA_ARGS__)
[...]
drivers/net/ethernet/intel/ixgb/ixgb_osdep.h:#define ENTER() pr_debug("%s\n", __func__);

Like many other network drivers, some of this is common code used for
non-Linux systems, and that's why there is some overlap with Linux
facilities. For example, here's the common ENA parts as it's situated
in DPDK as a PMD:

   http://dpdk.org/browse/dpdk/tree/drivers/net/ena/base/ena_com.c

When you compare to the DPDK version you can see that the common code
has already been contextualized for Linux in this patch in
anticipation of this type of feedback. (e.g., ENA_SPINLOCK_LOCK() ->
spin_lock_irqsave(), etc., as that would obviously never fly).

The Linux-specific bits (ena_netdev.c, ena_ethtool.c, etc.) don't make
use of any of the overlapping functionality needed for the common
code.

> And one can modify pr_fmt do make pr_debug et al. have whatever prefix
> one wants.

Yup, that's an easy improvement.

> I suspect there will be several rounds of review to weed out things
> like this.  You can preempt a lot of that by removing as much in your
> driver that the kernel has existing facilities for.

Are there other things that jump out at you? I felt like this was
pretty good for an initial submission in terms of striking a balance
between using a portable core while avoiding a lot of compatibility
shims.

--msw

[toc] | [prev] | [next] | [standalone]

#1422652

From: Matt Wilson <msw@amzn.com>
Date: Tue, 14 Jun 2016 23:23:36 -0700

> Point taken, though existing drivers (even fairly popular ones) also
> aren't as clean as you might like. A quick look around...

Existing drivers do undesirable things, film at 11...

Yet are never a reason to accept such things in new drivers.

> Like many other network drivers, some of this is common code used for
> non-Linux systems, and that's why there is some overlap with Linux
> facilities.

Again, never an excuse for such things.

> Are there other things that jump out at you?

I review hundreds of patches a day, I invested what I was able to
before moving on to other people's work.

Other developers must help review such a large driver submission, it
can't all be on me.

[toc] | [prev] | [next] | [standalone]

#1422670

On Tue, Jun 14, 2016 at 11:27:09PM -0700, David Miller wrote:
> From: Matt Wilson <msw@amzn.com>
> Date: Tue, 14 Jun 2016 23:23:36 -0700
> 
> > Point taken, though existing drivers (even fairly popular ones) also
> > aren't as clean as you might like. A quick look around...
> 
> Existing drivers do undesirable things, film at 11...
> 
> Yet are never a reason to accept such things in new drivers.

I generally agree with this philosophy.

> > Like many other network drivers, some of this is common code used for
> > non-Linux systems, and that's why there is some overlap with Linux
> > facilities.
> 
> Again, never an excuse for such things.

I suppose I was just happy to have the *majorly* objectionable parts
cleaned up...

> > Are there other things that jump out at you?
> 
> I review hundreds of patches a day, I invested what I was able to
> before moving on to other people's work.

I wasn't asking you to do more, only if you had anything else you
wanted to say before Netanel sends a v2. 

> Other developers must help review such a large driver submission, it
> can't all be on me.

And I'm certainly not saying it's all on you. I've been reviewing this
with the team for quite a while to get it in pretty reasonable (IMHO)
shape.

--msw

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web