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
| ||
|
Date: Tue, 28 Mar 2017 04:23:46 +0800 From: Hanjun Guo <hanjun.guo@...aro.org> To: ahs3@...hat.com, Lorenzo Pieralisi <lorenzo.pieralisi@....com>, Gabriele Paoloni <gabriele.paoloni@...wei.com> CC: Marc Zyngier <marc.zyngier@....com>, John Garry <john.garry@...wei.com>, "Guohanjun (Hanjun Guo)" <guohanjun@...wei.com>, "Rafael J. Wysocki" <rafael@...nel.org>, "Yimin (Leo)" <yimin@...wei.com>, Greg KH <gregkh@...uxfoundation.org>, Linuxarm <linuxarm@...wei.com>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Sinan Kaya <okaya@...eaurora.org>, "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>, Tomasz Nowicki <tn@...ihalf.com>, Thomas Gleixner <tglx@...utronix.de>, "linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, "majun (F)" <majun258@...wei.com>, dvhart@...radead.org Subject: Re: [PATCH v9 15/15] irqchip: mbigen: Add ACPI support Hi Al, Thanks for your feedback, reply inline. On 03/28/2017 02:56 AM, Al Stone wrote: > On 03/27/2017 09:27 AM, Lorenzo Pieralisi wrote: >> [+Al,Darren to comment on _DSD review process] >> >> On Mon, Mar 27, 2017 at 12:24:45PM +0000, Gabriele Paoloni wrote: >>> Hi Marc Many thanks for your comments >>>> Hanjun, John, >>>> >>>> On 22/03/17 14:12, John Garry wrote: >>>>> On 21/03/2017 14:45, Lorenzo Pieralisi wrote: >>>>>> On Tue, Mar 07, 2017 at 08:40:10PM +0800, Hanjun Guo wrote: >>>>>>> From: Hanjun Guo <hanjun.guo@...aro.org> >>>>>>> >>>>>>> With the preparation of platform msi support and interrupt producer >>>>>>> in DSDT, we can add mbigen ACPI support now. >>>>>>> >>>>>>> We are using Interrupt resource type in _CRS methd to indicate >>>> number >>>>>>> of irq pins instead of num_pins in DT to avoid _DSD usage in this >>>> case. >>>>>>> >>>>>>> For mbigen, >>>>>>> Device(MBI0) { >>>>>>> Name(_HID, "HISI0152") >>>>>>> Name(_UID, Zero) >>>>>>> Name(_CRS, ResourceTemplate() { >>>>>>> Memory32Fixed(ReadWrite, 0xa0080000, 0x10000) >>>>>>> Interrupt(ResourceProducer,...) {12,14,....} >>>>>> >>>>>> What do these interrupt numbers represent ? This looks wrong to me. >>>>>> An interrupt descriptor is there to describe the interrupts a device >>>>>> can generate; you are using it just to add a "standard" (that is >>>>>> not standard at all) way of counting the number of vectors allocated >>>>>> to this specific chip and that's just wrong. >>>>>> >>>>> >>>>> As I understand, the count of interrupts we are declaring for the >>>> mbigen >>>>> is the same as the sum of interrupts for that mbigen's children. >>>>> >>>>> So at the point we probe the mbigen, can we just deference the >>>> children >>>>> to count their interrupts, and use this as the #msis? >>>>> >>>>>> Can't you use something like Agustin did in the QCOM combiner: >>>>>> >>>>>> drivers/irqchip/qcom-irq-combiner.c >>>>>> >>>>>> to detect the MSI vector length (ie by describing the MBIgen through >>>>>> generic registers and use the bit width to compute the vector >>>>>> lenght) ? I am not sure how feasible it is given that my knowledge >>>>>> of MBIgen is pretty poor. >>>>>> >>>>>> I understand we want to avoid _DSD properties but we should not >>>>>> work around standard bindings to achieve that goal. >>>>>> >>>>> >>>>> We use "num-pins" for dt solution, but it is not so welcome here. >>>> >>>> Well, this device is already completely out of any standard description >>>> on the ACPI side. And given that it bloats both the ACPI tables and the >>>> kernel data structures, I can only suggest that you take advantage of >>>> _DSD here, as misusing the standard properties is not something that we >>>> should condone. It will also make the driver more manageable, as it >>>> will >>>> use similar properties on both firmware implementations. >>>> >>>> I feel like I need to stress the urgency here. We're at -rc4, and still >>>> with unsolved issues. None of us want to miss the next merge window. >>>> >>> >>> As follow up our guys would work on a solution whose ACPI table looks >>> like the following one: >>> >>> For mbigen, >>> Device(MBI0) { >>> Name(_HID, "HISI0152") >>> Name(_UID, Zero) >>> Name(_CRS, ResourceTemplate() { >>> Memory32Fixed(ReadWrite, 0xa0080000, 0x10000) >>> }) >>> >>> Name(_DSD, Package () { >>> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), >>> Package () >>> { >>> Package () {"num-pins", xxx} >>> } >>> }) >>> } > > Please fix the indentation here; the ASL parser doesn't care, but I don't > see '}'s as clearly as it does :). In particular, _DSD usage *must* be for > a device and it does not look like that if one doesn't carefully count all > the parentheses and braces. I will fix it :) > >>> >>> For devices, >>> Device(COM0) { >>> Name(_HID, "ACPIIDxx") >>> Name(_UID, Zero) >>> Name(_CRS, ResourceTemplate() { >>> Memory32Fixed(ReadWrite, 0xb0030000, 0x10000) >>> Interrupt(ResourceConsumer,..., "\_SB.MBI0") {12} >>> }) >>> } >>> >>> >>> Marc, Lorenzo if you are ok with the above we will submit v10 based on this... >> >> I am ok with it. I am not 100% up-to-date on what's the status on _DSD >> bindings/review/guidelines but it would be certainly a good idea to >> kickstart the process for MBIgen which basically means following this >> as far as I know (and post to the relevant mailing list): >> >> https://github.com/ahs3/dsd/blob/master/documentation/process_rules.txt > > So, let's correct this bit: consider that URL deprecated, please, and refer > instead to the text that was agreed upon and in the kernel tree: > > Documentation/acpi/DSD-properties-rules.txt > >> Al and Darren may add to that as they have more insights. > > Since this use of _DSD is very specific to this device, and this device > only, I don't have any real objections. Thanks for the confirmation. > I will say that "num-pins" is > not terribly descriptive so it would be really good to either use a much > more descriptive name or add plenty of commentary in the code -- and > preferably both. I would recommend including the ASL in the code comments, > with a description of how the property is to be used and what it means. "num-pins" is used for mbigen DT support: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/interrupt-controller/hisilicon,mbigen-v2.txt I think we can use the name unchanged and adding more comments as you suggested, will cc you for this updated patch. Thanks Hanjun
Powered by blists - more mailing lists