linux-kernel - Possible issue at the kirin-pcie driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210706113503.66091e94@coco.lan>
Date:   Tue, 6 Jul 2021 11:35:03 +0200
From:   Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
To:     Xiaowei Song <songxiaowei@...ilicon.com>,
        Dejin Zheng <zhengdejin5@...il.com>,
        Manivannan Sadhasivam <mani@...nel.org>,
        Binghui Wang <wangbinghui@...ilicon.com>, linuxarm@...wei.com
Cc:     Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Rob Herring <robh@...nel.org>, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org, mauro.chehab@...wei.com
Subject: Possible issue at the kirin-pcie driver

Hi,

I was asked by Rob Herring to convert the kiring-pcie driver on two parts,
splitting the PHY logic from it, in order to be able to add PHY support 
for Hikey 970 at drivers/pci/controller/dwc/pcie-kirin.c.

While doing so, I noticed something weird issue at the driver, with regards
to a certain register (PCIE_APB_PHY_STATUS0), as shown below:

...

	#define PCIE_APB_PHY_STATUS0	0x400
...
	static inline u32 kirin_apb_ctrl_readl(struct kirin_pcie *kirin_pcie, u32 reg)
	{
		return readl(kirin_pcie->apb_base + reg);
	}
...
	static inline u32 kirin_apb_phy_readl(struct kirin_pcie *kirin_pcie, u32 reg)
	{
		return readl(kirin_pcie->phy_base + reg);
	}
...
	static int kirin_pcie_phy_init(struct kirin_pcie *kirin_pcie)
	{
...
		reg_val = kirin_apb_phy_readl(kirin_pcie, PCIE_APB_PHY_STATUS0);
		if (reg_val & PIPE_CLK_STABLE) {
                	dev_err(dev, "PIPE clk is not stable\n");
			return -EINVAL;
		}
	}
...
	static int kirin_pcie_link_up(struct dw_pcie *pci)
	{
		struct kirin_pcie *kirin_pcie = to_kirin_pcie(pci);
		u32 val = kirin_apb_ctrl_readl(kirin_pcie, PCIE_APB_PHY_STATUS0);

		if ((val & PCIE_LINKUP_ENABLE) == PCIE_LINKUP_ENABLE)
			return 1;

		return 0;

		u32 val = kirin_apb_ctrl_readl(kirin_pcie, PCIE_APB_PHY_STATUS0);

		if ((val & PCIE_LINKUP_ENABLE) == PCIE_LINKUP_ENABLE)
			return 1;

Basically, the code at kirin_pcie_phy_init() use this register as if it is 
part of the PHY memory region (0xf3f20000 + 0x400), while the code at 
kirin_pcie_link_up() considers is as belonging to the APB memory
region (0xff3fe000 + 0x400).

It sounds to me that there's a mistake somewhere. I mean, either:

1. there is a cut-and-paste error, caused it to access the wrong memory
   region, e.g. at kirin_pcie_link_up() the logic should be:

	u32 val = kirin_apb_phy_readl(kirin_pcie, PCIE_APB_PHY_STATUS0);

   instead of:

	u32 val = kirin_apb_ctrl_readl(kirin_pcie, PCIE_APB_PHY_STATUS0);

   (or the reverse)

2. Both memory regions have a register at address 0x400 with similar
   names that ended being merged into the same macro;

3. the register for APB PHY status0 is duplicated on both regions and,
   on both, they are at region_base + 0x400.

I suspect that it is (1), but, as I don't have any datasheets or
register map, I can't tell for sure.

Could someone with access to the datahseets shed the light?

Thanks,
Mauro