* Re: [PATCH] [POWERPC] Rework EXC_LEVEL_EXCEPTION_PROLOG code
From: Paul Mackerras @ 2008-05-01 8:24 UTC (permalink / raw)
To: Kumar Gala; +Cc: linuxppc-dev
In-Reply-To: <A9B566F7-D00B-4202-9B85-427C38EE6070@kernel.crashing.org>
Kumar Gala writes:
> > It depends on whether a critical or machine check handler can ever do
> > anything to generate a signal or a reschedule. If they can't, then
> > there is no problem.
>
> They can if the come from user space. I'm question what it means to
> send a signal based on receiving an async exception.
The most common cases are (a) something that ultimately generates
input on a tty (e.g. a character arriving on a serial port) and that
input turns out to be a ^C or similar, or (b) something that signals
I/O completion and the program doing the I/O has requested
notification by a SIGIO. But in general any driver code can send a
signal to userspace if it wants.
> > If they can, then we have to be very careful. If a critical or
> > machine check happens at a point where normal interrupts are disabled
> > then we have to be extremely careful not to do anything that the code
> > we've interrupted assumes can't happen - so we'd better not try to
> > take any spinlocks, for example. That severely limits what the
> > handler can do. It probably shouldn't even call printk, for
> > instance, or wake any process up, and definitely shouldn't call
> > schedule (or schedule_preempt) on the way out.
>
> how do we provide someone stick a kprobe on such code today?
-ENOPARSE
> So I'm not if there is any good way to preclude the handlers
> associated with these exceptions from doing the things you listed.
In that case, you'd better expect to see system freezes, memory
corruption and general instability.
Paul.
^ permalink raw reply
* Re: [PATCH 5/6] [POWERPC] Move device_to_mask() to dma-mapping.h
From: Segher Boessenkool @ 2008-05-01 9:04 UTC (permalink / raw)
To: markn; +Cc: linuxppc-dev, paulus
In-Reply-To: <20080501043737.892888612@au1.ibm.com>
> Move device_to_mask() to dma-mapping.h because we need to use it from
> outside dma_64.c in a later patch.
Why does this need to become an inline function?
Segher
^ permalink raw reply
* RE: [patch 2.6.25-git] spi_mpc83xx much improved driver
From: Joakim Tjernlund @ 2008-05-01 10:42 UTC (permalink / raw)
To: 'David Brownell', 'Andrew Morton'
Cc: spi-devel-general, 12o3l, linuxppc-dev
In-Reply-To: <200804301555.34110.david-b@pacbell.net>
> -----Original Message-----
> From: David Brownell [mailto:david-b@pacbell.net]
> Sent: den 1 maj 2008 00:56
> To: Andrew Morton
> Cc: spi-devel-general@lists.sourceforge.net; =
joakim.tjernlund@transmode.se; linuxppc-dev@ozlabs.org;
> 12o3l@tiscali.nl
> Subject: Re: [patch 2.6.25-git] spi_mpc83xx much improved driver
>=20
> On Wednesday 30 April 2008, Andrew Morton wrote:
> > > +=A0=A0=A0=A0=A0spin_lock_irq(&mpc83xx_spi->lock);
> >
> > irq-safe.
> >
> > > ...
> > >
> > > +=A0=A0=A0=A0=A0spin_lock(&mpc83xx_spi->lock);
> >
> > not irq-safe.
> >
> > Deliberate?
>=20
> That latter one is inside an #if 0/#endif block, so it won't matter.
> The #if 0 block bothered me. Maybe Joakim can remove it.
The non irq-safe spin_lock was there before my patch and now it is
removed with the #if 0/#endif block. I don't need this code but
I left it there disabled to make it clear for everyone else
that it was removed. I can remove it but I rather leave it
there for one release so everyone will have a chance to see
it and object if they feel differently.
>=20
> (By the way, I'd feel safer about this patch if there were an
> ack or two from more PPC folk...)
Yeah, that would be good.
Jocke
^ permalink raw reply
* Re: [PATCH 1/6] [POWERPC] sysdev: implement FSL GTM support
From: Anton Vorontsov @ 2008-05-01 11:43 UTC (permalink / raw)
To: Kumar Gala; +Cc: linuxppc-dev
In-Reply-To: <9E78A635-CB7B-4A51-9FD5-2922747D4847@kernel.crashing.org>
On Wed, Apr 30, 2008 at 11:00:42PM -0500, Kumar Gala wrote:
>
> On Apr 29, 2008, at 2:00 PM, Anton Vorontsov wrote:
>> GTM stands for General-purpose Timers Module and able to generate
>> timer{1,2,3,4} interrupts. These timers are used by the drivers that
>> need time precise interrupts (like for USB transactions scheduling for
>> the Freescale USB Host controller as found in some QE and CPM chips),
>> or these timers could be used as wakeup events from the CPU deep-sleep
>> mode.
>>
>> Things unimplemented:
>> 1. Cascaded (32 bit) timers (1-2, 3-4).
>> This is straightforward to implement when needed, two timers should
>> be marked as "requested" and configured as appropriate.
>> 2. Super-cascaded (64 bit) timers (1-2-3-4).
>> This is also straightforward to implement when needed, all timers
>> should be marked as "requested" and configured as appropriate.
>>
>> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
>> ---
>> Documentation/powerpc/booting-without-of.txt | 37 +++-
>> arch/powerpc/Kconfig | 5 +
>> arch/powerpc/sysdev/Makefile | 1 +
>> arch/powerpc/sysdev/fsl_gtm.c | 424 +++++++++++++++++
>> +++++++++
>> include/asm-powerpc/fsl_gtm.h | 47 +++
>> 5 files changed, 513 insertions(+), 1 deletions(-)
>> create mode 100644 arch/powerpc/sysdev/fsl_gtm.c
>> create mode 100644 include/asm-powerpc/fsl_gtm.h
>>
>>
>
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index 4e40c12..4070a78 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -538,6 +538,11 @@ config FSL_LBC
>> help
>> Freescale Localbus support
>>
>> +config FSL_GTM
>> + bool
>> + help
>> + Freescale General-purpose Timers support
>> +
>
> what chips actually use this? just QE or 83xx?
83xx/85xx, QE, CPM2 and CPM1 (no support).
Probably some more old and new chips.
--
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
^ permalink raw reply
* Re: [PATCH] [POWERPC] Rework EXC_LEVEL_EXCEPTION_PROLOG code
From: Kumar Gala @ 2008-05-01 13:17 UTC (permalink / raw)
To: Paul Mackerras; +Cc: linuxppc-dev
In-Reply-To: <18457.32297.485053.356477@cargo.ozlabs.ibm.com>
On May 1, 2008, at 3:24 AM, Paul Mackerras wrote:
> Kumar Gala writes:
>
>>> It depends on whether a critical or machine check handler can ever
>>> do
>>> anything to generate a signal or a reschedule. If they can't, then
>>> there is no problem.
>>
>> They can if the come from user space. I'm question what it means to
>> send a signal based on receiving an async exception.
>
> The most common cases are (a) something that ultimately generates
> input on a tty (e.g. a character arriving on a serial port) and that
> input turns out to be a ^C or similar, or (b) something that signals
> I/O completion and the program doing the I/O has requested
> notification by a SIGIO. But in general any driver code can send a
> signal to userspace if it wants.
ok. Was just wondering how the async exception know that the signal
it wanted to send belonged to the particular process that is running.
But I guess there are cases that the signal is really intended for who
ever is currently running?
>>> If they can, then we have to be very careful. If a critical or
>>> machine check happens at a point where normal interrupts are
>>> disabled
>>> then we have to be extremely careful not to do anything that the
>>> code
>>> we've interrupted assumes can't happen - so we'd better not try to
>>> take any spinlocks, for example. That severely limits what the
>>> handler can do. It probably shouldn't even call printk, for
>>> instance, or wake any process up, and definitely shouldn't call
>>> schedule (or schedule_preempt) on the way out.
Do we ensure that synchronous exceptions will not occur in these cases
that kernel code things interrupts are disabled in?
>> how do we provide someone stick a kprobe on such code today?
>
> -ENOPARSE
I was asking how we prevent the cases you were describing working w/
kprobes today. Since it ends up single stepping in kernel codes its
possible that someone sets a kprobe in code that shouldn't be
interrupted, yet we'd cause a SingleStep Exception.
>> So I'm not if there is any good way to preclude the handlers
>> associated with these exceptions from doing the things you listed.
>
> In that case, you'd better expect to see system freezes, memory
> corruption and general instability.
So the case I'm trying to make work is debug and kprobes. This case
seems like we have pretty good control over what the "handler" does.
Are there checks we can add to BUG_ON() so we are at least aware of
the code attempts to do something it shouldnt?
- k
^ permalink raw reply
* Re: [PATCH] [POWERPC] Rework EXC_LEVEL_EXCEPTION_PROLOG code
From: Kumar Gala @ 2008-05-01 13:22 UTC (permalink / raw)
To: benh; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <1209628423.18023.286.camel@pasglop>
>> So we have 4 actual exceptions:
>> * CriticalInput (some external device signaled this. There are two
>> concepts of critical. One is error the other is high priority)
>> However this would have the same caveats as any ExternalInput
>> handler.
>
> No, it's worse. It can interrupt code that normally has
> local_irq_disabled() and thus doesn't expect to be interrupted. That
> means that everything becomes unsafe including locks etc....
Fair. Should local_irq_disable() clear MSR_SE & MSR_BE on classic
parts?
> Note that driver that want to make active use of that probably want
> some
> explicit local_crit_irq_disable/enable functions to be able to
> implement
> some sort of synchronization.
Or we could just have local_irq_disable -- clear MSR_EE and MSR_CE
>> * Watchdog - pretty severe if this fires.
>>
>> * Debug - user space debug is pretty straight forward. However we
>> have features like kprobes that require kernel level support.
>
> Which means we have to be extra careful, in fact, I consider it a
> design
> bug of BookE to have made debug be a critical interrupt...
I consider the whole BookE debug arch a design bug :)
- k
^ permalink raw reply
* Re: [PATCH] [POWERPC] Rework EXC_LEVEL_EXCEPTION_PROLOG code
From: Josh Boyer @ 2008-05-01 14:26 UTC (permalink / raw)
To: Kumar Gala; +Cc: Paul Mackerras, linuxppc-dev
In-Reply-To: <CF55CEE2-CAF5-4728-947E-C1C37E262EDB@kernel.crashing.org>
On Thu, 2008-05-01 at 08:22 -0500, Kumar Gala wrote:
> >> So we have 4 actual exceptions:
> >> * CriticalInput (some external device signaled this. There are two
> >> concepts of critical. One is error the other is high priority)
> >> However this would have the same caveats as any ExternalInput
> >> handler.
> >
> > No, it's worse. It can interrupt code that normally has
> > local_irq_disabled() and thus doesn't expect to be interrupted. That
> > means that everything becomes unsafe including locks etc....
>
> Fair. Should local_irq_disable() clear MSR_SE & MSR_BE on classic
> parts?
>
> > Note that driver that want to make active use of that probably want
> > some
> > explicit local_crit_irq_disable/enable functions to be able to
> > implement
> > some sort of synchronization.
>
> Or we could just have local_irq_disable -- clear MSR_EE and MSR_CE
That sort of defeats the purpose of using critical interrupts in the
"higher priority" sense, doesn't it? Seems it would effectively
eliminate that level altogether. And it would also mask off the
watchdog interrupt, which I don't think you want to do.
Ben suggested local_crit_irq_disable. That would suffice for drivers
that know they are configured as CE and need to do locking primitives,
but that isn't very common. On 4xx there isn't even an interface for a
driver to request it's interrupts be set to CE and we init all UICs to
have UIC_CR be 0.
So for 4xx at least, most of this discussion is theoretical for now.
However, I can see value in having CEs being used for certain devices,
etc so it would be nice to figure it out in the long run.
josh
^ permalink raw reply
* Re: [PATCH] [POWERPC] mpc5200: Allow for fixed speed MII configurations
From: Grant Likely @ 2008-05-01 14:33 UTC (permalink / raw)
To: wg, linuxppc-dev, paulus, netdev, wd, Domen Puncer,
Sylvain Munaut
In-Reply-To: <20080429230620.11248.25729.stgit@trillian.secretlab.ca>
On Tue, Apr 29, 2008 at 5:06 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
>
> Various improvements for configuring the MPC5200 MII link from the
> device tree:
> * Look for 'current-speed' property for fixed speed MII links
> * Look for 'fsl,7-wire-mode' property for boards using the 7 wire mode
> * move definition of private data structure out of the header file
>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Any more comments on this patch? I want to push it to Paulus, but I'd
like to have someone ack it first.
Wolfgang, you used the previous version of this patch. Does this one
work for you?
Cheers,
g.
> ---
>
> .../powerpc/mpc52xx-device-tree-bindings.txt | 11 ++
> drivers/net/fec_mpc52xx.c | 96 +++++++++++++++-----
> drivers/net/fec_mpc52xx.h | 19 ----
> 3 files changed, 85 insertions(+), 41 deletions(-)
>
> diff --git a/Documentation/powerpc/mpc52xx-device-tree-bindings.txt b/Documentation/powerpc/mpc52xx-device-tree-bindings.txt
> index cda7a7d..6f12f1c 100644
> --- a/Documentation/powerpc/mpc52xx-device-tree-bindings.txt
> +++ b/Documentation/powerpc/mpc52xx-device-tree-bindings.txt
> @@ -237,6 +237,17 @@ Each GPIO controller node should have the empty property gpio-controller and
> according to the bit numbers in the GPIO control registers. The second cell
> is for flags which is currently unsused.
>
> +8) FEC nodes
> +The FEC node can specify one of the following properties to configure
> +the MII link:
> +"fsl,7-wire-mode" - An empty property that specifies the link uses 7-wire
> + mode instead of MII
> +"current-speed" - Specifies that the MII should be configured for a fixed
> + speed. This property should contain two cells. The
> + first cell specifies the speed in Mbps and the second
> + should be '0' for half duplex and '1' for full duplex
> +"phy-handle" - Contains a phandle to an Ethernet PHY.
> +
> IV - Extra Notes
> ================
>
> diff --git a/drivers/net/fec_mpc52xx.c b/drivers/net/fec_mpc52xx.c
> index d21b7ab..eeb4433 100644
> --- a/drivers/net/fec_mpc52xx.c
> +++ b/drivers/net/fec_mpc52xx.c
> @@ -43,6 +43,29 @@
>
> #define DRIVER_NAME "mpc52xx-fec"
>
> +#define FEC5200_PHYADDR_NONE (-1)
> +#define FEC5200_PHYADDR_7WIRE (-2)
> +
> +/* Private driver data structure */
> +struct mpc52xx_fec_priv {
> + int duplex;
> + int speed;
> + int r_irq;
> + int t_irq;
> + struct mpc52xx_fec __iomem *fec;
> + struct bcom_task *rx_dmatsk;
> + struct bcom_task *tx_dmatsk;
> + spinlock_t lock;
> + int msg_enable;
> +
> + /* MDIO link details */
> + int phy_addr;
> + unsigned int phy_speed;
> + struct phy_device *phydev;
> + enum phy_state link;
> +};
> +
> +
> static irqreturn_t mpc52xx_fec_interrupt(int, void *);
> static irqreturn_t mpc52xx_fec_rx_interrupt(int, void *);
> static irqreturn_t mpc52xx_fec_tx_interrupt(int, void *);
> @@ -223,7 +246,7 @@ static int mpc52xx_fec_phy_start(struct net_device *dev)
> struct mpc52xx_fec_priv *priv = netdev_priv(dev);
> int err;
>
> - if (!priv->has_phy)
> + if (priv->phy_addr < 0)
> return 0;
>
> err = mpc52xx_fec_init_phy(dev);
> @@ -243,7 +266,7 @@ static void mpc52xx_fec_phy_stop(struct net_device *dev)
> {
> struct mpc52xx_fec_priv *priv = netdev_priv(dev);
>
> - if (!priv->has_phy)
> + if (!priv->phydev)
> return;
>
> phy_disconnect(priv->phydev);
> @@ -255,7 +278,7 @@ static void mpc52xx_fec_phy_stop(struct net_device *dev)
> static int mpc52xx_fec_phy_mii_ioctl(struct mpc52xx_fec_priv *priv,
> struct mii_ioctl_data *mii_data, int cmd)
> {
> - if (!priv->has_phy)
> + if (!priv->phydev)
> return -ENOTSUPP;
>
> return phy_mii_ioctl(priv->phydev, mii_data, cmd);
> @@ -265,7 +288,7 @@ static void mpc52xx_fec_phy_hw_init(struct mpc52xx_fec_priv *priv)
> {
> struct mpc52xx_fec __iomem *fec = priv->fec;
>
> - if (!priv->has_phy)
> + if (priv->phydev)
> return;
>
> out_be32(&fec->mii_speed, priv->phy_speed);
> @@ -704,7 +727,7 @@ static void mpc52xx_fec_start(struct net_device *dev)
> rcntrl = FEC_RX_BUFFER_SIZE << 16; /* max frame length */
> rcntrl |= FEC_RCNTRL_FCE;
>
> - if (priv->has_phy)
> + if (priv->phy_addr != FEC5200_PHYADDR_7WIRE)
> rcntrl |= FEC_RCNTRL_MII_MODE;
>
> if (priv->duplex == DUPLEX_FULL)
> @@ -864,7 +887,10 @@ mpc52xx_fec_probe(struct of_device *op, const struct of_device_id *match)
> struct net_device *ndev;
> struct mpc52xx_fec_priv *priv = NULL;
> struct resource mem;
> - const phandle *ph;
> + struct device_node *phy_node;
> + const phandle *phy_handle;
> + const u32 *prop;
> + int prop_size;
>
> phys_addr_t rx_fifo;
> phys_addr_t tx_fifo;
> @@ -950,24 +976,36 @@ mpc52xx_fec_probe(struct of_device *op, const struct of_device_id *match)
> priv->msg_enable = netif_msg_init(debug, MPC52xx_MESSAGES_DEFAULT);
> priv->duplex = DUPLEX_FULL;
>
> - /* is the phy present in device tree? */
> - ph = of_get_property(op->node, "phy-handle", NULL);
> - if (ph) {
> - const unsigned int *prop;
> - struct device_node *phy_dn;
> - priv->has_phy = 1;
> -
> - phy_dn = of_find_node_by_phandle(*ph);
> - prop = of_get_property(phy_dn, "reg", NULL);
> - priv->phy_addr = *prop;
> + /*
> + * Link mode configuration
> + */
>
> - of_node_put(phy_dn);
> + /* Start with safe defaults for link connection */
> + priv->phy_addr = FEC5200_PHYADDR_NONE;
> + priv->speed = 100;
> + priv->duplex = 0;
> + priv->phy_speed = ((mpc52xx_find_ipb_freq(op->node) >> 20) / 5) << 1;
> +
> + /* the 7-wire property means don't use MII mode */
> + if (of_find_property(op->node, "fsl,7-wire-mode", NULL))
> + priv->phy_addr = FEC5200_PHYADDR_7WIRE;
> +
> + /* The current speed preconfigures the speed of the MII link */
> + prop = of_get_property(op->node, "current-speed", &prop_size);
> + if (prop && (prop_size >= sizeof(u32) * 2)) {
> + priv->speed = prop[0];
> + priv->duplex = prop[1] ? DUPLEX_FULL : DUPLEX_HALF;
> + }
>
> - /* Phy speed */
> - priv->phy_speed = ((mpc52xx_find_ipb_freq(op->node) >> 20) / 5) << 1;
> - } else {
> - dev_info(&ndev->dev, "can't find \"phy-handle\" in device"
> - " tree, using 7-wire mode\n");
> + /* If there is a phy handle, setup link to that phy */
> + phy_handle = of_get_property(op->node, "phy-handle", &prop_size);
> + if (phy_handle && (prop_size >= sizeof(phandle))) {
> + phy_node = of_find_node_by_phandle(*phy_handle);
> + prop = of_get_property(phy_node, "reg", &prop_size);
> + if (prop && (prop_size >= sizeof(u32)))
> + if ((*prop >= 0) && (*prop < PHY_MAX_ADDR))
> + priv->phy_addr = *prop;
> + of_node_put(phy_node);
> }
>
> /* Hardware init */
> @@ -982,6 +1020,20 @@ mpc52xx_fec_probe(struct of_device *op, const struct of_device_id *match)
> if (rv < 0)
> goto probe_error;
>
> + /* Now report the link setup */
> + switch (priv->phy_addr) {
> + case FEC5200_PHYADDR_NONE:
> + dev_info(&ndev->dev, "Fixed speed MII link: %i%cD\n",
> + priv->speed, priv->duplex ? 'F' : 'H');
> + break;
> + case FEC5200_PHYADDR_7WIRE:
> + dev_info(&ndev->dev, "using 7-wire PHY mode\n");
> + break;
> + default:
> + dev_info(&ndev->dev, "Using PHY at MDIO address %i\n",
> + priv->phy_addr);
> + }
> +
> /* We're done ! */
> dev_set_drvdata(&op->dev, ndev);
>
> diff --git a/drivers/net/fec_mpc52xx.h b/drivers/net/fec_mpc52xx.h
> index 8b1f753..a227a52 100644
> --- a/drivers/net/fec_mpc52xx.h
> +++ b/drivers/net/fec_mpc52xx.h
> @@ -26,25 +26,6 @@
>
> #define FEC_WATCHDOG_TIMEOUT ((400*HZ)/1000)
>
> -struct mpc52xx_fec_priv {
> - int duplex;
> - int r_irq;
> - int t_irq;
> - struct mpc52xx_fec __iomem *fec;
> - struct bcom_task *rx_dmatsk;
> - struct bcom_task *tx_dmatsk;
> - spinlock_t lock;
> - int msg_enable;
> -
> - int has_phy;
> - unsigned int phy_speed;
> - unsigned int phy_addr;
> - struct phy_device *phydev;
> - enum phy_state link;
> - int speed;
> -};
> -
> -
> /* ======================================================================== */
> /* Hardware register sets & bits */
> /* ======================================================================== */
>
>
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH v2 4/6] [POWERPC] QE: implement support for the GPIO LIB API
From: Timur Tabi @ 2008-05-01 14:33 UTC (permalink / raw)
To: cbouatmailru; +Cc: linuxppc-dev
In-Reply-To: <20080430232430.GA14233@zarina>
Anton Vorontsov wrote:
> No. The spec indeed says that 0 for low, non-0 for high.
Fair enough.
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: [PATCH v2 4/6] [POWERPC] QE: implement support for the GPIO LIB API
From: Timur Tabi @ 2008-05-01 14:33 UTC (permalink / raw)
To: avorontsov; +Cc: linuxppc-dev
In-Reply-To: <20080430193040.GA9239@polina.dev.rtsoft.ru>
Anton Vorontsov wrote:
> This is needed to access QE GPIOs via Linux GPIO API.
>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
Acked-By: Timur Tabi <timur@freescale.com>
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: [PATCH] [POWERPC] Rework EXC_LEVEL_EXCEPTION_PROLOG code
From: Kumar Gala @ 2008-05-01 14:31 UTC (permalink / raw)
To: jwboyer; +Cc: Paul Mackerras, linuxppc-dev
In-Reply-To: <1209652012.2946.124.camel@vader.jdub.homelinux.org>
On May 1, 2008, at 9:26 AM, Josh Boyer wrote:
> On Thu, 2008-05-01 at 08:22 -0500, Kumar Gala wrote:
>>>> So we have 4 actual exceptions:
>>>> * CriticalInput (some external device signaled this. There are two
>>>> concepts of critical. One is error the other is high priority)
>>>> However this would have the same caveats as any ExternalInput
>>>> handler.
>>>
>>> No, it's worse. It can interrupt code that normally has
>>> local_irq_disabled() and thus doesn't expect to be interrupted. That
>>> means that everything becomes unsafe including locks etc....
>>
>> Fair. Should local_irq_disable() clear MSR_SE & MSR_BE on classic
>> parts?
>>
>>> Note that driver that want to make active use of that probably want
>>> some
>>> explicit local_crit_irq_disable/enable functions to be able to
>>> implement
>>> some sort of synchronization.
>>
>> Or we could just have local_irq_disable -- clear MSR_EE and MSR_CE
>
> That sort of defeats the purpose of using critical interrupts in the
> "higher priority" sense, doesn't it? Seems it would effectively
> eliminate that level altogether. And it would also mask off the
> watchdog interrupt, which I don't think you want to do.
>
> Ben suggested local_crit_irq_disable. That would suffice for drivers
> that know they are configured as CE and need to do locking primitives,
> but that isn't very common. On 4xx there isn't even an interface
> for a
> driver to request it's interrupts be set to CE and we init all UICs to
> have UIC_CR be 0.
>
> So for 4xx at least, most of this discussion is theoretical for now.
> However, I can see value in having CEs being used for certain devices,
> etc so it would be nice to figure it out in the long run.
I agree. my first priority is to get debug interrupts cleaned up so
we can kprobe and do other debug things in the kernel.
I'm only aware of one case of someone asking for critical inputs on
our parts w/linux.
- k
- k
^ permalink raw reply
* [PATCH] Updated: Reworked Cell OProfile: SPU mutex lock fix
From: Carl Love @ 2008-05-01 14:43 UTC (permalink / raw)
To: linuxppc-dev, cbe-oss-dev, linux-kernel, Arnd Bergmann,
oprofile-list
Sorry, looks like my mailer mangled the file.
This is a reworked patch to fix the SPU data storage. Currently, the
SPU escape sequences and program counter data is being added directly
into the kernel buffer without holding the buffer_mutex lock. This
patch changes how the data is stored. A new function,
oprofile_add_value, is added into the oprofile driver to allow adding
generic data to the per cpu buffers. This enables a series of calls
to the oprofile_add_value to enter the needed SPU escape sequences
and SPU program data into the kernel buffer via the per cpu buffers
without any additional processing. The oprofile_add_value function is
generic so it could be used by other architecures as well provided
the needed postprocessing was added to opreport.
Finally, this patch backs out the changes previously added to the
oprofile generic code for handling the architecture specific
ops.sync_start and ops.sync_stop that allowed the architecture
to skip the per CPU buffer creation.
Signed-off-by: Carl Love <carll@us.ibm.com>
Index: Cell_kernel_4_15_2008/arch/powerpc/oprofile/cell/pr_util.h
===================================================================
--- Cell_kernel_4_15_2008.orig/arch/powerpc/oprofile/cell/pr_util.h
+++ Cell_kernel_4_15_2008/arch/powerpc/oprofile/cell/pr_util.h
@@ -20,11 +20,6 @@
#include <asm/cell-regs.h>
#include <asm/spu.h>
-/* Defines used for sync_start */
-#define SKIP_GENERIC_SYNC 0
-#define SYNC_START_ERROR -1
-#define DO_GENERIC_SYNC 1
-
struct spu_overlay_info { /* map of sections within an SPU overlay */
unsigned int vma; /* SPU virtual memory address from elf */
unsigned int size; /* size of section from elf */
@@ -85,7 +80,7 @@ void stop_spu_profiling(void);
int spu_sync_start(void);
/* remove the hooks */
-int spu_sync_stop(void);
+void spu_sync_stop(void);
/* Record SPU program counter samples to the oprofile event buffer. */
void spu_sync_buffer(int spu_num, unsigned int *samples,
Index: Cell_kernel_4_15_2008/arch/powerpc/oprofile/cell/spu_task_sync.c
===================================================================
--- Cell_kernel_4_15_2008.orig/arch/powerpc/oprofile/cell/spu_task_sync.c
+++ Cell_kernel_4_15_2008/arch/powerpc/oprofile/cell/spu_task_sync.c
@@ -31,11 +31,12 @@
#define RELEASE_ALL 9999
-static DEFINE_SPINLOCK(buffer_lock);
+static DEFINE_SPINLOCK(add_value_lock);
static DEFINE_SPINLOCK(cache_lock);
static int num_spu_nodes;
int spu_prof_num_nodes;
int last_guard_val[MAX_NUMNODES * 8];
+static int spu_ctx_sw_seen[MAX_NUMNODES * 8];
/* Container for caching information about an active SPU task. */
struct cached_info {
@@ -289,6 +290,7 @@ static int process_context_switch(struct
int retval;
unsigned int offset = 0;
unsigned long spu_cookie = 0, app_dcookie;
+ int cpu_buf;
retval = prepare_cached_spu_info(spu, objectId);
if (retval)
@@ -303,17 +305,28 @@ static int process_context_switch(struct
goto out;
}
- /* Record context info in event buffer */
- spin_lock_irqsave(&buffer_lock, flags);
- add_event_entry(ESCAPE_CODE);
- add_event_entry(SPU_CTX_SWITCH_CODE);
- add_event_entry(spu->number);
- add_event_entry(spu->pid);
- add_event_entry(spu->tgid);
- add_event_entry(app_dcookie);
- add_event_entry(spu_cookie);
- add_event_entry(offset);
- spin_unlock_irqrestore(&buffer_lock, flags);
+ /* Record context info in event buffer. Note, there are 4x more
+ * SPUs then CPUs. Map the SPU events/data for a given SPU to
+ * the same CPU buffer. Need to ensure the cntxt switch data and
+ * samples stay in order.
+ */
+ cpu_buf = spu->number >> 2;
+ spin_lock_irqsave(&add_value_lock, flags);
+ oprofile_add_value(ESCAPE_CODE, cpu_buf);
+ oprofile_add_value(SPU_CTX_SWITCH_CODE, cpu_buf);
+ oprofile_add_value(spu->number, cpu_buf);
+ oprofile_add_value(spu->pid, cpu_buf);
+ oprofile_add_value(spu->tgid, cpu_buf);
+ oprofile_add_value(app_dcookie, cpu_buf);
+ oprofile_add_value(spu_cookie, cpu_buf);
+ oprofile_add_value(offset, cpu_buf);
+
+ /* Set flag to indicate SPU PC data can now be written out. If
+ * the SPU program counter data is seen before an SPU context
+ * record is seen, the postprocessing will fail.
+ */
+ spu_ctx_sw_seen[spu->number] = 1;
+ spin_unlock_irqrestore(&add_value_lock, flags);
smp_wmb(); /* insure spu event buffer updates are written */
/* don't want entries intermingled... */
out:
@@ -333,7 +346,6 @@ static int spu_active_notify(struct noti
unsigned long flags;
struct spu *the_spu = data;
- pr_debug("SPU event notification arrived\n");
if (!val) {
spin_lock_irqsave(&cache_lock, flags);
retval = release_cached_info(the_spu->number);
@@ -363,38 +375,38 @@ static int number_of_online_nodes(void)
/* The main purpose of this function is to synchronize
* OProfile with SPUFS by registering to be notified of
* SPU task switches.
- *
- * NOTE: When profiling SPUs, we must ensure that only
- * spu_sync_start is invoked and not the generic sync_start
- * in drivers/oprofile/oprof.c. A return value of
- * SKIP_GENERIC_SYNC or SYNC_START_ERROR will
- * accomplish this.
*/
int spu_sync_start(void)
{
int k;
- int ret = SKIP_GENERIC_SYNC;
+ int ret = 0;
int register_ret;
- unsigned long flags = 0;
+ int cpu;
spu_prof_num_nodes = number_of_online_nodes();
num_spu_nodes = spu_prof_num_nodes * 8;
- spin_lock_irqsave(&buffer_lock, flags);
- add_event_entry(ESCAPE_CODE);
- add_event_entry(SPU_PROFILING_CODE);
- add_event_entry(num_spu_nodes);
- spin_unlock_irqrestore(&buffer_lock, flags);
+ /* The SPU_PROFILING_CODE escape sequence must proceed
+ * the SPU context switch info.
+ */
+ for_each_online_cpu(cpu) {
+ oprofile_add_value(ESCAPE_CODE, cpu);
+ oprofile_add_value(SPU_PROFILING_CODE, cpu);
+ oprofile_add_value((unsigned long int)
+ num_spu_nodes, cpu);
+ }
/* Register for SPU events */
register_ret = spu_switch_event_register(&spu_active);
if (register_ret) {
- ret = SYNC_START_ERROR;
+ ret = -1;
goto out;
}
- for (k = 0; k < (MAX_NUMNODES * 8); k++)
+ for (k = 0; k < (MAX_NUMNODES * 8); k++) {
last_guard_val[k] = 0;
+ spu_ctx_sw_seen[k] = 0;
+ }
pr_debug("spu_sync_start -- running.\n");
out:
return ret;
@@ -432,7 +444,7 @@ void spu_sync_buffer(int spu_num, unsign
map = c_info->map;
the_spu = c_info->the_spu;
- spin_lock(&buffer_lock);
+ spin_lock(&add_value_lock);
for (i = 0; i < num_samples; i++) {
unsigned int sample = *(samples+i);
int grd_val = 0;
@@ -452,31 +464,38 @@ void spu_sync_buffer(int spu_num, unsign
break;
}
- add_event_entry(file_offset | spu_num_shifted);
+ /* We must ensure that the SPU context switch has been written
+ * out before samples for the SPU. Otherwise, the SPU context
+ * information is not available and the postprocessing of the
+ * SPU PC will fail with no available anonymous map information.
+ */
+ if (spu_ctx_sw_seen[spu_num])
+ oprofile_add_value((file_offset | spu_num_shifted),
+ (spu_num >> 2));
}
- spin_unlock(&buffer_lock);
+ spin_unlock(&add_value_lock);
out:
spin_unlock_irqrestore(&cache_lock, flags);
}
-int spu_sync_stop(void)
+void spu_sync_stop(void)
{
unsigned long flags = 0;
- int ret = spu_switch_event_unregister(&spu_active);
- if (ret) {
- printk(KERN_ERR "SPU_PROF: "
- "%s, line %d: spu_switch_event_unregister returned %d\n",
- __FUNCTION__, __LINE__, ret);
- goto out;
- }
+
+ /* Ignoring the return value from the unregister
+ * call. A failed return value simply says there
+ * was no registered event. Hence there will not
+ * be any calls to process a switch event that
+ * could cause a problem.
+ */
+ spu_switch_event_unregister(&spu_active);
spin_lock_irqsave(&cache_lock, flags);
- ret = release_cached_info(RELEASE_ALL);
+ release_cached_info(RELEASE_ALL);
spin_unlock_irqrestore(&cache_lock, flags);
-out:
pr_debug("spu_sync_stop -- done.\n");
- return ret;
+ return;
}
Index: Cell_kernel_4_15_2008/arch/powerpc/oprofile/op_model_cell.c
===================================================================
--- Cell_kernel_4_15_2008.orig/arch/powerpc/oprofile/op_model_cell.c
+++ Cell_kernel_4_15_2008/arch/powerpc/oprofile/op_model_cell.c
@@ -1191,15 +1191,15 @@ static int cell_sync_start(void)
if (spu_cycle_reset)
return spu_sync_start();
else
- return DO_GENERIC_SYNC;
+ return 0;
}
-static int cell_sync_stop(void)
+static void cell_sync_stop(void)
{
if (spu_cycle_reset)
- return spu_sync_stop();
- else
- return 1;
+ spu_sync_stop();
+
+ return;
}
struct op_powerpc_model op_model_cell = {
Index: Cell_kernel_4_15_2008/drivers/oprofile/buffer_sync.c
===================================================================
--- Cell_kernel_4_15_2008.orig/drivers/oprofile/buffer_sync.c
+++ Cell_kernel_4_15_2008/drivers/oprofile/buffer_sync.c
@@ -521,6 +521,20 @@ void sync_buffer(int cpu)
} else if (s->event == CPU_TRACE_BEGIN) {
state = sb_bt_start;
add_trace_begin();
+ } else if (s->event == VALUE_HEADER_ID) {
+ /* The next event contains a value
+ * to enter directly into the event buffer.
+ */
+ increment_tail(cpu_buf);
+ i++; /* one less entry in buffer to process */
+
+ s = &cpu_buf->buffer[cpu_buf->tail_pos];
+
+ if (unlikely(is_code(s->eip)))
+ add_event_entry(s->event);
+ else
+ printk("KERN_ERR Oprofile per CPU" \
+ "buffer sequence error.\n");
} else {
struct mm_struct * oldmm = mm;
Index: Cell_kernel_4_15_2008/drivers/oprofile/cpu_buffer.c
===================================================================
--- Cell_kernel_4_15_2008.orig/drivers/oprofile/cpu_buffer.c
+++ Cell_kernel_4_15_2008/drivers/oprofile/cpu_buffer.c
@@ -224,6 +224,29 @@ static void oprofile_end_trace(struct op
cpu_buf->tracing = 0;
}
+/*
+ * The first entry in the per cpu buffer consists of the escape code and
+ * the VALUE_HEADER_ID value. The next entry consists of an escape code
+ * with the value to store. The syn_buffer routine takes the value from
+ * the second entry and put it into the kernel buffer.
+ */
+void oprofile_add_value(unsigned long value, int cpu) {
+ struct oprofile_cpu_buffer * cpu_buf = &cpu_buffer[cpu];
+
+ /* Enter a sequence of two events. The first event says the
+ * next event contains a value that is to be put directly into the
+ * event buffer.
+ */
+
+ if (nr_available_slots(cpu_buf) < 3) {
+ cpu_buf->sample_lost_overflow++;
+ return;
+ }
+
+ add_sample(cpu_buf, ESCAPE_CODE, VALUE_HEADER_ID);
+ add_sample(cpu_buf, ESCAPE_CODE, value);
+}
+
void oprofile_add_ext_sample(unsigned long pc, struct pt_regs * const regs,
unsigned long event, int is_kernel)
{
Index: Cell_kernel_4_15_2008/drivers/oprofile/cpu_buffer.h
===================================================================
--- Cell_kernel_4_15_2008.orig/drivers/oprofile/cpu_buffer.h
+++ Cell_kernel_4_15_2008/drivers/oprofile/cpu_buffer.h
@@ -54,5 +54,6 @@ void cpu_buffer_reset(struct oprofile_cp
/* transient events for the CPU buffer -> event buffer */
#define CPU_IS_KERNEL 1
#define CPU_TRACE_BEGIN 2
+#define VALUE_HEADER_ID 3
#endif /* OPROFILE_CPU_BUFFER_H */
Index: Cell_kernel_4_15_2008/drivers/oprofile/oprof.c
===================================================================
--- Cell_kernel_4_15_2008.orig/drivers/oprofile/oprof.c
+++ Cell_kernel_4_15_2008/drivers/oprofile/oprof.c
@@ -53,24 +53,13 @@ int oprofile_setup(void)
* us missing task deaths and eventually oopsing
* when trying to process the event buffer.
*/
- if (oprofile_ops.sync_start) {
- int sync_ret = oprofile_ops.sync_start();
- switch (sync_ret) {
- case 0:
- goto post_sync;
- case 1:
- goto do_generic;
- case -1:
- goto out3;
- default:
- goto out3;
- }
- }
-do_generic:
+ if (oprofile_ops.sync_start
+ && ((err = oprofile_ops.sync_start())))
+ goto out2;
+
if ((err = sync_start()))
goto out3;
-post_sync:
is_setup = 1;
mutex_unlock(&start_mutex);
return 0;
@@ -133,20 +122,9 @@ out:
void oprofile_shutdown(void)
{
mutex_lock(&start_mutex);
- if (oprofile_ops.sync_stop) {
- int sync_ret = oprofile_ops.sync_stop();
- switch (sync_ret) {
- case 0:
- goto post_sync;
- case 1:
- goto do_generic;
- default:
- goto post_sync;
- }
- }
-do_generic:
+ if (oprofile_ops.sync_stop)
+ oprofile_ops.sync_stop();
sync_stop();
-post_sync:
if (oprofile_ops.shutdown)
oprofile_ops.shutdown();
is_setup = 0;
Index: Cell_kernel_4_15_2008/include/linux/oprofile.h
===================================================================
--- Cell_kernel_4_15_2008.orig/include/linux/oprofile.h
+++ Cell_kernel_4_15_2008/include/linux/oprofile.h
@@ -56,12 +56,10 @@ struct oprofile_operations {
/* Stop delivering interrupts. */
void (*stop)(void);
/* Arch-specific buffer sync functions.
- * Return value = 0: Success
- * Return value = -1: Failure
- * Return value = 1: Run generic sync function
+ * Sync start: Return 0 for Success, -1 for Failure
*/
int (*sync_start)(void);
- int (*sync_stop)(void);
+ void (*sync_stop)(void);
/* Initiate a stack backtrace. Optional. */
void (*backtrace)(struct pt_regs * const regs, unsigned int depth);
@@ -84,13 +82,6 @@ int oprofile_arch_init(struct oprofile_o
void oprofile_arch_exit(void);
/**
- * Add data to the event buffer.
- * The data passed is free-form, but typically consists of
- * file offsets, dcookies, context information, and ESCAPE codes.
- */
-void add_event_entry(unsigned long data);
-
-/**
* Add a sample. This may be called from any context. Pass
* smp_processor_id() as cpu.
*/
@@ -106,6 +97,17 @@ void oprofile_add_sample(struct pt_regs
void oprofile_add_ext_sample(unsigned long pc, struct pt_regs * const regs,
unsigned long event, int is_kernel);
+/*
+ * Add a value to the per CPU buffer. The value is passed from the per CPU
+ * buffer to the kernel buffer with no additional processing. The assumption
+ * is any processing of the value will be done in the postprocessor. This
+ * function should only be used for special architecture specific data.
+ * Currently only used by the CELL processor.
+ *
+ * This function does not perform a backtrace.
+ */
+void oprofile_add_value(unsigned long value, int cpu);
+
/* Use this instead when the PC value is not from the regs. Doesn't
* backtrace. */
void oprofile_add_pc(unsigned long pc, int is_kernel, unsigned long event);
Index: Cell_kernel_4_15_2008/include/asm-powerpc/oprofile_impl.h
===================================================================
--- Cell_kernel_4_15_2008.orig/include/asm-powerpc/oprofile_impl.h
+++ Cell_kernel_4_15_2008/include/asm-powerpc/oprofile_impl.h
@@ -48,7 +48,7 @@ struct op_powerpc_model {
void (*stop) (void);
void (*global_stop) (void);
int (*sync_start)(void);
- int (*sync_stop)(void);
+ void (*sync_stop)(void);
void (*handle_interrupt) (struct pt_regs *,
struct op_counter_config *);
int num_counters;
Index: Cell_kernel_4_15_2008/drivers/oprofile/event_buffer.h
===================================================================
--- Cell_kernel_4_15_2008.orig/drivers/oprofile/event_buffer.h
+++ Cell_kernel_4_15_2008/drivers/oprofile/event_buffer.h
@@ -17,6 +17,14 @@ int alloc_event_buffer(void);
void free_event_buffer(void);
+
+/**
+ * Add data to the event buffer.
+ * The data passed is free-form, but typically consists of
+ * file offsets, dcookies, context information, and ESCAPE codes.
+ */
+void add_event_entry(unsigned long data);
+
/* wake up the process sleeping on the event file */
void wake_up_buffer_waiter(void);
^ permalink raw reply
* Re: powerpc boot regression
From: Badari Pulavarty @ 2008-05-01 14:51 UTC (permalink / raw)
To: Tony Breeds; +Cc: linux-kernel, LinuxPPC-dev, y-goto, akpm, David Miller
In-Reply-To: <20080501051304.GM20457@bakeyournoodle.com>
On Thu, 2008-05-01 at 15:13 +1000, Tony Breeds wrote:
> On Tue, Apr 29, 2008 at 11:12:41PM -0700, David Miller wrote:
> >
> > This commit causes bootup failures on sparc64:
> >
> > commit 86f6dae1377523689bd8468fed2f2dd180fc0560
> > Author: Yasunori Goto <y-goto@jp.fujitsu.com>
> > Date: Mon Apr 28 02:13:33 2008 -0700
> >
> > memory hotplug: allocate usemap on the section with pgdat
>
>
> <snip>
>
> We're seeing a boot failure on powerpc. git bisect points the problem
> at this commit. However reverting just this one comitt doesn't fix the
> regression. I also needed to revert
> 04753278769f3b6c3b79a080edb52f21d83bf6e2 (memory hotplug: register
> section/node id to free")
>
> Problem seen on power4, power5 and ps3.
I don't see the problem on my power5 machine. Can you send the
config ? Is CONFIG_SPARSEMEM_VMEMMAP enabled ?
Please let me know.
Thanks,
Badari
>
> If the fix isn't trivial, can we get that patch series reverted?
>
> FWIW a boot looks like:
> zImage starting: loaded at 0x00400000 (sp: 0x02f5fea0)
> Allocating 0xa2d4c8 bytes for kernel ...
> OF version = 'IBM,SF240_332'
> gunzipping (0x01400000 <- 0x00407000:0x0079aa5d)...done 0x9a8ee0 bytes
>
> Linux/PowerPC load: root=/dev/sdc3 ro
> Finalizing device tree... using OF tree (promptr=02039a68)
> OF stdout device is: /vdevice/vty@30000000
> Hypertas detected, assuming LPAR !
> command line: root=/dev/sdc3 ro
> memory layout at init:
> alloc_bottom : 0000000001e32000
> alloc_top : 0000000008000000
> alloc_top_hi : 0000000080000000
> rmo_top : 0000000008000000
> ram_top : 0000000080000000
> Looking for displays
> instantiating rtas at 0x00000000076a1000 ... done
> 0000000000000000 : boot cpu 0000000000000000
> copying OF device tree ...
> Building dt strings...
> Building dt structure...
> Device tree strings 0x0000000001e33000 -> 0x0000000001e3417c
> Device tree struct 0x0000000001e35000 -> 0x0000000001e3d000
> Calling quiesce ...
> returning from prom_init
> Crash kernel location must be 0x2000000
> Reserving 0MB of memory at 32MB for crashkernel (System RAM: 2048MB)
> Using pSeries machine description
> console [udbg0] enabled
> Partition configured for 20 cpus.
> CPU maps initialized for 2 threads per core
> Starting Linux PPC64 #52 SMP Thu May 1 14:04:46 EST 2008
> -----------------------------------------------------
> ppc64_pft_size = 0x19
> physicalMemorySize = 0x80000000
> htab_hash_mask = 0x3ffff
> -----------------------------------------------------
> Initializing cgroup subsys cpuset
> Linux version 2.6.25 (tony@Sprygo) (gcc version 4.0.4 20060507 (prerelease) (Debian 4.0.3-3)) #52 SMP Thu May 1 14:04:46 EST 2008
> [boot]0012 Setup Arch
> sparse_early_usemap_alloc: allocation failed
> <snip 127 more>
> EEH: No capable adapters found
> PPC64 nvram contains 7168 bytes
> Zone PFN ranges:
> DMA 0 -> 524288
> Normal 524288 -> 524288
> Movable zone start PFN for each node
> early_node_map[1] active PFN ranges
> 0: 0 -> 524288
> [boot]0015 Setup Done
> Built 1 zonelists in Zone order, mobility grouping on. Total pages: 517120
> Kernel command line: root=/dev/sdc3 ro
> [boot]0020 XICS Init
> [boot]0021 XICS Done
> PID hash table entries: 4096 (order: 12, 32768 bytes)
> clocksource: timebase mult[1545815] shift[22] registered
> Console: colour dummy device 80x25
> console handover: boot [udbg0] -> real [hvc0]
> Dentry cache hash table entries: 262144 (order: 9, 2097152 bytes)
> Inode-cache hash table entries: 131072 (order: 8, 1048576 bytes)
> Unable to handle kernel paging request for data at address 0xcf00000000030858
> Faulting instruction address: 0xc0000000000c4530
> Oops: Kernel access of bad area, sig: 11 [#1]
> SMP NR_CPUS=32 pSeries
> Modules linked in:
> NIP: c0000000000c4530 LR: c0000000007e8790 CTR: 80000000001af404
> REGS: c000000000987ae0 TRAP: 0300 Not tainted (2.6.25)
> MSR: 8000000000009032 <EE,ME,IR,DR> CR: 24000088 XER: 20000002
> DAR: cf00000000030858, DSISR: 0000000040010000
> TASK = c000000000862650[0] 'swapper' THREAD: c000000000984000 CPU: 0
> GPR00: 0000000020000000 c000000000987d60 c000000000981f18 cf00000000030858
> GPR04: 0000000000000000 0000000000000001 0000000000000000 c0000000009f2a68
> GPR08: c0000000009f2a58 00000000000001b8 0000000000000000 cf00000000030858
> GPR12: 0000000000004000 c0000000009ae400 0000000000000000 0000000000000000
> GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000001
> GPR20: c00000007ffe8000 0000000000080000 0000000000000001 c0000000009f2848
> GPR24: cf00000000030200 0000000040000000 0000000000000dc0 000000000000001d
> GPR28: ffffffffe0000000 cf00000000030890 c0000000008e5088 0000000000000dde
> NIP [c0000000000c4530] .__free_pages_bootmem+0xc/0xa8
> LR [c0000000007e8790] .free_all_bootmem_core+0x140/0x218
> Call Trace:
> [c000000000987d60] [0000000001c0a438] 0x1c0a438 (unreliable)
> [c000000000987e40] [c0000000007d29c8] .mem_init+0x68/0x1b0
> [c000000000987ed0] [c0000000007c091c] .start_kernel+0x34c/0x45c
> [c000000000987f90] [c00000000000859c] .start_here_common+0x4c/0xb0
> Instruction dump:
> 780007c6 792907c6 7c630214 38000038 7863a302 7c6301d2 4d820020 7c634a14
> 4bffffa0 7c862379 7c6b1b78 40820028 <e8030000> 39200000 7800a842 78005800
> ---[ end trace 8640abe69a316dee ]---
> Kernel panic - not syncing: Attempted to kill the idle task!
> Rebooting in 180 seconds..
>
> Yours Tony
>
> linux.conf.au http://www.marchsouth.org/
> Jan 19 - 24 2009 The Australian Linux Technical Conference!
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [PATCH 1/2] i2c: Add support for device alias names
From: Kay Sievers @ 2008-05-01 15:51 UTC (permalink / raw)
To: Jean Delvare; +Cc: linuxppc-dev list, Paul Mundt, Scott Wood, Linux I2C
In-Reply-To: <20080501100409.1b04fdb7@hyperion.delvare>
On Thu, 2008-05-01 at 10:04 +0200, Jean Delvare wrote:
> On Mon, 28 Apr 2008 18:16:17 +0200, Kay Sievers wrote:
> > On Mon, 2008-04-28 at 17:40 +0200, Jean Delvare wrote:
> > > Why would i2c device modaliases ever contain multiple strings? A devi=
ce
> > > can't have multiple names, can it?
> >=20
> > Like ACPI/PNP devices, which can have several compat id's, which means
> > that a single device can have "multiple names":
> > $ cat /sys/bus/pnp/devices/00:09/id
> > IBM0057
> > PNP0f13
>=20
> Ah, I didn't know about this. Now I'm curious how it can work. Does it
> mean that several drivers attempt to bind to this device?=20
=EF=BB=BFThey are usually all id's for the same type of device, and don't d=
escribe
multiple functions at the same time. In most cases the vendor id's, like
"IBM0057" here, do not match anything.
> > > Can't we just stop handle_moddevtable() from adding a tailing "*"
> > > automatically, and just let the device types which need it, add it on
> > > their own?
> >=20
> > For a lot subsystems it's fine to have it appended, as there is a
> > defined list of identifiers, which must appear in the same order, and
> > new identifiers are appended to the end. So the "*" still matches
> > modules with possibly extended modalias strings.
>=20
> I understand the logic, however I am skeptical how useful it is in
> practice. If we add an identifier to the device aliases, then we also
> update the corresponding modalias, so no in-tree driver can break. The
> only case where it makes a difference, as far as I can see, is for
> out-of-tree drivers. Am I correct?
=EF=BB=BFThat sounds correct, yes.
> On top of that, I doubt that we
> actually add new identifiers that frequently, do we?
Not that I know.
> > We would also need to review all buses which export modalias, if they
> > need the "*" or not, and add them by hand, if needed.
> >=20
> > I guess, it's easier to introduce an additional parameter to
> > file2alias::do_table() and suppress the trailing "*" for i2c?
>=20
> That's one possibility, but I had a slightly different approach, which
> is to just let the type-specific handlers add the trailing "*" by
> themselves if they need it. This allows for optimization in a few
> cases.
>=20
> Subject: modpost: i2c aliases need no trailing wildcard
>=20
> Not all device type aliases need a trailing wildcard, in particular
> the i2c aliases don't. Don't add a wildcard by default in do_table(),
> instead let each device type handler add it if needed.
...
> The patch only changes the i2c aliases, all the rest is the same as
> before (unless I messed up somewhere, that is.) Do you think this would
> be acceptable for upstream? If you think it's better to add a parameter
> to do_table() and let it add the "*" as it did so far, that's also fine
> with me, I can update the patch to do that.
Looks fine to me.
If the content of:
/lib/modules/$(uname -r)/modules.alias
looks correct after the patch, this should go in, I think.
Thanks,
Kay
^ permalink raw reply
* Re: [PATCH] [POWERPC] Rework EXC_LEVEL_EXCEPTION_PROLOG code
From: Scott Wood @ 2008-05-01 16:14 UTC (permalink / raw)
To: Kumar Gala; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <2DF41779-732B-4477-9DAC-147CDC5C9885@kernel.crashing.org>
On Thu, May 01, 2008 at 08:17:07AM -0500, Kumar Gala wrote:
> On May 1, 2008, at 3:24 AM, Paul Mackerras wrote:
> >The most common cases are (a) something that ultimately generates
> >input on a tty (e.g. a character arriving on a serial port) and that
> >input turns out to be a ^C or similar, or (b) something that signals
> >I/O completion and the program doing the I/O has requested
> >notification by a SIGIO. But in general any driver code can send a
> >signal to userspace if it wants.
And, of course, SIGALRM and similar timer mechanisms.
> ok. Was just wondering how the async exception know that the signal
> it wanted to send belonged to the particular process that is running.
> But I guess there are cases that the signal is really intended for who
> ever is currently running?
No, it knows based on its own data structures who it's intended for --
and sometimes that happens to be the currently running process.
-Scott
^ permalink raw reply
* Re: [PATCH] [POWERPC] Rework EXC_LEVEL_EXCEPTION_PROLOG code
From: Scott Wood @ 2008-05-01 16:22 UTC (permalink / raw)
To: Kumar Gala; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <20080501161451.GH17142@ld0162-tx32.am.freescale.net>
On Thu, May 01, 2008 at 11:14:51AM -0500, Scott Wood wrote:
> On Thu, May 01, 2008 at 08:17:07AM -0500, Kumar Gala wrote:
> > ok. Was just wondering how the async exception know that the signal
> > it wanted to send belonged to the particular process that is running.
> > But I guess there are cases that the signal is really intended for who
> > ever is currently running?
>
> No, it knows based on its own data structures who it's intended for --
> and sometimes that happens to be the currently running process.
Oh, except for things like SIGXCPU and SIGVTALRM, which do target the
currently running process.
-Scott
^ permalink raw reply
* Re: [PATCH] Updated: Reworked Cell OProfile: SPU mutex lock fix
From: Maynard Johnson @ 2008-05-01 16:29 UTC (permalink / raw)
To: Carl Love
Cc: linuxppc-dev, oprofile-list, cbe-oss-dev, Arnd Bergmann,
linux-kernel
In-Reply-To: <1209652994.17842.1.camel@carll-linux-desktop>
Carl Love wrote:
> Sorry, looks like my mailer mangled the file.
>
> This is a reworked patch to fix the SPU data storage. Currently, the
> SPU escape sequences and program counter data is being added directly
> into the kernel buffer without holding the buffer_mutex lock. This
> patch changes how the data is stored. A new function,
> oprofile_add_value, is added into the oprofile driver to allow adding
> generic data to the per cpu buffers. This enables a series of calls
> to the oprofile_add_value to enter the needed SPU escape sequences
> and SPU program data into the kernel buffer via the per cpu buffers
> without any additional processing. The oprofile_add_value function is
> generic so it could be used by other architecures as well provided
> the needed postprocessing was added to opreport.
>
> Finally, this patch backs out the changes previously added to the
> oprofile generic code for handling the architecture specific
> ops.sync_start and ops.sync_stop that allowed the architecture
> to skip the per CPU buffer creation.
>
> Signed-off-by: Carl Love <carll@us.ibm.com>
>
Looks fine to me. All of my previous review comments have been addressed.
-Maynard
[snip]
^ permalink raw reply
* Re: [PATCH] [POWERPC] Rework EXC_LEVEL_EXCEPTION_PROLOG code
From: Kumar Gala @ 2008-05-01 16:33 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <20080501161451.GH17142@ld0162-tx32.am.freescale.net>
On May 1, 2008, at 11:14 AM, Scott Wood wrote:
> On Thu, May 01, 2008 at 08:17:07AM -0500, Kumar Gala wrote:
>> On May 1, 2008, at 3:24 AM, Paul Mackerras wrote:
>>> The most common cases are (a) something that ultimately generates
>>> input on a tty (e.g. a character arriving on a serial port) and that
>>> input turns out to be a ^C or similar, or (b) something that signals
>>> I/O completion and the program doing the I/O has requested
>>> notification by a SIGIO. But in general any driver code can send a
>>> signal to userspace if it wants.
>
> And, of course, SIGALRM and similar timer mechanisms.
>
>> ok. Was just wondering how the async exception know that the signal
>> it wanted to send belonged to the particular process that is running.
>> But I guess there are cases that the signal is really intended for
>> who
>> ever is currently running?
>
> No, it knows based on its own data structures who it's intended for --
> and sometimes that happens to be the currently running process.
Let me ask the question differently. Are there cases that some event
occurs in the system and a signal is delivered to the current process
regardless of what that process is. I'm guessing so, based on the tty
example.
So for the specific case I'm looking at (kprobes & debug exceptions
from kernel space), I think its reasonable to BUG_ON() if thread_info-
>flags changes such that TIF_SIGPENDING or TIF_NEED_RESCHED get set
we aren't from user-space.
- k
^ permalink raw reply
* Re: [PATCH] [POWERPC] mpc5200: Allow for fixed speed MII configurations
From: Wolfgang Grandegger @ 2008-05-01 16:38 UTC (permalink / raw)
To: Grant Likely; +Cc: netdev, linuxppc-dev, paulus, Domen Puncer
In-Reply-To: <fa686aa40805010733o6bb5b522hd16669b732f1892@mail.gmail.com>
Hi Grant,
Grant Likely wrote:
> On Tue, Apr 29, 2008 at 5:06 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
>> From: Grant Likely <grant.likely@secretlab.ca>
>>
>> Various improvements for configuring the MPC5200 MII link from the
>> device tree:
>> * Look for 'current-speed' property for fixed speed MII links
>> * Look for 'fsl,7-wire-mode' property for boards using the 7 wire mode
>> * move definition of private data structure out of the header file
>>
>> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
>
> Any more comments on this patch? I want to push it to Paulus, but I'd
> like to have someone ack it first.
>
> Wolfgang, you used the previous version of this patch. Does this one
> work for you?
Sorry for the late answer. The patch works fine (under Linux 2.6.24) on
my board with a 3-port Micrel ethernet switch. There is still a minor
issue, though:
[...deletions...]
>> diff --git a/drivers/net/fec_mpc52xx.c b/drivers/net/fec_mpc52xx.c
>> index d21b7ab..eeb4433 100644
>> --- a/drivers/net/fec_mpc52xx.c
>> +++ b/drivers/net/fec_mpc52xx.c
[...deletions...]
>> @@ -950,24 +976,36 @@ mpc52xx_fec_probe(struct of_device *op, const struct of_device_id *match)
>> priv->msg_enable = netif_msg_init(debug, MPC52xx_MESSAGES_DEFAULT);
>> priv->duplex = DUPLEX_FULL;
More concerning this line below.
>>
>> - /* is the phy present in device tree? */
>> - ph = of_get_property(op->node, "phy-handle", NULL);
>> - if (ph) {
>> - const unsigned int *prop;
>> - struct device_node *phy_dn;
>> - priv->has_phy = 1;
>> -
>> - phy_dn = of_find_node_by_phandle(*ph);
>> - prop = of_get_property(phy_dn, "reg", NULL);
>> - priv->phy_addr = *prop;
>> + /*
>> + * Link mode configuration
>> + */
>>
>> - of_node_put(phy_dn);
>> + /* Start with safe defaults for link connection */
>> + priv->phy_addr = FEC5200_PHYADDR_NONE;
>> + priv->speed = 100;
>> + priv->duplex = 0;
priv->duplex is re-defined here. And instead of "0" we should use
DUPLEX_HALF.
Wolfgang.
^ permalink raw reply
* Re: [PATCH] [POWERPC] Rework EXC_LEVEL_EXCEPTION_PROLOG code
From: Scott Wood @ 2008-05-01 16:42 UTC (permalink / raw)
To: Kumar Gala; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <78125A27-D99A-447D-9C65-4705DD153567@kernel.crashing.org>
On Thu, May 01, 2008 at 11:33:34AM -0500, Kumar Gala wrote:
> Let me ask the question differently. Are there cases that some event
> occurs in the system and a signal is delivered to the current process
> regardless of what that process is.
Yes, timers that expire based on CPU usage.
> I'm guessing so, based on the tty example.
I don't think tty interrupts would -- it'd go to the process group
associated with the tty.
> So for the specific case I'm looking at (kprobes & debug exceptions
> from kernel space), I think its reasonable to BUG_ON() if thread_info-
> >flags changes such that TIF_SIGPENDING or TIF_NEED_RESCHED get set
> we aren't from user-space.
Why? It may not happen currently, but it seems more future-proof to just
copy the flags. And it's certainly not reasonable for normal interrupts.
-Scott
^ permalink raw reply
* Re: [PATCH] [POWERPC] mpc5200: Allow for fixed speed MII configurations
From: Grant Likely @ 2008-05-01 16:52 UTC (permalink / raw)
To: Wolfgang Grandegger; +Cc: netdev, linuxppc-dev, paulus, Domen Puncer
In-Reply-To: <4819F1F3.2000406@grandegger.com>
On Thu, May 1, 2008 at 10:38 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
> Hi Grant,
>
>
> Grant Likely wrote:
> > On Tue, Apr 29, 2008 at 5:06 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> >> From: Grant Likely <grant.likely@secretlab.ca>
> >>
> >> Various improvements for configuring the MPC5200 MII link from the
> >> device tree:
> >> * Look for 'current-speed' property for fixed speed MII links
> >> * Look for 'fsl,7-wire-mode' property for boards using the 7 wire mode
> >> * move definition of private data structure out of the header file
> >>
> >> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> >
> > Any more comments on this patch? I want to push it to Paulus, but I'd
> > like to have someone ack it first.
> >
> > Wolfgang, you used the previous version of this patch. Does this one
> > work for you?
>
> Sorry for the late answer. The patch works fine (under Linux 2.6.24) on
> my board with a 3-port Micrel ethernet switch. There is still a minor
> issue, though:
>
> >> - of_node_put(phy_dn);
> >> + /* Start with safe defaults for link connection */
> >> + priv->phy_addr = FEC5200_PHYADDR_NONE;
> >> + priv->speed = 100;
> >> + priv->duplex = 0;
>
> priv->duplex is re-defined here. And instead of "0" we should use
> DUPLEX_HALF.
Oops,
Fixed.
If you reply with your 'acked-by' line, then I'll push this one to
Paul so it can get into .26
Thanks,
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH] [POWERPC] mpc5200: Allow for fixed speed MII configurations
From: Wolfgang Grandegger @ 2008-05-01 16:59 UTC (permalink / raw)
To: Grant Likely; +Cc: netdev, linuxppc-dev, paulus, Domen Puncer
In-Reply-To: <fa686aa40805010952t58af0071rc308956e9fb13fbf@mail.gmail.com>
Grant Likely wrote:
> On Thu, May 1, 2008 at 10:38 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> Hi Grant,
>>
>>
>> Grant Likely wrote:
>> > On Tue, Apr 29, 2008 at 5:06 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
>> >> From: Grant Likely <grant.likely@secretlab.ca>
>> >>
>> >> Various improvements for configuring the MPC5200 MII link from the
>> >> device tree:
>> >> * Look for 'current-speed' property for fixed speed MII links
>> >> * Look for 'fsl,7-wire-mode' property for boards using the 7 wire mode
>> >> * move definition of private data structure out of the header file
>> >>
>> >> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
>> >
>> > Any more comments on this patch? I want to push it to Paulus, but I'd
>> > like to have someone ack it first.
>> >
>> > Wolfgang, you used the previous version of this patch. Does this one
>> > work for you?
>>
>> Sorry for the late answer. The patch works fine (under Linux 2.6.24) on
>> my board with a 3-port Micrel ethernet switch. There is still a minor
>> issue, though:
>>
>> >> - of_node_put(phy_dn);
>> >> + /* Start with safe defaults for link connection */
>> >> + priv->phy_addr = FEC5200_PHYADDR_NONE;
>> >> + priv->speed = 100;
>> >> + priv->duplex = 0;
>>
>> priv->duplex is re-defined here. And instead of "0" we should use
>> DUPLEX_HALF.
>
> Oops,
>
> Fixed.
>
> If you reply with your 'acked-by' line, then I'll push this one to
> Paul so it can get into .26
Yes, of course. Add
Acked-by: Wolfgang Grandegger <wg@grandegger.com>
please.
Wolfgang.
^ permalink raw reply
* Re: [PATCH] [POWERPC] Rework EXC_LEVEL_EXCEPTION_PROLOG code
From: Kumar Gala @ 2008-05-01 17:48 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <20080501164228.GA17992@ld0162-tx32.am.freescale.net>
On May 1, 2008, at 11:42 AM, Scott Wood wrote:
> On Thu, May 01, 2008 at 11:33:34AM -0500, Kumar Gala wrote:
>> Let me ask the question differently. Are there cases that some event
>> occurs in the system and a signal is delivered to the current process
>> regardless of what that process is.
>
> Yes, timers that expire based on CPU usage.
>
>> I'm guessing so, based on the tty example.
>
> I don't think tty interrupts would -- it'd go to the process group
> associated with the tty.
>
>> So for the specific case I'm looking at (kprobes & debug exceptions
>> from kernel space), I think its reasonable to BUG_ON() if
>> thread_info-
>>> flags changes such that TIF_SIGPENDING or TIF_NEED_RESCHED get set
>> we aren't from user-space.
>
> Why? It may not happen currently, but it seems more future-proof to
> just
> copy the flags. And it's certainly not reasonable for normal
> interrupts.
copying the flags isn't the issue. Its acting on the flags thats the
problem. I'm not 100% sure the C code that might clear the flags is
consistent on how it access them. So if one bit of code clears
task_struct->stack->thread_info->flags and other clears
thread_info(STACK)->flags we get into an issue on how to merge after
that.
I'd rather be safe for now since we don't need to send a signal in the
debug from kernel case.
- k
^ permalink raw reply
* Please pull linux-2.6-mpc52xx.git
From: Grant Likely @ 2008-05-01 18:04 UTC (permalink / raw)
To: Paul Mackerras, shengping.liu, linuxppc-dev
Paul, here's a couple more for 2.6.26. One bug fix and one minor
change that I've had on the back burner for a month or so. This
should be the last of any non-bugfix changes through my tree.
Cheers,
g.
git-request-pull powerpc git://git.secretlab.ca/git/linux-2.6-mpc52xx.git
The following changes since commit eabd90944b3a00766e84da3d117ea0f3e0a3b1a3:
Michael Ellerman (1):
[POWERPC] Fix crashkernel= handling when no crashkernel= specified
are available in the git repository at:
git://git.secretlab.ca/git/linux-2.6-mpc52xx.git for-2.6.26
Andrew Liu (1):
Fix a potential issue in mpc52xx uart driver
Grant Likely (1):
[POWERPC] mpc5200: Allow for fixed speed MII configurations
.../powerpc/mpc52xx-device-tree-bindings.txt | 11 ++
drivers/net/fec_mpc52xx.c | 97 +++++++++++++++-----
drivers/net/fec_mpc52xx.h | 19 ----
drivers/serial/mpc52xx_uart.c | 2 +
4 files changed, 87 insertions(+), 42 deletions(-)
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH] Fix a potential issue in mpc52xx uart driver
From: Grant Likely @ 2008-05-01 18:07 UTC (permalink / raw)
To: Andrew Liu; +Cc: linuxppc-dev
In-Reply-To: <4816CFF9.5030505@windriver.com>
On Tue, Apr 29, 2008 at 1:36 AM, Andrew Liu <shengping.liu@windriver.com> wrote:
> mpc52xx_uart_int and __uart_put_char both try to acquire the
> port->lock. Therefore the function sequence of:
>
> mpc52xx_uart_int--> ...-->flush_to_ldisc-->...-->__uart_put_char
>
> can potentially trigger a deadlock. To avoid this deadlock a fix
> similar to that found in the 8250.c serial driver is applied. The
> deadlock is avoided by releasing the lock before pushing a buffer
> and reacquiring it when completed.
>
> Signed-off-by: Andrew Liu <shengping.liu@windriver.com>
Your patch is whitespace damaged and does not apply (tabs have been
converted to spaces). It was trivial so I've applied it manually, but
please be careful when posting future patches.
Thanks,
g.
>
> diff --git a/drivers/serial/mpc52xx_uart.c b/drivers/serial/mpc52xx_uart.c
> index d93b357..5f95e53 100644
> --- a/drivers/serial/mpc52xx_uart.c
> +++ b/drivers/serial/mpc52xx_uart.c
> @@ -783,7 +783,9 @@ mpc52xx_uart_int_rx_chars(struct uart_port *port)
> }
> }
>
> + spin_unlock(&port->lock);
> tty_flip_buffer_push(tty);
> + spin_lock(&port->lock);
>
> return psc_ops->raw_rx_rdy(port);
> }
>
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ 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