Re: [RFC 02/11] Add RoCE driver framework

2016-09-20 Thread Leon Romanovsky 
On Tue, Sep 20, 2016 at 03:04:12PM +, Elior, Ariel wrote:
> > From: Leon Romanovsky [mailto:l...@kernel.org]
> > On Thu, Sep 15, 2016 at 05:11:03AM +, Mintz, Yuval wrote:
> > > > As a summary, I didn't see in your responses any real life example 
> > > > where you will
> > > > need global debug level for your driver.
> > >
> > > Not sure what you you're expecting - a list of BZs /private e-mails where
> > > user reproductions were needed?
> > > You're basically ignoring my claims that such are used, instead wanting
> > > "evidence". I'm not going to try and produce any such.
> >
> > I asked an example and not evidence, where "modprobe your_driver
> > debug=1" will be superior to "modprobe your_driver dyndbg==pmf".
> >
> > https://www.kernel.org/doc/Documentation/dynamic-debug-howto.txt
> Hi,
>
> dyndbg vs module param:
> Dynamic debug has two very nice features: dynamic activation/configuration 
> and per line/file/module/format activation. The module param has neither, but 
> it does have a few merits which I am not sure dyndbg has:
> (1)
> It can activate printouts according to *flow*. A lot of thought has been put 
> into associating the right printout in our driver at the right verbosity 
> level with the right "flow tag" (e.g. QEDR_MSG_INIT, QEDR_MSG_QP). The module 
> parameter accepts a bitmask which allows setting any subset of these flows. 
> This means that with the correct values for the parameter I can open only 
> "init" printouts, or only "Memory Region" printouts, even if these cross 
> multiple files / functions and don't share a common format. Presumably, one 
> would claim that we could add the "flow tag" to the format to every printout 
> according to its flow, but that would encumber the printouts, and also 
> doesn't scale well to printouts which belong to multiple flows, where the 
> current approach allows this (QEDR_MSG_SQ | QEDR_MSG_RQ).

Dynamic prints are enabled per-print. The best possible granularity
which you can achieve.

> (2)
> As Yuval pointed out, there are users out there which have no trouble loading 
> a driver with a module parameter, at probe or at kernel boot, but would be at 
> a loss in mounting debugfs and dumping a matchspec script into a node there. 
> As kernel developers, educating users is part of what we do, but it comes 
> with a cost.

You are free to add this parameter to your out-of-tree driver. As I said
before to Yuval your module "debug" is equal to "dyndbg". Your users can
use it and is already available in kernel.

> (3)
> debugfs can be compiled out of the kernel in some codesize sensitive 
> environments, or may not exist in an emergency shell or kdump kernel, whereas 
> the module parameter would always be there.
>
> Simply allowing our module parameter mechanism to be dynamically activated is 
> very straightforward - we planned on adding a debugfs node for that anyway. 
> But I would keep the module parameter for the sake of those less capable 
> users and also for when debugfs is not available as detailed in (3) above.

This is an example why we don't want this module parameter, once added it
will be with us forever.

>
> I certainly see the merits of joining an existing infrastructure instead of 
> implementing our own thing, but I would like to know if there are ways of 
> obtaining the merits I listed for our approach via that infrastructure.
>
> Leon, aside of commenting on the above, can you give an example of a driver 
> which in your opinion does a good job of using dyndbg?

All modern drivers in this subsystem are supporting dyndbg out of the
box.

General note, can you please configure your email to wrap lines? It is
very hard to read, follow and respond to lines with >800 symbols in them.

>
> Thanks,
> Ariel
>


signature.asc
Description: PGP signature

RE: [RFC 02/11] Add RoCE driver framework

2016-09-20 Thread Elior, Ariel 
> From: Leon Romanovsky [mailto:l...@kernel.org]
> On Thu, Sep 15, 2016 at 05:11:03AM +, Mintz, Yuval wrote:
> > > As a summary, I didn't see in your responses any real life example where 
> > > you will
> > > need global debug level for your driver.
> >
> > Not sure what you you're expecting - a list of BZs /private e-mails where
> > user reproductions were needed?
> > You're basically ignoring my claims that such are used, instead wanting
> > "evidence". I'm not going to try and produce any such.
> 
> I asked an example and not evidence, where "modprobe your_driver
> debug=1" will be superior to "modprobe your_driver dyndbg==pmf".
> 
> https://www.kernel.org/doc/Documentation/dynamic-debug-howto.txt
Hi,

dyndbg vs module param:
Dynamic debug has two very nice features: dynamic activation/configuration and 
per line/file/module/format activation. The module param has neither, but it 
does have a few merits which I am not sure dyndbg has:
(1)
It can activate printouts according to *flow*. A lot of thought has been put 
into associating the right printout in our driver at the right verbosity level 
with the right "flow tag" (e.g. QEDR_MSG_INIT, QEDR_MSG_QP). The module 
parameter accepts a bitmask which allows setting any subset of these flows. 
This means that with the correct values for the parameter I can open only 
"init" printouts, or only "Memory Region" printouts, even if these cross 
multiple files / functions and don't share a common format. Presumably, one 
would claim that we could add the "flow tag" to the format to every printout 
according to its flow, but that would encumber the printouts, and also doesn't 
scale well to printouts which belong to multiple flows, where the current 
approach allows this (QEDR_MSG_SQ | QEDR_MSG_RQ).
(2)
As Yuval pointed out, there are users out there which have no trouble loading a 
driver with a module parameter, at probe or at kernel boot, but would be at a 
loss in mounting debugfs and dumping a matchspec script into a node there. As 
kernel developers, educating users is part of what we do, but it comes with a 
cost.
(3)
debugfs can be compiled out of the kernel in some codesize sensitive 
environments, or may not exist in an emergency shell or kdump kernel, whereas 
the module parameter would always be there.

Simply allowing our module parameter mechanism to be dynamically activated is 
very straightforward - we planned on adding a debugfs node for that anyway. But 
I would keep the module parameter for the sake of those less capable users and 
also for when debugfs is not available as detailed in (3) above.

I certainly see the merits of joining an existing infrastructure instead of 
implementing our own thing, but I would like to know if there are ways of 
obtaining the merits I listed for our approach via that infrastructure.

Leon, aside of commenting on the above, can you give an example of a driver 
which in your opinion does a good job of using dyndbg?

Thanks,
Ariel

Re: [RFC 02/11] Add RoCE driver framework

2016-09-14 Thread Leon Romanovsky 
On Thu, Sep 15, 2016 at 05:11:03AM +, Mintz, Yuval wrote:
> > As a summary, I didn't see in your responses any real life example where 
> > you will
> > need global debug level for your driver.
>
> Not sure what you you're expecting - a list of BZs /private e-mails where
> user reproductions were needed?
> You're basically ignoring my claims that such are used, instead wanting
> "evidence". I'm not going to try and produce any such.

I asked an example and not evidence, where "modprobe your_driver
debug=1" will be superior to "modprobe your_driver dyndbg==pmf".

https://www.kernel.org/doc/Documentation/dynamic-debug-howto.txt


signature.asc
Description: PGP signature

RE: [RFC 02/11] Add RoCE driver framework

2016-09-14 Thread Mintz, Yuval 
> > > If you want dynamic prints, you have two options:
> > > 1. Add support of ethtool to whole RDMA stack.
> > > 2. Use dynamic tracing infrastructure.
> >
> > > Which option do you prefer?
> > Option 3 - continuing this discussion. :-)
> 
> Sorry,
> I was under impression that you want this driver to be merged, but it looks 
> like It
> was incorrect assumption. Let's continue discussion.

No, this is an RFC - there's no chance for *this* to merge, but this is exactly
the right time to discuss this sort of stuff.

> > Perhaps I misread your intentions - I thought that by dynamic debug
> > you meant that all debug in RDMA should be pr_debug() based, and
> > therefore my objection regarding the ease with which users can
> > configure it.
> 
> It is not for all RDMA, but in your proposed driver. You are adding this 
> "debug"
> module argument to your module.

I don't get your answer. 
I made a generic remark [and actually one in favor of your arguments],
and instead of saying something meaningful you bash the driver.

> > If all you meant was 'dynamically set' as opposed to 'statically set'
> > then I agree that having that sort of configurability is preferable
> > [Even though end-user would still probably prefer a module parameter
> > for reproductions; As the name implies, 'debug' isn't meant to be used
> > in other situations].
> 
> We are not adding code just for fun, but for a real reason, and especially
> interfaces which will be visible to user.
> 
> The overall expectation from the driver's authors that they are submitting 
> driver
> which doesn't have bringup issues. For real life scenarios, where the bugs 
> will be
> reveled after some time of usage, this global debug is useless.

This has nothing to do with bringup; Real drivers are experiencing issues years 
after
they're productized.

> > Do notice you would be harming user-experience of reproductions though
> > - as it would have to follow different mechanisms to open debug prints
> > of various qed* components.
> 
> I don't understand this point at all. Do you think that it is normal to ask 
> user to
> debug your driver? Is this called "user-experience"?

No, I call this 'user involved in fixing the driver' - it has nothing to do with
user-experience. Sometimes user have specifics in his system that can't
be easily identified and thus lab reproductions fail, and the user assists
in the reproduction. While I never claimed this is good practice it does happen.

> As a summary, I didn't see in your responses any real life example where you 
> will
> need global debug level for your driver.

Not sure what you you're expecting - a list of BZs /private e-mails where
user reproductions were needed?
You're basically ignoring my claims that such are used, instead wanting
"evidence". I'm not going to try and produce any such.

Doug - I think we need a definite answer from you here; Doesn't look like
this discussion would bear any fruit.
If a debug module parameter is completely unacceptable, we'd remove it
[regardless of what I think about it].

Re: [RFC 02/11] Add RoCE driver framework

2016-09-14 Thread Leon Romanovsky 
On Wed, Sep 14, 2016 at 06:25:38PM +, Mintz, Yuval wrote:
> > > > > >> >> +uint debug;
> > > > > >> >> +module_param(debug, uint, 0);
> > > > > > >>> +MODULE_PARM_DESC(debug, "Default debug msglevel");
> > > > > >>
> > > > > >> >Why are you adding this as a module parameter?
> > > > > >>
> > > > > >>  I believe this is mostly to follow same line as qede which also 
> > > > > >>defines
> > > > > > > 'debug' module parameter for allowing easy user control of debug
> > > > > > > prints [& specifically for probe prints, which can't be controlled
> > > > > > > otherwise].
> > > > >
> > > > > > Can you give us an example where dynamic debug and tracing 
> > > > > > infrastructures
> > > > > > are not enough?
> > > > >
> > > > > > AFAIK, most of these debug module parameters are legacy copy/paste
> > > > > > code which is useless in real life scenarios.
> > > > >
> > > > > Define 'enough'; Using dynamic debug you can provide all the necessary
> > > > > information and at an even better granularity that's achieved by 
> > > > > suggested
> > >  > > infrastructure,  but is harder for an end-user to use. Same goes for 
> > > tracing.
> > > > >
> > > > > The 'debug' option provides an easy grouping for prints related to a 
> > > > > specific
> > > > > area in the driver.
> > > >
> > > > It is hard to agree with you that user which knows how-to load modules
> > > > with parameters won't success to enable debug prints.
> > >
> > > I think you're giving too much credit to the end-user. :-D
> > >
> > > > In addition, global increase in debug level for whole driver will create
> > > > printk storm in dmesg and give nothing to debuggability.
> > >
> > > So basically, what you're claiming is that ethtool 'msglvl' setting for 
> > > devices
> > > is completely obselete. While this *might* be true, we use it extensively
> > > in our qede and qed drivers; The debug module parameter merely provides
> > > a manner of setting the debug value prior to initial probe for all 
> > > interfaces.
> > > qedr follows the same practice.
>
> > Thanks for this excellent example. Ethtool 'msglvl' adds this
> > dynamically, while your DEBUG argument works for loading module
> > only.
>
> > If you want dynamic prints, you have two options:
> > 1. Add support of ethtool to whole RDMA stack.
> > 2. Use dynamic tracing infrastructure.
>
> > Which option do you prefer?
> Option 3 - continuing this discussion. :-)

Sorry,
I was under impression that you want this driver to be merged, but it
looks like It was incorrect assumption. Let's continue discussion.

>
> Perhaps I misread your intentions - I thought that by dynamic debug
> you meant that all debug in RDMA should be pr_debug() based, and
> therefore my objection regarding the ease with which users can
> configure it.

It is not for all RDMA, but in your proposed driver. You are adding this
"debug" module argument to your module.

> If all you meant was 'dynamically set' as opposed to 'statically set'
> then I agree that having that sort of configurability is preferable
> [Even though end-user would still probably prefer a module
> parameter for reproductions; As the name implies, 'debug' isn't
> meant to be used in other situations].

We are not adding code just for fun, but for a real reason, and
especially interfaces which will be visible to user.

The overall expectation from the driver's authors that they are
submitting driver which doesn't have bringup issues. For real
life scenarios, where the bugs will be reveled after some time of
usage, this global debug is useless.

>
> The other thing to consider are the probe-time prints.
> Problem is, you wouldn't have a control node between probe
> and until after the probing would be over, so it would be a bit
> hard to configure that.
> You can always think of some generic method of fixing that as well
> [sysfs node for the entire system for probe-time prints, perhaps?]

/sys/kernel/debug/tracing
/sys/kernel/debug/dynamic_debug

>
> Do notice you would be harming user-experience of reproductions
> though - as it would have to follow different mechanisms to open
> debug prints of various qed* components.

I don't understand this point at all. Do you think that it is normal to
ask user to debug your driver? Is this called "user-experience"?

And regarding the second point, the old code is not an excuse to
copy/paste bad practices.

As a summary, I didn't see in your responses any real life example where
you will need global debug level for your driver.

Thanks


signature.asc
Description: PGP signature

Re: [RFC 02/11] Add RoCE driver framework

2016-09-14 Thread Mintz, Yuval 
> > > > >> >> +uint debug;
> > > > >> >> +module_param(debug, uint, 0);
> > > > > >>> +MODULE_PARM_DESC(debug, "Default debug msglevel");
> > > > >>
> > > > >> >Why are you adding this as a module parameter?
> > > > >>
> > > > >>  I believe this is mostly to follow same line as qede which also 
> > > > >>defines
> > > > > > 'debug' module parameter for allowing easy user control of debug
> > > > > > prints [& specifically for probe prints, which can't be controlled
> > > > > > otherwise].
> > > >
> > > > > Can you give us an example where dynamic debug and tracing 
> > > > > infrastructures
> > > > > are not enough?
> > > >
> > > > > AFAIK, most of these debug module parameters are legacy copy/paste
> > > > > code which is useless in real life scenarios.
> > > >
> > > > Define 'enough'; Using dynamic debug you can provide all the necessary
> > > > information and at an even better granularity that's achieved by 
> > > > suggested
> >  > > infrastructure,  but is harder for an end-user to use. Same goes for 
> > tracing.
> > > >
> > > > The 'debug' option provides an easy grouping for prints related to a 
> > > > specific
> > > > area in the driver.
> > >
> > > It is hard to agree with you that user which knows how-to load modules
> > > with parameters won't success to enable debug prints.
> >
> > I think you're giving too much credit to the end-user. :-D
> >
> > > In addition, global increase in debug level for whole driver will create
> > > printk storm in dmesg and give nothing to debuggability.
> >
> > So basically, what you're claiming is that ethtool 'msglvl' setting for 
> > devices
> > is completely obselete. While this *might* be true, we use it extensively
> > in our qede and qed drivers; The debug module parameter merely provides
> > a manner of setting the debug value prior to initial probe for all 
> > interfaces.
> > qedr follows the same practice.

> Thanks for this excellent example. Ethtool 'msglvl' adds this
> dynamically, while your DEBUG argument works for loading module
> only.

> If you want dynamic prints, you have two options:
> 1. Add support of ethtool to whole RDMA stack.
> 2. Use dynamic tracing infrastructure.

> Which option do you prefer?
Option 3 - continuing this discussion. :-)

Perhaps I misread your intentions - I thought that by dynamic debug
you meant that all debug in RDMA should be pr_debug() based, and
therefore my objection regarding the ease with which users can
configure it. 
If all you meant was 'dynamically set' as opposed to 'statically set'
then I agree that having that sort of configurability is preferable
[Even though end-user would still probably prefer a module
parameter for reproductions; As the name implies, 'debug' isn't
meant to be used in other situations].

The other thing to consider are the probe-time prints.
Problem is, you wouldn't have a control node between probe 
and until after the probing would be over, so it would be a bit
hard to configure that.
You can always think of some generic method of fixing that as well
[sysfs node for the entire system for probe-time prints, perhaps?]

Do notice you would be harming user-experience of reproductions
though - as it would have to follow different mechanisms to open
debug prints of various qed* components.

Re: [RFC 02/11] Add RoCE driver framework

2016-09-14 Thread Leon Romanovsky 
On Wed, Sep 14, 2016 at 08:15:23AM +, Mintz, Yuval wrote:
> > > >> >> +uint debug;
> > > >> >> +module_param(debug, uint, 0);
> > > > >>> +MODULE_PARM_DESC(debug, "Default debug msglevel");
> > > >>
> > > >> >Why are you adding this as a module parameter?
> > > >>
> > > >>  I believe this is mostly to follow same line as qede which also 
> > > >>defines
> > > > > 'debug' module parameter for allowing easy user control of debug
> > > > > prints [& specifically for probe prints, which can't be controlled
> > > > > otherwise].
> > >
> > > > Can you give us an example where dynamic debug and tracing 
> > > > infrastructures
> > > > are not enough?
> > >
> > > > AFAIK, most of these debug module parameters are legacy copy/paste
> > > > code which is useless in real life scenarios.
> > >
> > > Define 'enough'; Using dynamic debug you can provide all the necessary
> > > information and at an even better granularity that's achieved by suggested
> > > infrastructure,  but is harder for an end-user to use. Same goes for 
> > > tracing.
> > >
> > > The 'debug' option provides an easy grouping for prints related to a 
> > > specific
> > > area in the driver.
> >
> > It is hard to agree with you that user which knows how-to load modules
> > with parameters won't success to enable debug prints.
>
> I think you're giving too much credit to the end-user. :-D
>
> > In addition, global increase in debug level for whole driver will create
> > printk storm in dmesg and give nothing to debuggability.
>
> So basically, what you're claiming is that ethtool 'msglvl' setting for 
> devices
> is completely obselete. While this *might* be true, we use it extensively
> in our qede and qed drivers; The debug module parameter merely provides
> a manner of setting the debug value prior to initial probe for all interfaces.
> qedr follows the same practice.

Thanks for this excellent example. Ethtool 'msglvl' adds this
dynamically, while your DEBUG argument works for loading module
only.

If you want dynamic prints, you have two options:
1. Add support of ethtool to whole RDMA stack.
2. Use dynamic tracing infrastructure.

Which option do you prefer?

>


signature.asc
Description: PGP signature

Re: [RFC 02/11] Add RoCE driver framework

2016-09-14 Thread Mintz, Yuval 
> > >> >> +uint debug;
> > >> >> +module_param(debug, uint, 0);
> > > >>> +MODULE_PARM_DESC(debug, "Default debug msglevel");
> > >>
> > >> >Why are you adding this as a module parameter?
> > >>
> > >>  I believe this is mostly to follow same line as qede which also defines
> > > > 'debug' module parameter for allowing easy user control of debug
> > > > prints [& specifically for probe prints, which can't be controlled
> > > > otherwise].
> >
> > > Can you give us an example where dynamic debug and tracing infrastructures
> > > are not enough?
> >
> > > AFAIK, most of these debug module parameters are legacy copy/paste
> > > code which is useless in real life scenarios.
> >
> > Define 'enough'; Using dynamic debug you can provide all the necessary
> > information and at an even better granularity that's achieved by suggested
> > infrastructure,  but is harder for an end-user to use. Same goes for 
> > tracing.
> >
> > The 'debug' option provides an easy grouping for prints related to a 
> > specific
> > area in the driver.
> 
> It is hard to agree with you that user which knows how-to load modules
> with parameters won't success to enable debug prints.

I think you're giving too much credit to the end-user. :-D

> In addition, global increase in debug level for whole driver will create
> printk storm in dmesg and give nothing to debuggability.

So basically, what you're claiming is that ethtool 'msglvl' setting for devices
is completely obselete. While this *might* be true, we use it extensively
in our qede and qed drivers; The debug module parameter merely provides
a manner of setting the debug value prior to initial probe for all interfaces.
qedr follows the same practice.

RE: [RFC 02/11] Add RoCE driver framework

2016-09-14 Thread Amrani, Ram 
> > +   if ((event != NETDEV_CHANGENAME) && (event !=
> > NETDEV_CHANGEADDR))
> 
> nit: You don't really need the extra parens here.
> 
Sure, thanks. Will remove.

RE: [RFC 02/11] Add RoCE driver framework

2016-09-13 Thread Steve Wise 
> Adds a skeletal implementation of the qed* RoCE driver -
> basically the ability to communicate with the qede driver and
> receive notifications from it regarding various init/exit events.
> 
> Signed-off-by: Rajesh Borundia 
> Signed-off-by: Ram Amrani 
> ---
>  drivers/infiniband/Kconfig   |   2 +
>  drivers/infiniband/hw/Makefile   |   1 +
>  drivers/infiniband/hw/qedr/Kconfig   |   7 +
>  drivers/infiniband/hw/qedr/Makefile  |   3 +
>  drivers/infiniband/hw/qedr/main.c| 293 +
>  drivers/infiniband/hw/qedr/qedr.h|  60 ++
>  drivers/net/ethernet/qlogic/qede/Makefile|   1 +
>  drivers/net/ethernet/qlogic/qede/qede.h  |   9 +
>  drivers/net/ethernet/qlogic/qede/qede_main.c |  35 ++-
>  drivers/net/ethernet/qlogic/qede/qede_roce.c | 309
> +++
>  include/linux/qed/qed_if.h   |   3 +-
>  include/linux/qed/qede_roce.h|  88 
>  include/uapi/linux/pci_regs.h|   3 +
>  13 files changed, 803 insertions(+), 11 deletions(-)
>  create mode 100644 drivers/infiniband/hw/qedr/Kconfig
>  create mode 100644 drivers/infiniband/hw/qedr/Makefile
>  create mode 100644 drivers/infiniband/hw/qedr/main.c
>  create mode 100644 drivers/infiniband/hw/qedr/qedr.h
>  create mode 100644 drivers/net/ethernet/qlogic/qede/qede_roce.c
>  create mode 100644 include/linux/qed/qede_roce.h



> @@ -189,8 +189,7 @@ static int qede_netdev_event(struct notifier_block *this,
> unsigned long event,
>   struct ethtool_drvinfo drvinfo;
>   struct qede_dev *edev;
> 
> - /* Currently only support name change */
> - if (event != NETDEV_CHANGENAME)
> + if ((event != NETDEV_CHANGENAME) && (event !=
> NETDEV_CHANGEADDR))

nit: You don't really need the extra parens here.

Re: [RFC 02/11] Add RoCE driver framework

2016-09-13 Thread Leon Romanovsky 
On Tue, Sep 13, 2016 at 09:22:04AM +, Ram Amrani wrote:
> Thanks for your comments.
> See my replies in line with [Ram].

Please configure your email client
https://www.kernel.org/doc/Documentation/email-clients.txt

Thanks


signature.asc
Description: PGP signature

Re: [RFC 02/11] Add RoCE driver framework

2016-09-13 Thread Leon Romanovsky 
On Tue, Sep 13, 2016 at 07:18:01AM +, Mintz, Yuval wrote:
> >> >> +uint debug;
> >> >> +module_param(debug, uint, 0);
> > >>> +MODULE_PARM_DESC(debug, "Default debug msglevel");
> >>
> >> >Why are you adding this as a module parameter?
> >>
> >>  I believe this is mostly to follow same line as qede which also defines
> > > 'debug' module parameter for allowing easy user control of debug
> > > prints [& specifically for probe prints, which can't be controlled
> > > otherwise].
>
> > Can you give us an example where dynamic debug and tracing infrastructures
> > are not enough?
>
> > AFAIK, most of these debug module parameters are legacy copy/paste
> > code which is useless in real life scenarios.
>
> Define 'enough'; Using dynamic debug you can provide all the necessary
> information and at an even better granularity that's achieved by suggested
> infrastructure,  but is harder for an end-user to use. Same goes for tracing.
>
> The 'debug' option provides an easy grouping for prints related to a specific
> area in the driver.

It is hard to agree with you that user which knows how-to load modules
with parameters won't success to enable debug prints.

In addition, global increase in debug level for whole driver will create
printk storm in dmesg and give nothing to debuggability.

Thanks


signature.asc
Description: PGP signature

RE: [RFC 02/11] Add RoCE driver framework

2016-09-13 Thread Ram Amrani 
Thanks for your comments.
See my replies in line with [Ram].



-Original Message-
From: Mark Bloch [mailto:ma...@mellanox.com] 
Sent: Monday, September 12, 2016 9:44 PM
To: Ram Amrani <ram.amr...@qlogic.com>; dledf...@redhat.com; David Miller 
<da...@davemloft.net>
Cc: Yuval Mintz <yuval.mi...@qlogic.com>; Ariel Elior <ariel.el...@qlogic.com>; 
Michal Kalderon <michal.kalde...@qlogic.com>; Rajesh Borundia 
<rajesh.borun...@qlogic.com>; linux-r...@vger.kernel.org; netdev 
<netdev@vger.kernel.org>
Subject: Re: [RFC 02/11] Add RoCE driver framework


Hi Ram,

Just a few thoughts 

On 12/09/2016 19:07, Ram Amrani wrote:
> Adds a skeletal implementation of the qed* RoCE driver - basically the 
> ability to communicate with the qede driver and receive notifications 
> from it regarding various init/exit events.
> 
> Signed-off-by: Rajesh Borundia <rajesh.borun...@qlogic.com>
> Signed-off-by: Ram Amrani <ram.amr...@qlogic.com>
> ---
>  drivers/infiniband/Kconfig   |   2 +
>  drivers/infiniband/hw/Makefile   |   1 +
>  drivers/infiniband/hw/qedr/Kconfig   |   7 +
>  drivers/infiniband/hw/qedr/Makefile  |   3 +
>  drivers/infiniband/hw/qedr/main.c| 293 +
>  drivers/infiniband/hw/qedr/qedr.h|  60 ++
>  drivers/net/ethernet/qlogic/qede/Makefile|   1 +
>  drivers/net/ethernet/qlogic/qede/qede.h  |   9 +
>  drivers/net/ethernet/qlogic/qede/qede_main.c |  35 ++-  
> drivers/net/ethernet/qlogic/qede/qede_roce.c | 309 +++
>  include/linux/qed/qed_if.h   |   3 +-
>  include/linux/qed/qede_roce.h|  88 
>  include/uapi/linux/pci_regs.h|   3 +
>  13 files changed, 803 insertions(+), 11 deletions(-)  create mode 
> 100644 drivers/infiniband/hw/qedr/Kconfig
>  create mode 100644 drivers/infiniband/hw/qedr/Makefile
>  create mode 100644 drivers/infiniband/hw/qedr/main.c  create mode 
> 100644 drivers/infiniband/hw/qedr/qedr.h  create mode 100644 
> drivers/net/ethernet/qlogic/qede/qede_roce.c
>  create mode 100644 include/linux/qed/qede_roce.h

[SNIP]

> +
> +MODULE_DESCRIPTION("QLogic 40G/100G ROCE Driver"); 
> +MODULE_AUTHOR("QLogic Corporation"); MODULE_LICENSE("Dual BSD/GPL"); 
> +MODULE_VERSION(QEDR_MODULE_VERSION);
> +
> +uint debug;
> +module_param(debug, uint, 0);
> +MODULE_PARM_DESC(debug, "Default debug msglevel");

Why are you adding this as a module parameter? 
[Ram] Yuval commented on this in a previous e-mail


> +static LIST_HEAD(qedr_dev_list);
> +static DEFINE_SPINLOCK(qedr_devlist_lock);
> +

You already have a qedr_dev_list mutex in the qede_roce.c file, why do you need 
this spinlock as well?

 [Ram] qedr_devlist_lock - a static (local) list of qedr devices maintained by 
qedr, protected by spinlock. Not in used in the current patches.
qedr_dev_list_lock (with '_') - a static (local) list of qedr devices 
maintained by qede, protected by mutex.
We'll consider removing the first as it is currently not used and/or rename 
them to be more distinct.



> +void qedr_ib_dispatch_event(struct qedr_dev *dev, u8 port_num,
> + enum ib_event_type type)
> +{
> + struct ib_event ibev;
> +
> + ibev.device = >ibdev;
> + ibev.element.port_num = port_num;
> + ibev.event = type;
> +
> + ib_dispatch_event();
> +}
> +
> +static enum rdma_link_layer qedr_link_layer(struct ib_device *device,
> + u8 port_num)
> +{
> + return IB_LINK_LAYER_ETHERNET;
> +}
> +
> +static int qedr_register_device(struct qedr_dev *dev) {
> + strlcpy(dev->ibdev.name, "qedr%d", IB_DEVICE_NAME_MAX);
> +
> + memcpy(dev->ibdev.node_desc, QEDR_NODE_DESC, sizeof(QEDR_NODE_DESC));
> + dev->ibdev.owner = THIS_MODULE;
> +
> + dev->ibdev.get_link_layer = qedr_link_layer;
> +
> + return 0;
> +}
> +
> +/* QEDR sysfs interface */
> +static ssize_t show_rev(struct device *device, struct device_attribute *attr,
> + char *buf)
> +{
> + struct qedr_dev *dev = dev_get_drvdata(device);
> +
> + return scnprintf(buf, PAGE_SIZE, "0x%x\n", dev->pdev->vendor); }
> +
> +static ssize_t show_fw_ver(struct device *device, struct device_attribute 
> *attr,
> +char *buf)
> +{
> + return scnprintf(buf, PAGE_SIZE, "%s\n", "FW_VER_TO_ADD"); }

Ira Weiny has added a generic way to expose firmware versions in the rdma 
stack, can you have please have a look at 
c73428230d98d1352bcc69cd8306c292a85e1e42 and see how he converted the mlx5_ib 
module to

Re: [RFC 02/11] Add RoCE driver framework

2016-09-13 Thread Mintz, Yuval 
>> >> +uint debug;
>> >> +module_param(debug, uint, 0);
> >>> +MODULE_PARM_DESC(debug, "Default debug msglevel");
>>
>> >Why are you adding this as a module parameter?
>>
>>  I believe this is mostly to follow same line as qede which also defines
> > 'debug' module parameter for allowing easy user control of debug
> > prints [& specifically for probe prints, which can't be controlled
> > otherwise].

> Can you give us an example where dynamic debug and tracing infrastructures
> are not enough?

> AFAIK, most of these debug module parameters are legacy copy/paste
> code which is useless in real life scenarios.

Define 'enough'; Using dynamic debug you can provide all the necessary
information and at an even better granularity that's achieved by suggested
infrastructure,  but is harder for an end-user to use. Same goes for tracing.

The 'debug' option provides an easy grouping for prints related to a specific
area in the driver.

Re: [RFC 02/11] Add RoCE driver framework

2016-09-13 Thread Leon Romanovsky 
On Mon, Sep 12, 2016 at 07:07:36PM +0300, Ram Amrani wrote:
> Adds a skeletal implementation of the qed* RoCE driver -
> basically the ability to communicate with the qede driver and
> receive notifications from it regarding various init/exit events.
>
> Signed-off-by: Rajesh Borundia 
> Signed-off-by: Ram Amrani 
> ---

<...>

> +
> +#define QEDR_MODULE_VERSION  "8.10.10.0"

I am strongly against module versions. You should rely on official
kernel version.

> +#define QEDR_NODE_DESC "QLogic 579xx RoCE HCA"
> +#define DP_NAME(dev) ((dev)->ibdev.name)


signature.asc
Description: PGP signature

Re: [RFC 02/11] Add RoCE driver framework

2016-09-13 Thread Leon Romanovsky 
On Mon, Sep 12, 2016 at 07:17:35PM +, Yuval Mintz wrote:
> >> +uint debug;
> >> +module_param(debug, uint, 0);
> >> +MODULE_PARM_DESC(debug, "Default debug msglevel");
>
> >Why are you adding this as a module parameter?
>
> I believe this is mostly to follow same line as qede which also defines
> 'debug' module parameter for allowing easy user control of debug
> prints [& specifically for probe prints, which can't be controlled
> otherwise].

Can you give us an example where dynamic debug and tracing infrastructures
are not enough?

AFAIK, most of these debug module parameters are legacy copy/paste
code which is useless in real life scenarios.

Thanks


>  --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


signature.asc
Description: PGP signature

Re: [RFC 02/11] Add RoCE driver framework

2016-09-12 Thread Mark Bloch 

Hi Ram,

Just a few thoughts 

On 12/09/2016 19:07, Ram Amrani wrote:
> Adds a skeletal implementation of the qed* RoCE driver -
> basically the ability to communicate with the qede driver and
> receive notifications from it regarding various init/exit events.
> 
> Signed-off-by: Rajesh Borundia 
> Signed-off-by: Ram Amrani 
> ---
>  drivers/infiniband/Kconfig   |   2 +
>  drivers/infiniband/hw/Makefile   |   1 +
>  drivers/infiniband/hw/qedr/Kconfig   |   7 +
>  drivers/infiniband/hw/qedr/Makefile  |   3 +
>  drivers/infiniband/hw/qedr/main.c| 293 +
>  drivers/infiniband/hw/qedr/qedr.h|  60 ++
>  drivers/net/ethernet/qlogic/qede/Makefile|   1 +
>  drivers/net/ethernet/qlogic/qede/qede.h  |   9 +
>  drivers/net/ethernet/qlogic/qede/qede_main.c |  35 ++-
>  drivers/net/ethernet/qlogic/qede/qede_roce.c | 309 
> +++
>  include/linux/qed/qed_if.h   |   3 +-
>  include/linux/qed/qede_roce.h|  88 
>  include/uapi/linux/pci_regs.h|   3 +
>  13 files changed, 803 insertions(+), 11 deletions(-)
>  create mode 100644 drivers/infiniband/hw/qedr/Kconfig
>  create mode 100644 drivers/infiniband/hw/qedr/Makefile
>  create mode 100644 drivers/infiniband/hw/qedr/main.c
>  create mode 100644 drivers/infiniband/hw/qedr/qedr.h
>  create mode 100644 drivers/net/ethernet/qlogic/qede/qede_roce.c
>  create mode 100644 include/linux/qed/qede_roce.h

[SNIP]

> +
> +MODULE_DESCRIPTION("QLogic 40G/100G ROCE Driver");
> +MODULE_AUTHOR("QLogic Corporation");
> +MODULE_LICENSE("Dual BSD/GPL");
> +MODULE_VERSION(QEDR_MODULE_VERSION);
> +
> +uint debug;
> +module_param(debug, uint, 0);
> +MODULE_PARM_DESC(debug, "Default debug msglevel");

Why are you adding this as a module parameter? 


> +static LIST_HEAD(qedr_dev_list);
> +static DEFINE_SPINLOCK(qedr_devlist_lock);
> +

You already have a qedr_dev_list mutex in the qede_roce.c file,
why do you need this spinlock as well?

> +void qedr_ib_dispatch_event(struct qedr_dev *dev, u8 port_num,
> + enum ib_event_type type)
> +{
> + struct ib_event ibev;
> +
> + ibev.device = >ibdev;
> + ibev.element.port_num = port_num;
> + ibev.event = type;
> +
> + ib_dispatch_event();
> +}
> +
> +static enum rdma_link_layer qedr_link_layer(struct ib_device *device,
> + u8 port_num)
> +{
> + return IB_LINK_LAYER_ETHERNET;
> +}
> +
> +static int qedr_register_device(struct qedr_dev *dev)
> +{
> + strlcpy(dev->ibdev.name, "qedr%d", IB_DEVICE_NAME_MAX);
> +
> + memcpy(dev->ibdev.node_desc, QEDR_NODE_DESC, sizeof(QEDR_NODE_DESC));
> + dev->ibdev.owner = THIS_MODULE;
> +
> + dev->ibdev.get_link_layer = qedr_link_layer;
> +
> + return 0;
> +}
> +
> +/* QEDR sysfs interface */
> +static ssize_t show_rev(struct device *device, struct device_attribute *attr,
> + char *buf)
> +{
> + struct qedr_dev *dev = dev_get_drvdata(device);
> +
> + return scnprintf(buf, PAGE_SIZE, "0x%x\n", dev->pdev->vendor);
> +}
> +
> +static ssize_t show_fw_ver(struct device *device, struct device_attribute 
> *attr,
> +char *buf)
> +{
> + return scnprintf(buf, PAGE_SIZE, "%s\n", "FW_VER_TO_ADD");
> +}

Ira Weiny has added a generic way to expose firmware versions in the rdma stack,
can you have please have a look at c73428230d98d1352bcc69cd8306c292a85e1e42 and
see how he converted the mlx5_ib module to use it.

> +static ssize_t show_hca_type(struct device *device,
> +  struct device_attribute *attr, char *buf)
> +{
> + return scnprintf(buf, PAGE_SIZE, "%s\n", "HCA_TYPE_TO_SET");
> +}
> +
> +static DEVICE_ATTR(hw_rev, S_IRUGO, show_rev, NULL);
> +static DEVICE_ATTR(fw_ver, S_IRUGO, show_fw_ver, NULL);
> +static DEVICE_ATTR(hca_type, S_IRUGO, show_hca_type, NULL);
> +
> +static struct device_attribute *qedr_attributes[] = {
> + _attr_hw_rev,
> + _attr_fw_ver,
> + _attr_hca_type
> +};
> +
> +static void qedr_remove_sysfiles(struct qedr_dev *dev)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(qedr_attributes); i++)
> + device_remove_file(>ibdev.dev, qedr_attributes[i]);
> +}
> +
> +void qedr_config_debug(uint debug, u32 *p_dp_module, u8 *p_dp_level)
> +{
> + *p_dp_level = 0;
> + *p_dp_module = 0;
> +
> + if (debug & QED_LOG_VERBOSE_MASK) {
> + *p_dp_level = QED_LEVEL_VERBOSE;
> + *p_dp_module = (debug & 0x3FFF);
> + } else if (debug & QED_LOG_INFO_MASK) {
> + *p_dp_level = QED_LEVEL_INFO;
> + } else if (debug & QED_LOG_NOTICE_MASK) {
> + *p_dp_level = QED_LEVEL_NOTICE;
> + }
> +}
> +
> +static void qedr_pci_set_atomic(struct qedr_dev *dev, struct pci_dev *pdev)
> +{
> + struct pci_dev *bridge;
> + u32

Re: [RFC 02/11] Add RoCE driver framework

2016-09-12 Thread Yuval Mintz 
>> +uint debug;
>> +module_param(debug, uint, 0);
>> +MODULE_PARM_DESC(debug, "Default debug msglevel");

>Why are you adding this as a module parameter? 

I believe this is mostly to follow same line as qede which also defines
'debug' module parameter for allowing easy user control of debug
prints [& specifically for probe prints, which can't be controlled
otherwise].