linux-kernel - Re: [PATCH v3 1/9] ARM: OMAP2+: gpmc: driver conversion

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

Message-ID: <4F85A77E.30203@ti.com>
Date:	Wed, 11 Apr 2012 10:47:10 -0500
From:	Jon Hunter <jon-hunter@...com>
To:	"Mohammed, Afzal" <afzal@...com>
CC:	"tony@...mide.com" <tony@...mide.com>,
	"Hilman, Kevin" <khilman@...com>,
	"linux@....linux.org.uk" <linux@....linux.org.uk>,
	"dwmw2@...radead.org" <dwmw2@...radead.org>,
	"sameo@...ux.intel.com" <sameo@...ux.intel.com>,
	"grinberg@...pulab.co.il" <grinberg@...pulab.co.il>,
	"mike@...pulab.co.il" <mike@...pulab.co.il>,
	"Menon, Nishanth" <nm@...com>,
	"artem.bityutskiy@...ux.intel.com" <artem.bityutskiy@...ux.intel.com>,
	"vimal.newwork@...il.com" <vimal.newwork@...il.com>,
	"dbaryshkov@...il.com" <dbaryshkov@...il.com>,
	"linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Hiremath, Vaibhav" <hvaibhav@...com>
Subject: Re: [PATCH v3 1/9] ARM: OMAP2+: gpmc: driver conversion

Hi Afzal,

On 04/11/2012 12:11 AM, Mohammed, Afzal wrote:
> Hi Jon,
> 
> On Wed, Apr 11, 2012 at 00:53:14, Hunter, Jon wrote:
>> I agree with your argument but I was thinking today only OMAP uses the 
>> GPMC so we could not worry about this. Ok, leave as-is, but can we 
>> modify the code as follows as the "else if" is not really needed...
>>
>> if (gpmc->num_irq < GPMC_NR_IRQ) {
>> 	dev_warn(gpmc->dev, "Insufficient interrupts for device\n");
>> 	return -EINVAL;
>> }
>>
>> gpmc->num_irq = GPMC_NR_IRQ;
> 
> Yes, it is better
> 
>>
>>>>
>>>> Furthermore, GPMC_NR_IRQ is defined as 6 which is correct for OMAP2/3
>>>> but not for OMAP4/5, it is 5. Therefore, we need to detect whether we
>>>> are using an OMAP2/3 or OMAP4+ and set num_irqs based upon this. This
>>>> could be done in the probe and we can avoid passing this.
>>>
>>> Is it dependent on OMAPX or GPMC IP version? if it is IP version, then driver
>>> can be enhanced to handle it, if not, platform has to pass this information.
>>
>> Here are the GPMC IP revisions ...
>>
>> OMAP5430 = 0x00000060
>> OMAP4430 = 0x00000060
>> OMAP3630 = 0x00000050
>> OMAP3430 = 0x00000050
>>
>> So this should work for OMAP. We should check OMAP2 as well. What about 
>> the AMxxx devices?
> 
> 
> I badly needed this information, thanks.
> 
> AM3359 = 0x00000060, it has only 2 waitpin interrupts

Great so this is consistent!

>>>>>>> +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>>>>>> +	if (res == NULL)
>>>>>>> +		dev_warn(gpmc->dev, "Failed to get resource: irq\n");
>>>>>>> +	else
>>>>>>> +		gpmc->master_irq = res->start;
>>>>>>
>>>>>> Why not return an error if the IRQ is not found? We don't know if anyone
>>>>>> will be trying to use them.
>>>>>
>>>>> Why do you want to do that ?
>>>>
>>>> Because this indicates a BUG :-)
>>>
>>> I disagree, this need not be considered a bug always,
>>> for eg. If gpmc irq is not connected to intc
>>
>> Ok, so for devices existing today this indicates a bug ;-)
> 
> I do not want to consider that case to be bug enough for probe
> to fail, there are other drivers which does similar enhancing
> its use cases,
> 
> eg. 1e351a9 mfd: Make TPS65910 usable without interrupts

Ok, fine.

>>
>> At a minimum you need to improve the error handing here. If the 
>> platform_get_resource fails you are still calling "gpmc_setup_irq()" 
>> which appears to be pointless. It would be better if the gpmc irq chip 
>> is not initialised in this case so that drivers attempting to request 
>> these irqs failed.
> 
> Please see gpmc_setup_irq, if irq is not present, it returns in the
> beginning, and gpmc_irq_chip is not initialized in that case.

Yes you are right.

>>>>>>> +	for (gdq = gp->device_pdata, gd = gpmc->device; *gdq; gdq++, i++) {
>>>>>>> +		ret = gpmc_setup_device(*gdq, gd, gpmc);
>>>>>>> +		if (IS_ERR_VALUE(ret))
>>>>>>> +			dev_err(gpmc->dev, "gpmc setup on %s failed\n",
>>>>>>> +								(*gdq)->name);
>>>>>>> +		else
>>>>>>> +			gd++;
>>>>>>> +	}
>>>>>>
>>>>>> Would a while loop be simpler?
>>>>>
>>>>> My preference is to go with "for"
>>>>
>>>> Ok, just wondering if this could be cleaned up a little.
>>>
>>> For travelling through array of pointers, for looks natural to me, if you
>>> have a better way, please send it, it can be folded in next version.
>>
>> Could you have num_devices to indicate how many platform devices there 
>> are and then a simple for-loop of 0 to num_devices?
> 
> This will cause coding to be done by platform to be less simple, and my
> preference is not to use another variable

Hehe, I wondered if that would make life a little more difficult. Ok
lets leave it for now.

Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/