LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Sujeev Dias <sdias@codeaurora.org>
Cc: Arnd Bergmann <arnd@arndb.de>,
	linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	Tony Truong <truong@codeaurora.org>
Subject: Re: [PATCH v1 1/4] mhi_bus: core: Add support for MHI host interface
Date: Fri, 27 Apr 2018 09:22:11 +0200
Message-ID: <20180427072211.GA6735@kroah.com> (raw)
In-Reply-To: <1524795811-21399-2-git-send-email-sdias@codeaurora.org>

On Thu, Apr 26, 2018 at 07:23:28PM -0700, Sujeev Dias wrote:
> MHI Host Interface is a communication protocol to be used by the host
> to control and communcate with modem over a high speed peripheral bus.
> This module will allow host to communicate with external devices that
> support MHI protocol.
> 
> Signed-off-by: Sujeev Dias <sdias@codeaurora.org>

No one else has ever reviewed this code before?  That's not good, please
at the very least, have someone else at your company go over it first.
I don't want to be the ones having to point out all of the "obvious"
issues :)

> ---
>  Documentation/00-INDEX                        |    2 +
>  Documentation/devicetree/bindings/bus/mhi.txt |  141 +++
>  Documentation/mhi.txt                         |  235 ++++
>  drivers/bus/Kconfig                           |   17 +
>  drivers/bus/Makefile                          |    1 +
>  drivers/bus/mhi/Makefile                      |    8 +
>  drivers/bus/mhi/core/Makefile                 |    1 +
>  drivers/bus/mhi/core/mhi_boot.c               |  593 ++++++++++
>  drivers/bus/mhi/core/mhi_dtr.c                |  177 +++
>  drivers/bus/mhi/core/mhi_init.c               | 1290 +++++++++++++++++++++
>  drivers/bus/mhi/core/mhi_internal.h           |  732 ++++++++++++
>  drivers/bus/mhi/core/mhi_main.c               | 1476 +++++++++++++++++++++++++
>  drivers/bus/mhi/core/mhi_pm.c                 | 1177 ++++++++++++++++++++
>  include/linux/mhi.h                           |  694 ++++++++++++
>  include/linux/mod_devicetable.h               |   11 +
>  15 files changed, 6555 insertions(+)

And a 6555 line patch is a bit hard to consume all at once.  Can't this
be split up into much more reviewable chunks?  Look at how some of the
other new bus subsystems got added to the tree recently.  They were
submitted in longer patch series, but smaller sized patches
individually.  That makes things much easier to review.

For example, there is no reason your debugfs stuff needs to be in this
initial patch.  That should be in a separate one, right?  Same for
firmware download.  Please take the time to break this up into logical
steps.

Like my son's math teacher keeps telling him, "show your work, not just
an answer at the bottom of the page".

Also, it is required by the DT maintainers to split that file alone up
into a separate patch to be even considered for merging.

One thing I can tell you right now that isn't acceptable:

> +#ifdef CONFIG_MHI_DEBUG

Don't have a separate config option for debugging.  No one will enable
it, which makes it pointless.   Everything has to be dynamic these days.

> +
> +#define MHI_VERB(fmt, ...) do { \
> +		if (mhi_cntrl->klog_lvl <= MHI_MSG_LVL_VERBOSE) \
> +			pr_debug("[D][%s] " fmt, __func__, ##__VA_ARGS__);\
> +} while (0)
> +
> +#else
> +
> +#define MHI_VERB(fmt, ...)
> +
> +#endif
> +
> +#define MHI_LOG(fmt, ...) do {	\
> +		if (mhi_cntrl->klog_lvl <= MHI_MSG_LVL_INFO) \
> +			pr_info("[I][%s] " fmt, __func__, ##__VA_ARGS__);\
> +} while (0)
> +
> +#define MHI_ERR(fmt, ...) do {	\
> +		if (mhi_cntrl->klog_lvl <= MHI_MSG_LVL_ERROR) \
> +			pr_err("[E][%s] " fmt, __func__, ##__VA_ARGS__); \
> +} while (0)
> +
> +#define MHI_CRITICAL(fmt, ...) do { \
> +		if (mhi_cntrl->klog_lvl <= MHI_MSG_LVL_CRITICAL) \
> +			pr_alert("[C][%s] " fmt, __func__, ##__VA_ARGS__); \
> +} while (0)
> +

And do not roll your own debugging/logging macros.  Use what is given to
you (dev_info(), dev_err(), dev_dbg()), they are there for a reason.  By
going around them, you circumvent the whole of the kernel logging
infrastructure and declare that your tiny bus is somehow more "special"
than it.

And I doubt you want to make such a statement :)

thanks,

greg k-h

  reply index

Thread overview: 47+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-27  2:23 MHI initial design review Sujeev Dias
2018-04-27  2:23 ` [PATCH v1 1/4] mhi_bus: core: Add support for MHI host interface Sujeev Dias
2018-04-27  7:22   ` Greg Kroah-Hartman [this message]
2018-04-28 14:28     ` Sujeev Dias
2018-04-28 15:50       ` Greg Kroah-Hartman
2018-04-27  7:23   ` Greg Kroah-Hartman
2018-04-27 12:18   ` Arnd Bergmann
2018-04-28 16:08     ` Sujeev Dias
2018-04-28  0:28   ` kbuild test robot
2018-04-28  2:52   ` kbuild test robot
2018-05-03 19:21   ` Pavel Machek
2018-05-04  3:05     ` Sujeev Dias
2018-06-22 23:03   ` Randy Dunlap
2018-04-27  2:23 ` [PATCH v1 2/4] mhi_bus: controller: MHI support for QCOM modems Sujeev Dias
2018-04-27 11:32   ` Arnd Bergmann
2018-04-28 15:40     ` Sujeev Dias
2018-04-28  3:05   ` kbuild test robot
2018-04-28  3:12   ` kbuild test robot
2018-04-27  2:23 ` [PATCH v1 3/4] mhi_bus: dev: netdev: add network interface driver Sujeev Dias
2018-04-27 11:19   ` Arnd Bergmann
2018-04-28 15:25     ` Sujeev Dias
2018-04-27  2:23 ` [PATCH v1 4/4] mhi_bus: dev: uci: add user space " Sujeev Dias
2018-04-27 11:36   ` Arnd Bergmann
2018-04-28  1:03   ` kbuild test robot
2018-04-28  5:16   ` [PATCH] mhi_bus: dev: uci: fix semicolon.cocci warnings kbuild test robot
2018-04-28  5:16   ` [PATCH v1 4/4] mhi_bus: dev: uci: add user space interface driver kbuild test robot
2018-07-09 20:08 ` MHI code review Sujeev Dias
2018-07-09 20:08   ` [PATCH v2 1/7] mhi_bus: core: initial checkin for modem host interface bus driver Sujeev Dias
2018-07-09 20:50     ` Greg Kroah-Hartman
2018-07-09 20:52     ` Greg Kroah-Hartman
2018-07-10  6:36     ` Greg Kroah-Hartman
2018-07-11 19:30     ` Rob Herring
2018-08-09 18:39     ` Randy Dunlap
2018-07-09 20:08   ` [PATCH v2 2/7] mhi_bus: core: add power management support Sujeev Dias
2018-07-09 20:08   ` [PATCH v2 3/7] mhi_bus: core: add support for data transfer Sujeev Dias
2018-07-10  6:29     ` Greg Kroah-Hartman
2018-07-09 20:08   ` [PATCH v2 4/7] mhi_bus: core: add support for handling ioctl cmds Sujeev Dias
2018-07-09 20:08   ` [PATCH v2 5/7] mhi_bus: core: add support to get external modem time Sujeev Dias
2018-07-11 19:32     ` Rob Herring
2018-08-09 20:17     ` Randy Dunlap
2018-07-09 20:08   ` [PATCH v2 6/7] mhi_bus: controller: MHI support for QCOM modems Sujeev Dias
2018-07-11 19:36     ` Rob Herring
2018-07-09 20:08   ` [PATCH v2 7/7] mhi_bus: dev: uci: add user space interface driver Sujeev Dias
2019-04-30 15:10   ` MHI code review Daniele Palmas
2019-06-12 17:54     ` Sujeev Dias
2019-06-12 20:58       ` Daniele Palmas
2019-06-12 18:00     ` Sujeev Dias

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180427072211.GA6735@kroah.com \
    --to=gregkh@linuxfoundation.org \
    --cc=arnd@arndb.de \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=sdias@codeaurora.org \
    --cc=truong@codeaurora.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox