On 1/30/09 2:05 PM, Andrew Morton wrote:
> On Fri, 30 Jan 2009 09:54:42 -0700 dougthomp...@xmission.com wrote:
>> From: Grant Erickson <gerick...@nuovations.com>
>
> Perhaps a powerpc mailing list should have been cc'ed?
The first round patch went to Doug, the BlueSmoke (EDAC) mailing list and
the Linux/PowerPC mailing list. However, because the original patch was
split in two, subsequent revisions of just the EDAC piece went to Doug and
BlueSmoke. Doug then forwarded it to linux-kernel.
What's the preferred sign-off, ACK chain for this subsystem? Through
PowerPC/4xx or PowerPC GIT upstream or through you and -mm upstream?
> These comments try really hard to be in kerneldoc form, but don't quite
> succeed.
>
> And I don't think that kerneldoc understands `...@return'? It should :(
Aye. I was mistakenly under the impression that Doxygen == kernel-doc;
however, that's clearly not the case. The next revision of the patch will
have these rectified.
>> +static int
>> +ppc4xx_edac_generate_bank_message(const struct mem_ctl_info *mci,
>> + const struct ppc4xx_ecc_status *status,
>> + char *buffer,
>> + size_t size)
>> +{
>> + int n, total = 0;
>> + size_t row, rows;
>
> It seems strange to use a size_t here.
Stylistically, I tend to use 'size_t' for 'unsigned type where I am counting
things'. However, I can see where this usage might be confusing and
surprising for some.
The next revision of the patch will use 'unsigned int'.
>> +static void
>> +ppc4xx_edac_handle_ce(struct mem_ctl_info *mci,
>> + const struct ppc4xx_ecc_status *status)
>> +{
>> + int row;
>> + char message[PPC4XX_EDAC_MESSAGE_SIZE];
>
> 256 bytes on the stack is getting a bit large.
Would you characterize this as a "getting a bit large, but still OK" or
"getting a bit large, this MUST be changed"?
I took my guidance from drivers/edac/i5[04]000_edac.c which allocate around
200 bytes on the stack for a similar use.
However, Josh Boyer had suggested that given all the snprintf going on in
the interrupt handler, a work queue might be a better way to go. ISR timings
for a sample population of 300 events are/were:
Ticks Time / us
--------------------------------
Minimum 4150 10.38
Maximum 9075 22.69
Mean 8024 20.06
Median 8297 20.74
Mode 8869 22.17
Std. Dev. 864 2.16
--------------------------------
In short, if this is a MUST rather than a SHOULD, reworking the driver to
pull the message buffers off the stack and implementing a work loop might be
a two-for-one rework opportunity.
>> +static void
>> +ppc4xx_edac_handle_ue(struct mem_ctl_info *mci,
>> + const struct ppc4xx_ecc_status *status)
>> +{
>> + const u64 bear = ((u64)status->bearh << 32 | status->bearl);
>> + const unsigned long pfn = bear >> PAGE_SHIFT;
>
> The term `pfn' has a specific meaining in-kernel, and I have a
> suspicion that this variable doesn't match it.
I changed 'pfn' and 'poff' to 'page' and 'offset' respectively, in the next
revision of the patch.
Thanks for your feedback!
Regards,
Grant
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
