| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20170617133931.GJ11129@bhelgaas-glaptop.roam.corp.google.com>
Date: Sat, 17 Jun 2017 08:39:31 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: songxiaowei <songxiaowei@...ilicon.com>
Cc: Guodong Xu <guodong.xu@...aro.org>, Arnd Bergmann <arnd@...db.de>,
kbuild test robot <lkp@...el.com>,
"kbuild-all@...org" <kbuild-all@...org>,
Bjorn Helgaas <bhelgaas@...gle.com>, Kishon <kishon@...com>,
Jingoo Han <jingoohan1@...il.com>,
Tomasz Nowicki <tn@...ihalf.com>,
Keith Busch <keith.busch@...el.com>,
Niklas Cassel <niklas.cassel@...s.com>,
Duc Dang <dhdang@....com>,
"liudongdong (C)" <liudongdong3@...wei.com>,
Gabriele Paoloni <gabriele.paoloni@...wei.com>,
Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will.deacon@....com>,
"chenyao (F)" <chenyao11@...wei.com>,
"Chenfeng (puck)" <puck.chen@...ilicon.com>,
Wangbinghui <wangbinghui@...ilicon.com>,
Suzhuangluan <suzhuangluan@...ilicon.com>,
linux-pci <linux-pci@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: 答复: [PATCH v9 2/4] arm64: dts:
hisi: add kirin pcie node
On Sat, Jun 17, 2017 at 08:33:08AM +0000, songxiaowei wrote:
> Hi Bjorn,
>
> There are serval comments I can not understand, please help me to give more details.
Sure. BTW, for some reason your response is all double-spaced, which
makes it a little hard to read. Also, it includes HTML and an image,
which means the mailing list probably rejected it:
http://vger.kernel.org/majordomo-info.html#taboo
> diff --git a/Documentation/devicetree/bindings/pci/kirin-pcie.txt b/Documentation/devicetree/bindings/pci/kirin-pcie.txt
>
> index 68ffa0fbcd73..20357d840af1 100644
>
> --- a/Documentation/devicetree/bindings/pci/kirin-pcie.txt
>
> +++ b/Documentation/devicetree/bindings/pci/kirin-pcie.txt
>
> @@ -24,8 +24,8 @@ Example based on kirin960:
>
> pcie@...00000 {
>
> compatible = "hisilicon,kirin-pcie";
>
> - reg = <0x0 0xf4000000 0x0 0x1000>, <0x0 0xff3fe000 0x0 0x1000>,
>
> - <0x0 0xf3f20000 0x0 0x40000>, <0x0 0xF4000000 0 0x2000>;
>
> + reg = <0x0 0xf4000000 0x0 0x1000>, <0x0 0xff3fe000 0x0 0x1000>,
>
> + <0x0 0xf3f20000 0x0 0x40000>, <0x0 0xf4000000 0x0 0x2000>;
>
> [songxiaowei] The difference is add one more space between "0x0" and "0x1000" in the first element.
>
> But, I can't get your mind.
It changes from this:
reg = <0x0 0xf4000000 0x0 0x1000>, <0x0 0xff3fe000 0x0 0x1000>,
<0x0 0xf3f20000 0x0 0x40000>, <0x0 0xF4000000 0 0x2000>;
to this:
reg = <0x0 0xf4000000 0x0 0x1000>, <0x0 0xff3fe000 0x0 0x1000>,
<0x0 0xf3f20000 0x0 0x40000>, <0x0 0xf4000000 0x0 0x2000>;
The extra space makes the elements align vertically. Columns of
numbers are conventionally right-aligned, which makes it easier to
compare their sizes.
This hunk also changed 0xF4000000 to 0xf4000000 in the second line, so
hex constants use lower-case consistently.
> --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
>
> +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
>
> @@ -159,12 +159,12 @@
>
> pcie@...00000 {
>
> compatible = "hisilicon,kirin960-pcie";
>
> - reg = <0x0 0xf4000000 0x0 0x1000>,
>
> - <0x0 0xff3fe000 0x0 0x1000>,
>
> + reg = <0x0 0xf4000000 0x0 0x1000>,
>
> + <0x0 0xff3fe000 0x0 0x1000>,
>
> [songxiaowei] Can your tell why using two spaces? Reg is listed as <addr_hi addr_lo size_hi size_lo>.
Again, just to make the sizes right-aligned so it's easier to compare
the sizes.
You can ignore this one if you want. There are lots of examples in
the tree that don't align these. I just think it looks sloppy when
things are almost but not quite aligned.
> - <0x0 0xF5000000 0x0 0x2000>;
> + <0x0 0xf5000000 0x0 0x2000>;
Another case of making hex constants consistently lower-case.
> diff --git a/drivers/pci/dwc/pcie-kirin.c b/drivers/pci/dwc/pcie-kirin.c index f63e6548efae..41520dd1d5e5 100644
>
> --- a/drivers/pci/dwc/pcie-kirin.c
>
> +++ b/drivers/pci/dwc/pcie-kirin.c
>
> @@ -35,8 +35,8 @@
>
> #define REF_CLK_FREQ 100000000
>
> /* PCIe ELBI registers */
>
> -#define SOC_PCIECTRL_CTRL0_ADDR 0x000
>
> -#define SOC_PCIECTRL_CTRL1_ADDR 0x004
>
> +#define SOC_PCIECTRL_CTRL0_ADDR 0x000
>
> +#define SOC_PCIECTRL_CTRL1_ADDR 0x004
>
> #define SOC_PCIEPHY_CTRL2_ADDR 0x008
>
> #define SOC_PCIEPHY_CTRL3_ADDR 0x00c
> ...
> [songxiaowei] The space of the name of these macro definition and
>
> the value are really different from 1 to 3 Tab, but it seem likes as bellow opened by vim.
>
> [cid:image001.png@...2E786.14883370]
The image shows this:
+#define REF_CLK_FREQ 100000000
+
+/* PCIe ELBI registers */
+#define SOC_PCIECTRL_CTRL0_ADDR 0x000
+#define SOC_PCIECTRL_CTRL1_ADDR 0x004
+#define SOC_PCIEPHY_CTRL2_ADDR 0x008
+#define SOC_PCIEPHY_CTRL3_ADDR 0x00c
So things are nicely aligned in the *patch*. But we don't care about
alignment in the patch. What we want is alignment in the *file*,
which ends up looking like this with your original patch:
#define REF_CLK_FREQ 100000000
/* PCIe ELBI registers */
#define SOC_PCIECTRL_CTRL0_ADDR 0x000
#define SOC_PCIECTRL_CTRL1_ADDR 0x004
#define SOC_PCIEPHY_CTRL2_ADDR 0x008
#define SOC_PCIEPHY_CTRL3_ADDR 0x00c
My incremental diff probably makes the patch look ugly, but the
resulting file looks nicer.
> struct kirin_pcie {
>
> - struct dw_pcie *pci;
>
> - void __iomem *apb_base;
> ...
> + struct dw_pcie *pci;
>
> + void __iomem *apb_base;
> ...
> [songxiaowei] it seems the variables list in the same coloumn with vim.
Yes, these variables were aligned in your original patch; it's just
that they used two or three tabs, when one or two was sufficient. So
there's extra separation. Not a big deal.
Bjorn
Powered by blists - more mailing lists