Re: [PATCH v4 2/2] EDAC: add EDAC driver for DMC520

[James Morse <james.morse@arm.com>]

Hi,

On 04/07/2019 01:16, Lei Wang wrote:
>> #include <linux/spinlock.h> ?
>>
>> It's best to keep this list sorted, it makes it easier for the maintainer to resolve
>> conflicts when header files get split/moved-around.
> 
> It builds fine in our distro. The header seems to have been inherited 
> from some other header files.

Heh, you can't rely on this! Someone will disturb the header-soup, and inexplicably your
driver will stop building. If your using a thing, you need to include the headers that
define it.


>>> +#define DMC520_BUS_WIDTH	8  /* Data bus width is 64bits/8Bytes */

>> Can you point me to where this comes from in the datasheet[0]?
>> I see it talk in "1.3 Features" of "either a 32-bit wide data SDRAM interface or a 64-bit
>> wide data SDRAM interface".
>>
>> If this is a choice that was made on your platform it needs to be described in the DT.
>>
>> (I may be confused between SDRAM/DDR/DRAM, as 2.3.3. "PHY interface" seems to describe one
>> connecting to the other.)

( a bit more reading shows these are all terms for pretty much the same thing )


> I didn't find a configuration for bus width from the dmc520 doc. I 
> search online and it seems like more than one places state 64-bit as 
> DDR's data bus width, e.g. this one 
> (https://en.wikipedia.org/wiki/DDR_SDRAM) mentions "DDR memory bus width 
> per channel is 64 bits (72 for ECC memory)." So I took it as a define 
> here for 8 bytes.

Ooer. Your platform might not be the same as wikipedia's!
From the datasheet it looks like this is configurable for DMC520, if so we shouldn't
hard-code the value in the driver...

~ (more datasheet digging) ~

Bingo: 'format_control' can be read in all states and has a 'memory_width' field that
indicates if the PHY is connected to a 'x32 DDR device' or 'x64 DDR device'. (I'm guessing
those double bit values are for address + data)

This probably affects the 'grain' too.


>>> +static void dmc520_handle_dram_ecc_errors(struct mem_ctl_info *mci,
>>> +					  bool is_ce)
>>> +{
>>> +	struct ecc_error_info info;
>>> +	struct dmc520_edac *edac;
>>> +	u32 cnt;
>>> +	char message[EDAC_MSG_BUF_SIZE];
>>> +	unsigned long flags;
>>> +
>>> +	edac = mci->pvt_info;
>>> +	dmc520_get_dram_ecc_error_info(edac, is_ce, &info);
>>> +
>>> +	cnt = dmc520_get_dram_ecc_error_count(edac, is_ce);
>>> +
>>> +	if (cnt > 0) {
>>> +		snprintf(message, ARRAY_SIZE(message),
>>> +			 "rank:%d bank:%d row:%d col:%d",
>>> +			 info.rank, info.bank,
>>> +			 info.row, info.col);
>>> +
>>> +		spin_lock_irqsave(&edac->ecc_lock, flags);
>> irqsave/irqrestore is overkill as this function is only called from an interrupt handler.
>> There is no way for this to be called with interrupts unmasked.

> Still feel spin_lock_irqsave and spin_unlock_irqstore are the safest. If 
> a processor is on isr for interrupt line 1 and calls 
> dmc520_handle_dram_ecc_errors, only line 1 is disabled and and other 
> lines can still interrupt calling dmc520_handle_dram_ecc_errors again. 

But not on the same CPU, (which is the problem the irqsave helpers solve).

When the CPU takes an interrupt the hardware sets a status bit to prevent this CPU taking
any other interrupt. On armv8, this is PSTATE.I, its set automatically by the hardware. If
CPUs didn't do this, you could never guarantee that any interrupt would ever be handled.

You are right a second interrupt may occur, but it can't interrupt this CPU until it
returns from the Interrupt, or clears the status bit. If the second interrupt is taken at
the same time its because it was routed to a different CPU. A regular spin_lock() stops
any problems here, the second CPU has to wait for the first CPU to release the lock.

spin_lock_irqsave() is for a more complicated problem. If you used the spinlock in process
context, (e.g. your probe function), as well as interrupt context, then its possible the
interrupt is taken while the lock is held in process context on the same CPU. This causes
your interrupt handler to wait forever for the lock. The irqsave helpers stop this by
masking interrupts when taking the lock, so that this 'same CPU' sequence can't happen.

Because you don't take the lock in process context, you don't need the irqsave variants.


>>> +	for (intr_index = 0; intr_index < nintr; ++intr_index) {
>>> +		if (edac->interrupt_mask_all & edac->interrupt_masks[intr_index]) {
>>> +			edac_printk(KERN_ERR, EDAC_MC,
>>> +				"interrupt-config error: "
>>> +				"element %d's interrupt mask %d has overlap.\n",
>>> +				intr_index, edac->interrupt_masks[intr_index]);
>>> +			goto err_free_mc;
>>> +		}
>>> +
>>> +		edac->interrupt_mask_all |= edac->interrupt_masks[intr_index];
>>> +	}

>> Ah, so the driver doesn't support overlapping masks... but wasn't this the reason for
>> describing the interrupts with these masks in the first place?
>> (It looks like the DT-folk want this as named interrupts)
>>
>> lore.kernel.org/r/BYAPR21MB1319BC4D079B918AB038A4D590010@BYAPR21MB1319.namprd21.prod.outlook.com
>>
>> Would this driver support the configuration you gave there?

> The interrupt line to mask mapping is to solve how to flexibly adapting 
> to possible hardware implementations. dmc520 supports multiple 
> interrupts (3.3.169 interrupt_control). And these interrupts may have 
> different ways to be wired to physical interrupt lines. As in the above 
> link, in this particular brcm implementation:
> 
> Line 841: source dram_ecc_errc_int
> Line 843: source dram_ecc_errd_int
> Line 839: source dram_ecc_errc_int and dram_ecc_errd_int
> 
> Two straightforward possibilities for implementing ecc counts for ce/ue: 
> 1. We chose to use the single source line. 2. It's possible to implement 
> using the combined-source line too. Our implementation would support 
> either of these cases.
> 
> Of course there might be other possibilities that involve overlapping, 
> such as including all above 3 interrupt lines into the DT. But this 
> unlikely is of any real value of use. Our implementation does not 
> support this case.

Right, so the driver does support this, but not at the same time as independant interrupts.


>> With the bool/enum and interrupt-disabling things fixed:
>> Reviewed-by: James Morse <james.morse@arm.com>
>>
> New to the upstreaming review process. Does this last comment mean we're 
> closer? :)

Heh, yes. This translates as: If you post a subsequent version with those two issues
fixed, please include that Reviewed-by tag next to your Signed-off-by.

{
If you could also summarise the changes you make next to the diffstat, it allows people
who have given tags to only look at the bits you changed, (instead of playing spot the
difference).

As an example:
https://lore.kernel.org/r/20190521172139.21277-3-julien.grall@arm.com

git knows to discard the changes-between-versions and diffstat bits when the patch is
applied, they don't end up in the log.
}

What happens next? Your series gets more review and collects tags. This will include the
maintainers of each tree you're touching either giving tags, or queueing the series. From
there it sits in linux-next until the next merge-window, when the maintainer will send a
pull-request to Linus. Eventually it ends up in the release-candidates, and finally a
released kernel.


(N.B: your mail is still coming base64 encoded, so its very unlikely the maintainer can
pick it up.)


Thanks,

James