* Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: free irq before irq_dispose_mapping()
From: Lars-Peter Clausen @ 2014-12-01 16:59 UTC (permalink / raw)
To: Timur Tabi, Markus Pargmann, Jiada Wang
Cc: alsa-devel, tiwai, broonie, lgirdwood, linux-kernel, nicoleotsuka,
Li.Xiubo, linuxppc-dev
In-Reply-To: <547C9CAD.1000008@tabi.org>
On 12/01/2014 05:51 PM, Timur Tabi wrote:
> On 12/01/2014 10:49 AM, Lars-Peter Clausen wrote:
>
>> The driver creates the mapping by calling irq_of_parse_and_map(), so it
>> also has to dispose the mapping. But the easy way out is to simply use
>> platform_get_irq() instead of irq_of_parse_map(). In this case the
>> mapping is not managed by the device but by the of core, so the device
>> has not to dispose the mapping.
>
> Is this a problem unique to the SSI driver? Maybe devm_free_irq() should
> also dispose of the mapping?
>
If the mapping was not created by the device, the device shouldn't dispose
it. Mapping and requesting the interrupt are two independent operations.
^ permalink raw reply
* Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: free irq before irq_dispose_mapping()
From: Lars-Peter Clausen @ 2014-12-01 16:49 UTC (permalink / raw)
To: Markus Pargmann, Jiada Wang
Cc: alsa-devel, tiwai, linux-kernel, broonie, lgirdwood, timur,
nicoleotsuka, Li.Xiubo, linuxppc-dev
In-Reply-To: <20141201065046.GB27289@pengutronix.de>
On 12/01/2014 07:50 AM, Markus Pargmann wrote:
[...]
>
> devm_request_irq() is used by other drivers too, this should not be a
> problem. Looking at the code it seems that irq_dispose_mapping may not
> be necessary with devm_request_irq(). So I think it would be better to
> remove irq_dispose_mapping() instead.
The driver creates the mapping by calling irq_of_parse_and_map(), so it also
has to dispose the mapping. But the easy way out is to simply use
platform_get_irq() instead of irq_of_parse_map(). In this case the mapping
is not managed by the device but by the of core, so the device has not to
dispose the mapping.
- Lars
^ permalink raw reply
* Re: [PATCH] i2c: mpc: add register documentation to Freescale I2C driver
From: Wolfram Sang @ 2014-12-01 17:21 UTC (permalink / raw)
To: Danielle Costantino; +Cc: linuxppc-dev, linux-i2c
In-Reply-To: <547A4192.30102@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 586 bytes --]
On Sat, Nov 29, 2014 at 01:58:42PM -0800, Danielle Costantino wrote:
> i2c: mpc: add register documentation to Freescale I2C driver
This should be in one patch.
>
> return -ETIMEDOUT for all time-out error conditions
This should be in a seperate patch.
> and warn on arbitration lost.
And this should be dropped. "Arbitration lost" lost is not an error, it
is specified behaviour of I2C.
> if (!(cmd_err & CSR_MCF)) {
> - dev_dbg(i2c->dev, "unfinished\n");
> + dev_warn(i2c->dev, "unfinished\n");
Are you sure this helps a regular user?
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH] i2c-qoriq: modified compatibility for correct prescaler
From: Wolfram Sang @ 2014-12-01 17:23 UTC (permalink / raw)
To: Danielle Costantino
Cc: Valentin Longchamp, Linux device trees, Boschung, Rainer,
Brunck, Holger, Linux I2C, Scott Wood, Linux PowerPC Kernel
In-Reply-To: <CAAVjN7fJyOh64p5UGRmv-UTnF47cERDfbHyHTuzQmQj8xTDEOQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 446 bytes --]
> I saw that this patch was marked as not applicable, but on most qoriq
> devices the pre-scaler is 2 especially for p2020/p2010 devices
> arch/powerpc/boot/dts/fsl/p2020si-post.dtsi
Just for completeness: "Not applicable" given from patchwork of the i2c
subsystem means this patch is not for the i2c subsystem. In this case,
it is for powerpc because it was modifying powerpc dts files only. That
doesn't say anything about the patch itself.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 2/2]powerpc: rewrite local_* to use CR5 flag
From: Gabriel Paubert @ 2014-12-01 18:01 UTC (permalink / raw)
To: Madhavan Srinivasan; +Cc: rusty, paulus, anton, linuxppc-dev
In-Reply-To: <1417090721-25298-3-git-send-email-maddy@linux.vnet.ibm.com>
On Thu, Nov 27, 2014 at 05:48:41PM +0530, Madhavan Srinivasan wrote:
> This patch re-write the current local_* functions to CR5 based one.
> Base flow for each function is
>
> {
> set cr5(eq)
> load
> ..
> store
> clear cr5(eq)
> }
>
> Above set of instructions are followed by a fixup section which points
> to the entry of the function incase of interrupt in the flow. If the
> interrupt happens to be after the store, we just continue to last
> instruction in that block.
>
> Currently only asm/local.h has been rewrite, and local64 is TODO.
> Also the entire change is only for PPC64.
>
> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/asm/local.h | 306 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 306 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/local.h b/arch/powerpc/include/asm/local.h
> index b8da913..a26e5d3 100644
> --- a/arch/powerpc/include/asm/local.h
> +++ b/arch/powerpc/include/asm/local.h
> @@ -11,6 +11,310 @@ typedef struct
>
> #define LOCAL_INIT(i) { ATOMIC_LONG_INIT(i) }
>
> +#ifdef CONFIG_PPC64
> +
> +static __inline__ long local_read(local_t *l)
> +{
> + long t;
> +
> + __asm__ __volatile__(
> +"1: crset 22\n"
> +"2:" PPC_LL" %0,0(%1)\n"
> +"3: crclr 22\n"
> +"4:\n"
> +" .section __ex_table,\"a\"\n"
> + PPC_LONG_ALIGN "\n"
> + PPC_LONG "2b,1b\n"
> + PPC_LONG "3b,3b\n"
> +" .previous\n"
> + : "=&r" (t)
> + : "r" (&(l->a.counter)));
> +
> + return t;
> +}
> +
> +static __inline__ void local_set(local_t *l, long i)
> +{
> + long t;
> +
> + __asm__ __volatile__(
> +"1: crset 22\n"
> +"2:" PPC_LL" %0,0(%1)\n"
> +"3:" PPC405_ERR77(0,%2)
> +"4:" PPC_STL" %0,0(%2)\n"
> +"5: crclr 22\n"
> +"6:\n"
> +" .section __ex_table,\"a\"\n"
> + PPC_LONG_ALIGN "\n"
> + PPC_LONG "2b,1b\n"
> + PPC_LONG "3b,1b\n"
> + PPC_LONG "4b,1b\n"
> + PPC_LONG "5b,5b\n"
> +" .previous\n"
> + : "=&r" (t)
> + : "r" (&(i)), "r" (&(l->a.counter)));
> +}
> +
Apart from the other comments on bloat which can very likely
be removed by tracing backwards for a few instructions, removing
the exception table entries which are 2 or 4 (64 bit?) times as
large as the instruction sequence, I don't understand at all why
you need these sequences for the local_read and local_set functions.
After all these are single instructions (why do you perform a read before
the write in set when the result of the read is never used?).
I believe read and set are better mapped to access_once (or assign_once
or whatever it's called after the recent discussion on linux-kernel).
You don't even need a memory barrier if it's for a single thread,
so you could get away with a single volatile access to the variables.
For the other ones, I think that what you do is correct, except that
the workaround for PPC405 erratum 77 is not needed since this erratum
only affects the stwcx. instruction and the whole point of the patch
is to avoid the use of an l?arx/st?cx. pair.
Regards,
Gabriel
> +static __inline__ void local_add(long i, local_t *l)
> +{
> + long t;
> +
> + __asm__ __volatile__(
> +"1: crset 22\n"
> +"2:" PPC_LL" %0,0(%2)\n"
> +"3: add %0,%1,%0\n"
> +"4:" PPC405_ERR77(0,%2)
> +"5:" PPC_STL" %0,0(%2)\n"
> +"6: crclr 22\n"
> +"7:\n"
> +" .section __ex_table,\"a\"\n"
> + PPC_LONG_ALIGN "\n"
> + PPC_LONG "2b,1b\n"
> + PPC_LONG "3b,1b\n"
> + PPC_LONG "4b,1b\n"
> + PPC_LONG "5b,1b\n"
> + PPC_LONG "6b,6b\n"
> +" .previous\n"
> + : "=&r" (t)
> + : "r" (i), "r" (&(l->a.counter)));
> +}
> +
> +static __inline__ void local_sub(long i, local_t *l)
> +{
> + long t;
> +
> + __asm__ __volatile__(
> +"1: crset 22\n"
> +"2:" PPC_LL" %0,0(%2)\n"
> +"3: subf %0,%1,%0\n"
> +"4:" PPC405_ERR77(0,%2)
> +"5:" PPC_STL" %0,0(%2)\n"
> +"6: crclr 22\n"
> +"7:\n"
> +" .section __ex_table,\"a\"\n"
> + PPC_LONG_ALIGN "\n"
> + PPC_LONG "2b,1b\n"
> + PPC_LONG "3b,1b\n"
> + PPC_LONG "4b,1b\n"
> + PPC_LONG "5b,1b\n"
> + PPC_LONG "6b,6b\n"
> +" .previous\n"
> + : "=&r" (t)
> + : "r" (i), "r" (&(l->a.counter)));
> +}
> +
> +static __inline__ long local_add_return(long a, local_t *l)
> +{
> + long t;
> +
> + __asm__ __volatile__(
> +"1: crset 22\n"
> +"2:" PPC_LL" %0,0(%2)\n"
> +"3: add %0,%1,%0\n"
> +"4:" PPC405_ERR77(0,%2)
> +"5:" PPC_STL "%0,0(%2)\n"
> +"6: crclr 22\n"
> +"7:\n"
> +" .section __ex_table,\"a\"\n"
> + PPC_LONG_ALIGN "\n"
> + PPC_LONG "2b,1b\n"
> + PPC_LONG "3b,1b\n"
> + PPC_LONG "4b,1b\n"
> + PPC_LONG "5b,1b\n"
> + PPC_LONG "6b,6b\n"
> +" .previous\n"
> + : "=&r" (t)
> + : "r" (a), "r" (&(l->a.counter))
> + : "cc", "memory");
> +
> + return t;
> +}
> +
> +
> +#define local_add_negative(a, l) (local_add_return((a), (l)) < 0)
> +
> +static __inline__ long local_sub_return(long a, local_t *l)
> +{
> + long t;
> +
> + __asm__ __volatile__(
> +"1: crset 22\n"
> +"2:" PPC_LL" %0,0(%2)\n"
> +"3: subf %0,%1,%0\n"
> +"4:" PPC405_ERR77(0,%2)
> +"5:" PPC_STL "%0,0(%2)\n"
> +"6: crclr 22\n"
> +"7:\n"
> +" .section __ex_table,\"a\"\n"
> + PPC_LONG_ALIGN "\n"
> + PPC_LONG "2b,1b\n"
> + PPC_LONG "3b,1b\n"
> + PPC_LONG "4b,1b\n"
> + PPC_LONG "5b,1b\n"
> + PPC_LONG "6b,6b\n"
> +" .previous\n"
> + : "=&r" (t)
> + : "r" (a), "r" (&(l->a.counter))
> + : "cc", "memory");
> +
> + return t;
> +}
> +
> +static __inline__ long local_inc_return(local_t *l)
> +{
> + long t;
> +
> + __asm__ __volatile__(
> +"1: crset 22\n"
> +"2:" PPC_LL" %0,0(%1)\n"
> +"3: addic %0,%0,1\n"
> +"4:" PPC405_ERR77(0,%1)
> +"5:" PPC_STL "%0,0(%1)\n"
> +"6: crclr 22\n"
> +"7:\n"
> +" .section __ex_table,\"a\"\n"
> + PPC_LONG_ALIGN "\n"
> + PPC_LONG "2b,1b\n"
> + PPC_LONG "3b,1b\n"
> + PPC_LONG "4b,1b\n"
> + PPC_LONG "5b,1b\n"
> + PPC_LONG "6b,6b\n"
> +" .previous"
> + : "=&r" (t)
> + : "r" (&(l->a.counter))
> + : "cc", "xer", "memory");
> +
> + return t;
> +}
> +
> +/*
> + * local_inc_and_test - increment and test
> + * @l: pointer of type local_t
> + *
> + * Atomically increments @l by 1
> + * and returns true if the result is zero, or false for all
> + * other cases.
> + */
> +#define local_inc_and_test(l) (local_inc_return(l) == 0)
> +
> +static __inline__ long local_dec_return(local_t *l)
> +{
> + long t;
> +
> + __asm__ __volatile__(
> +"1: crset 22\n"
> +"2:" PPC_LL" %0,0(%1)\n"
> +"3: addic %0,%0,-1\n"
> +"4:" PPC405_ERR77(0,%1)
> +"5:" PPC_STL "%0,0(%1)\n"
> +"6: crclr 22\n"
> +"7:\n"
> +" .section __ex_table,\"a\"\n"
> + PPC_LONG_ALIGN "\n"
> + PPC_LONG "2b,1b\n"
> + PPC_LONG "3b,1b\n"
> + PPC_LONG "4b,1b\n"
> + PPC_LONG "5b,1b\n"
> + PPC_LONG "6b,6b\n"
> +" .previous\n"
> + : "=&r" (t)
> + : "r" (&(l->a.counter))
> + : "cc", "xer", "memory");
> +
> + return t;
> +}
> +
> +#define local_inc(l) local_inc_return(l)
> +#define local_dec(l) local_dec_return(l)
> +
> +#define local_cmpxchg(l, o, n) \
> + (cmpxchg_local(&((l)->a.counter), (o), (n)))
> +#define local_xchg(l, n) (xchg_local(&((l)->a.counter), (n)))
> +
> +/**
> + * local_add_unless - add unless the number is a given value
> + * @l: pointer of type local_t
> + * @a: the amount to add to v...
> + * @u: ...unless v is equal to u.
> + *
> + * Atomically adds @a to @l, so long as it was not @u.
> + * Returns non-zero if @l was not @u, and zero otherwise.
> + */
> +static __inline__ int local_add_unless(local_t *l, long a, long u)
> +{
> + long t;
> +
> + __asm__ __volatile__ (
> +"1: crset 22\n"
> +"2:" PPC_LL" %0,0(%1)\n"
> +"3: cmpw 0,%0,%3 \n"
> +"4: beq- 9f \n"
> +"5: add %0,%2,%0 \n"
> +"6:" PPC405_ERR77(0,%1)
> +"7:" PPC_STL" %0,0(%1) \n"
> +"8: subf %0,%2,%0 \n"
> +"9: crclr 22\n"
> +"10:\n"
> +" .section __ex_table,\"a\"\n"
> + PPC_LONG_ALIGN "\n"
> + PPC_LONG "2b,1b\n"
> + PPC_LONG "3b,1b\n"
> + PPC_LONG "4b,1b\n"
> + PPC_LONG "5b,1b\n"
> + PPC_LONG "6b,1b\n"
> + PPC_LONG "7b,1b\n"
> + PPC_LONG "8b,8b\n"
> + PPC_LONG "9b,9b\n"
> +" .previous\n"
> + : "=&r" (t)
> + : "r" (&(l->a.counter)), "r" (a), "r" (u)
> + : "cc", "memory");
> +
> + return t != u;
> +}
> +
> +#define local_inc_not_zero(l) local_add_unless((l), 1, 0)
> +
> +#define local_sub_and_test(a, l) (local_sub_return((a), (l)) == 0)
> +#define local_dec_and_test(l) (local_dec_return((l)) == 0)
> +
> +/*
> + * Atomically test *l and decrement if it is greater than 0.
> + * The function returns the old value of *l minus 1.
> + */
> +static __inline__ long local_dec_if_positive(local_t *l)
> +{
> + long t;
> +
> + __asm__ __volatile__(
> +"1: crset 22\n"
> +"2:" PPC_LL" %0,0(%1)\n"
> +"3: cmpwi %0,1\n"
> +"4: addi %0,%0,-1\n"
> +"5: blt- 8f\n"
> +"6:" PPC405_ERR77(0,%1)
> +"7:" PPC_STL "%0,0(%1)\n"
> +"8: crclr 22\n"
> +"9:\n"
> +" .section__ex_table,\"a\"\n"
> + PPC_LONG_ALIGN "\n"
> + PPC_LONG "2b,1b\n"
> + PPC_LONG "3b,1b\n"
> + PPC_LONG "4b,1b\n"
> + PPC_LONG "5b,1b\n"
> + PPC_LONG "6b,1b\n"
> + PPC_LONG "7b,1b\n"
> + PPC_LONG "8b,8b\n"
> +" .previous\n"
> + : "=&b" (t)
> + : "r" (&(l->a.counter))
> + : "cc", "memory");
> +
> + return t;
> +}
> +
> +#else
> +
> #define local_read(l) atomic_long_read(&(l)->a)
> #define local_set(l,i) atomic_long_set(&(l)->a, (i))
>
> @@ -162,6 +466,8 @@ static __inline__ long local_dec_if_positive(local_t *l)
> return t;
> }
>
> +#endif
> +
> /* Use these for per-cpu local_t variables: on some archs they are
> * much more efficient than these naive implementations. Note they take
> * a variable, not an address.
> --
> 1.9.1
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: free irq before irq_dispose_mapping()
From: Timur Tabi @ 2014-12-01 18:48 UTC (permalink / raw)
To: Lars-Peter Clausen, Markus Pargmann, Jiada Wang
Cc: alsa-devel, tiwai, broonie, lgirdwood, linux-kernel, nicoleotsuka,
Li.Xiubo, linuxppc-dev
In-Reply-To: <547C9C34.9080602@metafoo.de>
On 12/01/2014 10:49 AM, Lars-Peter Clausen wrote:
> The driver creates the mapping by calling irq_of_parse_and_map(), so it
> also has to dispose the mapping.
I agree with Markus, this does seem weird. It sounds like you're saying
that irq_of_parse_and_map() and devm_request_irq() are incompatible. A
quick grep shows the following drivers that call both functions:
ata/pata_mpc52xx.c
built-in.o
cpufreq/exynos5440-cpufreq.c
crypto/omap-sham.c
dma/moxart-dma.c
edac/mpc85xx_edac.c
hsi/clients/nokia-modem.c
i2c/busses/i2c-wmt.c
input/serio/apbps2.c
mmc/host/omap_hsmmc.c
mmc/host/moxart-mmc.c
mtd/nand/mpc5121_nfc.c
net/ethernet/arc/emac_main.c
net/ethernet/moxa/moxart_ether.c
pci/host/pcie-rcar.c
pinctrl/samsung/pinctrl-exynos5440.c
pinctrl/samsung/pinctrl-exynos.c
pinctrl/pinctrl-bcm2835.c
spi/spi-bcm2835.c
spi/spi-mpc512x-psc.c
staging/xillybus/xillybus_of.c
thermal/samsung/exynos_tmu.c
^ permalink raw reply
* Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: free irq before irq_dispose_mapping()
From: Mark Brown @ 2014-12-01 19:24 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: alsa-devel, Jiada Wang, linux-kernel, lgirdwood, timur,
nicoleotsuka, tiwai, Li.Xiubo, Markus Pargmann, linuxppc-dev
In-Reply-To: <547C9C34.9080602@metafoo.de>
[-- Attachment #1: Type: text/plain, Size: 859 bytes --]
On Mon, Dec 01, 2014 at 05:49:56PM +0100, Lars-Peter Clausen wrote:
> On 12/01/2014 07:50 AM, Markus Pargmann wrote:
> >devm_request_irq() is used by other drivers too, this should not be a
> >problem. Looking at the code it seems that irq_dispose_mapping may not
> >be necessary with devm_request_irq(). So I think it would be better to
> >remove irq_dispose_mapping() instead.
> The driver creates the mapping by calling irq_of_parse_and_map(), so it also
> has to dispose the mapping. But the easy way out is to simply use
> platform_get_irq() instead of irq_of_parse_map(). In this case the mapping
> is not managed by the device but by the of core, so the device has not to
> dispose the mapping.
It also has the advantage of not being DT specific so providing some
chance that future firmware interfaces can be supported without driver
modification.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply
* Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: free irq before irq_dispose_mapping()
From: Lars-Peter Clausen @ 2014-12-01 19:39 UTC (permalink / raw)
To: Timur Tabi, Markus Pargmann, Jiada Wang
Cc: alsa-devel, tiwai, broonie, lgirdwood, linux-kernel, nicoleotsuka,
Li.Xiubo, linuxppc-dev
In-Reply-To: <547CB817.10806@tabi.org>
On 12/01/2014 07:48 PM, Timur Tabi wrote:
> On 12/01/2014 10:49 AM, Lars-Peter Clausen wrote:
>> The driver creates the mapping by calling irq_of_parse_and_map(), so it
>> also has to dispose the mapping.
>
> I agree with Markus, this does seem weird. It sounds like you're saying
> that irq_of_parse_and_map() and devm_request_irq() are incompatible.
They probably are. You have to create the mapping before you request the IRQ
and if devm_request_irq() is used the IRQ is only freed again after the
remove function of the driver been called. Yet if a driver creates a mapping
in its probe function it should also dispose it in its remove function. So
you are stuck with either freeing the mapping before freeing the IRQ or
leaking the mapping.
My opinion on this is that devices should not create mappings and should
leave that to the core. This quite easily solves the dilemma.
> A quick grep shows the following drivers that call both functions:
>
Most of these drivers will probably work fine without irq_of_parse_and_map().
> ata/pata_mpc52xx.c
> built-in.o
> cpufreq/exynos5440-cpufreq.c
> crypto/omap-sham.c
> dma/moxart-dma.c
> edac/mpc85xx_edac.c
> hsi/clients/nokia-modem.c
> i2c/busses/i2c-wmt.c
> input/serio/apbps2.c
> mmc/host/omap_hsmmc.c
> mmc/host/moxart-mmc.c
> mtd/nand/mpc5121_nfc.c
> net/ethernet/arc/emac_main.c
> net/ethernet/moxa/moxart_ether.c
> pci/host/pcie-rcar.c
> pinctrl/samsung/pinctrl-exynos5440.c
> pinctrl/samsung/pinctrl-exynos.c
> pinctrl/pinctrl-bcm2835.c
> spi/spi-bcm2835.c
> spi/spi-mpc512x-psc.c
> staging/xillybus/xillybus_of.c
> thermal/samsung/exynos_tmu.c
>
^ permalink raw reply
* Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: free irq before irq_dispose_mapping()
From: Mark Brown @ 2014-12-01 19:41 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: alsa-devel, Jiada Wang, linux-kernel, Timur Tabi, lgirdwood,
nicoleotsuka, tiwai, Li.Xiubo, Markus Pargmann, linuxppc-dev
In-Reply-To: <547CC407.30308@metafoo.de>
[-- Attachment #1: Type: text/plain, Size: 412 bytes --]
On Mon, Dec 01, 2014 at 08:39:51PM +0100, Lars-Peter Clausen wrote:
> On 12/01/2014 07:48 PM, Timur Tabi wrote:
> >A quick grep shows the following drivers that call both functions:
> Most of these drivers will probably work fine without irq_of_parse_and_map().
I'd also note that quite a few of these drivers look pretty legacy - a
very large proportion are for old PowerPC hardware, though by no means
all.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply
* Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: free irq before irq_dispose_mapping()
From: Arnd Bergmann @ 2014-12-01 19:56 UTC (permalink / raw)
To: linuxppc-dev
Cc: alsa-devel, Lars-Peter Clausen, lgirdwood, Jiada Wang, Li.Xiubo,
Timur Tabi, linux-kernel, nicoleotsuka, tiwai, Mark Brown,
Markus Pargmann
In-Reply-To: <20141201194147.GP7712@sirena.org.uk>
On Monday 01 December 2014 19:41:47 Mark Brown wrote:
> On Mon, Dec 01, 2014 at 08:39:51PM +0100, Lars-Peter Clausen wrote:
> > On 12/01/2014 07:48 PM, Timur Tabi wrote:
>
> > >A quick grep shows the following drivers that call both functions:
>
> > Most of these drivers will probably work fine without irq_of_parse_and_map().
>
> I'd also note that quite a few of these drivers look pretty legacy - a
> very large proportion are for old PowerPC hardware, though by no means
> all.
Right, from the times before we were using platform_device for probing
device tree based devices and they had to map the interrupt themselves.
Some of them like arch/powerpc/sysdev/fsl_pci.c seem fine, this one
is does not expect to ever destroy a device, and it only unmaps the
interrupt if request_irq fails. drivers/ata/pata_mpc52xx.c on the
other hand seems wrong in the same was as drivers/edac/mpc85xx_edac.c
and sound/soc/fsl/fsl_ssi.c.
All other drivers that call irq_of_parse_and_map and pass that into
devm_request_irq just never unmap, and their interrupts are already
mapped by the platform code, so I think it's not even a leak.
Arnd
^ permalink raw reply
* Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: free irq before irq_dispose_mapping()
From: Timur Tabi @ 2014-12-01 19:59 UTC (permalink / raw)
To: Arnd Bergmann, linuxppc-dev
Cc: alsa-devel, Lars-Peter Clausen, Jiada Wang, Li.Xiubo, lgirdwood,
linux-kernel, nicoleotsuka, tiwai, Mark Brown, Markus Pargmann
In-Reply-To: <2547588.Qc4F2IfZke@wuerfel>
On 12/01/2014 01:56 PM, Arnd Bergmann wrote:
> All other drivers that call irq_of_parse_and_map and pass that into
> devm_request_irq just never unmap, and their interrupts are already
> mapped by the platform code, so I think it's not even a leak.
Does this mean that fsl_ssi.c should not be calling
irq_of_parse_and_map? How else should it get the IRQ?
^ permalink raw reply
* Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: free irq before irq_dispose_mapping()
From: Arnd Bergmann @ 2014-12-01 20:01 UTC (permalink / raw)
To: Timur Tabi
Cc: alsa-devel, Lars-Peter Clausen, Jiada Wang, Li.Xiubo,
linux-kernel, lgirdwood, nicoleotsuka, tiwai, Mark Brown,
Markus Pargmann, linuxppc-dev
In-Reply-To: <547CC89F.1030205@tabi.org>
On Monday 01 December 2014 13:59:27 Timur Tabi wrote:
> On 12/01/2014 01:56 PM, Arnd Bergmann wrote:
> > All other drivers that call irq_of_parse_and_map and pass that into
> > devm_request_irq just never unmap, and their interrupts are already
> > mapped by the platform code, so I think it's not even a leak.
>
> Does this mean that fsl_ssi.c should not be calling
> irq_of_parse_and_map? How else should it get the IRQ?
platform_get_irq()
Arnd
^ permalink raw reply
* Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: free irq before irq_dispose_mapping()
From: Timur Tabi @ 2014-12-01 20:11 UTC (permalink / raw)
To: Arnd Bergmann
Cc: alsa-devel, Lars-Peter Clausen, Jiada Wang, Li.Xiubo,
linux-kernel, lgirdwood, nicoleotsuka, tiwai, Mark Brown,
Markus Pargmann, linuxppc-dev
In-Reply-To: <3401240.VsRpsiV8WN@wuerfel>
On 12/01/2014 02:01 PM, Arnd Bergmann wrote:
>> >Does this mean that fsl_ssi.c should not be calling
>> >irq_of_parse_and_map? How else should it get the IRQ?
> platform_get_irq()
Ok, but that function also calls irq_create_of_mapping(). So it still
appears that the only way to get the IRQ is to map it, but then we can't
use devm_request_irq().
^ permalink raw reply
* Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: free irq before irq_dispose_mapping()
From: Mark Brown @ 2014-12-01 20:16 UTC (permalink / raw)
To: Arnd Bergmann
Cc: alsa-devel, Lars-Peter Clausen, lgirdwood, Jiada Wang, Timur Tabi,
linux-kernel, nicoleotsuka, tiwai, Li.Xiubo, Markus Pargmann,
linuxppc-dev
In-Reply-To: <3401240.VsRpsiV8WN@wuerfel>
[-- Attachment #1: Type: text/plain, Size: 758 bytes --]
On Mon, Dec 01, 2014 at 09:01:43PM +0100, Arnd Bergmann wrote:
> On Monday 01 December 2014 13:59:27 Timur Tabi wrote:
> > On 12/01/2014 01:56 PM, Arnd Bergmann wrote:
> > > All other drivers that call irq_of_parse_and_map and pass that into
> > > devm_request_irq just never unmap, and their interrupts are already
> > > mapped by the platform code, so I think it's not even a leak.
> > Does this mean that fsl_ssi.c should not be calling
> > irq_of_parse_and_map? How else should it get the IRQ?
> platform_get_irq()
Right, and just to emphasize what we were saying earlier the code was
fine when originally written - both mapping inside platform_get_irq()
and devm_ came along quite a while after the driver was originally
written.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply
* Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: free irq before irq_dispose_mapping()
From: Lars-Peter Clausen @ 2014-12-01 20:30 UTC (permalink / raw)
To: Timur Tabi, Arnd Bergmann
Cc: alsa-devel, Jiada Wang, Li.Xiubo, linux-kernel, lgirdwood,
nicoleotsuka, tiwai, Mark Brown, Markus Pargmann, linuxppc-dev
In-Reply-To: <547CCB78.5090007@tabi.org>
On 12/01/2014 09:11 PM, Timur Tabi wrote:
> On 12/01/2014 02:01 PM, Arnd Bergmann wrote:
>>> >Does this mean that fsl_ssi.c should not be calling
>>> >irq_of_parse_and_map? How else should it get the IRQ?
>> platform_get_irq()
>
> Ok, but that function also calls irq_create_of_mapping(). So it still
> appears that the only way to get the IRQ is to map it, but then we can't use
> devm_request_irq().
>
Hm... that's new. But it's not really a driver issue anymore if it is done
in the core. So I guess for now just use platform_get_irq() and ignore the
other issue.
^ permalink raw reply
* Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: free irq before irq_dispose_mapping()
From: Fabio Estevam @ 2014-12-01 20:40 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: alsa-devel@alsa-project.org, Arnd Bergmann, Liam Girdwood,
Jiada Wang, Mark Brown, Timur Tabi, linux-kernel, Nicolin Chen,
Takashi Iwai, Xiubo Li, Markus Pargmann,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <547CCFF7.2000205@metafoo.de>
On Mon, Dec 1, 2014 at 6:30 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 12/01/2014 09:11 PM, Timur Tabi wrote:
>>
>> On 12/01/2014 02:01 PM, Arnd Bergmann wrote:
>>>>
>>>> >Does this mean that fsl_ssi.c should not be calling
>>>> >irq_of_parse_and_map? How else should it get the IRQ?
>>>
>>> platform_get_irq()
>>
>>
>> Ok, but that function also calls irq_create_of_mapping(). So it still
>> appears that the only way to get the IRQ is to map it, but then we can't
>> use
>> devm_request_irq().
>>
>
> Hm... that's new. But it's not really a driver issue anymore if it is done
> in the core. So I guess for now just use platform_get_irq() and ignore the
> other issue.
With the suggested changes below, the removal of the driver works fine on a mx6:
root@freescale /$ modprobe snd-soc-fsl-ssi
root@freescale /$ modprobe snd-soc-imx-wm8962
[ 319.517679] input: WM8962 Beep Generator as
/devices/soc0/soc/2100000.aips-bus/21a0000.i2c/i2c-0/0-001a/input/input7
[ 319.543225] imx-wm8962 sound: wm8962 <-> 202c000.ssi mapping ok
root@freescale /$ rmmod snd-soc-imx-wm8962
root@freescale /$ rmmod snd-soc-fsl-ssi
sound/soc/fsl/fsl_ssi.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 32a31d9..c528f16 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -1361,7 +1361,7 @@ static int fsl_ssi_probe(struct platform_device *pdev)
return PTR_ERR(ssi_private->regs);
}
- ssi_private->irq = irq_of_parse_and_map(np, 0);
+ ssi_private->irq = platform_get_irq(pdev, 0);
if (!ssi_private->irq) {
dev_err(&pdev->dev, "no irq for node %s\n", np->full_name);
return -ENXIO;
@@ -1387,7 +1387,7 @@ static int fsl_ssi_probe(struct platform_device *pdev)
if (ssi_private->soc->imx) {
ret = fsl_ssi_imx_probe(pdev, ssi_private, iomem);
if (ret)
- goto error_irqmap;
+ return ret;
}
ret = snd_soc_register_component(&pdev->dev, &fsl_ssi_component,
@@ -1458,10 +1458,6 @@ error_asoc_register:
if (ssi_private->soc->imx)
fsl_ssi_imx_clean(pdev, ssi_private);
-error_irqmap:
- if (ssi_private->use_dma)
- irq_dispose_mapping(ssi_private->irq);
-
return ret;
}
@@ -1478,9 +1474,6 @@ static int fsl_ssi_remove(struct platform_device *pdev)
if (ssi_private->soc->imx)
fsl_ssi_imx_clean(pdev, ssi_private);
- if (ssi_private->use_dma)
- irq_dispose_mapping(ssi_private->irq);
-
return 0;
}
--
1.9.1
^ permalink raw reply related
* Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: free irq before irq_dispose_mapping()
From: Timur Tabi @ 2014-12-01 20:42 UTC (permalink / raw)
To: Fabio Estevam, Lars-Peter Clausen
Cc: alsa-devel@alsa-project.org, Arnd Bergmann, Jiada Wang,
Mark Brown, linux-kernel, Liam Girdwood, Nicolin Chen,
Takashi Iwai, Xiubo Li, Markus Pargmann,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <CAOMZO5B7MxLSaETUW0QyajdHMMX2nLg=t=5dVCNRePb6UzizRQ@mail.gmail.com>
On 12/01/2014 02:40 PM, Fabio Estevam wrote:
>> >Hm... that's new. But it's not really a driver issue anymore if it is done
>> >in the core. So I guess for now just use platform_get_irq() and ignore the
>> >other issue.
> With the suggested changes below, the removal of the driver works fine on a mx6:
Would the mapping continue to exist after the driver is unloaded? Can
you try multiple loads/unloads and see if interrupts still work?
^ permalink raw reply
* Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: free irq before irq_dispose_mapping()
From: Fabio Estevam @ 2014-12-01 21:03 UTC (permalink / raw)
To: Timur Tabi
Cc: alsa-devel@alsa-project.org, Lars-Peter Clausen, Arnd Bergmann,
Jiada Wang, Mark Brown, linux-kernel, Liam Girdwood, Nicolin Chen,
Takashi Iwai, Xiubo Li, Markus Pargmann,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <547CD2C6.2060107@tabi.org>
On Mon, Dec 1, 2014 at 6:42 PM, Timur Tabi <timur@tabi.org> wrote:
> Would the mapping continue to exist after the driver is unloaded? Can you
> try multiple loads/unloads and see if interrupts still work?
I tried multiple loads/unloads and audio works fine with those changes.
About the ssi irq we have:
- With the ssi driver loaded:
root@freescale /home$ cat /proc/interrupts | grep ssi
79: 0 0 0 0 GIC 79 202c000.ssi
- After removing the ssi driver:
root@freescale /home$ rmmod snd-soc-fsl-ssi
root@freescale /home$ cat /proc/interrupts | grep ssi
root@freescale /home$
,so it seems to behave properly.
^ permalink raw reply
* Re: [PATCH v2] slab: Fix nodeid bounds check for non-contiguous node IDs
From: David Rientjes @ 2014-12-01 21:06 UTC (permalink / raw)
To: Paul Mackerras
Cc: Andrew Morton, linuxppc-dev, linux-kernel, Pekka Enberg, linux-mm,
Christoph Lameter, Joonsoo Kim
In-Reply-To: <20141201042844.GB11234@drongo>
On Mon, 1 Dec 2014, Paul Mackerras wrote:
> The bounds check for nodeid in ____cache_alloc_node gives false
> positives on machines where the node IDs are not contiguous, leading
> to a panic at boot time. For example, on a POWER8 machine the node
> IDs are typically 0, 1, 16 and 17. This means that num_online_nodes()
> returns 4, so when ____cache_alloc_node is called with nodeid = 16 the
> VM_BUG_ON triggers, like this:
>
> kernel BUG at /home/paulus/kernel/kvm/mm/slab.c:3079!
> Oops: Exception in kernel mode, sig: 5 [#1]
> SMP NR_CPUS=1024 NUMA PowerNV
> Modules linked in:
> CPU: 0 PID: 0 Comm: swapper Not tainted 3.18.0-rc5-kvm+ #17
> task: c0000000013ba230 ti: c000000001494000 task.ti: c000000001494000
> NIP: c000000000264f6c LR: c000000000264f5c CTR: 0000000000000000
> REGS: c0000000014979a0 TRAP: 0700 Not tainted (3.18.0-rc5-kvm+)
> MSR: 9000000002021032 <SF,HV,VEC,ME,IR,DR,RI> CR: 28000448 XER: 20000000
> CFAR: c00000000047e978 SOFTE: 0
> GPR00: c000000000264f5c c000000001497c20 c000000001499d48 0000000000000004
> GPR04: 0000000000000100 0000000000000010 0000000000000068 ffffffffffffffff
> GPR08: 0000000000000000 0000000000000001 00000000082d0000 c000000000cca5a8
> GPR12: 0000000048000448 c00000000fda0000 000001003bd44ff0 0000000010020578
> GPR16: 000001003bd44ff8 000001003bd45000 0000000000000001 0000000000000000
> GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000010
> GPR24: c000000ffe000080 c000000000c824ec 0000000000000068 c000000ffe000080
> GPR28: 0000000000000010 c000000ffe000080 0000000000000010 0000000000000000
> NIP [c000000000264f6c] .____cache_alloc_node+0x6c/0x270
> LR [c000000000264f5c] .____cache_alloc_node+0x5c/0x270
> Call Trace:
> [c000000001497c20] [c000000000264f5c] .____cache_alloc_node+0x5c/0x270 (unreliable)
> [c000000001497cf0] [c00000000026552c] .kmem_cache_alloc_node_trace+0xdc/0x360
> [c000000001497dc0] [c000000000c824ec] .init_list+0x3c/0x128
> [c000000001497e50] [c000000000c827b4] .kmem_cache_init+0x1dc/0x258
> [c000000001497ef0] [c000000000c54090] .start_kernel+0x2a0/0x568
> [c000000001497f90] [c000000000008c6c] start_here_common+0x20/0xa8
> Instruction dump:
> 7c7d1b78 7c962378 4bda4e91 60000000 3c620004 38800100 386370d8 48219959
> 60000000 7f83e000 7d301026 5529effe <0b090000> 393c0010 79291f24 7d3d4a14
>
> To fix this, we instead compare the nodeid with MAX_NUMNODES, and
> additionally make sure it isn't negative (since nodeid is an int).
> The check is there mainly to protect the array dereference in the
> get_node() call in the next line, and the array being dereferenced is
> of size MAX_NUMNODES. If the nodeid is in range but invalid (for
> example if the node is off-line), the BUG_ON in the next line will
> catch that.
>
> Signed-off-by: Paul Mackerras <paulus@samba.org>
Acked-by: David Rientjes <rientjes@google.com>
^ permalink raw reply
* Re: [RFC PATCH 1/2]powerpc: foundation code to handle CR5 for local_t
From: Gabriel Paubert @ 2014-12-01 21:35 UTC (permalink / raw)
To: Madhavan Srinivasan; +Cc: rusty, paulus, anton, linuxppc-dev
In-Reply-To: <1417090721-25298-2-git-send-email-maddy@linux.vnet.ibm.com>
On Thu, Nov 27, 2014 at 05:48:40PM +0530, Madhavan Srinivasan wrote:
> This patch create the infrastructure to handle the CR based
> local_* atomic operations. Local atomic operations are fast
> and highly reentrant per CPU counters. Used for percpu
> variable updates. Local atomic operations only guarantee
> variable modification atomicity wrt the CPU which owns the
> data and these needs to be executed in a preemption safe way.
>
> Here is the design of this patch. Since local_* operations
> are only need to be atomic to interrupts (IIUC), patch uses
> one of the Condition Register (CR) fields as a flag variable. When
> entering the local_*, specific bit in the CR5 field is set
> and on exit, bit is cleared. CR bit checking is done in the
> interrupt return path. If CR5[EQ] bit set and if we return
> to kernel, we reset to start of local_* operation.
>
> Reason for this approach is that, currently l[w/d]arx/st[w/d]cx.
> instruction pair is used for local_* operations, which are heavy
> on cycle count and they dont support a local variant. So to
> see whether the new implementation helps, used a modified
> version of Rusty's benchmark code on local_t.
>
> https://lkml.org/lkml/2008/12/16/450
>
> Modifications:
> - increated the working set size from 1MB to 8MB,
> - removed cpu_local_inc test.
>
> Test ran
> - on POWER8 1S Scale out System 2.0GHz
> - on OPAL v3 with v3.18-rc4 patch kernel as Host
>
> Here are the values with the patch.
>
> Time in ns per iteration
>
> inc add read add_return
> atomic_long 67 67 18 69
> irqsave/rest 39 39 23 39
> trivalue 39 39 29 49
> local_t 26 26 24 26
>
> Since CR5 is used as a flag, have added CFLAGS to avoid CR5
> for the kernel compilation and CR5 is zeroed at the kernel
> entry.
>
> Tested the patch in a
> - pSeries LPAR,
> - Host with patched/unmodified guest kernel
>
> To check whether userspace see any CR5 corruption, ran a simple
> test which does,
> - set CR5 field,
> - while(1)
> - sleep or gettimeofday
> - chk bit set
>
> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> ---
> - I really appreciate feedback on the patchset.
> - Kindly comment if I should try with any other benchmark or
> workload to check the numbers.
> - Also, kindly recommand any know stress test for CR
>
> Makefile | 6 ++
> arch/powerpc/include/asm/exception-64s.h | 21 +++++-
> arch/powerpc/kernel/entry_64.S | 106 ++++++++++++++++++++++++++++++-
> arch/powerpc/kernel/exceptions-64s.S | 2 +-
> arch/powerpc/kernel/head_64.S | 8 +++
> 5 files changed, 138 insertions(+), 5 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 00d618b..2e271ad 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -706,6 +706,12 @@ endif
>
> KBUILD_CFLAGS += $(call cc-option, -fno-var-tracking-assignments)
>
> +ifdef CONFIG_PPC64
> +# We need this flag to force compiler not to use CR5, since
> +# local_t type code is based on this.
> +KBUILD_CFLAGS += -ffixed-cr5
> +endif
> +
> ifdef CONFIG_DEBUG_INFO
> ifdef CONFIG_DEBUG_INFO_SPLIT
> KBUILD_CFLAGS += $(call cc-option, -gsplit-dwarf, -g)
> diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
> index 77f52b2..c42919a 100644
> --- a/arch/powerpc/include/asm/exception-64s.h
> +++ b/arch/powerpc/include/asm/exception-64s.h
> @@ -306,7 +306,26 @@ do_kvm_##n: \
> std r10,0(r1); /* make stack chain pointer */ \
> std r0,GPR0(r1); /* save r0 in stackframe */ \
> std r10,GPR1(r1); /* save r1 in stackframe */ \
> - beq 4f; /* if from kernel mode */ \
> +BEGIN_FTR_SECTION; \
> + lis r9,4096; /* Create a mask with HV and PR */ \
> + rldicr r9,r9,32,31; /* bits, AND with the MSR */ \
> + mr r10,r9; /* to check for Hyp state */ \
> + ori r9,r9,16384; \
> + and r9,r12,r9; \
> + cmpd cr3,r10,r9; \
> + beq cr3,66f; /* Jump if we come from Hyp mode*/ \
> + mtcrf 0x04,r10; /* Clear CR5 if coming from usr */ \
I think you can do better than this, powerpc has a fantastic set
of rotate and mask instructions. If I understand correctly your
code you can replace it with the following:
rldicl r10,r12,4,63 /* Extract HV bit to LSB of r10*/
rlwinm r9,r12,19,0x02 /* Extract PR bit to 2nd to last bit of r9 */
or r9,r9,10
cmplwi cr3,r9,1 /* Check for HV=1 and PR=0 */
beq cr3,66f
mtcrf 0x04,r10 /* Bits going to cr5 bits are 0 in r10 */
and obviously similarly for the other instances of this code sequence.
This said, mtcrf is not necessarily the fastest way of setting cr5 if
you only want to clear the EQ bit. For example comparing the stack pointer
with 0 should have the same effect.
> +FTR_SECTION_ELSE; \
> + beq 4f; /* if kernel mode branch */ \
> + li r10,0; /* Clear CR5 incase of coming */ \
> + mtcrf 0x04,r10; /* from user. */ \
> + nop; /* This part of code is for */ \
> + nop; /* kernel with MSR[HV]=0, */ \
> + nop; /* MSR[PR]=0, so just chk for */ \
> + nop; /* MSR[PR] */ \
> + nop; \
> +ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE); \
> +66: beq 4f; /* if from kernel mode */ \
> ACCOUNT_CPU_USER_ENTRY(r9, r10); \
> SAVE_PPR(area, r9, r10); \
> 4: EXCEPTION_PROLOG_COMMON_2(area) \
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> index 0905c8d..e42bb99 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -68,7 +68,26 @@ system_call_common:
> 2: std r2,GPR2(r1)
> std r3,GPR3(r1)
> mfcr r2
> - std r4,GPR4(r1)
> +BEGIN_FTR_SECTION
> + lis r10,4096
> + rldicr r10,r10,32,31
> + mr r11,r10
> + ori r10,r10,16384
> + and r10,r12,r10
> + cmpd r11,r10
> + beq 67f
> + mtcrf 0x04,r11
> +FTR_SECTION_ELSE
> + beq 67f
> + li r11,0
> + mtcrf 0x04,r11
> + nop
> + nop
> + nop
> + nop
> + nop
> +ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE)
> +67: std r4,GPR4(r1)
> std r5,GPR5(r1)
> std r6,GPR6(r1)
> std r7,GPR7(r1)
> @@ -224,8 +243,26 @@ syscall_exit:
> BEGIN_FTR_SECTION
> stdcx. r0,0,r1 /* to clear the reservation */
> END_FTR_SECTION_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
> +BEGIN_FTR_SECTION
> + lis r4,4096
> + rldicr r4,r4,32,31
> + mr r6,r4
> + ori r4,r4,16384
> + and r4,r8,r4
> + cmpd cr3,r6,r4
> + beq cr3,65f
> + mtcr r5
> +FTR_SECTION_ELSE
> andi. r6,r8,MSR_PR
> - ld r4,_LINK(r1)
> + beq 65f
> + mtcr r5
> + nop
> + nop
> + nop
> + nop
> + nop
> +ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE)
> +65: ld r4,_LINK(r1)
>
> beq- 1f
> ACCOUNT_CPU_USER_EXIT(r11, r12)
> @@ -234,7 +271,11 @@ END_FTR_SECTION_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
> 1: ld r2,GPR2(r1)
> ld r1,GPR1(r1)
> mtlr r4
> +#ifdef CONFIG_PPC64
> + mtcrf 0xFB,r5
> +#else
> mtcr r5
> +#endif
> mtspr SPRN_SRR0,r7
> mtspr SPRN_SRR1,r8
> RFI
> @@ -804,7 +845,66 @@ ALT_FTR_SECTION_END_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
> */
> .globl fast_exception_return
> fast_exception_return:
> - ld r3,_MSR(r1)
> +
> + /*
> + * Now that we are about to exit from interrupt, lets check for
> + * cr5 eq bit. If it is set, then we may be in the middle of
> + * local_t update. In this case, we should rewind the NIP
> + * accordingly.
> + */
> + mfcr r3
> + andi. r4,r3,0x200
> + beq 63f
I believe that someonw has already mentioned that this is a very
convoluted way of testing a cr bit. This can be optimized to bne cr5,63f
> +
> + /*
> + * Now that the bit is set, lets check for return to User
> + */
> + ld r4,_MSR(r1)
> +BEGIN_FTR_SECTION
> + li r3,4096
> + rldicr r3,r3,32,31
> + mr r5,r3
> + ori r3,r3,16384
> + and r3,r4,r3
> + cmpd r5,r3
> + bne 63f
> +FTR_SECTION_ELSE
> + andi. r3,r4,MSR_PR
> + bne 63f
> + nop
> + nop
> + nop
> + nop
> + nop
> +ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE)
> +
> + /*
> + * Looks like we are returning to Kernel, so
> + * lets get the NIP and search the ex_table.
> + * Change the NIP based on the return value
> + */
> +lookup_ex_table:
> + ld r3,_NIP(r1)
> + bl search_exception_tables
> + cmpli 0,1,r3,0
> + bne 62f
> +
> + /*
> + * This is a panic case. Reason is that, we
> + * have the CR5 bit set, but we are not in
> + * local_* code and we are returning to Kernel.
> + */
> + ld r3,_NIP(r1)
> + mfcr r4
> + EMIT_BUG_ENTRY lookup_ex_table, __FILE__,__LINE__,BUGFLAG_WARNING
> +
> + /*
> + * Now save the return fixup address as NIP
> + */
> +62: ld r4,8(r3)
> + std r4,_NIP(r1)
> + crclr 22
> +63: ld r3,_MSR(r1)
> ld r4,_CTR(r1)
> ld r0,_LINK(r1)
> mtctr r4
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index 72e783e..edb75a9 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -637,7 +637,7 @@ masked_##_H##interrupt: \
> rldicl r10,r10,48,1; /* clear MSR_EE */ \
> rotldi r10,r10,16; \
> mtspr SPRN_##_H##SRR1,r10; \
> -2: mtcrf 0x80,r9; \
> +2: mtcrf 0x90,r9; \
> ld r9,PACA_EXGEN+EX_R9(r13); \
> ld r10,PACA_EXGEN+EX_R10(r13); \
> ld r11,PACA_EXGEN+EX_R11(r13); \
> diff --git a/arch/powerpc/kernel/head_64.S b/arch/powerpc/kernel/head_64.S
> index d48125d..02e49b3 100644
> --- a/arch/powerpc/kernel/head_64.S
> +++ b/arch/powerpc/kernel/head_64.S
> @@ -347,6 +347,14 @@ __mmu_off:
> *
> */
> __start_initialization_multiplatform:
> +
> + /*
> + * Before we do anything, lets clear CR5 field,
> + * so that we will have a clean start at entry
> + */
> + li r11,0
> + mtcrf 0x04,r11
> +
> /* Make sure we are running in 64 bits mode */
> bl enable_64b_mode
>
Regards,
Gabriel
^ permalink raw reply
* Re: [PATCH 02/10] mm: Add p[te|md] protnone helpers for use by NUMA balancing
From: Benjamin Herrenschmidt @ 2014-12-01 22:38 UTC (permalink / raw)
To: Mel Gorman
Cc: Rik van Riel, Linus Torvalds, Hugh Dickins, Linux Kernel,
Linux-MM, Ingo Molnar, Paul Mackerras, Aneesh Kumar, Sasha Levin,
Dave Jones, LinuxPPC-dev, Kirill Shutemov
In-Reply-To: <1416578268-19597-3-git-send-email-mgorman@suse.de>
On Fri, 2014-11-21 at 13:57 +0000, Mel Gorman wrote:
> #ifdef CONFIG_NUMA_BALANCING
> +/*
> + * These work without NUMA balancing but the kernel does not care. See the
> + * comment in include/asm-generic/pgtable.h
> + */
> +static inline int pte_protnone(pte_t pte)
> +{
> + return (pte_val(pte) &
> + (_PAGE_PRESENT | _PAGE_USER)) == _PAGE_PRESENT;
> +}
I would add a comment clarifying that this only works for user pages,
ie, this accessor will always return "true" for a kernel page on ppc.
> +static inline int pmd_protnone(pmd_t pmd)
> +{
> + return pte_protnone(pmd_pte(pmd));
> +}
> +
> static inline int pte_present(pte_t pte)
> {
> return pte_val(pte) & _PAGE_NUMA_MASK;
> diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
> index 081d6f4..2e25780 100644
> --- a/arch/x86/include/asm/pgtable.h
> +++ b/arch/x86/include/asm/pgtable.h
> @@ -502,6 +502,22 @@ static inline int pmd_present(pmd_t pmd)
> _PAGE_NUMA);
> }
>
> +#ifdef CONFIG_NUMA_BALANCING
> +/*
> + * These work without NUMA balancing but the kernel does not care. See the
> + * comment in include/asm-generic/pgtable.h
> + */
> +static inline int pte_protnone(pte_t pte)
> +{
> + return pte_flags(pte) & _PAGE_PROTNONE;
> +}
> +
> +static inline int pmd_protnone(pmd_t pmd)
> +{
> + return pmd_flags(pmd) & _PAGE_PROTNONE;
> +}
> +#endif /* CONFIG_NUMA_BALANCING */
> +
> static inline int pmd_none(pmd_t pmd)
> {
> /* Only check low word on 32-bit platforms, since it might be
> diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h
> index 177d597..d497d08 100644
> --- a/include/asm-generic/pgtable.h
> +++ b/include/asm-generic/pgtable.h
> @@ -688,6 +688,26 @@ static inline int pmd_trans_unstable(pmd_t *pmd)
> #endif
> }
>
> +#ifndef CONFIG_NUMA_BALANCING
> +/*
> + * Technically a PTE can be PROTNONE even when not doing NUMA balancing but
> + * the only case the kernel cares is for NUMA balancing and is only ever set
> + * when the VMA is accessible. For PROT_NONE VMAs, the PTEs are not marked
> + * _PAGE_PROTNONE so by by default, implement the helper as "always no". It
> + * is the responsibility of the caller to distinguish between PROT_NONE
> + * protections and NUMA hinting fault protections.
> + */
> +static inline int pte_protnone(pte_t pte)
> +{
> + return 0;
> +}
> +
> +static inline int pmd_protnone(pmd_t pmd)
> +{
> + return 0;
> +}
> +#endif /* CONFIG_NUMA_BALANCING */
> +
> #ifdef CONFIG_NUMA_BALANCING
> /*
> * _PAGE_NUMA distinguishes between an unmapped page table entry, an entry that
^ permalink raw reply
* Re: [PATCH 03/10] mm: Convert p[te|md]_numa users to p[te|md]_protnone_numa
From: Benjamin Herrenschmidt @ 2014-12-01 22:42 UTC (permalink / raw)
To: Mel Gorman
Cc: Rik van Riel, Linus Torvalds, Hugh Dickins, Linux Kernel,
Linux-MM, Ingo Molnar, Paul Mackerras, Aneesh Kumar, Sasha Levin,
Dave Jones, LinuxPPC-dev, Kirill Shutemov
In-Reply-To: <1416578268-19597-4-git-send-email-mgorman@suse.de>
On Fri, 2014-11-21 at 13:57 +0000, Mel Gorman wrote:
> Convert existing users of pte_numa and friends to the new helper. Note
> that the kernel is broken after this patch is applied until the other
> page table modifiers are also altered. This patch layout is to make
> review easier.
Aneesh, the removal of the DSISR_PROTFAULT checks, I wonder if we might
break something here ... (I know, I asked for them to be removed :-)
IE, we basically bounce all protection checks to the "normal" VMA
protection checking, so far so good...
But what about the subpage protection stuff ? Will that still work ?
Cheers,
Ben.
> Signed-off-by: Mel Gorman <mgorman@suse.de>
> Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
> Acked-by: Aneesh Kumar <aneesh.kumar@linux.vnet.ibm.com>
> ---
> arch/powerpc/kvm/book3s_hv_rm_mmu.c | 2 +-
> arch/powerpc/mm/fault.c | 5 -----
> arch/powerpc/mm/pgtable.c | 11 ++++++++---
> arch/powerpc/mm/pgtable_64.c | 3 ++-
> arch/x86/mm/gup.c | 4 ++--
> include/uapi/linux/mempolicy.h | 2 +-
> mm/gup.c | 10 +++++-----
> mm/huge_memory.c | 16 +++++++--------
> mm/memory.c | 4 ++--
> mm/mprotect.c | 39 ++++++++++---------------------------
> mm/pgtable-generic.c | 2 +-
> 11 files changed, 40 insertions(+), 58 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> index 084ad54..3e6ad3f 100644
> --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> @@ -235,7 +235,7 @@ long kvmppc_do_h_enter(struct kvm *kvm, unsigned long flags,
> pte_size = psize;
> pte = lookup_linux_pte_and_update(pgdir, hva, writing,
> &pte_size);
> - if (pte_present(pte) && !pte_numa(pte)) {
> + if (pte_present(pte) && !pte_protnone(pte)) {
> if (writing && !pte_write(pte))
> /* make the actual HPTE be read-only */
> ptel = hpte_make_readonly(ptel);
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index eb79907..b434153 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -398,8 +398,6 @@ good_area:
> * processors use the same I/D cache coherency mechanism
> * as embedded.
> */
> - if (error_code & DSISR_PROTFAULT)
> - goto bad_area;
> #endif /* CONFIG_PPC_STD_MMU */
>
> /*
> @@ -423,9 +421,6 @@ good_area:
> flags |= FAULT_FLAG_WRITE;
> /* a read */
> } else {
> - /* protection fault */
> - if (error_code & 0x08000000)
> - goto bad_area;
> if (!(vma->vm_flags & (VM_READ | VM_EXEC | VM_WRITE)))
> goto bad_area;
> }
> diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c
> index c90e602..83dfcb5 100644
> --- a/arch/powerpc/mm/pgtable.c
> +++ b/arch/powerpc/mm/pgtable.c
> @@ -172,9 +172,14 @@ static pte_t set_access_flags_filter(pte_t pte, struct vm_area_struct *vma,
> void set_pte_at(struct mm_struct *mm, unsigned long addr, pte_t *ptep,
> pte_t pte)
> {
> -#ifdef CONFIG_DEBUG_VM
> - WARN_ON(pte_val(*ptep) & _PAGE_PRESENT);
> -#endif
> + /*
> + * When handling numa faults, we already have the pte marked
> + * _PAGE_PRESENT, but we can be sure that it is not in hpte.
> + * Hence we can use set_pte_at for them.
> + */
> + VM_WARN_ON((pte_val(*ptep) & (_PAGE_PRESENT | _PAGE_USER)) ==
> + (_PAGE_PRESENT | _PAGE_USER));
> +
> /* Note: mm->context.id might not yet have been assigned as
> * this context might not have been activated yet when this
> * is called.
> diff --git a/arch/powerpc/mm/pgtable_64.c b/arch/powerpc/mm/pgtable_64.c
> index 87ff0c1..435ebf7 100644
> --- a/arch/powerpc/mm/pgtable_64.c
> +++ b/arch/powerpc/mm/pgtable_64.c
> @@ -718,7 +718,8 @@ void set_pmd_at(struct mm_struct *mm, unsigned long addr,
> pmd_t *pmdp, pmd_t pmd)
> {
> #ifdef CONFIG_DEBUG_VM
> - WARN_ON(pmd_val(*pmdp) & _PAGE_PRESENT);
> + WARN_ON((pmd_val(*pmdp) & (_PAGE_PRESENT | _PAGE_USER)) ==
> + (_PAGE_PRESENT | _PAGE_USER));
> assert_spin_locked(&mm->page_table_lock);
> WARN_ON(!pmd_trans_huge(pmd));
> #endif
> diff --git a/arch/x86/mm/gup.c b/arch/x86/mm/gup.c
> index 207d9aef..f32e12c 100644
> --- a/arch/x86/mm/gup.c
> +++ b/arch/x86/mm/gup.c
> @@ -84,7 +84,7 @@ static noinline int gup_pte_range(pmd_t pmd, unsigned long addr,
> struct page *page;
>
> /* Similar to the PMD case, NUMA hinting must take slow path */
> - if (pte_numa(pte)) {
> + if (pte_protnone(pte)) {
> pte_unmap(ptep);
> return 0;
> }
> @@ -178,7 +178,7 @@ static int gup_pmd_range(pud_t pud, unsigned long addr, unsigned long end,
> * slowpath for accounting purposes and so that they
> * can be serialised against THP migration.
> */
> - if (pmd_numa(pmd))
> + if (pmd_protnone(pmd))
> return 0;
> if (!gup_huge_pmd(pmd, addr, next, write, pages, nr))
> return 0;
> diff --git a/include/uapi/linux/mempolicy.h b/include/uapi/linux/mempolicy.h
> index 0d11c3d..9cd8b21 100644
> --- a/include/uapi/linux/mempolicy.h
> +++ b/include/uapi/linux/mempolicy.h
> @@ -67,7 +67,7 @@ enum mpol_rebind_step {
> #define MPOL_F_LOCAL (1 << 1) /* preferred local allocation */
> #define MPOL_F_REBINDING (1 << 2) /* identify policies in rebinding */
> #define MPOL_F_MOF (1 << 3) /* this policy wants migrate on fault */
> -#define MPOL_F_MORON (1 << 4) /* Migrate On pte_numa Reference On Node */
> +#define MPOL_F_MORON (1 << 4) /* Migrate On protnone Reference On Node */
>
>
> #endif /* _UAPI_LINUX_MEMPOLICY_H */
> diff --git a/mm/gup.c b/mm/gup.c
> index 0ca1df9..e5dab89 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -64,7 +64,7 @@ retry:
> migration_entry_wait(mm, pmd, address);
> goto retry;
> }
> - if ((flags & FOLL_NUMA) && pte_numa(pte))
> + if ((flags & FOLL_NUMA) && pte_protnone(pte))
> goto no_page;
> if ((flags & FOLL_WRITE) && !pte_write(pte)) {
> pte_unmap_unlock(ptep, ptl);
> @@ -193,7 +193,7 @@ struct page *follow_page_mask(struct vm_area_struct *vma,
> }
> return page;
> }
> - if ((flags & FOLL_NUMA) && pmd_numa(*pmd))
> + if ((flags & FOLL_NUMA) && pmd_protnone(*pmd))
> return no_page_table(vma, flags);
> if (pmd_trans_huge(*pmd)) {
> if (flags & FOLL_SPLIT) {
> @@ -740,10 +740,10 @@ static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end,
>
> /*
> * Similar to the PMD case below, NUMA hinting must take slow
> - * path
> + * path using the pte_protnone check.
> */
> if (!pte_present(pte) || pte_special(pte) ||
> - pte_numa(pte) || (write && !pte_write(pte)))
> + pte_protnone(pte) || (write && !pte_write(pte)))
> goto pte_unmap;
>
> VM_BUG_ON(!pfn_valid(pte_pfn(pte)));
> @@ -938,7 +938,7 @@ static int gup_pmd_range(pud_t pud, unsigned long addr, unsigned long end,
> * slowpath for accounting purposes and so that they
> * can be serialised against THP migration.
> */
> - if (pmd_numa(pmd))
> + if (pmd_protnone(pmd))
> return 0;
>
> if (!gup_huge_pmd(pmd, pmdp, addr, next, write,
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index a2cd021..f81fddf 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1222,7 +1222,7 @@ struct page *follow_trans_huge_pmd(struct vm_area_struct *vma,
> return ERR_PTR(-EFAULT);
>
> /* Full NUMA hinting faults to serialise migration in fault paths */
> - if ((flags & FOLL_NUMA) && pmd_numa(*pmd))
> + if ((flags & FOLL_NUMA) && pmd_protnone(*pmd))
> goto out;
>
> page = pmd_page(*pmd);
> @@ -1353,7 +1353,7 @@ int do_huge_pmd_numa_page(struct mm_struct *mm, struct vm_area_struct *vma,
>
> /*
> * Migrate the THP to the requested node, returns with page unlocked
> - * and pmd_numa cleared.
> + * and access rights restored.
> */
> spin_unlock(ptl);
> migrated = migrate_misplaced_transhuge_page(mm, vma,
> @@ -1368,7 +1368,7 @@ clear_pmdnuma:
> BUG_ON(!PageLocked(page));
> pmd = pmd_mknonnuma(pmd);
> set_pmd_at(mm, haddr, pmdp, pmd);
> - VM_BUG_ON(pmd_numa(*pmdp));
> + VM_BUG_ON(pmd_protnone(*pmdp));
> update_mmu_cache_pmd(vma, addr, pmdp);
> unlock_page(page);
> out_unlock:
> @@ -1514,7 +1514,7 @@ int change_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
> ret = 1;
> if (!prot_numa) {
> entry = pmdp_get_and_clear_notify(mm, addr, pmd);
> - if (pmd_numa(entry))
> + if (pmd_protnone(entry))
> entry = pmd_mknonnuma(entry);
> entry = pmd_modify(entry, newprot);
> ret = HPAGE_PMD_NR;
> @@ -1530,7 +1530,7 @@ int change_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
> * local vs remote hits on the zero page.
> */
> if (!is_huge_zero_page(page) &&
> - !pmd_numa(*pmd)) {
> + !pmd_protnone(*pmd)) {
> pmdp_set_numa(mm, addr, pmd);
> ret = HPAGE_PMD_NR;
> }
> @@ -1798,9 +1798,9 @@ static int __split_huge_page_map(struct page *page,
> pte_t *pte, entry;
> BUG_ON(PageCompound(page+i));
> /*
> - * Note that pmd_numa is not transferred deliberately
> - * to avoid any possibility that pte_numa leaks to
> - * a PROT_NONE VMA by accident.
> + * Note that NUMA hinting access restrictions are not
> + * transferred to avoid any possibility of altering
> + * permissions across VMAs.
> */
> entry = mk_pte(page + i, vma->vm_page_prot);
> entry = maybe_mkwrite(pte_mkdirty(entry), vma);
> diff --git a/mm/memory.c b/mm/memory.c
> index ae923f5..eaa46f1 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3214,7 +3214,7 @@ static int handle_pte_fault(struct mm_struct *mm,
> pte, pmd, flags, entry);
> }
>
> - if (pte_numa(entry))
> + if (pte_protnone(entry))
> return do_numa_page(mm, vma, address, entry, pte, pmd);
>
> ptl = pte_lockptr(mm, pmd);
> @@ -3292,7 +3292,7 @@ static int __handle_mm_fault(struct mm_struct *mm, struct vm_area_struct *vma,
> if (pmd_trans_splitting(orig_pmd))
> return 0;
>
> - if (pmd_numa(orig_pmd))
> + if (pmd_protnone(orig_pmd))
> return do_huge_pmd_numa_page(mm, vma, address,
> orig_pmd, pmd);
>
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index ace9345..e93ddac 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -75,36 +75,17 @@ static unsigned long change_pte_range(struct vm_area_struct *vma, pmd_t *pmd,
> oldpte = *pte;
> if (pte_present(oldpte)) {
> pte_t ptent;
> - bool updated = false;
> -
> - if (!prot_numa) {
> - ptent = ptep_modify_prot_start(mm, addr, pte);
> - if (pte_numa(ptent))
> - ptent = pte_mknonnuma(ptent);
> - ptent = pte_modify(ptent, newprot);
> - /*
> - * Avoid taking write faults for pages we
> - * know to be dirty.
> - */
> - if (dirty_accountable && pte_dirty(ptent) &&
> - (pte_soft_dirty(ptent) ||
> - !(vma->vm_flags & VM_SOFTDIRTY)))
> - ptent = pte_mkwrite(ptent);
> - ptep_modify_prot_commit(mm, addr, pte, ptent);
> - updated = true;
> - } else {
> - struct page *page;
> -
> - page = vm_normal_page(vma, addr, oldpte);
> - if (page && !PageKsm(page)) {
> - if (!pte_numa(oldpte)) {
> - ptep_set_numa(mm, addr, pte);
> - updated = true;
> - }
> - }
> + ptent = ptep_modify_prot_start(mm, addr, pte);
> + ptent = pte_modify(ptent, newprot);
> +
> + /* Avoid taking write faults for known dirty pages */
> + if (dirty_accountable && pte_dirty(ptent) &&
> + (pte_soft_dirty(ptent) ||
> + !(vma->vm_flags & VM_SOFTDIRTY))) {
> + ptent = pte_mkwrite(ptent);
> }
> - if (updated)
> - pages++;
> + ptep_modify_prot_commit(mm, addr, pte, ptent);
> + pages++;
> } else if (IS_ENABLED(CONFIG_MIGRATION) && !pte_file(oldpte)) {
> swp_entry_t entry = pte_to_swp_entry(oldpte);
>
> diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c
> index dfb79e0..4b8ad76 100644
> --- a/mm/pgtable-generic.c
> +++ b/mm/pgtable-generic.c
> @@ -193,7 +193,7 @@ void pmdp_invalidate(struct vm_area_struct *vma, unsigned long address,
> pmd_t *pmdp)
> {
> pmd_t entry = *pmdp;
> - if (pmd_numa(entry))
> + if (pmd_protnone(entry))
> entry = pmd_mknonnuma(entry);
> set_pmd_at(vma->vm_mm, address, pmdp, pmd_mknotpresent(entry));
> flush_tlb_range(vma, address, address + HPAGE_PMD_SIZE);
^ permalink raw reply
* Re: [PATCH 03/10] mm: Convert p[te|md]_numa users to p[te|md]_protnone_numa
From: Benjamin Herrenschmidt @ 2014-12-01 22:44 UTC (permalink / raw)
To: Mel Gorman
Cc: Rik van Riel, Linus Torvalds, Hugh Dickins, Linux Kernel,
Linux-MM, Ingo Molnar, Paul Mackerras, Aneesh Kumar, Sasha Levin,
Dave Jones, LinuxPPC-dev, Kirill Shutemov
In-Reply-To: <1416578268-19597-4-git-send-email-mgorman@suse.de>
On Fri, 2014-11-21 at 13:57 +0000, Mel Gorman wrote:
> void set_pte_at(struct mm_struct *mm, unsigned long addr, pte_t *ptep,
> pte_t pte)
> {
> -#ifdef CONFIG_DEBUG_VM
> - WARN_ON(pte_val(*ptep) & _PAGE_PRESENT);
> -#endif
> + /*
> + * When handling numa faults, we already have the pte marked
> + * _PAGE_PRESENT, but we can be sure that it is not in hpte.
> + * Hence we can use set_pte_at for them.
> + */
> + VM_WARN_ON((pte_val(*ptep) & (_PAGE_PRESENT | _PAGE_USER)) ==
> + (_PAGE_PRESENT | _PAGE_USER));
> +
His is that going to fare with set_pte_at() called for kernel pages ?
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 2/2] powerpc/mm: don't do tlbie for updatepp request with NO HPTE fault
From: Benjamin Herrenschmidt @ 2014-12-01 23:59 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: paulus, linuxppc-dev
In-Reply-To: <1415026295-25965-2-git-send-email-aneesh.kumar@linux.vnet.ibm.com>
On Mon, 2014-11-03 at 20:21 +0530, Aneesh Kumar K.V wrote:
> --- a/arch/powerpc/mm/hash_native_64.c
> +++ b/arch/powerpc/mm/hash_native_64.c
> @@ -283,11 +283,11 @@ static long native_hpte_remove(unsigned long hpte_group)
>
> static long native_hpte_updatepp(unsigned long slot, unsigned long newpp,
> unsigned long vpn, int bpsize,
> - int apsize, int ssize, int local)
> + int apsize, int ssize, unsigned long flags)
> {
> struct hash_pte *hptep = htab_address + slot;
> unsigned long hpte_v, want_v;
> - int ret = 0;
> + int ret = 0, local = 0;
>
> want_v = hpte_encode_avpn(vpn, bpsize, ssize);
>
> @@ -322,8 +322,15 @@ static long native_hpte_updatepp(unsigned long slot, unsigned long newpp,
> }
> native_unlock_hpte(hptep);
> }
> - /* Ensure it is out of the tlb too. */
> - tlbie(vpn, bpsize, apsize, ssize, local);
> +
> + if (flags & HPTE_LOCAL_UPDATE)
> + local = 1;
> + /*
> + * Ensure it is out of the tlb too if it is not a nohpte fault
> + */
> + if (!(flags & HPTE_NOHPTE_UPDATE))
> + tlbie(vpn, bpsize, apsize, ssize, local);
> +
> return ret;
> }
An additional refinement we discussed that I'd like you to test/measure
is to basically always be local for updatepp unless we have a flag that
forces us not to.
That flag would be set by copro faults only.
Can you do something on top of this series ?
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 2/2] powerpc/mm: don't do tlbie for updatepp request with NO HPTE fault
From: Michael Ellerman @ 2014-12-02 0:39 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: paulus, linuxppc-dev
In-Reply-To: <1415026295-25965-2-git-send-email-aneesh.kumar@linux.vnet.ibm.com>
On Mon, 2014-11-03 at 20:21 +0530, Aneesh Kumar K.V wrote:
> upatepp get called for a nohpte fault, when we find from the linux
> page table that the translation was hashed before. In that case
> we are sure that there is no existing translation, hence we could
> avoid doing tlbie.
We are sure there *was* no existing translation. It's possible that since the
nohpte fault occurred the translation has been loaded into the tlb.
Ben says that's OK, because updatepp is only ever relaxing permissions. But
please add some explanation of that to the changelog - it's not obvious.
> @@ -322,8 +322,15 @@ static long native_hpte_updatepp(unsigned long slot, unsigned long newpp,
> }
> native_unlock_hpte(hptep);
> }
> - /* Ensure it is out of the tlb too. */
> - tlbie(vpn, bpsize, apsize, ssize, local);
> +
> + if (flags & HPTE_LOCAL_UPDATE)
> + local = 1;
> + /*
> + * Ensure it is out of the tlb too if it is not a nohpte fault
> + */
> + if (!(flags & HPTE_NOHPTE_UPDATE))
> + tlbie(vpn, bpsize, apsize, ssize, local);
> +
> return ret;
> }
The context preceeding this hunk includes this comment:
/*
* We need to invalidate the TLB always because hpte_remove doesn't do
* a tlb invalidate. If a hash bucket gets full, we "evict" a more/less
* random entry from it. When we do that we don't invalidate the TLB
* (hpte_remove) because we assume the old translation is still
* technically "valid".
*/
Which seems out of sync with the code now.
cheers
^ 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