linux-kernel - Re: [PATCH 2/2 v3] irqchip/Layerscape: Add SCFG MSI controller support

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: <56D41A04.8030302@arm.com>
Date:	Mon, 29 Feb 2016 10:14:28 +0000
From:	Marc Zyngier <marc.zyngier@....com>
To:	Minghuan Lian <minghuan.lian@....com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	Jason Cooper <jason@...edaemon.net>,
	Roy Zang <roy.zang@....com>, Mingkai Hu <mingkai.hu@....com>,
	Stuart Yoder <stuart.yoder@....com>,
	Yang-Leo Li <leoyang.li@....com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2 v3] irqchip/Layerscape: Add SCFG MSI controller
 support

On 25/02/16 03:21, Minghuan Lian wrote:
> Hi Marc,
> 
> I am sorry for the delayed response due to the Chinese Spring Festival holiday.
> Thank you very much for the review.
> Please see my comments inline.
> 
> Thanks,
> Minghuan
> 

[...]

>>> +static int ls_scfg_msi_probe(struct platform_device *pdev) {
>>> +	struct ls_scfg_msi *msi_data;
>>> +	struct resource *res;
>>> +	int ret;
>>> +
>>> +	msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL);
>>> +	if (!msi_data)
>>> +		return -ENOMEM;
>>> +
>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +	msi_data->regs = devm_ioremap_resource(&pdev->dev, res);
>>> +	if (IS_ERR(msi_data->regs)) {
>>> +		dev_err(&pdev->dev, "failed to initialize 'regs'\n");
>>> +		return PTR_ERR(msi_data->regs);
>>> +	}
>>> +	msi_data->msiir_addr = res->start;
>>> +
>>> +	msi_data->irq = platform_get_irq(pdev, 0);
>>> +	if (msi_data->irq <= 0) {
>>> +		dev_err(&pdev->dev, "failed to get MSI irq\n");
>>> +		return -ENODEV;
>>> +	}
>>> +
>>> +	msi_data->pdev = pdev;
>>> +	msi_data->nr_irqs = MSI_MAX_IRQS;
>>
>> So this is hardcoded, always. Why do you need a nr_irqs variable at all?
> [Lian Minghuan-B31939] Currently, nr_irqs is always 32, but in the
> future, the MSI controller may be extended to support more IRQs. And,
> we may set nr_irqs the value of less than 32 to reserve some IRQs for
> special usage. So nr_irqs can bring flexibility

You have to choose: either this is configurable and you describe it in
DT, or this is not and you drop this field from the structure.

As for the "reserved interrupts", that would need to be described too.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...