linux-kernel - Re: [PATCH 2/2] PCI: generic: add description of property "interrupt-skip-mask"

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <56CFFC9B.20703@huawei.com>
Date:	Fri, 26 Feb 2016 15:19:55 +0800
From:	"Leizhen (ThunderTown)" <thunder.leizhen@...wei.com>
To:	Mark Rutland <mark.rutland@....com>
CC:	Bjorn Helgaas <bhelgaas@...gle.com>,
	linux-pci <linux-pci@...r.kernel.org>,
	Rob Herring <robh+dt@...nel.org>,
	Frank Rowand <frowand.list@...il.com>,
	Grant Likely <grant.likely@...aro.org>,
	Will Deacon <will.deacon@....com>,
	Pawel Moll <pawel.moll@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
	devicetree <devicetree@...r.kernel.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Zefan Li <lizefan@...wei.com>, Xinwei Hu <huxinwei@...wei.com>,
	Tianhong Ding <dingtianhong@...wei.com>,
	Hanjun Guo <guohanjun@...wei.com>,
	Yijing Wang <wangyijing@...wei.com>
Subject: Re: [PATCH 2/2] PCI: generic: add description of property
 "interrupt-skip-mask"



On 2016/2/25 20:20, Mark Rutland wrote:
> Hi,
> 
> In future, please send the binding document first in a series, per point
> 3 of Documentation/devicetree/bindings/submitting-patches.txt. It makes
> review easier/faster.
Thank you for your reminding.

> 
> On Thu, Feb 25, 2016 at 07:53:28PM +0800, Zhen Lei wrote:
>> Interrupt Pin register is read-only and optional. Some pci devices may use
>> msi/msix but leave the value of Interrupt Pin non-zero.
> 
> Is that permitted by the spec? Surely 'optional' means it must be zero
> if not implemented?

In <PCI Local Bus Specification Revision 3.0>:
Devices (or device functions) that do not use an interrupt pin must put a 0 in this register. This register is read-only.

So, do you think this is a hardware bug? But these pci-devices are not produced by our company.

In function init_service_irqs, it try msix first, then msi, Interrupt PIN is the last attemption. But of_irq_parse_pci() happened before this.


In fact, there also a familiar problem exist. As below:
pci 0000:42:00.0: BAR 7: no space for [io  size 0x1000]
pci 0000:42:00.0: BAR 7: failed to assign [io  size 0x1000]

There no "io space" on arm64, maybe only exist on X86. And the Memory Space Indicator also read-only in BAR register.

> 
>> In this case, the driver will print information as below: pci
>> 0000:40:00.0: of_irq_parse_pci() failed with rc=-22
>>
>> It's easily lead to misinterpret.
> 
> If this is limited to a subset of devices which we know are broken in
> this regard, can we not handle these cases explicitly?
Actually, we have another way to block this warning. Use "interrupt-map" to map it to a pesudo IRQ. But I think it will also be misunderstanded.

> 
>> Signed-off-by: Zhen Lei <thunder.leizhen@...wei.com>
>> ---
>>  Documentation/devicetree/bindings/pci/host-generic-pci.txt | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.txt b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
>> index 3f1d3fc..0f10978 100644
>> --- a/Documentation/devicetree/bindings/pci/host-generic-pci.txt
>> +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
>> @@ -70,6 +70,8 @@ Practice: Interrupt Mapping' and requires the following properties:
>>
>>  - interrupt-map-mask : <see aforementioned specification>
>>
>> +- interrupt-skip-mask: Explicitly declare which pci devices only use msi/msix
>> +but leave the value of Interrupt Pin non-zero.
> 
> Unlike the rest of the interrupt mapping properties, this is not
> described in  `Open Firmware Recommended Practice: Interrupt Mapping'.
> 
> This needs a far more complete description.
> 
> This also doesn't strike me as th right approach. The interrupt-map-mask
> property describe as relationship between the host-controller-provided
> interrupt lines and endpoints, while this seems to be a bug completely
> contained within an endpoint.

In <host-generic-pci.txt>:
// PCI_DEVICE(3)  INT#(1)  CONTROLLER(PHANDLE)  CONTROLLER_DATA(3)
    interrupt-map = <  0x0 0x0 0x0  0x1  &gic  0x0 0x4 0x1

PCI_DEVICE contain 3 cells. But only the first one be used in function of_irq_parse_pci.
laddr[0] = cpu_to_be32((pdev->bus->number << 16) | (pdev->devfn << 8));
laddr[1] = laddr[2] = cpu_to_be32(0);

And for INT#, I don't think there will some Pins used but others unused on a pci-device. So I can ommit it.

So, only laddr[0] mask need to be described.
> 
> Thanks,
> Mark.
> 
>>
>>  Example:
>>
>> --
>> 2.5.0
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo@...r.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> .
>