* Regression in 32-bit ppc kernel
From: Larry Finger @ 2012-04-24 22:58 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras; +Cc: linuxppc-dev, LKML
Hi,
Somewhere between v3.2 and v3.3, the kernel in my Powerbook G4 started issuing
the following traceback on bootup:
[ 40.264006] irq 23: nobody cared (try booting with the "irqpoll" option)
[ 40.264031] Call Trace:
[ 40.264070] [dfff3f00] [c000984c] show_stack+0x7c/0x194 (unreliable)
[ 40.264102] [dfff3f40] [c00a6840] __report_bad_irq+0x44/0xf4
[ 40.264119] [dfff3f60] [c00a6adc] note_interrupt+0x1ec/0x2ac
[ 40.264135] [dfff3f80] [c00a48a8] handle_irq_event_percpu+0x250/0x2b8
[ 40.264152] [dfff3fd0] [c00a4944] handle_irq_event+0x34/0x54
[ 40.264169] [dfff3fe0] [c00a7514] handle_fasteoi_irq+0xb4/0x124
[ 40.264192] [dfff3ff0] [c000f5bc] call_handle_irq+0x18/0x28
[ 40.264208] [dec85ce0] [c000719c] do_IRQ+0x114/0x1cc
[ 40.264226] [dec85d10] [c0015868] ret_from_except+0x0/0x1c
[ 40.264254] --- Exception: 501 at find_vma+0x10/0x80
[ 40.264259] LR = do_page_fault+0x26c/0x6ac
[ 40.264272] [dec85dd0] [c03f0128] do_page_fault+0x25c/0x6ac (unreliable)
[ 40.264289] [dec85f40] [c00155e4] handle_page_fault+0xc/0x80
[ 40.264327] --- Exception: 301 at 0x4800a174
The problem still exists in v3.4-rc3. I am currently doing a bisection of this
problem, but it will take a long time to complete.
Note: IRQ 23 is not active in v3.2.
Thanks,
Larry
^ permalink raw reply
* Re: [PATCH v2 1/2] powerpc/8xx: Fix NR_IRQ bugs and refactor 8xx interrupt controller
From: Scott Wood @ 2012-04-24 22:04 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1335266570.15830.11.camel@pasglop>
On 04/24/2012 06:22 AM, Benjamin Herrenschmidt wrote:
> On Tue, 2012-04-24 at 09:41 +0200, Joakim Tjernlund wrote:
>> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 2012/04/24 05:13:15:
>>>
>>> On Mon, 2012-04-23 at 16:30 -0600, Grant Likely wrote:
>>>> The mpc8xx driver uses a reference to NR_IRQS that is buggy. It uses
>>>> NR_IRQs for the array size of the ppc_cached_irq_mask bitmap, but
>>>> NR_IRQs could be smaller than the number of hardware irqs that
>>>> ppc_cached_irq_mask tracks.
>>>
>>> Joakim, any chance you can test this ASAP ? :-)
>>
>> Sorry, but no. We have not moved our 8xx boards to 3.x as it in maintenance
>> and takes too much space.
>
> Ah ok... Anybody else on the list who still has some 8xx gear ?
Boots OK on ep88xc.
-Scott
^ permalink raw reply
* [PATCH EDACv16 1/2] edac: Change internal representation to work with layers
From: Mauro Carvalho Chehab @ 2012-04-24 18:15 UTC (permalink / raw)
Cc: Mauro Carvalho Chehab, Jason Uhlenkott, Aristeu Rozanski,
Hitoshi Mitake, Shaohui Xie, Mark Gross, Dmitry Eremin-Solenikov,
Ranganathan Desikan, Egor Martovetsky, Niklas Söderlund,
Tim Small, Arvind R., Borislav Petkov, Chris Metcalf,
Olof Johansson, Doug Thompson, Linux Edac Mailing List,
Michal Marek, Jiri Kosina, Linux Kernel Mailing List, Joe Perches,
Andrew Morton, linuxppc-dev
In-Reply-To: <1335289087-11337-1-git-send-email-mchehab@redhat.com>
Change the EDAC internal representation to work with non-csrow
based memory controllers.
There are lots of those memory controllers nowadays, and more
are coming. So, the EDAC internal representation needs to be
changed, in order to work with those memory controllers, while
preserving backward compatibility with the old ones.
The edac core were written with the idea that memory controllers
are able to directly access csrows, and that the channels are
used inside a csrows select.
This is not true for FB-DIMM and RAMBUS memory controllers.
Also, some recent advanced memory controllers don't present a per-csrows
view. Instead, they view memories as DIMM's, instead of ranks, accessed
via csrow/channel.
So, change the allocation and error report routines to allow
them to work with all types of architectures.
This will allow the removal of several hacks on FB-DIMM and RAMBUS
memory controllers on the next patches.
Also, several tests were done on different platforms using different
x86 drivers.
TODO: a multi-rank DIMM's are currently represented by multiple DIMM
entries at struct dimm_info. That means that changing a label for one
rank won't change the same label for the other ranks at the same dimm.
Such bug is there since the beginning of the EDAC, so it is not a big
deal. However, on several drivers, it is possible to fix this issue, but
it should be a per-driver fix, as the csrow => DIMM arrangement may not
be equal for all. So, don't try to fix it here yet.
PS.: I tried to make this patch as short as possible, preceding it with
several other patches that simplified the logic here. Yet, as the
internal API changes, all drivers need changes. The changes are
generally bigger on the drivers for FB-DIMM's.
FIXME: while the FB-DIMMs are not converted to use the new
design, uncorrected errors will show just one channel. In
the past, all changes were on a big patch with about 150K.
As it needed to be split, in order to be accepted by the
EDAC ML at vger, we've opted to have this small drawback.
As an advantage, it is now easier to review the patch series.
Cc: Aristeu Rozanski <arozansk@redhat.com>
Cc: Doug Thompson <norsk5@yahoo.com>
Cc: Borislav Petkov <borislav.petkov@amd.com>
Cc: Mark Gross <mark.gross@intel.com>
Cc: Jason Uhlenkott <juhlenko@akamai.com>
Cc: Tim Small <tim@buttersideup.com>
Cc: Ranganathan Desikan <ravi@jetztechnologies.com>
Cc: "Arvind R." <arvino55@gmail.com>
Cc: Olof Johansson <olof@lixom.net>
Cc: Egor Martovetsky <egor@pasemi.com>
Cc: Chris Metcalf <cmetcalf@tilera.com>
Cc: Michal Marek <mmarek@suse.cz>
Cc: Jiri Kosina <jkosina@suse.cz>
Cc: Joe Perches <joe@perches.com>
Cc: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Hitoshi Mitake <h.mitake@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: "Niklas Söderlund" <niklas.soderlund@ericsson.com>
Cc: Shaohui Xie <Shaohui.Xie@freescale.com>
Cc: Josh Boyer <jwboyer@gmail.com>
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
v16: Only context changes
drivers/edac/edac_core.h | 92 ++++++-
drivers/edac/edac_mc.c | 682 ++++++++++++++++++++++++++++------------------
include/linux/edac.h | 40 ++-
3 files changed, 526 insertions(+), 288 deletions(-)
diff --git a/drivers/edac/edac_core.h b/drivers/edac/edac_core.h
index e48ab31..7201bb1 100644
--- a/drivers/edac/edac_core.h
+++ b/drivers/edac/edac_core.h
@@ -447,8 +447,13 @@ static inline void pci_write_bits32(struct pci_dev *pdev, int offset,
#endif /* CONFIG_PCI */
-extern struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
- unsigned nr_chans, int edac_index);
+struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
+ unsigned nr_chans, int edac_index);
+struct mem_ctl_info *new_edac_mc_alloc(unsigned edac_index,
+ unsigned n_layers,
+ struct edac_mc_layer *layers,
+ bool rev_order,
+ unsigned sz_pvt);
extern int edac_mc_add_mc(struct mem_ctl_info *mci);
extern void edac_mc_free(struct mem_ctl_info *mci);
extern struct mem_ctl_info *edac_mc_find(int idx);
@@ -467,24 +472,80 @@ extern int edac_mc_find_csrow_by_page(struct mem_ctl_info *mci,
* reporting logic and function interface - reduces conditional
* statement clutter and extra function arguments.
*/
-extern void edac_mc_handle_ce(struct mem_ctl_info *mci,
+
+void edac_mc_handle_error(const enum hw_event_mc_err_type type,
+ struct mem_ctl_info *mci,
+ const unsigned long page_frame_number,
+ const unsigned long offset_in_page,
+ const unsigned long syndrome,
+ const int layer0,
+ const int layer1,
+ const int layer2,
+ const char *msg,
+ const char *other_detail,
+ const void *mcelog);
+
+static inline void edac_mc_handle_ce(struct mem_ctl_info *mci,
unsigned long page_frame_number,
unsigned long offset_in_page,
unsigned long syndrome, int row, int channel,
- const char *msg);
-extern void edac_mc_handle_ce_no_info(struct mem_ctl_info *mci,
- const char *msg);
-extern void edac_mc_handle_ue(struct mem_ctl_info *mci,
+ const char *msg)
+{
+ edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
+ page_frame_number, offset_in_page, syndrome,
+ row, channel, -1, msg, NULL, NULL);
+}
+
+static inline void edac_mc_handle_ce_no_info(struct mem_ctl_info *mci,
+ const char *msg)
+{
+ edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
+ 0, 0, 0, -1, -1, -1, msg, NULL, NULL);
+}
+
+static inline void edac_mc_handle_ue(struct mem_ctl_info *mci,
unsigned long page_frame_number,
unsigned long offset_in_page, int row,
- const char *msg);
-extern void edac_mc_handle_ue_no_info(struct mem_ctl_info *mci,
- const char *msg);
-extern void edac_mc_handle_fbd_ue(struct mem_ctl_info *mci, unsigned int csrow,
- unsigned int channel0, unsigned int channel1,
- char *msg);
-extern void edac_mc_handle_fbd_ce(struct mem_ctl_info *mci, unsigned int csrow,
- unsigned int channel, char *msg);
+ const char *msg)
+{
+ edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
+ page_frame_number, offset_in_page, 0,
+ row, -1, -1, msg, NULL, NULL);
+}
+
+static inline void edac_mc_handle_ue_no_info(struct mem_ctl_info *mci,
+ const char *msg)
+{
+ edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
+ 0, 0, 0, -1, -1, -1, msg, NULL, NULL);
+}
+
+static inline void edac_mc_handle_fbd_ue(struct mem_ctl_info *mci,
+ unsigned int csrow,
+ unsigned int channel0,
+ unsigned int channel1,
+ char *msg)
+{
+ /*
+ *FIXME: The error can also be at channel1 (e. g. at the second
+ * channel of the same branch). The fix is to push
+ * edac_mc_handle_error() call into each driver
+ */
+ edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
+ 0, 0, 0,
+ csrow, channel0, -1, msg, NULL, NULL);
+}
+
+static inline void edac_mc_handle_fbd_ce(struct mem_ctl_info *mci,
+ unsigned int csrow,
+ unsigned int channel, char *msg)
+{
+ edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
+ 0, 0, 0,
+ csrow, channel, -1, msg, NULL, NULL);
+}
+
+
/*
* edac_device APIs
@@ -496,6 +557,7 @@ extern void edac_device_handle_ue(struct edac_device_ctl_info *edac_dev,
extern void edac_device_handle_ce(struct edac_device_ctl_info *edac_dev,
int inst_nr, int block_nr, const char *msg);
extern int edac_device_alloc_index(void);
+extern const char *edac_layer_name[];
/*
* edac_pci APIs
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 6ec967a..4d4d8b7 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -44,9 +44,25 @@ static void edac_mc_dump_channel(struct rank_info *chan)
debugf4("\tchannel = %p\n", chan);
debugf4("\tchannel->chan_idx = %d\n", chan->chan_idx);
debugf4("\tchannel->csrow = %p\n\n", chan->csrow);
- debugf4("\tdimm->ce_count = %d\n", chan->dimm->ce_count);
- debugf4("\tdimm->label = '%s'\n", chan->dimm->label);
- debugf4("\tdimm->nr_pages = 0x%x\n", chan->dimm->nr_pages);
+ debugf4("\tchannel->dimm = %p\n", chan->dimm);
+}
+
+static void edac_mc_dump_dimm(struct dimm_info *dimm)
+{
+ int i;
+
+ debugf4("\tdimm = %p\n", dimm);
+ debugf4("\tdimm->label = '%s'\n", dimm->label);
+ debugf4("\tdimm->nr_pages = 0x%x\n", dimm->nr_pages);
+ debugf4("\tdimm location ");
+ for (i = 0; i < dimm->mci->n_layers; i++) {
+ printk(KERN_CONT "%d", dimm->location[i]);
+ if (i < dimm->mci->n_layers - 1)
+ printk(KERN_CONT ".");
+ }
+ printk(KERN_CONT "\n");
+ debugf4("\tdimm->grain = %d\n", dimm->grain);
+ debugf4("\tdimm->nr_pages = 0x%x\n", dimm->nr_pages);
}
static void edac_mc_dump_csrow(struct csrow_info *csrow)
@@ -70,6 +86,8 @@ static void edac_mc_dump_mci(struct mem_ctl_info *mci)
debugf4("\tmci->edac_check = %p\n", mci->edac_check);
debugf3("\tmci->nr_csrows = %d, csrows = %p\n",
mci->nr_csrows, mci->csrows);
+ debugf3("\tmci->nr_dimms = %d, dimns = %p\n",
+ mci->tot_dimms, mci->dimms);
debugf3("\tdev = %p\n", mci->dev);
debugf3("\tmod_name:ctl_name = %s:%s\n", mci->mod_name, mci->ctl_name);
debugf3("\tpvt_info = %p\n\n", mci->pvt_info);
@@ -157,10 +175,25 @@ void *edac_align_ptr(void **p, unsigned size, int n_elems)
}
/**
- * edac_mc_alloc: Allocate a struct mem_ctl_info structure
- * @size_pvt: size of private storage needed
- * @nr_csrows: Number of CWROWS needed for this MC
- * @nr_chans: Number of channels for the MC
+ * edac_mc_alloc: Allocate and partially fills a struct mem_ctl_info structure
+ * @edac_index: Memory controller number
+ * @n_layers: Number of layers at the MC hierarchy
+ * layers: Describes each layer as seen by the Memory Controller
+ * @rev_order: Fills csrows/cs channels at the reverse order
+ * @size_pvt: size of private storage needed
+ *
+ *
+ * FIXME: drivers handle multi-rank memories on different ways: on some
+ * drivers, one multi-rank memory is mapped as one DIMM, while, on others,
+ * a single multi-rank DIMM would be mapped into several "dimms".
+ *
+ * Non-csrow based drivers (like FB-DIMM and RAMBUS ones) will likely report
+ * such DIMMS properly, but the CSROWS-based ones will likely do the wrong
+ * thing, as two chip select values are used for dual-rank memories (and 4, for
+ * quad-rank ones). I suspect that this issue could be solved inside the EDAC
+ * core for SDRAM memories, but it requires further study at JEDEC JESD 21C.
+ *
+ * In summary, solving this issue is not easy, as it requires a lot of testing.
*
* Everything is kmalloc'ed as one big chunk - more efficient.
* Only can be used if all structures have the same lifetime - otherwise
@@ -172,18 +205,41 @@ void *edac_align_ptr(void **p, unsigned size, int n_elems)
* NULL allocation failed
* struct mem_ctl_info pointer
*/
-struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
- unsigned nr_chans, int edac_index)
+struct mem_ctl_info *new_edac_mc_alloc(unsigned edac_index,
+ unsigned n_layers,
+ struct edac_mc_layer *layers,
+ bool rev_order,
+ unsigned sz_pvt)
{
void *ptr = NULL;
struct mem_ctl_info *mci;
- struct csrow_info *csi, *csrow;
+ struct edac_mc_layer *lay;
+ struct csrow_info *csi, *csr;
struct rank_info *chi, *chp, *chan;
struct dimm_info *dimm;
+ u32 *ce_per_layer[EDAC_MAX_LAYERS], *ue_per_layer[EDAC_MAX_LAYERS];
void *pvt;
- unsigned size;
- int row, chn;
+ unsigned size, tot_dimms, count, pos[EDAC_MAX_LAYERS];
+ unsigned tot_csrows, tot_cschannels;
+ int i, j;
int err;
+ int row, chn;
+
+ BUG_ON(n_layers > EDAC_MAX_LAYERS);
+ /*
+ * Calculate the total amount of dimms and csrows/cschannels while
+ * in the old API emulation mode
+ */
+ tot_dimms = 1;
+ tot_cschannels = 1;
+ tot_csrows = 1;
+ for (i = 0; i < n_layers; i++) {
+ tot_dimms *= layers[i].size;
+ if (layers[i].is_virt_csrow)
+ tot_csrows *= layers[i].size;
+ else
+ tot_cschannels *= layers[i].size;
+ }
/* Figure out the offsets of the various items from the start of an mc
* structure. We want the alignment of each item to be at least as
@@ -191,12 +247,21 @@ struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
* hardcode everything into a single struct.
*/
mci = edac_align_ptr(&ptr, sizeof(*mci), 1);
- csi = edac_align_ptr(&ptr, sizeof(*csi), nr_csrows);
- chi = edac_align_ptr(&ptr, sizeof(*chi), nr_csrows * nr_chans);
- dimm = edac_align_ptr(&ptr, sizeof(*dimm), nr_csrows * nr_chans);
+ lay = edac_align_ptr(&ptr, sizeof(*lay), n_layers);
+ csi = edac_align_ptr(&ptr, sizeof(*csi), tot_csrows);
+ chi = edac_align_ptr(&ptr, sizeof(*chi), tot_csrows * tot_cschannels);
+ dimm = edac_align_ptr(&ptr, sizeof(*dimm), tot_dimms);
+ count = 1;
+ for (i = 0; i < n_layers; i++) {
+ count *= layers[i].size;
+ ce_per_layer[i] = edac_align_ptr(&ptr, sizeof(u32), count);
+ ue_per_layer[i] = edac_align_ptr(&ptr, sizeof(u32), count);
+ }
pvt = edac_align_ptr(&ptr, sz_pvt, 1);
size = ((unsigned long)pvt) + sz_pvt;
+ debugf1("%s(): allocating %u bytes for mci data (%d dimms, %d csrows/channels)\n",
+ __func__, size, tot_dimms, tot_csrows * tot_cschannels);
mci = kzalloc(size, GFP_KERNEL);
if (mci == NULL)
return NULL;
@@ -204,42 +269,99 @@ struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
/* Adjust pointers so they point within the memory we just allocated
* rather than an imaginary chunk of memory located at address 0.
*/
+ lay = (struct edac_mc_layer *)(((char *)mci) + ((unsigned long)lay));
csi = (struct csrow_info *)(((char *)mci) + ((unsigned long)csi));
chi = (struct rank_info *)(((char *)mci) + ((unsigned long)chi));
dimm = (struct dimm_info *)(((char *)mci) + ((unsigned long)dimm));
+ for (i = 0; i < n_layers; i++) {
+ mci->ce_per_layer[i] = (u32 *)((char *)mci + ((unsigned long)ce_per_layer[i]));
+ mci->ue_per_layer[i] = (u32 *)((char *)mci + ((unsigned long)ue_per_layer[i]));
+ }
pvt = sz_pvt ? (((char *)mci) + ((unsigned long)pvt)) : NULL;
/* setup index and various internal pointers */
mci->mc_idx = edac_index;
mci->csrows = csi;
mci->dimms = dimm;
+ mci->tot_dimms = tot_dimms;
mci->pvt_info = pvt;
- mci->nr_csrows = nr_csrows;
+ mci->n_layers = n_layers;
+ mci->layers = lay;
+ memcpy(mci->layers, layers, sizeof(*lay) * n_layers);
+ mci->nr_csrows = tot_csrows;
+ mci->num_cschannel = tot_cschannels;
/*
- * For now, assumes that a per-csrow arrangement for dimms.
- * This will be latter changed.
+ * Fills the csrow struct
*/
- dimm = mci->dimms;
-
- for (row = 0; row < nr_csrows; row++) {
- csrow = &csi[row];
- csrow->csrow_idx = row;
- csrow->mci = mci;
- csrow->nr_channels = nr_chans;
- chp = &chi[row * nr_chans];
- csrow->channels = chp;
-
- for (chn = 0; chn < nr_chans; chn++) {
+ for (row = 0; row < tot_csrows; row++) {
+ csr = &csi[row];
+ csr->csrow_idx = row;
+ csr->mci = mci;
+ csr->nr_channels = tot_cschannels;
+ chp = &chi[row * tot_cschannels];
+ csr->channels = chp;
+
+ for (chn = 0; chn < tot_cschannels; chn++) {
chan = &chp[chn];
chan->chan_idx = chn;
- chan->csrow = csrow;
+ chan->csrow = csr;
+ }
+ }
- mci->csrows[row].channels[chn].dimm = dimm;
- dimm->csrow = row;
- dimm->csrow_channel = chn;
- dimm++;
- mci->nr_dimms++;
+ /*
+ * Fills the dimm struct
+ */
+ memset(&pos, 0, sizeof(pos));
+ row = 0;
+ chn = 0;
+ debugf4("%s: initializing %d dimms\n", __func__, tot_dimms);
+ for (i = 0; i < tot_dimms; i++) {
+ chan = &csi[row].channels[chn];
+ dimm = EDAC_DIMM_PTR(lay, mci->dimms, n_layers,
+ pos[0], pos[1], pos[2]);
+ dimm->mci = mci;
+
+ debugf2("%s: %d: dimm%zd (%d:%d:%d): row %d, chan %d\n", __func__,
+ i, (dimm - mci->dimms),
+ pos[0], pos[1], pos[2], row, chn);
+
+ /* Copy DIMM location */
+ for (j = 0; j < n_layers; j++)
+ dimm->location[j] = pos[j];
+
+ /* Link it to the csrows old API data */
+ chan->dimm = dimm;
+ dimm->csrow = row;
+ dimm->cschannel = chn;
+
+ /* Increment csrow location */
+ if (!rev_order) {
+ for (j = n_layers - 1; j >= 0; j--)
+ if (!layers[j].is_virt_csrow)
+ break;
+ chn++;
+ if (chn == tot_cschannels) {
+ chn = 0;
+ row++;
+ }
+ } else {
+ for (j = n_layers - 1; j >= 0; j--)
+ if (layers[j].is_virt_csrow)
+ break;
+ row++;
+ if (row == tot_csrows) {
+ row = 0;
+ chn++;
+ }
+ }
+
+ /* Increment dimm location */
+ for (j = n_layers - 1; j >= 0; j--) {
+ pos[j]++;
+ if (pos[j] < layers[j].size)
+ break;
+ pos[j] = 0;
}
}
@@ -263,6 +385,57 @@ struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
*/
return mci;
}
+EXPORT_SYMBOL_GPL(new_edac_mc_alloc);
+
+/**
+ * edac_mc_alloc: Allocate and partially fills a struct mem_ctl_info structure
+ * @edac_index: Memory controller number
+ * @n_layers: Nu
+mber of layers at the MC hierarchy
+ * layers: Describes each layer as seen by the Memory Controller
+ * @rev_order: Fills csrows/cs channels at the reverse order
+ * @size_pvt: size of private storage needed
+ *
+ *
+ * FIXME: drivers handle multi-rank memories on different ways: on some
+ * drivers, one multi-rank memory is mapped as one DIMM, while, on others,
+ * a single multi-rank DIMM would be mapped into several "dimms".
+ *
+ * Non-csrow based drivers (like FB-DIMM and RAMBUS ones) will likely report
+ * such DIMMS properly, but the CSROWS-based ones will likely do the wrong
+ * thing, as two chip select values are used for dual-rank memories (and 4, for
+ * quad-rank ones). I suspect that this issue could be solved inside the EDAC
+ * core for SDRAM memories, but it requires further study at JEDEC JESD 21C.
+ *
+ * In summary, solving this issue is not easy, as it requires a lot of testing.
+ *
+ * Everything is kmalloc'ed as one big chunk - more efficient.
+ * Only can be used if all structures have the same lifetime - otherwise
+ * you have to allocate and initialize your own structures.
+ *
+ * Use edac_mc_free() to free mc structures allocated by this function.
+ *
+ * Returns:
+ * NULL allocation failed
+ * struct mem_ctl_info pointer
+ */
+
+struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
+ unsigned nr_chans, int edac_index)
+{
+ unsigned n_layers = 2;
+ struct edac_mc_layer layers[n_layers];
+
+ layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
+ layers[0].size = nr_csrows;
+ layers[0].is_virt_csrow = true;
+ layers[1].type = EDAC_MC_LAYER_CHANNEL;
+ layers[1].size = nr_chans;
+ layers[1].is_virt_csrow = false;
+
+ return new_edac_mc_alloc(edac_index, ARRAY_SIZE(layers), layers,
+ false, sz_pvt);
+}
EXPORT_SYMBOL_GPL(edac_mc_alloc);
/**
@@ -528,7 +701,6 @@ EXPORT_SYMBOL(edac_mc_find);
* edac_mc_add_mc: Insert the 'mci' structure into the mci global list and
* create sysfs entries associated with mci structure
* @mci: pointer to the mci structure to be added to the list
- * @mc_idx: A unique numeric identifier to be assigned to the 'mci' structure.
*
* Return:
* 0 Success
@@ -555,6 +727,8 @@ int edac_mc_add_mc(struct mem_ctl_info *mci)
edac_mc_dump_channel(&mci->csrows[i].
channels[j]);
}
+ for (i = 0; i < mci->tot_dimms; i++)
+ edac_mc_dump_dimm(&mci->dimms[i]);
}
#endif
mutex_lock(&mem_ctls_mutex);
@@ -712,261 +886,249 @@ int edac_mc_find_csrow_by_page(struct mem_ctl_info *mci, unsigned long page)
}
EXPORT_SYMBOL_GPL(edac_mc_find_csrow_by_page);
-/* FIXME - setable log (warning/emerg) levels */
-/* FIXME - integrate with evlog: http://evlog.sourceforge.net/ */
-void edac_mc_handle_ce(struct mem_ctl_info *mci,
- unsigned long page_frame_number,
- unsigned long offset_in_page, unsigned long syndrome,
- int row, int channel, const char *msg)
+const char *edac_layer_name[] = {
+ [EDAC_MC_LAYER_BRANCH] = "branch",
+ [EDAC_MC_LAYER_CHANNEL] = "channel",
+ [EDAC_MC_LAYER_SLOT] = "slot",
+ [EDAC_MC_LAYER_CHIP_SELECT] = "csrow",
+};
+EXPORT_SYMBOL_GPL(edac_layer_name);
+
+static void edac_increment_ce_error(struct mem_ctl_info *mci,
+ bool enable_filter,
+ unsigned pos[EDAC_MAX_LAYERS])
{
- unsigned long remapped_page;
- char *label = NULL;
- u32 grain;
+ int i, index = 0;
- debugf3("MC%d: %s()\n", mci->mc_idx, __func__);
+ mci->ce_mc++;
- /* FIXME - maybe make panic on INTERNAL ERROR an option */
- if (row >= mci->nr_csrows || row < 0) {
- /* something is wrong */
- edac_mc_printk(mci, KERN_ERR,
- "INTERNAL ERROR: row out of range "
- "(%d >= %d)\n", row, mci->nr_csrows);
- edac_mc_handle_ce_no_info(mci, "INTERNAL ERROR");
+ if (!enable_filter) {
+ mci->ce_noinfo_count++;
return;
}
- if (channel >= mci->csrows[row].nr_channels || channel < 0) {
- /* something is wrong */
- edac_mc_printk(mci, KERN_ERR,
- "INTERNAL ERROR: channel out of range "
- "(%d >= %d)\n", channel,
- mci->csrows[row].nr_channels);
- edac_mc_handle_ce_no_info(mci, "INTERNAL ERROR");
- return;
- }
-
- label = mci->csrows[row].channels[channel].dimm->label;
- grain = mci->csrows[row].channels[channel].dimm->grain;
+ for (i = 0; i < mci->n_layers; i++) {
+ if (pos[i] < 0)
+ break;
+ index += pos[i];
+ mci->ce_per_layer[i][index]++;
- if (edac_mc_get_log_ce())
- /* FIXME - put in DIMM location */
- edac_mc_printk(mci, KERN_WARNING,
- "CE page 0x%lx, offset 0x%lx, grain %d, syndrome "
- "0x%lx, row %d, channel %d, label \"%s\": %s\n",
- page_frame_number, offset_in_page,
- grain, syndrome, row, channel,
- label, msg);
+ if (i < mci->n_layers - 1)
+ index *= mci->layers[i + 1].size;
+ }
+}
- mci->ce_count++;
- mci->csrows[row].ce_count++;
- mci->csrows[row].channels[channel].dimm->ce_count++;
- mci->csrows[row].channels[channel].ce_count++;
+static void edac_increment_ue_error(struct mem_ctl_info *mci,
+ bool enable_filter,
+ unsigned pos[EDAC_MAX_LAYERS])
+{
+ int i, index = 0;
- if (mci->scrub_mode & SCRUB_SW_SRC) {
- /*
- * Some MC's can remap memory so that it is still available
- * at a different address when PCI devices map into memory.
- * MC's that can't do this lose the memory where PCI devices
- * are mapped. This mapping is MC dependent and so we call
- * back into the MC driver for it to map the MC page to
- * a physical (CPU) page which can then be mapped to a virtual
- * page - which can then be scrubbed.
- */
- remapped_page = mci->ctl_page_to_phys ?
- mci->ctl_page_to_phys(mci, page_frame_number) :
- page_frame_number;
+ mci->ue_mc++;
- edac_mc_scrub_block(remapped_page, offset_in_page, grain);
+ if (!enable_filter) {
+ mci->ce_noinfo_count++;
+ return;
}
-}
-EXPORT_SYMBOL_GPL(edac_mc_handle_ce);
-void edac_mc_handle_ce_no_info(struct mem_ctl_info *mci, const char *msg)
-{
- if (edac_mc_get_log_ce())
- edac_mc_printk(mci, KERN_WARNING,
- "CE - no information available: %s\n", msg);
+ for (i = 0; i < mci->n_layers; i++) {
+ if (pos[i] < 0)
+ break;
+ index += pos[i];
+ mci->ue_per_layer[i][index]++;
- mci->ce_noinfo_count++;
- mci->ce_count++;
+ if (i < mci->n_layers - 1)
+ index *= mci->layers[i + 1].size;
+ }
}
-EXPORT_SYMBOL_GPL(edac_mc_handle_ce_no_info);
-void edac_mc_handle_ue(struct mem_ctl_info *mci,
- unsigned long page_frame_number,
- unsigned long offset_in_page, int row, const char *msg)
+#define OTHER_LABEL " or "
+void edac_mc_handle_error(const enum hw_event_mc_err_type type,
+ struct mem_ctl_info *mci,
+ const unsigned long page_frame_number,
+ const unsigned long offset_in_page,
+ const unsigned long syndrome,
+ const int layer0,
+ const int layer1,
+ const int layer2,
+ const char *msg,
+ const char *other_detail,
+ const void *mcelog)
{
- int len = EDAC_MC_LABEL_LEN * 4;
- char labels[len + 1];
- char *pos = labels;
- int chan;
- int chars;
- char *label = NULL;
+ unsigned long remapped_page;
+ /* FIXME: too much for stack: move it to some pre-alocated area */
+ char detail[80], location[80];
+ char label[(EDAC_MC_LABEL_LEN + 1 + sizeof(OTHER_LABEL)) * mci->tot_dimms];
+ char *p;
+ int row = -1, chan = -1;
+ int pos[EDAC_MAX_LAYERS] = { layer0, layer1, layer2 };
+ int i;
u32 grain;
+ bool enable_filter = false;
debugf3("MC%d: %s()\n", mci->mc_idx, __func__);
- /* FIXME - maybe make panic on INTERNAL ERROR an option */
- if (row >= mci->nr_csrows || row < 0) {
- /* something is wrong */
- edac_mc_printk(mci, KERN_ERR,
- "INTERNAL ERROR: row out of range "
- "(%d >= %d)\n", row, mci->nr_csrows);
- edac_mc_handle_ue_no_info(mci, "INTERNAL ERROR");
- return;
- }
-
- grain = mci->csrows[row].channels[0].dimm->grain;
- label = mci->csrows[row].channels[0].dimm->label;
- chars = snprintf(pos, len + 1, "%s", label);
- len -= chars;
- pos += chars;
-
- for (chan = 1; (chan < mci->csrows[row].nr_channels) && (len > 0);
- chan++) {
- label = mci->csrows[row].channels[chan].dimm->label;
- chars = snprintf(pos, len + 1, ":%s", label);
- len -= chars;
- pos += chars;
+ /* Check if the event report is consistent */
+ for (i = 0; i < mci->n_layers; i++) {
+ if (pos[i] >= (int)mci->layers[i].size) {
+ if (type == HW_EVENT_ERR_CORRECTED) {
+ p = "CE";
+ mci->ce_mc++;
+ } else {
+ p = "UE";
+ mci->ue_mc++;
+ }
+ edac_mc_printk(mci, KERN_ERR,
+ "INTERNAL ERROR: %s value is out of range (%d >= %d)\n",
+ edac_layer_name[mci->layers[i].type],
+ pos[i], mci->layers[i].size);
+ /*
+ * Instead of just returning it, let's use what's
+ * known about the error. The increment routines and
+ * the DIMM filter logic will do the right thing by
+ * pointing the likely damaged DIMMs.
+ */
+ pos[i] = -1;
+ }
+ if (pos[i] >= 0)
+ enable_filter = true;
}
- if (edac_mc_get_log_ue())
- edac_mc_printk(mci, KERN_EMERG,
- "UE page 0x%lx, offset 0x%lx, grain %d, row %d, "
- "labels \"%s\": %s\n", page_frame_number,
- offset_in_page, grain, row, labels, msg);
-
- if (edac_mc_get_panic_on_ue())
- panic("EDAC MC%d: UE page 0x%lx, offset 0x%lx, grain %d, "
- "row %d, labels \"%s\": %s\n", mci->mc_idx,
- page_frame_number, offset_in_page,
- grain, row, labels, msg);
-
- mci->ue_count++;
- mci->csrows[row].ue_count++;
-}
-EXPORT_SYMBOL_GPL(edac_mc_handle_ue);
+ /*
+ * Get the dimm label/grain that applies to the match criteria.
+ * As the error algorithm may not be able to point to just one memory,
+ * the logic here will get all possible labels that could pottentially
+ * be affected by the error.
+ * On FB-DIMM memory controllers, for uncorrected errors, it is common
+ * to have only the MC channel and the MC dimm (also called as "rank")
+ * but the channel is not known, as the memory is arranged in pairs,
+ * where each memory belongs to a separate channel within the same
+ * branch.
+ * It will also get the max grain, over the error match range
+ */
+ grain = 0;
+ p = label;
+ *p = '\0';
+ for (i = 0; i < mci->tot_dimms; i++) {
+ struct dimm_info *dimm = &mci->dimms[i];
-void edac_mc_handle_ue_no_info(struct mem_ctl_info *mci, const char *msg)
-{
- if (edac_mc_get_panic_on_ue())
- panic("EDAC MC%d: Uncorrected Error", mci->mc_idx);
+ if (layer0 >= 0 && layer0 != dimm->location[0])
+ continue;
+ if (layer1 >= 0 && layer1 != dimm->location[1])
+ continue;
+ if (layer2 >= 0 && layer2 != dimm->location[2])
+ continue;
- if (edac_mc_get_log_ue())
- edac_mc_printk(mci, KERN_WARNING,
- "UE - no information available: %s\n", msg);
- mci->ue_noinfo_count++;
- mci->ue_count++;
-}
-EXPORT_SYMBOL_GPL(edac_mc_handle_ue_no_info);
+ if (dimm->grain > grain)
+ grain = dimm->grain;
-/*************************************************************
- * On Fully Buffered DIMM modules, this help function is
- * called to process UE events
- */
-void edac_mc_handle_fbd_ue(struct mem_ctl_info *mci,
- unsigned int csrow,
- unsigned int channela,
- unsigned int channelb, char *msg)
-{
- int len = EDAC_MC_LABEL_LEN * 4;
- char labels[len + 1];
- char *pos = labels;
- int chars;
- char *label;
-
- if (csrow >= mci->nr_csrows) {
- /* something is wrong */
- edac_mc_printk(mci, KERN_ERR,
- "INTERNAL ERROR: row out of range (%d >= %d)\n",
- csrow, mci->nr_csrows);
- edac_mc_handle_ue_no_info(mci, "INTERNAL ERROR");
- return;
+ /*
+ * If the error is memory-controller wide, there's no sense
+ * on seeking for the affected DIMMs, as everything may be
+ * affected. Also, don't show errors for non-filled dimm's.
+ */
+ if (enable_filter && dimm->nr_pages) {
+ if (p != label) {
+ strcpy(p, OTHER_LABEL);
+ p += strlen(OTHER_LABEL);
+ }
+ strcpy(p, dimm->label);
+ p += strlen(p);
+ *p = '\0';
+
+ /*
+ * get csrow/channel of the dimm, in order to allow
+ * incrementing the compat API counters
+ */
+ debugf4("%s: dimm csrows (%d,%d)\n",
+ __func__, dimm->csrow, dimm->cschannel);
+ if (row == -1)
+ row = dimm->csrow;
+ else if (row >= 0 && row != dimm->csrow)
+ row = -2;
+ if (chan == -1)
+ chan = dimm->cschannel;
+ else if (chan >= 0 && chan != dimm->cschannel)
+ chan = -2;
+ }
}
-
- if (channela >= mci->csrows[csrow].nr_channels) {
- /* something is wrong */
- edac_mc_printk(mci, KERN_ERR,
- "INTERNAL ERROR: channel-a out of range "
- "(%d >= %d)\n",
- channela, mci->csrows[csrow].nr_channels);
- edac_mc_handle_ue_no_info(mci, "INTERNAL ERROR");
- return;
+ if (!enable_filter) {
+ strcpy(label, "any memory");
+ } else {
+ debugf4("%s: csrow/channel to increment: (%d,%d)\n",
+ __func__, row, chan);
+ if (p == label)
+ strcpy(label, "unknown memory");
+ if (type == HW_EVENT_ERR_CORRECTED) {
+ if (row >= 0) {
+ mci->csrows[row].ce_count++;
+ if (chan >= 0)
+ mci->csrows[row].channels[chan].ce_count++;
+ }
+ } else
+ if (row >= 0)
+ mci->csrows[row].ue_count++;
}
- if (channelb >= mci->csrows[csrow].nr_channels) {
- /* something is wrong */
- edac_mc_printk(mci, KERN_ERR,
- "INTERNAL ERROR: channel-b out of range "
- "(%d >= %d)\n",
- channelb, mci->csrows[csrow].nr_channels);
- edac_mc_handle_ue_no_info(mci, "INTERNAL ERROR");
- return;
+ /* Fill the RAM location data */
+ p = location;
+ for (i = 0; i < mci->n_layers; i++) {
+ if (pos[i] < 0)
+ continue;
+ p += sprintf(p, "%s %d ",
+ edac_layer_name[mci->layers[i].type],
+ pos[i]);
}
- mci->ue_count++;
- mci->csrows[csrow].ue_count++;
-
- /* Generate the DIMM labels from the specified channels */
- label = mci->csrows[csrow].channels[channela].dimm->label;
- chars = snprintf(pos, len + 1, "%s", label);
- len -= chars;
- pos += chars;
-
- chars = snprintf(pos, len + 1, "-%s",
- mci->csrows[csrow].channels[channelb].dimm->label);
-
- if (edac_mc_get_log_ue())
- edac_mc_printk(mci, KERN_EMERG,
- "UE row %d, channel-a= %d channel-b= %d "
- "labels \"%s\": %s\n", csrow, channela, channelb,
- labels, msg);
-
- if (edac_mc_get_panic_on_ue())
- panic("UE row %d, channel-a= %d channel-b= %d "
- "labels \"%s\": %s\n", csrow, channela,
- channelb, labels, msg);
-}
-EXPORT_SYMBOL(edac_mc_handle_fbd_ue);
+ /* Memory type dependent details about the error */
+ if (type == HW_EVENT_ERR_CORRECTED)
+ snprintf(detail, sizeof(detail),
+ "page 0x%lx offset 0x%lx grain %d syndrome 0x%lx",
+ page_frame_number, offset_in_page,
+ grain, syndrome);
+ else
+ snprintf(detail, sizeof(detail),
+ "page 0x%lx offset 0x%lx grain %d",
+ page_frame_number, offset_in_page, grain);
+
+ if (type == HW_EVENT_ERR_CORRECTED) {
+ if (edac_mc_get_log_ce())
+ edac_mc_printk(mci, KERN_WARNING,
+ "CE %s on %s (%s%s %s)\n",
+ msg, label, location,
+ detail, other_detail);
+ edac_increment_ce_error(mci, enable_filter, pos);
+
+ if (mci->scrub_mode & SCRUB_SW_SRC) {
+ /*
+ * Some MC's can remap memory so that it is still
+ * available at a different address when PCI devices
+ * map into memory.
+ * MC's that can't do this lose the memory where PCI
+ * devices are mapped. This mapping is MC dependent
+ * and so we call back into the MC driver for it to
+ * map the MC page to a physical (CPU) page which can
+ * then be mapped to a virtual page - which can then
+ * be scrubbed.
+ */
+ remapped_page = mci->ctl_page_to_phys ?
+ mci->ctl_page_to_phys(mci, page_frame_number) :
+ page_frame_number;
+
+ edac_mc_scrub_block(remapped_page,
+ offset_in_page, grain);
+ }
+ } else {
+ if (edac_mc_get_log_ue())
+ edac_mc_printk(mci, KERN_WARNING,
+ "UE %s on %s (%s%s %s)\n",
+ msg, label, location, detail, other_detail);
-/*************************************************************
- * On Fully Buffered DIMM modules, this help function is
- * called to process CE events
- */
-void edac_mc_handle_fbd_ce(struct mem_ctl_info *mci,
- unsigned int csrow, unsigned int channel, char *msg)
-{
- char *label = NULL;
+ if (edac_mc_get_panic_on_ue())
+ panic("UE %s on %s (%s%s %s)\n",
+ msg, label, location, detail, other_detail);
- /* Ensure boundary values */
- if (csrow >= mci->nr_csrows) {
- /* something is wrong */
- edac_mc_printk(mci, KERN_ERR,
- "INTERNAL ERROR: row out of range (%d >= %d)\n",
- csrow, mci->nr_csrows);
- edac_mc_handle_ce_no_info(mci, "INTERNAL ERROR");
- return;
+ edac_increment_ue_error(mci, enable_filter, pos);
}
- if (channel >= mci->csrows[csrow].nr_channels) {
- /* something is wrong */
- edac_mc_printk(mci, KERN_ERR,
- "INTERNAL ERROR: channel out of range (%d >= %d)\n",
- channel, mci->csrows[csrow].nr_channels);
- edac_mc_handle_ce_no_info(mci, "INTERNAL ERROR");
- return;
- }
-
- label = mci->csrows[csrow].channels[channel].dimm->label;
-
- if (edac_mc_get_log_ce())
- /* FIXME - put in DIMM location */
- edac_mc_printk(mci, KERN_WARNING,
- "CE row %d, channel %d, label \"%s\": %s\n",
- csrow, channel, label, msg);
-
- mci->ce_count++;
- mci->csrows[csrow].ce_count++;
- mci->csrows[csrow].channels[channel].dimm->ce_count++;
- mci->csrows[csrow].channels[channel].ce_count++;
}
-EXPORT_SYMBOL(edac_mc_handle_fbd_ce);
+EXPORT_SYMBOL_GPL(edac_mc_handle_error);
diff --git a/include/linux/edac.h b/include/linux/edac.h
index 3b8798d..412d5cd 100644
--- a/include/linux/edac.h
+++ b/include/linux/edac.h
@@ -412,18 +412,20 @@ struct edac_mc_layer {
/* FIXME: add the proper per-location error counts */
struct dimm_info {
char label[EDAC_MC_LABEL_LEN + 1]; /* DIMM label on motherboard */
- unsigned memory_controller;
- unsigned csrow;
- unsigned csrow_channel;
+
+ /* Memory location data */
+ unsigned location[EDAC_MAX_LAYERS];
+
+ struct mem_ctl_info *mci; /* the parent */
u32 grain; /* granularity of reported error in bytes */
enum dev_type dtype; /* memory device type */
enum mem_type mtype; /* memory dimm type */
enum edac_type edac_mode; /* EDAC mode for this dimm */
- u32 nr_pages; /* number of pages in csrow */
+ u32 nr_pages; /* number of pages on this dimm */
- u32 ce_count; /* Correctable Errors for this dimm */
+ unsigned csrow, cschannel; /* Points to the old API data */
};
/**
@@ -443,9 +445,10 @@ struct dimm_info {
*/
struct rank_info {
int chan_idx;
- u32 ce_count;
struct csrow_info *csrow;
struct dimm_info *dimm;
+
+ u32 ce_count; /* Correctable Errors for this csrow */
};
struct csrow_info {
@@ -497,6 +500,11 @@ struct mcidev_sysfs_attribute {
ssize_t (*store)(struct mem_ctl_info *, const char *,size_t);
};
+struct edac_hierarchy {
+ char *name;
+ unsigned nr;
+};
+
/* MEMORY controller information structure
*/
struct mem_ctl_info {
@@ -541,13 +549,16 @@ struct mem_ctl_info {
unsigned long (*ctl_page_to_phys) (struct mem_ctl_info * mci,
unsigned long page);
int mc_idx;
- int nr_csrows;
struct csrow_info *csrows;
+ unsigned nr_csrows, num_cschannel;
+ /* Memory Controller hierarchy */
+ unsigned n_layers;
+ struct edac_mc_layer *layers;
/*
* DIMM info. Will eventually remove the entire csrows_info some day
*/
- unsigned nr_dimms;
+ unsigned tot_dimms;
struct dimm_info *dimms;
/*
@@ -562,12 +573,15 @@ struct mem_ctl_info {
const char *dev_name;
char proc_name[MC_PROC_NAME_MAX_LEN + 1];
void *pvt_info;
- u32 ue_noinfo_count; /* Uncorrectable Errors w/o info */
- u32 ce_noinfo_count; /* Correctable Errors w/o info */
- u32 ue_count; /* Total Uncorrectable Errors for this MC */
- u32 ce_count; /* Total Correctable Errors for this MC */
+ u32 ue_count; /* Total Uncorrectable Errors for this MC */
+ u32 ce_count; /* Total Correctable Errors for this MC */
unsigned long start_time; /* mci load start time (in jiffies) */
+ /* drivers shouldn't access this struct directly */
+ unsigned ce_noinfo_count, ue_noinfo_count;
+ unsigned ce_mc, ue_mc;
+ u32 *ce_per_layer[EDAC_MAX_LAYERS], *ue_per_layer[EDAC_MAX_LAYERS];
+
struct completion complete;
/* edac sysfs device control */
@@ -580,7 +594,7 @@ struct mem_ctl_info {
* by the low level driver.
*
* Set by the low level driver to provide attributes at the
- * controller level, same level as 'ue_count' and 'ce_count' above.
+ * controller level.
* An array of structures, NULL terminated
*
* If attributes are desired, then set to array of attributes
--
1.7.8
^ permalink raw reply related
* Re: [EDAC ABI v13 24/25] edac: change the mem allocation scheme to make Documentation/kobject.txt happy
From: Joe Perches @ 2012-04-22 6:37 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Arvind R., Michal Marek, linuxppc-dev, Mark Gross, Shaohui Xie,
Jason Uhlenkott, Dmitry Eremin-Solenikov, Jiri Kosina,
Ranganathan Desikan, Borislav Petkov, Chris Metcalf,
Linux Kernel Mailing List, Egor Martovetsky, Aristeu Rozanski,
Olof Johansson, Doug Thompson, Andrew Morton, Tim Small,
Hitoshi Mitake, Linux Edac Mailing List
In-Reply-To: <4F900FB3.6000607@redhat.com>
On Thu, 2012-04-19 at 10:14 -0300, Mauro Carvalho Chehab wrote:
> diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
[]
> @@ -296,7 +296,7 @@ struct mem_ctl_info *edac_mc_alloc(unsigned edac_index,
> /*
> * Alocate and fill the csrow/channels structs
> */
> - mci->csrows = kzalloc(sizeof(*mci->csrows) * tot_csrows, GFP_KERNEL);
> + mci->csrows = kcalloc(sizeof(*mci->csrows), tot_csrows, GFP_KERNEL);
trivia: the first 2 args to kcalloc should be swapped.
static inline void *kcalloc(size_t n, size_t size, gfp_t flags)
kcalloc(tot_csrows, sizeof(*mci->csrows), GFP_KERNEL);
[]
> @@ -307,7 +307,7 @@ struct mem_ctl_info *edac_mc_alloc(unsigned edac_index,
> csr->csrow_idx = row;
> csr->mci = mci;
> csr->nr_channels = tot_cschannels;
> - csr->channels = kzalloc(sizeof(*csr->channels) * tot_cschannels,
> + csr->channels = kcalloc(sizeof(*csr->channels), tot_cschannels,
and here.
[]
> @@ -323,7 +323,7 @@ struct mem_ctl_info *edac_mc_alloc(unsigned edac_index,
> /*
> * Allocate and fill the dimm structs
> */
> - mci->dimms = kzalloc(sizeof(*mci->dimms) * tot_dimms, GFP_KERNEL);
> + mci->dimms = kcalloc(sizeof(*mci->dimms), tot_dimms, GFP_KERNEL);
and here too.
^ permalink raw reply
* Re: PowerPC radeon KMS - is it possible?
From: Gerhard Pircher @ 2012-04-24 17:10 UTC (permalink / raw)
To: "Michel Dänzer"; +Cc: linuxppc-dev
In-Reply-To: <1335276900.3383.23.camel@thor.local>
-------- Original-Nachricht --------
> Datum: Tue, 24 Apr 2012 16:15:00 +0200
> Von: "Michel Dänzer" <michel@daenzer.net>
> An: Gerhard Pircher <gerhard_pircher@gmx.net>
> CC: linuxppc-dev@lists.ozlabs.org
> Betreff: Re: PowerPC radeon KMS - is it possible?
> On Mon, 2012-04-23 at 18:45 +0200, Gerhard Pircher wrote:
> > > Von: "Michel Dänzer" <michel@daenzer.net>
> > > On Fre, 2012-04-20 at 18:14 +0200, Gerhard Pircher wrote:
> > > > > Von: "Michel Dänzer" <michel@daenzer.net>
> > > > > [...]
> > > > > > Could it be that the memory is finally mapped uncacheable by
> > > > > > radeon_bo_kmap()/ttm_bo_kmap()/..some other TTM
> > > > > > functions../vmap()?
> > > > >
> > > > > Yeah, AFAICT, basically ttm_io_prot() defines the mapping
> > > > > attributes, and vmap() is used to enforce them for kernel
> > > > > mappings.
> > > > Okay, that sounds like the approach used by arch/powerpc/mm/dma-
> > > > noncoherent.c in my ("green") ears. What about the PCIGART mode?
> > > > Is the driver free to use cached memory in this mode?
> > >
> > > Yes, it assumes PCI(e) GART to be CPU cache coherent.
> > Okay. I guess it should be possible to modify it so that it makes use
> > of uncacheable memory - just for testing!?
>
> Sure. Just set man->available_caching and man->default_caching as in the
> AGP case in radeon_init_mem_type().
Thanks for the info! I'll play around with it.
> > PCIGART was working "somehow" on my platform up to the ~2.6.39 kernel,
> > i.e. I could login to GNOME and open a program until the machine
> > locked-up. :-)
>
> But it's worse with newer kernels?
Yes, it's worse. But that's surely the fault of the buggy northbridge.
I believe the bridge doesn't like some of the code that the driver uses
to avoid ordering issues. But I still have to find out where the lockups
exactly happen.
> > BTW: I see that the uninorth driver defines needs_scratch_page. What
> > is this actually good for?
>
> It causes the code in drivers/char/agp/backend.c to allocate a scratch
> page (bridge->scratch_page) which the driver can use for unused GART
> entries.
Okay, so it would make sense to set this for my platform's AGP bridge,
which doesn't seem to support a "GATT entry valid" bit.
Gerhard
--
Empfehlen Sie GMX DSL Ihren Freunden und Bekannten und wir
belohnen Sie mit bis zu 50,- Euro! https://freundschaftswerbung.gmx.de
^ permalink raw reply
* Re: PowerPC radeon KMS - is it possible?
From: Michel Dänzer @ 2012-04-24 14:15 UTC (permalink / raw)
To: Gerhard Pircher; +Cc: linuxppc-dev
In-Reply-To: <20120423164504.235670@gmx.net>
On Mon, 2012-04-23 at 18:45 +0200, Gerhard Pircher wrote:
> > Von: "Michel D=C3=A4nzer" <michel@daenzer.net>
> > On Fre, 2012-04-20 at 18:14 +0200, Gerhard Pircher wrote:=20
> > > > Von: "Michel D=C3=A4nzer" <michel@daenzer.net>
> > > > On Fre, 2012-04-20 at 13:15 +0200, Gerhard Pircher wrote:=20
> > > > >=20
> > > > > What I didn't understand yet is how this uncacheable memory is
> > > > > allocated (well, I never took the time to look at this again). Th=
e
> > > > > functions in ttm_page_alloc.c seem to allocate normal cacheable
> > > > > memory and try to set the page flags with set_pages_array_uc(),
> > > > > which is more or less a no-op on powerpc. ttm_page_alloc_dma.c on
> > > > > the other side is only used with SWIOTLB!?
> > > > [...]=20
> > > > > Could it be that the memory is finally mapped uncacheable by
> > > > radeon_bo_kmap()/
> > > > > ttm_bo_kmap()/..some other TTM functions../vmap()?
> > > >=20
> > > > Yeah, AFAICT, basically ttm_io_prot() defines the mapping
> > > > attributes, and vmap() is used to enforce them for kernel mappings.
> > > Okay, that sounds like the approach used by arch/powerpc/mm/dma-
> > > noncoherent.c in my ("green") ears. What about the PCIGART mode?
> > > Is the driver free to use cached memory in this mode?
> >=20
> > Yes, it assumes PCI(e) GART to be CPU cache coherent.
> Okay. I guess it should be possible to modify it so that it makes use
> of uncacheable memory - just for testing!?
Sure. Just set man->available_caching and man->default_caching as in the
AGP case in radeon_init_mem_type().=20
> PCIGART was working "somehow" on my platform up to the ~2.6.39 kernel,
> i.e. I could login to GNOME and open a program until the machine
> locked-up. :-)
But it's worse with newer kernels?
> BTW: I see that the uninorth driver defines needs_scratch_page. What
> is this actually good for?
It causes the code in drivers/char/agp/backend.c to allocate a scratch
page (bridge->scratch_page) which the driver can use for unused GART
entries.=20
--=20
Earthling Michel D=C3=A4nzer | http://www.amd.c=
om
Libre software enthusiast | Debian, X and DRI developer
^ permalink raw reply
* Re: [PATCH v2 1/2] powerpc/8xx: Fix NR_IRQ bugs and refactor 8xx interrupt controller
From: Benjamin Herrenschmidt @ 2012-04-24 11:22 UTC (permalink / raw)
To: Joakim Tjernlund; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <OF4123672B.F86C969F-ONC12579EA.002A2C1F-C12579EA.002A48C2@transmode.se>
On Tue, 2012-04-24 at 09:41 +0200, Joakim Tjernlund wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 2012/04/24 05:13:15:
> >
> > On Mon, 2012-04-23 at 16:30 -0600, Grant Likely wrote:
> > > The mpc8xx driver uses a reference to NR_IRQS that is buggy. It uses
> > > NR_IRQs for the array size of the ppc_cached_irq_mask bitmap, but
> > > NR_IRQs could be smaller than the number of hardware irqs that
> > > ppc_cached_irq_mask tracks.
> >
> > Joakim, any chance you can test this ASAP ? :-)
>
> Sorry, but no. We have not moved our 8xx boards to 3.x as it in maintenance
> and takes too much space.
Ah ok... Anybody else on the list who still has some 8xx gear ?
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH v2 1/2] powerpc/8xx: Fix NR_IRQ bugs and refactor 8xx interrupt controller
From: Joakim Tjernlund @ 2012-04-24 7:41 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1335237195.24897.0.camel@pasglop>
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 2012/04/24 05:13:15:
>
> On Mon, 2012-04-23 at 16:30 -0600, Grant Likely wrote:
> > The mpc8xx driver uses a reference to NR_IRQS that is buggy. It uses
> > NR_IRQs for the array size of the ppc_cached_irq_mask bitmap, but
> > NR_IRQs could be smaller than the number of hardware irqs that
> > ppc_cached_irq_mask tracks.
>
> Joakim, any chance you can test this ASAP ? :-)
Sorry, but no. We have not moved our 8xx boards to 3.x as it in maintenance
and takes too much space.
^ permalink raw reply
* Re: [PATCH v2 1/2] powerpc/8xx: Fix NR_IRQ bugs and refactor 8xx interrupt controller
From: Benjamin Herrenschmidt @ 2012-04-24 3:13 UTC (permalink / raw)
To: Grant Likely; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1335220202-32533-2-git-send-email-grant.likely@secretlab.ca>
On Mon, 2012-04-23 at 16:30 -0600, Grant Likely wrote:
> The mpc8xx driver uses a reference to NR_IRQS that is buggy. It uses
> NR_IRQs for the array size of the ppc_cached_irq_mask bitmap, but
> NR_IRQs could be smaller than the number of hardware irqs that
> ppc_cached_irq_mask tracks.
Joakim, any chance you can test this ASAP ? :-)
Thanks !
Cheers,
Ben.
> Also, while fixing that problem, it became apparent that the interrupt
> controller only supports 32 interrupt numbers, but it is written as if
> it supports multiple register banks which is more complicated.
>
> This patch pulls out the buggy reference to NR_IRQs and fixes the size
> of the ppc_cached_irq_mask to match the number of HW irqs. It also
> drops the now-unnecessary code since ppc_cached_irq_mask is no longer
> an array.
>
> v2: - Rename ppc_cached_irq_mask to mpc8xx_cached_irq_mask
> - Drop duplicate forward declaration
>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> arch/powerpc/sysdev/mpc8xx_pic.c | 61 +++++++++++++-------------------------
> 1 file changed, 20 insertions(+), 41 deletions(-)
>
> diff --git a/arch/powerpc/sysdev/mpc8xx_pic.c b/arch/powerpc/sysdev/mpc8xx_pic.c
> index d5f5416..b724622 100644
> --- a/arch/powerpc/sysdev/mpc8xx_pic.c
> +++ b/arch/powerpc/sysdev/mpc8xx_pic.c
> @@ -18,69 +18,45 @@
> extern int cpm_get_irq(struct pt_regs *regs);
>
> static struct irq_domain *mpc8xx_pic_host;
> -#define NR_MASK_WORDS ((NR_IRQS + 31) / 32)
> -static unsigned long ppc_cached_irq_mask[NR_MASK_WORDS];
> +static unsigned long mpc8xx_cached_irq_mask;
> static sysconf8xx_t __iomem *siu_reg;
>
> -int cpm_get_irq(struct pt_regs *regs);
> +static inline unsigned long mpc8xx_irqd_to_bit(struct irq_data *d)
> +{
> + return 0x80000000 >> irqd_to_hwirq(d);
> +}
>
> static void mpc8xx_unmask_irq(struct irq_data *d)
> {
> - int bit, word;
> - unsigned int irq_nr = (unsigned int)irqd_to_hwirq(d);
> -
> - bit = irq_nr & 0x1f;
> - word = irq_nr >> 5;
> -
> - ppc_cached_irq_mask[word] |= (1 << (31-bit));
> - out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask[word]);
> + mpc8xx_cached_irq_mask |= mpc8xx_irqd_to_bit(d);
> + out_be32(&siu_reg->sc_simask, mpc8xx_cached_irq_mask);
> }
>
> static void mpc8xx_mask_irq(struct irq_data *d)
> {
> - int bit, word;
> - unsigned int irq_nr = (unsigned int)irqd_to_hwirq(d);
> -
> - bit = irq_nr & 0x1f;
> - word = irq_nr >> 5;
> -
> - ppc_cached_irq_mask[word] &= ~(1 << (31-bit));
> - out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask[word]);
> + mpc8xx_cached_irq_mask &= ~mpc8xx_irqd_to_bit(d);
> + out_be32(&siu_reg->sc_simask, mpc8xx_cached_irq_mask);
> }
>
> static void mpc8xx_ack(struct irq_data *d)
> {
> - int bit;
> - unsigned int irq_nr = (unsigned int)irqd_to_hwirq(d);
> -
> - bit = irq_nr & 0x1f;
> - out_be32(&siu_reg->sc_sipend, 1 << (31-bit));
> + out_be32(&siu_reg->sc_sipend, mpc8xx_irqd_to_bit(d));
> }
>
> static void mpc8xx_end_irq(struct irq_data *d)
> {
> - int bit, word;
> - unsigned int irq_nr = (unsigned int)irqd_to_hwirq(d);
> -
> - bit = irq_nr & 0x1f;
> - word = irq_nr >> 5;
> -
> - ppc_cached_irq_mask[word] |= (1 << (31-bit));
> - out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask[word]);
> + mpc8xx_cached_irq_mask |= mpc8xx_irqd_to_bit(d);
> + out_be32(&siu_reg->sc_simask, mpc8xx_cached_irq_mask);
> }
>
> static int mpc8xx_set_irq_type(struct irq_data *d, unsigned int flow_type)
> {
> - if (flow_type & IRQ_TYPE_EDGE_FALLING) {
> - irq_hw_number_t hw = (unsigned int)irqd_to_hwirq(d);
> + /* only external IRQ senses are programmable */
> + if ((flow_type & IRQ_TYPE_EDGE_FALLING) && !(irqd_to_hwirq(d) & 1)) {
> unsigned int siel = in_be32(&siu_reg->sc_siel);
> -
> - /* only external IRQ senses are programmable */
> - if ((hw & 1) == 0) {
> - siel |= (0x80000000 >> hw);
> - out_be32(&siu_reg->sc_siel, siel);
> - __irq_set_handler_locked(d->irq, handle_edge_irq);
> - }
> + siel |= mpc8xx_irqd_to_bit(d);
> + out_be32(&siu_reg->sc_siel, siel);
> + __irq_set_handler_locked(d->irq, handle_edge_irq);
> }
> return 0;
> }
> @@ -132,6 +108,9 @@ static int mpc8xx_pic_host_xlate(struct irq_domain *h, struct device_node *ct,
> IRQ_TYPE_EDGE_FALLING,
> };
>
> + if (intspec[0] > 0x1f)
> + return 0;
> +
> *out_hwirq = intspec[0];
> if (intsize > 1 && intspec[1] < 4)
> *out_flags = map_pic_senses[intspec[1]];
^ permalink raw reply
* Re: [PATCH][2/3][RFC] TDM Framework
From: Scott Wood @ 2012-04-24 0:34 UTC (permalink / raw)
To: Poonam Aggrwal; +Cc: Sandeep Singh, linuxppc-dev
In-Reply-To: <1331384221-29661-1-git-send-email-poonam.aggrwal@freescale.com>
On 03/10/2012 06:57 AM, Poonam Aggrwal wrote:
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index ad6c1eb..25f7f5b 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -130,4 +130,5 @@ source "drivers/virt/Kconfig"
>
> source "drivers/net/dpa/NetCommSw/Kconfig"
>
> +source "drivers/tdm/Kconfig"
> endmenu
When posting patches to this list, please ensure that they are based on
an upstream Linux kernel and not a Freescale BSP kernel.
> +if TDM
> +
> +config TDM_DEBUG_CORE
> + bool "TDM Core debugging messages"
> + help
> + Say Y here if you want the TDM core to produce a bunch of debug
> + messages to the system log. Select this if you are having a
> + problem with TDM support and want to see more of what is going on.
> +
> +endif # TDM
Please use the normal kernel mechanisms for controlling debug messages.
> diff --git a/drivers/tdm/tdm-core.c b/drivers/tdm/tdm-core.c
> new file mode 100644
> index 0000000..cdda260
> --- /dev/null
> +++ b/drivers/tdm/tdm-core.c
> @@ -0,0 +1,1146 @@
> +/* driver/tdm/tdm-core.c
> + *
> + * Copyright (C) 2012 Freescale Semiconductor, Inc, All rights reserved.
> + *
> + * TDM core is the interface between TDM clients and TDM devices.
> + * It is also intended to serve as an interface for line controld
> + * devices later on.
> + *
> + * Author:Hemant Agrawal <hemant@freescale.com>
> + * Rajesh Gumasta <rajesh.gumasta@freescale.com>
> + *
> + * Modified by Sandeep Kr Singh <sandeep@freescale.com>
> + * Poonam Aggarwal <poonam.aggarwal@freescale.com>
> + * 1. Added framework based initialization of device.
> + * 2. All the init/run time configuration is now done by framework.
> + * 3. Added channel level operations.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +/* if read write debug required */
> +#undef TDM_CORE_DEBUG
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/slab.h>
> +#include <linux/tdm.h>
> +#include <linux/init.h>
> +#include <linux/idr.h>
> +#include <linux/mutex.h>
> +#include <linux/completion.h>
> +#include <linux/hardirq.h>
> +#include <linux/irqflags.h>
> +#include <linux/list.h>
> +#include <linux/uaccess.h>
> +#include <linux/io.h>
> +#include "device/tdm_fsl.h"
What is a reference to tdm_fsl.h doing in the presumably
hardware-independent tdm-core.c?
> +static DEFINE_MUTEX(tdm_core_lock);
> +static DEFINE_IDR(tdm_adapter_idr);
> +/* List of TDM adapters registered with TDM framework */
> +LIST_HEAD(adapter_list);
> +
> +/* List of TDM clients registered with TDM framework */
> +LIST_HEAD(driver_list);
> +
> +/* In case the previous data is not fetched by the client driver, the
> + * de-interleaving function will discard the old data and rewrite the
> + * new data */
> +static int use_latest_tdm_data = 1;
Describe when one would want to change this, and provide a better way to
change it (e.g. module parameter or sysfs knob).
/*
* Linux kernel
* multi-line comment style
* is like this.
*/
> +/* this tasklet is created for each adapter instance */
> +static void tdm_data_tasklet_fn(unsigned long);
> +
> +/* tries to match client driver with the adapter */
> +static int tdm_device_match(struct tdm_driver *driver, struct tdm_adapter *adap)
> +{
> + /* match on an id table if there is one */
> + if (driver->id_table && driver->id_table->name[0]) {
> + if (!(strcmp(driver->id_table->name, adap->name)))
> + return (int)driver->id_table;
> + }
> + return TDM_E_OK;
> +}
s/TDM_E_OK/0/g
> +static int tdm_attach_driver_adap(struct tdm_driver *driver,
> + struct tdm_adapter *adap)
> +{
> + /* if driver is already attached to any other adapter, return*/
> + if (driver->adapter && (driver->adapter != adap))
> + return TDM_E_OK;
Why can't one driver service multiple adapters? How would multiple
drivers service one adapter? Are there sub-devices that need individual
controlling?
> + driver->adapter = adap;
> +
> + if (driver->attach_adapter) {
> + if (driver->attach_adapter(adap) < 0)
> + /* We ignore the return code; if it fails, too bad */
> + pr_err("attach_adapter failed for driver [%s]\n",
> + driver->name);
Why ignore errors?
> +/* Detach client driver and adapter */
> +static int tdm_detach_driver_adap(struct tdm_driver *driver,
> + struct tdm_adapter *adap)
> +{
> + int res = TDM_E_OK;
> +
> + if (!driver->adapter || (driver->adapter != adap))
> + return TDM_E_OK;
Shouldn't this be an error?
> + if (!driver->detach_adapter)
> + return TDM_E_OK;
> +
> + adap->drv_count--;
If the driver doesn't have a detach_adapter method, you skip
decrementing the count and leave the tasklet lying around?
> +/* TDM adapter Registration/De-registration with TDM framework */
> +
> +static int tdm_register_adapter(struct tdm_adapter *adap)
> +{
> + int res = TDM_E_OK;
> + struct tdm_driver *driver, *next;
> +
> + if (!adap) {
> + pr_err("%s:Invalid handle\n", __func__);
> + return -EINVAL;
> + }
Pointers aren't handles. The caller should not be passing NULL, and it
would be more useful to crash and get a backtrace if it does. It's not
realistic to check every pointer for being NULL when there's no
legitimate reason it could be NULL, and it doesn't help you if you have
some other bad value besides NULL.
> + mutex_init(&adap->adap_lock);
> + INIT_LIST_HEAD(&adap->myports);
> + spin_lock_init(&adap->portlist_lock);
> +
> + adap->drv_count = 0;
> + adap->tasklet_conf = 0;
> +
> + list_add_tail(&adap->list, &adapter_list);
> +
> + /* initialization of driver by framework in default configuration */
> + init_config_adapter(adap);
> +
> + /* Notify drivers */
> + pr_info("adapter [%s] registered\n", adap->name);
This is too noisy. You haven't even gotten a match yet.
> +/*
> + * tdm_add_adapter - declare tdm adapter, use dynamic device number
Why are there device numbers at all?
I suspect there's a fair bit of copy and paste going on of another
subsystem's quirks (i2c?). I don't see any mention in the copyright
block of this code having been derived from anything else, though...
> +
> +
> +/**
> + * tdm_del_adapter - unregister TDM adapter
> + * @adap: the adapter being unregistered
> + *
> + * This unregisters an TDM adapter which was previously registered
> + * by @tdm_add_adapter.
> + */
> +int tdm_del_adapter(struct tdm_adapter *adap)
> +{
> + int res = TDM_E_OK;
> + struct tdm_adapter *found;
> + struct tdm_driver *driver, *next;
> +
> + if (!adap) {
> + pr_err("%s:Invalid handle\n", __func__);
> + return -EINVAL;
> + }
> +
> + /* First make sure that this adapter was ever added */
> + mutex_lock(&tdm_core_lock);
> + found = idr_find(&tdm_adapter_idr, adap->id);
> + mutex_unlock(&tdm_core_lock);
> + if (found != adap) {
> + pr_err("tdm-core: attempting to delete unregistered "
> + "adapter [%s]\n", adap->name);
> + return -EINVAL;
> + }
> +
> + /*disable and kill the data processing tasklet */
> + if (adap->tasklet_conf) {
> + tasklet_disable(&adap->tdm_data_tasklet);
> + tasklet_kill(&adap->tdm_data_tasklet);
> + adap->tasklet_conf = 0;
> + }
> +
> + /* Detach any active ports. This can't fail, thus we do not
> + checking the returned value. */
> + mutex_lock(&tdm_core_lock);
> + list_for_each_entry_safe(driver, next, &driver_list, list) {
> + if (tdm_device_match(driver, adap)) {
> + tdm_detach_driver_adap(driver, adap);
> + pr_info(
> + "Driver(ID=%d) is detached from Adapter %s(ID = %d)\n",
> + driver->id, adap->name, adap->id);
> + }
> + }
> + mutex_unlock(&tdm_core_lock);
> +
> + mutex_lock(&tdm_core_lock);
> + idr_remove(&tdm_adapter_idr, adap->id);
> + mutex_unlock(&tdm_core_lock);
Why are you dropping the lock then reacquiring it?
> +/* TDM Client Drivers Registration/De-registration Functions */
> +int tdm_register_driver(struct tdm_driver *driver)
> +{
> + int res = TDM_E_OK;
> + struct tdm_adapter *adap, *next;
> +
> + list_add_tail(&driver->list, &driver_list);
> +
> + mutex_lock(&tdm_core_lock);
> + /* Walk the adapters that are already present */
> + list_for_each_entry_safe(adap, next, &adapter_list, list) {
> + if (tdm_device_match(driver, adap)) {
> + res = tdm_attach_driver_adap(driver, adap);
> + pr_info("TDM Driver(ID=%d)is attached with Adapter"
> + "%s(ID = %d) drv_count=%d", driver->id,
> + adap->name, adap->id, adap->drv_count);
> + break;
> + }
> + }
Indentation.
> + mutex_unlock(&tdm_core_lock);
> +
> + return res;
> +}
> +EXPORT_SYMBOL(tdm_register_driver);
> +
> +/*
> + * tdm_unregister_driver - unregister TDM client driver from TDM framework
> + * @driver: the driver being unregistered
> + */
> +void tdm_unregister_driver(struct tdm_driver *driver)
> +{
> + if (!driver) {
> + pr_err("%s:Invalid handle\n", __func__);
> + return;
> + }
> + /* A driver can register to only one adapter,
> + * so no need to browse the list */
Whitespace.
> + mutex_lock(&tdm_core_lock);
> + tdm_detach_driver_adap(driver, driver->adapter);
> + mutex_unlock(&tdm_core_lock);
> +
> + list_del(&driver->list);
> +
> + pr_debug("tdm-core: driver [%s] unregistered\n", driver->name);
> +}
> +EXPORT_SYMBOL(tdm_unregister_driver);
> +
> +/* TDM Framework init and exit */
> +static int __init tdm_init(void)
> +{
> + pr_info("%s\n", __func__);
> + return TDM_E_OK;
> +}
> +
> +static void __exit tdm_exit(void)
> +{
> + pr_info("%s\n", __func__);
> + return;
> +}
Don't spam the console just because the driver got loaded or unloaded
(at this point you haven't even found the hardware).
> +/* We must initialize early, because some subsystems register tdm drivers
> + * in subsys_initcall() code, but are linked (and initialized) before tdm.
> + */
> +postcore_initcall(tdm_init);
> +module_exit(tdm_exit);
tdm_init doesn't do anything, so why does it need to run early?
> +/* Port Level APIs of TDM Framework */
> +unsigned int tdm_port_open(struct tdm_driver *driver, void **h_port)
Why is the return unsigned int? You're returning negative numbers.
Also consider having the return be a pointer, and use PTR_ERR/ERR_PTR --
or at least put a proper type on h_port (what is the "h_"?).
> +{
> + struct tdm_port *port;
> + struct tdm_adapter *adap;
> + unsigned long flags;
> + int res = TDM_E_OK;
> +
> + if (driver == NULL) {
> + pr_err("driver NULL\n");
> + return -ENODEV;
> + }
> + if (driver->adapter == NULL) {
> + pr_err("adapter NULL\n");
> + return -ENODEV;
> + }
Either make these pr_debug (or remove them), or make the message more
specific.
> + adap = tdm_get_adapter(driver->adapter->id);
> + if (!adap)
> + return -ENODEV;
> +
> + /* This creates an anonymous tdm_port, which may later be
> + * pointed to some slot.
> + *
> + */
> + port = kzalloc(sizeof(*port), GFP_KERNEL);
> + if (!port) {
> + res = -ENOMEM;
> + goto out;
> + }
> +
> + init_waitqueue_head(&port->ch_wait_queue);
> +
> +
> + port->rx_max_frames = NUM_SAMPLES_PER_FRAME;
> + port->port_cfg.port_mode = e_TDM_PORT_CHANNELIZED;
> +
> + port->in_use = 1;
When would a port have in_use be false, other than this brief window
where nothing else should be looking at it?
> + list_for_each_entry_safe(channel, temp, &port->mychannels, list) {
> + if (channel)
> + if (channel->in_use) {
> + pr_err("%s: Cannot close port. Channel in use\n",
> + __func__);
> + res = -ENXIO;
> + goto out;
> + }
> + }
Broken indentation.
> +unsigned int tdm_channel_read(void *h_port, void *h_channel,
> + void *p_data, u16 *size)
> +{
> + struct tdm_port *port;
> + struct tdm_channel *channel;
> + struct tdm_bd *rx_bd;
> + unsigned long flags;
> + int i, res = TDM_E_OK;
> + unsigned short *buf, *buf1;
> + port = (struct tdm_port *)h_port;
> + channel = (struct tdm_channel *)h_channel;
Unnecessary casts.
> + if ((port && channel) == 0) { /* invalid handle*/
This is an odd construct -- how about "if (!port || !channel)"?
> + pr_err("%s:Invalid Handle\n", __func__);
> + return -ENXIO;
> + }
> +
> + if (!port->in_use)
> + return -EIO;
> + if (!channel->p_ch_data || !channel->in_use)
> + return -EIO;
> +
> + spin_lock_irqsave(&channel->p_ch_data->rx_channel_lock, flags);
Shouldn't you test whether it's in use after you get the lock?
> + rx_bd = channel->p_ch_data->rx_out_data;
> +
> + if (rx_bd->flag) {
> + *size = rx_bd->length;
> + buf = (u16 *) p_data;
> + buf1 = (u16 *)rx_bd->p_data;
> + for (i = 0; i < NUM_SAMPLES_PER_FRAME; i++)
> + buf[i] = buf1[i];
> + rx_bd->flag = 0;
> + rx_bd->offset = 0;
> + channel->p_ch_data->rx_out_data = (rx_bd->wrap) ?
> + channel->p_ch_data->rx_data_fifo : rx_bd + 1;
> +
> + } else {
> + spin_unlock_irqrestore(&channel->p_ch_data->rx_channel_lock,
> + flags);
> + pr_info("No Data Available");
> + return -EAGAIN;
> + }
That pr_info() is inappropriate. This driver appears to be overly
chatty in general (and with quite vague messages) -- or is this debug
stuff that will be removed in a non-RFC patch?
> +unsigned int tdm_port_get_stats(void *h_port, struct tdm_port_stats *portStat)
> +{
> + struct tdm_port *port;
> + int port_num;
> + port = (struct tdm_port *)h_port;
> +
> + if (port == NULL || portStat == NULL) { /* invalid handle*/
> + pr_err("Invalid Handle");
> + return -ENXIO;
> + }
> + port_num = port->port_id;
> +
> + memcpy(portStat, &port->port_stat, sizeof(struct tdm_port_stats));
> +
> + pr_info("TDM Port %d Get Stats", port_num);
> +
> + return TDM_E_OK;
> +}
> +EXPORT_SYMBOL(tdm_port_get_stats);
> +
> +/* Data handling functions */
> +
> +static int tdm_data_rx_deinterleave(struct tdm_adapter *adap)
> +{
> + struct tdm_port *port, *next;
> + struct tdm_channel *channel, *temp;
> + struct tdm_bd *ch_bd;
> +
> + int i, buf_size, ch_data_len;
> + u16 *input_tdm_buffer;
> + u16 *pcm_buffer;
> + int slot_width;
> + int frame_ch_data_size;
> + bool ch_data;
> + int bytes_in_fifo_per_frame;
> + int bytes_slot_offset;
> +
> + ch_data_len = NUM_SAMPLES_PER_FRAME;
> + frame_ch_data_size = NUM_SAMPLES_PER_FRAME;
> + ch_data = 0;
> +
> + if (!adap) { /* invalid handle*/
> + pr_err("%s: Invalid Handle\n", __func__);
> + return -ENXIO;
> + }
> +
> + slot_width = adap->adapt_cfg.slot_width;
> + buf_size = tdm_adap_recv(adap, (void **)&input_tdm_buffer);
> + if (buf_size <= 0 || !input_tdm_buffer)
> + return -EINVAL;
> +
> + bytes_in_fifo_per_frame = buf_size/frame_ch_data_size;
> + bytes_slot_offset = bytes_in_fifo_per_frame/slot_width;
> +
> + /* de-interleaving for all ports*/
> + list_for_each_entry_safe(port, next, &adap->myports, list) {
> +
> + /* if the port is not open */
> + if (!port->in_use)
> + continue;
> +
> + list_for_each_entry_safe(channel, temp, &port->mychannels,
> + list) {
> + /* if the channel is not open */
> + if (!channel->in_use || !channel->p_ch_data)
> + continue;
> + ch_bd = channel->p_ch_data->rx_in_data;
> + spin_lock(&channel->p_ch_data->rx_channel_lock);
> + /*if old data is to be discarded */
> + if (use_latest_tdm_data)
> + if (ch_bd->flag) {
> + ch_bd->flag = 0;
> + ch_bd->offset = 0;
> + if (ch_bd == channel->p_ch_data->rx_out_data)
> + channel->p_ch_data->rx_out_data =
> + ch_bd->wrap ?
> + channel->p_ch_data->rx_data_fifo
> + : ch_bd+1;
> + port->port_stat.rx_pkt_drop_count++;
> + }
> + /* if the bd is empty */
> + if (!ch_bd->flag) {
> + if (ch_bd->offset == 0)
> + ch_bd->length = port->rx_max_frames;
> +
> + pcm_buffer = ch_bd->p_data + ch_bd->offset;
> + /* De-interleaving the data */
> + for (i = 0; i < ch_data_len; i++) {
> + pcm_buffer[i]
> + = input_tdm_buffer[i*bytes_slot_offset +
> + channel->ch_id];
> + }
> + ch_bd->offset += ch_data_len * slot_width;
> +
> + if (ch_bd->offset >=
> + (ch_bd->length - frame_ch_data_size)*
> + (adap->adapt_cfg.slot_width)) {
> + ch_bd->flag = 1;
> + ch_bd->offset = 0;
> + channel->p_ch_data->rx_in_data =
> + ch_bd->wrap ?
> + channel->p_ch_data->rx_data_fifo
> + : ch_bd+1;
> + ch_data = 1;
> + }
> + } else {
> + port->port_stat.rx_pkt_drop_count++;
> + }
> + spin_unlock(&channel->p_ch_data->rx_channel_lock);
> + }
Broken indentation. Spaces around operators.
> +int tdm_channel_close(u16 chanid, u16 ch_width, struct tdm_port *port,
> + struct tdm_channel *h_channel)
> +{
> + struct tdm_channel *channel;
> + unsigned long flags;
> + int res = TDM_E_OK;
> + channel = h_channel;
> +
> + if (!(port && channel)) {
> + pr_err("%s: Invalid handle\n", __func__);
> + res = -EINVAL;
> + goto out;
> + }
> +
> + if (ch_width != 1) {
> + pr_err("%s: Mode not supported\n", __func__);
> + res = -EINVAL;
> + goto out;
> + }
close() seems an odd time to be checking for supported channel width,
especially since you don't use that variable anywhere else in this function.
> +#ifndef _LINUX_TDM_H
> +#define _LINUX_TDM_H
> +
> +#ifdef __KERNEL__
Is this supposed to be a userspace header ever? If TDM exposes a
userspace interface, it needs to be documented.
> +#include <linux/types.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/device.h> /* for struct device */
> +#include <linux/sched.h> /* for completion */
> +#include <linux/mutex.h>
> +#include <linux/interrupt.h>
> +
> +#define CHANNEL_8BIT_LIN 0 /* 8 bit linear */
> +#define CHANNEL_8BIT_ULAW 1 /* 8 bit Mu-law */
> +#define CHANNEL_8BIT_ALAW 2 /* 8 bit A-law */
> +#define CHANNEL_16BIT_LIN 3 /* 16 bit Linear */
> +
> +#define NUM_CHANNELS 16
> +#define NUM_SAMPLES_PER_MS 8 /* 8 samples per milli sec per
> + channel. Req for voice data */
> +#define NUM_MS 10
> +#define NUM_SAMPLES_PER_FRAME (NUM_MS * NUM_SAMPLES_PER_MS) /* Number of
> + samples for 1 client buffer */
> +#define NUM_OF_TDM_BUF 3
These need proper namespacing -- plus, should all of these really be
hardcoded like this? Is this standard stuff that will always be the same?
> +/* General options */
> +
> +struct tdm_adapt_algorithm;
> +struct tdm_adapter;
> +struct tdm_port;
> +struct tdm_driver;
> +
> +/* Align addr on a size boundary - adjust address up if needed */
> +/* returns min value greater than size which is multiple of alignment */
> +static inline int ALIGN_SIZE(u64 size, u32 alignment)
> +{
> + return (size + alignment - 1) & (~(alignment - 1));
> +}
Use the already existing ALIGN().
> +/**
> + * struct tdm_driver - represent an TDM device driver
> + * @class: What kind of tdm device we instantiate (for detect)
> + * @id:Driver id
> + * @name: Name of the driver
> + * @attach_adapter: Callback for device addition (for legacy drivers)
> + * @detach_adapter: Callback for device removal (for legacy drivers)
> + * @probe: Callback for device binding
> + * @remove: Callback for device unbinding
> + * @shutdown: Callback for device shutdown
> + * @suspend: Callback for device suspend
> + * @resume: Callback for device resume
> + * @command: Callback for sending commands to device
> + * @id_table: List of TDM devices supported by this driver
> + * @list: List of drivers created (for tdm-core use only)
> + */
> +struct tdm_driver {
> + unsigned int class;
> + unsigned int id;
> + char name[TDM_NAME_SIZE];
> +
> + int (*attach_adapter)(struct tdm_adapter *);
> + int (*detach_adapter)(struct tdm_adapter *);
> +
> + /* Standard driver model interfaces */
> + int (*probe)(const struct tdm_device_id *);
> + int (*remove)(void);
> +
> + /* driver model interfaces that don't relate to enumeration */
> + void (*shutdown)(void);
> + int (*suspend)(pm_message_t mesg);
> + int (*resume)(void);
> +
> + /* a ioctl like command that can be used to perform specific functions
> + * with the device.
> + */
> + int (*command)(unsigned int cmd, void *arg);
Please describe what semantics you expect for the non-standard functions.
Where are the "ioctl like commands" defined? When would they be used?
I don't see it used in this patchset.
> +/* struct tdm_channel- represent a TDM channel for a port */
> +struct tdm_channel {
> + u16 ch_id; /* logical channel number */
> + struct list_head list; /* list of channels in a port*/
> + struct tdm_port *port; /* port for this channel */
> + u16 in_use; /* channel is enabled? */
> + struct tdm_ch_cfg ch_cfg; /* channel configuration */
> + struct tdm_ch_data *p_ch_data; /* data storage space for channel */
> +};
Why are ch_id and especially in_use u16?
> +/* tdm_adapt_algorithm is for accessing the routines of device */
> +struct tdm_adapt_algorithm {
> + u32 (*tdm_read)(struct tdm_adapter *, u16 **);
> + u32 (*tdm_get_write_buf)(struct tdm_adapter *, u16 **);
> + u32 (*tdm_write)(struct tdm_adapter *, void * , unsigned int len);
> + int (*tdm_enable)(struct tdm_adapter *);
> + int (*tdm_disable)(struct tdm_adapter *);
> +};
>
Provide parameter names and document the semantics you're expecting.
> +/* tdm_adapter_mode is to define in mode of the device */
> +enum tdm_adapter_mode {
> + e_TDM_ADAPTER_MODE_NONE = 0x00,
> + e_TDM_ADAPTER_MODE_T1 = 0x01,
> + e_TDM_ADAPTER_MODE_E1 = 0x02,
> + e_TDM_ADAPTER_MODE_T1_RAW = 0x10,
> + e_TDM_ADAPTER_MODE_E1_RAW = 0x20,
> +};
> +
> +/* tdm_port_mode defines the mode in which the port is configured to operate
> + * It can be channelized/full/fractional.
> + */
> +enum tdm_port_mode {
> + e_TDM_PORT_CHANNELIZED = 0 /* Channelized mode */
> + , e_TDM_PORT_FULL = 1 /* Full mode */
> + , e_TDM_PORT_FRACTIONAL = 2 /* Fractional mode */
> +};
> +
> +/* tdm_process_mode used for testing the tdm device in normal mode or internal
> + * loopback or external loopback
> + */
> +enum tdm_process_mode {
> + e_TDM_PROCESS_NORMAL = 0 /* Normal mode */
> + , e_TDM_PROCESS_INT_LPB = 1 /* Internal loop mode */
> + , e_TDM_PROCESS_EXT_LPB = 2 /* External Loopback mode */
> +};
Commas go at the end of lines, not the beginning.
No hungarian notation -- drop the "e_".
> +/* TDM configuration parameters */
> +struct fsl_tdm_adapt_cfg {
> + u8 num_ch; /* Number of channels in this adpater */
> + u8 ch_size_type; /* reciever/transmit channel
> + size for all channels */
> + u8 slot_width; /* 1 or 2 Is defined by channel type */
> + u8 frame_len; /* Length of frame in samples */
Is u8 really appropriate here? Maybe int?
What does "type" mean in "ch_size_type"?
> + u8 adap_mode; /* 0=None, 1= T1, 2= T1-FULL, 3=E1,
> + 4 = E1-FULL */
Use #defines or an enum.
> + int max_num_ports; /* Not Used: Max Number of ports that
> + can be created on this adapter */
If it's not used, why is it here?
> +/*
> + * tdm_adapter is the structure used to identify a physical tdm device along
> + * with the access algorithms necessary to access it.
> + */
> +struct tdm_adapter {
> + struct module *owner; /* owner of the adapter module */
> + unsigned int id; /* Adapter Id */
What does the id mean? Why do we need an arbitrary numberspace?
> + unsigned int class; /* classes to allow probing for */
What sort of values would go here?
> + unsigned int drv_count; /* Number of drivers associated with the
> + adapter */
> +
> + const struct tdm_adapt_algorithm *algo; /* the algorithm to access the
> + adapter*/
> +
> + char name[TDM_NAME_SIZE]; /* Name of Adapter */
> + struct mutex adap_lock;
> + struct device *parent; /*Not Used*/
Why is the parent device not used?
> + struct tasklet_struct tdm_data_tasklet; /* tasklet handle to perform
> + data processing*/
> + int tasklet_conf; /* flag for tasklet configuration */
> + int tdm_rx_flag;
What does "tdm_rx_flag" indicate? What about "tasklet_conf"?
> + struct list_head myports; /* list of ports, created on this
> + adapter */
> + struct list_head list;
If you've got more than one list, it's probably a bad idea for any of
them to be simply called "list".
> + spinlock_t portlist_lock; /* Spin Lock for port_list */
Comments should add new information, not just restate what the code
already said.
> +static inline void *tdm_get_adapdata(const struct tdm_adapter *dev)
> +{
> + return dev->data;
> +}
> +
> +static inline void tdm_set_adapdata(struct tdm_adapter *dev, void *data)
> +{
> + dev->data = data;
> +}
Is this really needed?
> +
> +/* functions exported by tdm.o */
> +
> +extern int tdm_add_adapter(struct tdm_adapter *);
> +extern int tdm_del_adapter(struct tdm_adapter *);
> +extern int tdm_register_driver(struct tdm_driver *);
> +extern void tdm_del_driver(struct tdm_driver *);
> +extern void tdm_unregister_driver(struct tdm_driver *);
> +extern void init_config_adapter(struct tdm_adapter *);
> +
> +extern unsigned int tdm_port_open(struct tdm_driver *, void **);
> +extern unsigned int tdm_port_close(void *);
> +extern unsigned int tdm_port_ioctl(void *, unsigned int, unsigned long);
> +extern unsigned int tdm_channel_read(void *, void *, void *, u16 *);
> +extern unsigned int tdm_channel_write(void *, void * , void *, u16);
> +extern unsigned int tdm_port_poll(void *, unsigned int);
> +
> +extern int tdm_channel_open(u16, u16, struct tdm_port *, void **);
> +extern int tdm_channel_close(u16, u16, struct tdm_port *,
> + struct tdm_channel *);
Please provide parameter names with prototypes.
The "extern" is unnecessary.
> +static inline int tdm_add_driver(struct tdm_driver *driver)
> +{
> + return tdm_register_driver(driver);
> +}
What is the semantic difference between tdm_add_driver() and
tdm_register_driver()? Why do they both exist?
-Scott
^ permalink raw reply
* [PATCH v2 0/2] Get rid of PowerPC users of NR_IRQS
From: Grant Likely @ 2012-04-23 22:36 UTC (permalink / raw)
To: linux-kernel, linuxppc-dev, Benjamin Herrenschmidt,
Joakim Tjernlund
In-Reply-To: <1335220202-32533-1-git-send-email-grant.likely@secretlab.ca>
Oops. Flubbed the subject on the below email. This is the cover
letter for the NR_IRQS patches, so the subject should have included
"[PATCH v2 0/2]"
On Mon, Apr 23, 2012 at 4:30 PM, Grant Likely <grant.likely@secretlab.ca> w=
rote:
> Hi Ben,
>
> Here's the 2nd version of the NR_IRQS cleanup patches for PowerPC. =A0I'v=
e
> addressed your comments and compile tested them, but I've not tried to
> boot. =A0If they check out for you then they should probably go into v3.4=
.
>
> g.
>
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* [PATCH v2 2/2] irqdomain/powerpc: Fix broken NR_IRQ references
From: Grant Likely @ 2012-04-23 22:30 UTC (permalink / raw)
To: linux-kernel, linuxppc-dev, Benjamin Herrenschmidt,
Joakim Tjernlund
In-Reply-To: <1335220202-32533-1-git-send-email-grant.likely@secretlab.ca>
The switch from using irq_map to irq_alloc_desc*() for managing irq
number allocations introduced new bugs in some of the powerpc
interrupt code. Several functions rely on the value of NR_IRQS to
determine the maximum irq number that could get allocated. However,
with sparse_irq and using irq_alloc_desc*() the maximum possible irq
number is now specified with 'nr_irqs' which may be a number larger
than NR_IRQS. This has caused breakage on powermac when
CONFIG_NR_IRQS is set to 32.
This patch removes most of the direct references to NR_IRQS in the
powerpc code and replaces them with either a nr_irqs reference or by
using the common for_each_irq_desc() macro. The powerpc-specific
for_each_irq() macro is removed at the same time.
Also, the Cell axon_msi driver is refactored to remove the global
build assumption on the size of NR_IRQS and instead add a limit to the
maximum irq number when calling irq_domain_add_nomap().
v2: Reinstate comment on 65536 irq limit deleted in v1.
Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/include/asm/irq.h | 4 ----
arch/powerpc/kernel/irq.c | 6 +-----
arch/powerpc/kernel/machine_kexec.c | 7 ++-----
arch/powerpc/platforms/cell/axon_msi.c | 8 +++-----
arch/powerpc/platforms/cell/beat_interrupt.c | 2 +-
arch/powerpc/platforms/powermac/pic.c | 6 +++---
arch/powerpc/sysdev/cpm2_pic.c | 3 +--
arch/powerpc/sysdev/xics/xics-common.c | 7 +++----
8 files changed, 14 insertions(+), 29 deletions(-)
diff --git a/arch/powerpc/include/asm/irq.h b/arch/powerpc/include/asm/irq.h
index e648af9..0e40843 100644
--- a/arch/powerpc/include/asm/irq.h
+++ b/arch/powerpc/include/asm/irq.h
@@ -18,10 +18,6 @@
#include <linux/atomic.h>
-/* Define a way to iterate across irqs. */
-#define for_each_irq(i) \
- for ((i) = 0; (i) < NR_IRQS; ++(i))
-
extern atomic_t ppc_n_lost_interrupts;
/* This number is used when no interrupt has been assigned */
diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index 5ec1b23..43eb74f 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -330,14 +330,10 @@ void migrate_irqs(void)
alloc_cpumask_var(&mask, GFP_KERNEL);
- for_each_irq(irq) {
+ for_each_irq_desc(irq, desc) {
struct irq_data *data;
struct irq_chip *chip;
- desc = irq_to_desc(irq);
- if (!desc)
- continue;
-
data = irq_desc_get_irq_data(desc);
if (irqd_is_per_cpu(data))
continue;
diff --git a/arch/powerpc/kernel/machine_kexec.c b/arch/powerpc/kernel/machine_kexec.c
index c957b12..5df7777 100644
--- a/arch/powerpc/kernel/machine_kexec.c
+++ b/arch/powerpc/kernel/machine_kexec.c
@@ -23,14 +23,11 @@
void machine_kexec_mask_interrupts(void) {
unsigned int i;
+ struct irq_desc *desc;
- for_each_irq(i) {
- struct irq_desc *desc = irq_to_desc(i);
+ for_each_irq_desc(i, desc) {
struct irq_chip *chip;
- if (!desc)
- continue;
-
chip = irq_desc_get_chip(desc);
if (!chip)
continue;
diff --git a/arch/powerpc/platforms/cell/axon_msi.c b/arch/powerpc/platforms/cell/axon_msi.c
index d09f3e8..85825b5 100644
--- a/arch/powerpc/platforms/cell/axon_msi.c
+++ b/arch/powerpc/platforms/cell/axon_msi.c
@@ -114,7 +114,7 @@ static void axon_msi_cascade(unsigned int irq, struct irq_desc *desc)
pr_devel("axon_msi: woff %x roff %x msi %x\n",
write_offset, msic->read_offset, msi);
- if (msi < NR_IRQS && irq_get_chip_data(msi) == msic) {
+ if (msi < nr_irqs && irq_get_chip_data(msi) == msic) {
generic_handle_irq(msi);
msic->fifo_virt[idx] = cpu_to_le32(0xffffffff);
} else {
@@ -276,9 +276,6 @@ static int axon_msi_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
if (rc)
return rc;
- /* We rely on being able to stash a virq in a u16 */
- BUILD_BUG_ON(NR_IRQS > 65536);
-
list_for_each_entry(entry, &dev->msi_list, list) {
virq = irq_create_direct_mapping(msic->irq_domain);
if (virq == NO_IRQ) {
@@ -392,7 +389,8 @@ static int axon_msi_probe(struct platform_device *device)
}
memset(msic->fifo_virt, 0xff, MSIC_FIFO_SIZE_BYTES);
- msic->irq_domain = irq_domain_add_nomap(dn, 0, &msic_host_ops, msic);
+ /* We rely on being able to stash a virq in a u16, so limit irqs to < 65536 */
+ msic->irq_domain = irq_domain_add_nomap(dn, 65536, &msic_host_ops, msic);
if (!msic->irq_domain) {
printk(KERN_ERR "axon_msi: couldn't allocate irq_domain for %s\n",
dn->full_name);
diff --git a/arch/powerpc/platforms/cell/beat_interrupt.c b/arch/powerpc/platforms/cell/beat_interrupt.c
index f9a48af..8c6dc42 100644
--- a/arch/powerpc/platforms/cell/beat_interrupt.c
+++ b/arch/powerpc/platforms/cell/beat_interrupt.c
@@ -248,6 +248,6 @@ void beatic_deinit_IRQ(void)
{
int i;
- for (i = 1; i < NR_IRQS; i++)
+ for (i = 1; i < nr_irqs; i++)
beat_destruct_irq_plug(i);
}
diff --git a/arch/powerpc/platforms/powermac/pic.c b/arch/powerpc/platforms/powermac/pic.c
index 66ad93d..c4e6305 100644
--- a/arch/powerpc/platforms/powermac/pic.c
+++ b/arch/powerpc/platforms/powermac/pic.c
@@ -57,9 +57,9 @@ static int max_real_irqs;
static DEFINE_RAW_SPINLOCK(pmac_pic_lock);
-#define NR_MASK_WORDS ((NR_IRQS + 31) / 32)
-static unsigned long ppc_lost_interrupts[NR_MASK_WORDS];
-static unsigned long ppc_cached_irq_mask[NR_MASK_WORDS];
+/* The max irq number this driver deals with is 128; see max_irqs */
+static DECLARE_BITMAP(ppc_lost_interrupts, 128);
+static DECLARE_BITMAP(ppc_cached_irq_mask, 128);
static int pmac_irq_cascade = -1;
static struct irq_domain *pmac_pic_host;
diff --git a/arch/powerpc/sysdev/cpm2_pic.c b/arch/powerpc/sysdev/cpm2_pic.c
index d3be961..10386b6 100644
--- a/arch/powerpc/sysdev/cpm2_pic.c
+++ b/arch/powerpc/sysdev/cpm2_pic.c
@@ -51,8 +51,7 @@
static intctl_cpm2_t __iomem *cpm2_intctl;
static struct irq_domain *cpm2_pic_host;
-#define NR_MASK_WORDS ((NR_IRQS + 31) / 32)
-static unsigned long ppc_cached_irq_mask[NR_MASK_WORDS];
+static unsigned long ppc_cached_irq_mask[2]; /* 2 32-bit registers */
static const u_char irq_to_siureg[] = {
1, 1, 1, 1, 1, 1, 1, 1,
diff --git a/arch/powerpc/sysdev/xics/xics-common.c b/arch/powerpc/sysdev/xics/xics-common.c
index ea5e204..cd1d18d 100644
--- a/arch/powerpc/sysdev/xics/xics-common.c
+++ b/arch/powerpc/sysdev/xics/xics-common.c
@@ -188,6 +188,7 @@ void xics_migrate_irqs_away(void)
{
int cpu = smp_processor_id(), hw_cpu = hard_smp_processor_id();
unsigned int irq, virq;
+ struct irq_desc *desc;
/* If we used to be the default server, move to the new "boot_cpuid" */
if (hw_cpu == xics_default_server)
@@ -202,8 +203,7 @@ void xics_migrate_irqs_away(void)
/* Allow IPIs again... */
icp_ops->set_priority(DEFAULT_PRIORITY);
- for_each_irq(virq) {
- struct irq_desc *desc;
+ for_each_irq_desc(virq, desc) {
struct irq_chip *chip;
long server;
unsigned long flags;
@@ -212,9 +212,8 @@ void xics_migrate_irqs_away(void)
/* We can't set affinity on ISA interrupts */
if (virq < NUM_ISA_INTERRUPTS)
continue;
- desc = irq_to_desc(virq);
/* We only need to migrate enabled IRQS */
- if (!desc || !desc->action)
+ if (!desc->action)
continue;
if (desc->irq_data.domain != xics_host)
continue;
--
1.7.9.5
^ permalink raw reply related
* [PATCH v2 1/2] powerpc/8xx: Fix NR_IRQ bugs and refactor 8xx interrupt controller
From: Grant Likely @ 2012-04-23 22:30 UTC (permalink / raw)
To: linux-kernel, linuxppc-dev, Benjamin Herrenschmidt,
Joakim Tjernlund
In-Reply-To: <1335220202-32533-1-git-send-email-grant.likely@secretlab.ca>
The mpc8xx driver uses a reference to NR_IRQS that is buggy. It uses
NR_IRQs for the array size of the ppc_cached_irq_mask bitmap, but
NR_IRQs could be smaller than the number of hardware irqs that
ppc_cached_irq_mask tracks.
Also, while fixing that problem, it became apparent that the interrupt
controller only supports 32 interrupt numbers, but it is written as if
it supports multiple register banks which is more complicated.
This patch pulls out the buggy reference to NR_IRQs and fixes the size
of the ppc_cached_irq_mask to match the number of HW irqs. It also
drops the now-unnecessary code since ppc_cached_irq_mask is no longer
an array.
v2: - Rename ppc_cached_irq_mask to mpc8xx_cached_irq_mask
- Drop duplicate forward declaration
Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/sysdev/mpc8xx_pic.c | 61 +++++++++++++-------------------------
1 file changed, 20 insertions(+), 41 deletions(-)
diff --git a/arch/powerpc/sysdev/mpc8xx_pic.c b/arch/powerpc/sysdev/mpc8xx_pic.c
index d5f5416..b724622 100644
--- a/arch/powerpc/sysdev/mpc8xx_pic.c
+++ b/arch/powerpc/sysdev/mpc8xx_pic.c
@@ -18,69 +18,45 @@
extern int cpm_get_irq(struct pt_regs *regs);
static struct irq_domain *mpc8xx_pic_host;
-#define NR_MASK_WORDS ((NR_IRQS + 31) / 32)
-static unsigned long ppc_cached_irq_mask[NR_MASK_WORDS];
+static unsigned long mpc8xx_cached_irq_mask;
static sysconf8xx_t __iomem *siu_reg;
-int cpm_get_irq(struct pt_regs *regs);
+static inline unsigned long mpc8xx_irqd_to_bit(struct irq_data *d)
+{
+ return 0x80000000 >> irqd_to_hwirq(d);
+}
static void mpc8xx_unmask_irq(struct irq_data *d)
{
- int bit, word;
- unsigned int irq_nr = (unsigned int)irqd_to_hwirq(d);
-
- bit = irq_nr & 0x1f;
- word = irq_nr >> 5;
-
- ppc_cached_irq_mask[word] |= (1 << (31-bit));
- out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask[word]);
+ mpc8xx_cached_irq_mask |= mpc8xx_irqd_to_bit(d);
+ out_be32(&siu_reg->sc_simask, mpc8xx_cached_irq_mask);
}
static void mpc8xx_mask_irq(struct irq_data *d)
{
- int bit, word;
- unsigned int irq_nr = (unsigned int)irqd_to_hwirq(d);
-
- bit = irq_nr & 0x1f;
- word = irq_nr >> 5;
-
- ppc_cached_irq_mask[word] &= ~(1 << (31-bit));
- out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask[word]);
+ mpc8xx_cached_irq_mask &= ~mpc8xx_irqd_to_bit(d);
+ out_be32(&siu_reg->sc_simask, mpc8xx_cached_irq_mask);
}
static void mpc8xx_ack(struct irq_data *d)
{
- int bit;
- unsigned int irq_nr = (unsigned int)irqd_to_hwirq(d);
-
- bit = irq_nr & 0x1f;
- out_be32(&siu_reg->sc_sipend, 1 << (31-bit));
+ out_be32(&siu_reg->sc_sipend, mpc8xx_irqd_to_bit(d));
}
static void mpc8xx_end_irq(struct irq_data *d)
{
- int bit, word;
- unsigned int irq_nr = (unsigned int)irqd_to_hwirq(d);
-
- bit = irq_nr & 0x1f;
- word = irq_nr >> 5;
-
- ppc_cached_irq_mask[word] |= (1 << (31-bit));
- out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask[word]);
+ mpc8xx_cached_irq_mask |= mpc8xx_irqd_to_bit(d);
+ out_be32(&siu_reg->sc_simask, mpc8xx_cached_irq_mask);
}
static int mpc8xx_set_irq_type(struct irq_data *d, unsigned int flow_type)
{
- if (flow_type & IRQ_TYPE_EDGE_FALLING) {
- irq_hw_number_t hw = (unsigned int)irqd_to_hwirq(d);
+ /* only external IRQ senses are programmable */
+ if ((flow_type & IRQ_TYPE_EDGE_FALLING) && !(irqd_to_hwirq(d) & 1)) {
unsigned int siel = in_be32(&siu_reg->sc_siel);
-
- /* only external IRQ senses are programmable */
- if ((hw & 1) == 0) {
- siel |= (0x80000000 >> hw);
- out_be32(&siu_reg->sc_siel, siel);
- __irq_set_handler_locked(d->irq, handle_edge_irq);
- }
+ siel |= mpc8xx_irqd_to_bit(d);
+ out_be32(&siu_reg->sc_siel, siel);
+ __irq_set_handler_locked(d->irq, handle_edge_irq);
}
return 0;
}
@@ -132,6 +108,9 @@ static int mpc8xx_pic_host_xlate(struct irq_domain *h, struct device_node *ct,
IRQ_TYPE_EDGE_FALLING,
};
+ if (intspec[0] > 0x1f)
+ return 0;
+
*out_hwirq = intspec[0];
if (intsize > 1 && intspec[1] < 4)
*out_flags = map_pic_senses[intspec[1]];
--
1.7.9.5
^ permalink raw reply related
* Get rid of PowerPC users of NR_IRQS
From: Grant Likely @ 2012-04-23 22:30 UTC (permalink / raw)
To: linux-kernel, linuxppc-dev, Benjamin Herrenschmidt,
Joakim Tjernlund
Hi Ben,
Here's the 2nd version of the NR_IRQS cleanup patches for PowerPC. I've
addressed your comments and compile tested them, but I've not tried to
boot. If they check out for you then they should probably go into v3.4.
g.
^ permalink raw reply
* RE: pci node question
From: Benjamin Herrenschmidt @ 2012-04-23 20:40 UTC (permalink / raw)
To: Yoder Stuart-B08248; +Cc: linuxppc-dev@lists.ozlabs.org
In-Reply-To: <9F6FE96B71CF29479FF1CDC8046E1503347AB5@039-SN1MPN1-002.039d.mgd.msft.net>
On Mon, 2012-04-23 at 20:32 +0000, Yoder Stuart-B08248 wrote:
> /* config space address of the device 0,0,0. OF convention is that
> the size of config space is 0. */
>
> ...is that accurate?
Dunno about OF convention here, but it's what I've seen done. In any
case, anything with a PCI device in the .dts will have that, I don't
know whether that's worth putting such a large comment in every .dts
around...
Cheers,
Ben.
^ permalink raw reply
* RE: pci node question
From: Yoder Stuart-B08248 @ 2012-04-23 20:32 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev@lists.ozlabs.org
In-Reply-To: <1335212491.21619.23.camel@pasglop>
DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogQmVuamFtaW4gSGVycmVu
c2NobWlkdCBbbWFpbHRvOmJlbmhAa2VybmVsLmNyYXNoaW5nLm9yZ10NCj4gU2VudDogTW9uZGF5
LCBBcHJpbCAyMywgMjAxMiAzOjIyIFBNDQo+IFRvOiBZb2RlciBTdHVhcnQtQjA4MjQ4DQo+IENj
OiBLdW1hciBHYWxhOyBsaW51eHBwYy1kZXZAbGlzdHMub3psYWJzLm9yZw0KPiBTdWJqZWN0OiBS
RTogcGNpIG5vZGUgcXVlc3Rpb24NCj4gDQo+IE9uIE1vbiwgMjAxMi0wNC0yMyBhdCAxNTo0OCAr
MDAwMCwgWW9kZXIgU3R1YXJ0LUIwODI0OCB3cm90ZToNCj4gPiBIbW1tLi4uIEkgdmFndWVseSB1
bmRlcnN0YW5kIHdoYXQgeW91IGFyZSBzYXlpbmcuICAgQnV0IHdoeSBpcyB0aGUNCj4gPiBsZW5n
dGggemVybywgdGhhdCBpcyB3aGF0IGRvZXNuJ3QgbWFrZSBzZW5zZSBvbiB0aGUgc3VyZmFjZT8N
Cj4gDQo+IEFoIGluZGVlZCwgb24gd291bGQgdGhpbmsgdGhhdCB3ZSBjb3VsZCByZXByZXNlbnQg
dGhlIHJlYWwgc2l6ZSBvZiB0aGUNCj4gY29uZmlnIHNwYWNlIHRoZXJlLi4uIGJ1dCBJJ3ZlIGFs
d2F5cyBzZWVuIHRoaXMgYXMgMCBpbiByZWFsIE9GDQo+IGltcGxlbWVudGF0aW9ucyBhcyB3ZWxs
LCBhdCBsZWFzdCB0aGUgRzUgZGV2aWNlLXRyZWUgSSdtIGxvb2tpbmcgYXQgbm93DQo+IGhhcyBp
dCB0aGF0IHdheS4NCj4gDQo+ID4gV2UgcmVhbGx5IHNob3VsZCBwdXQgYSBjb21tZW50IG9uIHRo
YXQgcmVnIHByb3BlcnR5LS0gY2FuIHlvdSBzdWdnZXN0DQo+ID4gYSBzaG9ydCBjb21tZW50IHRo
YXQgd291bGQgY2FwdHVyZSB3aGF0IHRoZSByZWcgd2l0aCBhbGwgemVyb3MNCj4gPiBpcyBmb3I/
ICAuLi5JJ2xsIHN1Ym1pdCBhIHBhdGNoLg0KPiANCj4gSHJtLi4gImNvbmZpZyBhZGRyZXNzIG9m
IHRoZSBkZXZpY2U6IDAsMCwwIiA/DQoNCkhvdyBhYm91dDoNCg0KICAvKiBjb25maWcgc3BhY2Ug
YWRkcmVzcyBvZiB0aGUgZGV2aWNlIDAsMCwwLiBPRiBjb252ZW50aW9uIGlzIHRoYXQNCiAgICAg
dGhlIHNpemUgb2YgY29uZmlnIHNwYWNlIGlzIDAuICovDQoNCi4uLmlzIHRoYXQgYWNjdXJhdGU/
DQoNClN0dWFydA0K
^ permalink raw reply
* Re: [PATCH 1/3] powerpc/mpic: Fix confusion between hw_irq and virq
From: Benjamin Herrenschmidt @ 2012-04-23 20:22 UTC (permalink / raw)
To: Grant Likely; +Cc: linuxppc-dev
In-Reply-To: <20120423173228.BA05A3E089A@localhost>
On Mon, 2012-04-23 at 11:32 -0600, Grant Likely wrote:
> On Fri, 20 Apr 2012 13:29:34 +1000, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> > mpic_is_ipi() takes a virq and immediately converts it to a hw_irq.
> >
> > However, one of the two call sites calls it with a ... hw_irq. The
> > other call site also happens to have the hw_irq at hand, so let's
> > change it to just take that as an argument. Also change mpic_is_tm()
> > for consistency.
> >
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>
> Looks good to me. Are you pushing these up to Linus for v3.4?
Already did :-)
Cheers,
Ben.
^ permalink raw reply
* RE: pci node question
From: Benjamin Herrenschmidt @ 2012-04-23 20:21 UTC (permalink / raw)
To: Yoder Stuart-B08248; +Cc: linuxppc-dev@lists.ozlabs.org
In-Reply-To: <9F6FE96B71CF29479FF1CDC8046E150334747D@039-SN1MPN1-002.039d.mgd.msft.net>
On Mon, 2012-04-23 at 15:48 +0000, Yoder Stuart-B08248 wrote:
> Hmmm... I vaguely understand what you are saying. But why is the
> length zero, that is what doesn't make sense on the surface?
Ah indeed, on would think that we could represent the real size of the
config space there... but I've always seen this as 0 in real OF
implementations as well, at least the G5 device-tree I'm looking at now
has it that way.
> We really should put a comment on that reg property-- can you suggest
> a short comment that would capture what the reg with all zeros
> is for? ...I'll submit a patch.
Hrm.. "config address of the device: 0,0,0" ?
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 1/3] powerpc/mpic: Fix confusion between hw_irq and virq
From: Grant Likely @ 2012-04-23 17:32 UTC (permalink / raw)
To: Benjamin Herrenschmidt, linuxppc-dev
In-Reply-To: <1334892574.31646.0.camel@pasglop>
On Fri, 20 Apr 2012 13:29:34 +1000, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> mpic_is_ipi() takes a virq and immediately converts it to a hw_irq.
>
> However, one of the two call sites calls it with a ... hw_irq. The
> other call site also happens to have the hw_irq at hand, so let's
> change it to just take that as an argument. Also change mpic_is_tm()
> for consistency.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Looks good to me. Are you pushing these up to Linus for v3.4?
g.
^ permalink raw reply
* Re: PowerPC radeon KMS - is it possible?
From: Gerhard Pircher @ 2012-04-23 16:45 UTC (permalink / raw)
To: "Michel Dänzer"; +Cc: linuxppc-dev
In-Reply-To: <1335174966.5989.529.camel@thor.local>
-------- Original-Nachricht --------
> Datum: Mon, 23 Apr 2012 11:56:06 +0200
> Von: "Michel Dänzer" <michel@daenzer.net>
> An: Gerhard Pircher <gerhard_pircher@gmx.net>
> CC: linuxppc-dev@lists.ozlabs.org
> Betreff: Re: PowerPC radeon KMS - is it possible?
> On Fre, 2012-04-20 at 18:14 +0200, Gerhard Pircher wrote:
> > > Von: "Michel Dänzer" <michel@daenzer.net>
> > > On Fre, 2012-04-20 at 13:15 +0200, Gerhard Pircher wrote:
> > > > > Von: "Michel Dänzer" <michel@daenzer.net>
> > > > > On Don, 2012-04-19 at 13:48 +0200, Gerhard Pircher wrote:
> > > > > >
> > > > > > The "former case" is an explanation, why I see data corruption
> > > > > > with my< AGPGART driver (more or less a copy of the uninorth
> > > > > > driver) on my non-coherent platform. There are no cache
> > > > > > flushes done for writes to already mapped pages.
> > > > >
> > > > > As I said, the radeon driver always maps AGP memory uncacheable
> > > > > for the CPU, so no such CPU cache flushes should be necessary.
> > > > I know. We also discussed this topic over two years ago. :-)
> > > >
> > > > What I didn't understand yet is how this uncacheable memory is
> > > > allocated (well, I never took the time to look at this again). The
> > > > functions in ttm_page_alloc.c seem to allocate normal cacheable
> > > > memory and try to set the page flags with set_pages_array_uc(),
> > > > which is more or less a no-op on powerpc. ttm_page_alloc_dma.c on
> > > > the other side is only used with SWIOTLB!?
> > > [...]
> > > > Could it be that the memory is finally mapped uncacheable by
> > > radeon_bo_kmap()/
> > > > ttm_bo_kmap()/..some other TTM functions../vmap()?
> > >
> > > Yeah, AFAICT, basically ttm_io_prot() defines the mapping
> > > attributes, and vmap() is used to enforce them for kernel mappings.
> > Okay, that sounds like the approach used by arch/powerpc/mm/dma-
> > noncoherent.c in my ("green") ears. What about the PCIGART mode?
> > Is the driver free to use cached memory in this mode?
>
> Yes, it assumes PCI(e) GART to be CPU cache coherent.
Okay. I guess it should be possible to modify it so that it makes use
of uncacheable memory - just for testing!?
PCIGART was working "somehow" on my platform up to the ~2.6.39 kernel,
i.e. I could login to GNOME and open a program until the machine
locked-up. :-)
> > > > [ 5.878809] [drm:radeon_test_moves] *ERROR* Incorrect VRAM->GTT copy 0: Got 0xf1416ec0, expected 0xf1570ec0 (VRAM map 0xf1480000-0xf1580000)
> > > [...]
> > > > The VRAM->GTT copy totally puzzles me, as it returns a wrong page
> > > > address, but the offset is fine!?
> > >
> > > Maybe it's still the values from the GTT->VRAM test, i.e. either
> > > the GPU writes didn't make it to the memory mapped into the AGP
That's indeed the case. I changed the code so that gtt_map is
reinitialized with some magic value before the VRAM->GTT copy and the
debug output shows that the GPU writes don't make it to the memory -
or they are going to the wrong memory location, but that's harder to
track.
> > > GART (some AGP bridges are known to have issues with that) or the
> > > CPU doesn't see it.
> > What is the workaround for such an AGP bridge? If there is one at
> > all...
>
> The only workaround short of not using AGP would be not doing GPU->AGP
> transfers, but that's not implemented or even planned at all.
Okay.
> > > BTW, does your driver set cant_use_aperture, or is the linear
> > > aperture accessible by the CPU?
> > The driver sets cant_use_aperture. I couldn't get it working at all
> > without it.
>
> Does the hardware really not allow the CPU to access the linear aperture
> though? Because if it does, I wonder if that might be more reliable.
I'm afraid no, but I could try it out again.
BTW: I see that the uninorth driver defines needs_scratch_page. What
is this actually good for?
Thanks!
Gerhard
--
Empfehlen Sie GMX DSL Ihren Freunden und Bekannten und wir
belohnen Sie mit bis zu 50,- Euro! https://freundschaftswerbung.gmx.de
^ permalink raw reply
* RE: pci node question
From: Yoder Stuart-B08248 @ 2012-04-23 15:48 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Kumar Gala; +Cc: linuxppc-dev@lists.ozlabs.org
In-Reply-To: <1334956286.3197.16.camel@pasglop>
DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogQmVuamFtaW4gSGVycmVu
c2NobWlkdCBbbWFpbHRvOmJlbmhAa2VybmVsLmNyYXNoaW5nLm9yZ10NCj4gU2VudDogRnJpZGF5
LCBBcHJpbCAyMCwgMjAxMiA0OjExIFBNDQo+IFRvOiBLdW1hciBHYWxhDQo+IENjOiBZb2RlciBT
dHVhcnQtQjA4MjQ4OyBsaW51eHBwYy1kZXZAbGlzdHMub3psYWJzLm9yZw0KPiBTdWJqZWN0OiBS
ZTogcGNpIG5vZGUgcXVlc3Rpb24NCj4gDQo+IE9uIEZyaSwgMjAxMi0wNC0yMCBhdCAxMzo1MyAt
MDUwMCwgS3VtYXIgR2FsYSB3cm90ZToNCj4gPiBPbiBBcHIgMjAsIDIwMTIsIGF0IDE6MzcgUE0s
IFlvZGVyIFN0dWFydC1CMDgyNDggd3JvdGU6DQo+ID4NCj4gPiA+IFRoZXJlIHdhcyByZWZhY3Rv
cmluZyBjaGFuZ2UgYSB3aGlsZSBiYWNrIHRoYXQgbW92ZWQNCj4gPiA+IHRoZSBpbnRlcnJ1cHQg
bWFwIGRvd24gaW50byB0aGUgdmlydHVhbCBwY2kgYnJpZGdlLg0KPiA+ID4NCj4gPiA+IGV4YW1w
bGU6DQo+ID4gPiA0MiAvKiBjb250cm9sbGVyIGF0IDB4MjAwMDAwICovDQo+ID4gPiA0MyAmcGNp
MCB7DQo+ID4gPiA0NCAgICAgICAgIGNvbXBhdGlibGUgPSAiZnNsLHAyMDQxLXBjaWUiLCAiZnNs
LHFvcmlxLXBjaWUtdjIuMiI7DQo+ID4gPiA0NSAgICAgICAgIGRldmljZV90eXBlID0gInBjaSI7
DQo+ID4gPiA0NiAgICAgICAgICNzaXplLWNlbGxzID0gPDI+Ow0KPiA+ID4gNDcgICAgICAgICAj
YWRkcmVzcy1jZWxscyA9IDwzPjsNCj4gPiA+IDQ4ICAgICAgICAgYnVzLXJhbmdlID0gPDB4MCAw
eGZmPjsNCj4gPiA+IDQ5ICAgICAgICAgY2xvY2stZnJlcXVlbmN5ID0gPDMzMzMzMzMzPjsNCj4g
PiA+IDUwICAgICAgICAgaW50ZXJydXB0cyA9IDwxNiAyIDEgMTU+Ow0KPiA+ID4gNTEgICAgICAg
ICBwY2llQDAgew0KPiA+ID4gNTIgICAgICAgICAgICAgICAgIHJlZyA9IDwwIDAgMCAwIDA+Ow0K
PiA+ID4gNTMgICAgICAgICAgICAgICAgICNpbnRlcnJ1cHQtY2VsbHMgPSA8MT47DQo+ID4gPiA1
NCAgICAgICAgICAgICAgICAgI3NpemUtY2VsbHMgPSA8Mj47DQo+ID4gPiA1NSAgICAgICAgICAg
ICAgICAgI2FkZHJlc3MtY2VsbHMgPSA8Mz47DQo+ID4gPiA1NiAgICAgICAgICAgICAgICAgZGV2
aWNlX3R5cGUgPSAicGNpIjsNCj4gPiA+IDU3ICAgICAgICAgICAgICAgICBpbnRlcnJ1cHRzID0g
PDE2IDIgMSAxNT47DQo+ID4gPiA1OCAgICAgICAgICAgICAgICAgaW50ZXJydXB0LW1hcC1tYXNr
ID0gPDB4ZjgwMCAwIDAgNz47DQo+ID4gPiA1OSAgICAgICAgICAgICAgICAgaW50ZXJydXB0LW1h
cCA9IDwNCj4gPiA+IDYwICAgICAgICAgICAgICAgICAgICAgICAgIC8qIElEU0VMIDB4MCAqLw0K
PiA+ID4gNjEgICAgICAgICAgICAgICAgICAgICAgICAgMDAwMCAwIDAgMSAmbXBpYyA0MCAxIDAg
MA0KPiA+ID4gNjIgICAgICAgICAgICAgICAgICAgICAgICAgMDAwMCAwIDAgMiAmbXBpYyAxIDEg
MCAwDQo+ID4gPiA2MyAgICAgICAgICAgICAgICAgICAgICAgICAwMDAwIDAgMCAzICZtcGljIDIg
MSAwIDANCj4gPiA+IDY0ICAgICAgICAgICAgICAgICAgICAgICAgIDAwMDAgMCAwIDQgJm1waWMg
MyAxIDAgMA0KPiA+ID4gNjUgICAgICAgICAgICAgICAgICAgICAgICAgPjsNCj4gPiA+IDY2ICAg
ICAgICAgfTsNCj4gPiA+IDY3IH07DQo+ID4gPg0KPiA+ID4gV2h5IHdhcyB0aGUgaW50ZXJydXB0
LW1hcCBtb3ZlZCBoZXJlPw0KPiA+DQo+ID4gSXRzIGJlZW4gYSB3aGlsZSwgYnV0IEkgdGhpbmsg
aSBtb3ZlZCBpdCBkb3duIGJlY2F1c2Ugb2Ygd2hpY2ggbm9kZSBpcyB1c2VkIGZvciBpbnRlcnJ1
cHQgaGFuZGxpbmcgaW4NCj4gbGludXguDQo+IA0KPiBZZWFoLCBpdCB3b3VsZCBzd2l6emxlIGlm
IGdvaW5nIGFjY3Jvc3MgdGhlIHZpcnR1YWwgUDJQIGJyaWRnZS4gT24gdGhlDQo+IG90aGVyIGhh
bmQsIGlmIGl0J3MgUENJZSwgdGhlIGNoaWxkcmVuIG9mIHRoZSB2UDJQIHNob3VsZCBhbHdheXMg
YmUgYXQNCj4gZGV2aWNlIDAgYW5kIHRodXMgdGhlIHN3aXp6bGluZyBzaG91bGQgYmUgYSBOT1Ag
bm8gPyBTbyB3ZSAtY291bGQtIGxlYXZlDQo+IGl0IGluIHRoZSBQSEIgdW5sZXNzIEknbSBtaXN0
YWtlbi4uLg0KPiANCj4gT24gdGhlIG90aGVyIGhhbmQsIHdlIHByb2JhYmx5IHdhbnQgdG8gZml4
IHRoZSBtYXBwaW5nIGNvZGUgdG8gaGFuZGxlDQo+IHRoaW5ncyBsaWtlIFNSLUlPViBQRnMgd2l0
aCBkaWZmZXJlbnQgZGV2aWNlIG51bWJlcnMuLi4gZG8gdGhvc2UgZ2V0DQo+IHN3aXp6bGVkID8g
SW4gdGhhdCBjYXNlIHdlIG1pZ2h0IHdhbnQgdG8ga2VlcCB0aGluZ3MgZG93biB0aGUgdmlydHVh
bA0KPiBicmlkZ2UuDQo+IA0KPiA+ID4gRG8gd2UgcmVhbGx5IG5lZWQgdGhlIGVycm9yIGludGVy
cnVwdCBzcGVjaWZpZWQgdHdpY2U/DQo+ID4NCj4gPiBJIHB1dCBpdCB0d2ljZSBiZWNhdXNlIGl0
IGhhcyBtdWx0aXBsZSBwdXJwb3Nlcywgb25lIGhhcyB0byBkbyB3aXRoIGludGVycnVwdHMgZGVm
aW5lZCBieSB0aGUgUENJIHNwZWMNCj4gdnMgb25lcyBkZWZpbmVkIHZpYSBGU0wgY29udHJvbGxl
ci4NCj4gPg0KPiA+ID4gV2h5IGlzIHRoZXJlIGEgemVyb2VkIG91dCByZWcgcHJvcGVydHk6IHJl
ZyA9IDwwIDAgMCAwIDA+ID8/DQo+ID4NCj4gPiBzY3JhdGNoaW5nIG15IGhlYWQsIHdoYXQgaGFw
cGVucyBpZiB5b3UgcmVtb3ZlIGl0Pw0KPiANCj4gSWYgeW91IHJlbW92ZSBpdCwgdGhlIGtlcm5l
bCB3aWxsIGZhaWwgdG8gbWF0Y2ggdGhlIHZQMlAgd2l0aCB0aGUNCj4gY29ycmVzcG9uZGluZyBz
dHJ1Y3QgcGNpX2Rldi4uLi4gSXQncyBub3QgInplcm9lZCBvdXQiLiBJdCBjb250YWlucyBhDQo+
IHZhbGlkIGJ1cy9kZXYvZm4gLi4uIG9mIDAsMCwwIDotKQ0KDQpIbW1tLi4uIEkgdmFndWVseSB1
bmRlcnN0YW5kIHdoYXQgeW91IGFyZSBzYXlpbmcuICAgQnV0IHdoeSBpcyB0aGUNCmxlbmd0aCB6
ZXJvLCB0aGF0IGlzIHdoYXQgZG9lc24ndCBtYWtlIHNlbnNlIG9uIHRoZSBzdXJmYWNlPw0KDQpX
ZSByZWFsbHkgc2hvdWxkIHB1dCBhIGNvbW1lbnQgb24gdGhhdCByZWcgcHJvcGVydHktLSBjYW4g
eW91IHN1Z2dlc3QNCmEgc2hvcnQgY29tbWVudCB0aGF0IHdvdWxkIGNhcHR1cmUgd2hhdCB0aGUg
cmVnIHdpdGggYWxsIHplcm9zIA0KaXMgZm9yPyAgLi4uSSdsbCBzdWJtaXQgYSBwYXRjaC4NCg0K
VGhhbmtzLA0KU3R1YXJ0DQo=
^ permalink raw reply
* Re: [PATCH 7/9] um: Should hold tasklist_lock while traversing processes
From: Anton Vorontsov @ 2012-04-23 15:40 UTC (permalink / raw)
To: Richard Weinberger
Cc: linaro-kernel, Mike Frysinger, Peter Zijlstra,
user-mode-linux-devel, linux-sh, patches, Oleg Nesterov,
linux-kernel, linux-mm, Paul Mundt, John Stultz, KOSAKI Motohiro,
uclinux-dist-devel, Russell King, Andrew Morton, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <4F956DF2.7020408@nod.at>
On Mon, Apr 23, 2012 at 04:57:54PM +0200, Richard Weinberger wrote:
> On 23.04.2012 09:09, Anton Vorontsov wrote:
> >Traversing the tasks requires holding tasklist_lock, otherwise it
> >is unsafe.
> >
> >p.s. However, I'm not sure that calling os_kill_ptraced_process()
> >in the atomic context is correct. It seem to work, but please
> >take a closer look.
> >
> >Signed-off-by: Anton Vorontsov<anton.vorontsov@linaro.org>
> >---
>
> You forgot my Ack and I've already explained why
> os_kill_ptraced_process() is fine.
Ouch, sorry!
--
Anton Vorontsov
Email: cbouatmailru@gmail.com
^ permalink raw reply
* Re: [PATCH 7/9] um: Should hold tasklist_lock while traversing processes
From: Richard Weinberger @ 2012-04-23 14:57 UTC (permalink / raw)
To: Anton Vorontsov
Cc: linaro-kernel, Mike Frysinger, Peter Zijlstra,
user-mode-linux-devel, linux-sh, patches, Oleg Nesterov,
linux-kernel, linux-mm, Paul Mundt, John Stultz, KOSAKI Motohiro,
uclinux-dist-devel, Russell King, Andrew Morton, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <20120423070925.GG30752@lizard>
On 23.04.2012 09:09, Anton Vorontsov wrote:
> Traversing the tasks requires holding tasklist_lock, otherwise it
> is unsafe.
>
> p.s. However, I'm not sure that calling os_kill_ptraced_process()
> in the atomic context is correct. It seem to work, but please
> take a closer look.
>
> Signed-off-by: Anton Vorontsov<anton.vorontsov@linaro.org>
> ---
You forgot my Ack and I've already explained why
os_kill_ptraced_process() is fine.
Thanks,
//richard
^ permalink raw reply
* Re: [PATCH 5/9] dmaengine: provide a common function for completing a dma descriptor
From: Russell King - ARM Linux @ 2012-04-23 11:13 UTC (permalink / raw)
To: Boojin Kim
Cc: 'Vinod Koul', 'Stephen Warren',
'Linus Walleij', 'Srinidhi Kasagar',
'Dan Williams', linuxppc-dev, linux-arm-kernel
In-Reply-To: <000001cd2141$26914d60$73b3e820$%kim@samsung.com>
On Mon, Apr 23, 2012 at 08:06:36PM +0900, Boojin Kim wrote:
> I met the DMA probing fail problem on Linux 3.4.
> It's because the return value on regulator_get() is changed
> from ENODEV to EPROBE_DEFER in case not to supply a vcore regulator.
> So, I try to change the check value about the return value of regulator_get()
> in amba_get_enable_vcore()from ENODEV to EPROBE_DEFER.
> How about it ? Do you already fix it too?
A fix has already been committed and sent upstream - about a week ago.
^ permalink raw reply
* RE: [PATCH 5/9] dmaengine: provide a common function for completing a dma descriptor
From: Boojin Kim @ 2012-04-23 11:06 UTC (permalink / raw)
To: 'Vinod Koul', 'Russell King - ARM Linux'
Cc: 'Stephen Warren', 'Linus Walleij',
'Srinidhi Kasagar', 'Dan Williams', linuxppc-dev,
linux-arm-kernel
In-Reply-To: <1335175278.31825.108.camel@vkoul-udesk3>
Vinod Koul wrote:
> Sent: Monday, April 23, 2012 7:01 PM
> To: Russell King - ARM Linux
> Cc: 'Stephen Warren'; 'Linus Walleij'; 'Srinidhi Kasagar'; Boojin Kim; 'Dan Williams'; 'Li Yang';
> linuxppc-dev@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 5/9] dmaengine: provide a common function for completing a dma descriptor
>
> On Mon, 2012-04-23 at 10:50 +0100, Russell King - ARM Linux wrote:
> > On Mon, Apr 23, 2012 at 06:40:06PM +0900, Boojin Kim wrote:
> > > I met a problem on DMA cyclic mode (DMA_CYCLIC) for sound playback.
> > > Kernel BUG occurs during DMA transfer with DMA cyclic mode.
> > > This patch makes the cookies into zero. But, cookies should be kept
> > > during cyclic mode because cyclic mode re-uses the cookies.
> >
> > The protection is there to prevent cookies being accidentally re-used.
> > If you're running a cyclic transfer, even then you shouldn't be completing
> > the same cookie time and time again - I think Vinod also concurs with this.
> Right :)
> I recently committed patch for imx-dma which doesn't mark the cyclic
> descriptor as complete. Descriptor represents a transaction and makes no
> sense to complete t if the transaction is still continuing.
Dear Vinod,
you already fixed it. :) thanks.
And I have other question. (Actually, It doesn't relate to this patch.)
I met the DMA probing fail problem on Linux 3.4.
It's because the return value on regulator_get() is changed
from ENODEV to EPROBE_DEFER in case not to supply a vcore regulator.
So, I try to change the check value about the return value of regulator_get()
in amba_get_enable_vcore()from ENODEV to EPROBE_DEFER.
How about it ? Do you already fix it too?
Thanks,
Boojin
> >
> > I think our preference is for cyclic transfers to entire remain uncompleted,
> > or to get a new cookie each time they allegedly "complete".
> No it is not complete. Cyclic never completes, it aborts when user
> wants. The "notification" interrupt is for updating the
> counters/notifying (timestamp/periods elapsed in sound), and shouldn't
> be used for anything else
>
> --
> ~Vinod
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox