Re: [PATCH 1/1] edac: new ppc4xx driver module

Re: [PATCH 1/1] edac: new ppc4xx driver module

[Grant Erickson]

On 1/30/09 2:05 PM, Andrew Morton wrote:
> On Fri, 30 Jan 2009 09:54:42 -0700 dougthompson@xmission.com wrote:
>> From: Grant Erickson <gerickson@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

Re: [PATCH 1/1] edac: new ppc4xx driver module

[Andrew Morton]

On Fri, 06 Feb 2009 11:40:41 -0800
Grant Erickson <gerickson@nuovations.com> wrote:

> On 1/30/09 2:05 PM, Andrew Morton wrote:
> > On Fri, 30 Jan 2009 09:54:42 -0700 dougthompson@xmission.com wrote:
> >> From: Grant Erickson <gerickson@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?

I don't trust those guys ;)

I'd merge it via -mm, while cc'ing various random powerpc personalities.

> >> +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"?

The former.

Re: [PATCH 1/1] edac: new ppc4xx driver module

[Josh Boyer]

On Fri, Feb 06, 2009 at 11:49:47AM -0800, Andrew Morton wrote:
>On Fri, 06 Feb 2009 11:40:41 -0800
>Grant Erickson <gerickson@nuovations.com> wrote:
>
>> On 1/30/09 2:05 PM, Andrew Morton wrote:
>> > On Fri, 30 Jan 2009 09:54:42 -0700 dougthompson@xmission.com wrote:
>> >> From: Grant Erickson <gerickson@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?
>
>I don't trust those guys ;)

I might take offense to that, but it seems that there aren't too many people
in general that you trust, so I guess that's fine. ;)

(And while you may or may not be joking, if there's something that I've done
to be considered not trustworthy then please let me know.)

>I'd merge it via -mm, while cc'ing various random powerpc personalities.

Erm, ok.  Since there doesn't appear to be an EDAC git tree for -next I
guess that makes sense.  I'll try to review the most recent version in the
next few days.

josh

Re: [PATCH 1/1] edac: new ppc4xx driver module

[Andrew Morton]

On Mon, 9 Feb 2009 16:01:19 -0500
Josh Boyer <jwboyer@linux.vnet.ibm.com> wrote:

> On Fri, Feb 06, 2009 at 11:49:47AM -0800, Andrew Morton wrote:
> >On Fri, 06 Feb 2009 11:40:41 -0800
> >Grant Erickson <gerickson@nuovations.com> wrote:
> >
> >> On 1/30/09 2:05 PM, Andrew Morton wrote:
> >> > On Fri, 30 Jan 2009 09:54:42 -0700 dougthompson@xmission.com wrote:
> >> >> From: Grant Erickson <gerickson@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?
> >
> >I don't trust those guys ;)
> 
> I might take offense to that, but it seems that there aren't too many people
> in general that you trust, so I guess that's fine. ;)
> 
> (And while you may or may not be joking, if there's something that I've done
> to be considered not trustworthy then please let me know.)

Every kernel cycle there are literally hundreds of patches which slip
through subsystem tree maintainers' fingers.  Patches which I unlose
for them.  Heaven knows how many patches get lost on mailing lists
which I don't read.

My mistrust is widespread and well-placed ;)

> >I'd merge it via -mm, while cc'ing various random powerpc personalities.
> 
> Erm, ok.  Since there doesn't appear to be an EDAC git tree for -next I
> guess that makes sense.  I'll try to review the most recent version in the
> next few days.

Thanks.