* Re: [PATCH] powerpc/85xx: Add P1021 PCI IDs and quirks
From: Kumar Gala @ 2010-08-31 21:44 UTC (permalink / raw)
To: Anton Vorontsov; +Cc: linuxppc-dev
In-Reply-To: <20100808140333.GA4706@oksana.dev.rtsoft.ru>
On Aug 8, 2010, at 9:03 AM, Anton Vorontsov wrote:
> This is needed for proper PCI-E support on P1021 SoCs.
>
> Signed-off-by: Anton Vorontsov <avorontsov@mvista.com>
> ---
> arch/powerpc/sysdev/fsl_pci.c | 2 ++
> include/linux/pci_ids.h | 2 ++
> 2 files changed, 4 insertions(+), 0 deletions(-)
applied
- k
^ permalink raw reply
* Re: [PATCH 5/7] arch/powerpc/sysdev/qe_lib/qe.c: Add of_node_put to avoid memory leak
From: Kumar Gala @ 2010-08-31 21:41 UTC (permalink / raw)
To: Julia Lawall
Cc: linuxppc-dev, devicetree-discuss, kernel-janitors, Timur Tabi,
linux-kernel
In-Reply-To: <1283075566-27441-6-git-send-email-julia@diku.dk>
On Aug 29, 2010, at 4:52 AM, Julia Lawall wrote:
> Add a call to of_node_put in the error handling code following a call to
> of_find_compatible_node.
>
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @r exists@
> local idexpression x;
> expression E,E1;
> statement S;
> @@
>
> *x =
> (of_find_node_by_path
> |of_find_node_by_name
> |of_find_node_by_phandle
> |of_get_parent
> |of_get_next_parent
> |of_get_next_child
> |of_find_compatible_node
> |of_match_node
> )(...);
> ...
> if (x == NULL) S
> <... when != x = E
> *if (...) {
> ... when != of_node_put(x)
> when != if (...) { ... of_node_put(x); ... }
> (
> return <+...x...+>;
> |
> * return ...;
> )
> }
> ...>
> of_node_put(x);
> // </smpl>
>
> Signed-off-by: Julia Lawall <julia@diku.dk>
>
> ---
> arch/powerpc/sysdev/qe_lib/qe.c | 1 +
> 1 file changed, 1 insertion(+)
applied to merge
- k
^ permalink raw reply
* Re: [PATCH] powerpc/85xx: Fix compile issue with p1022_ds due to lmb rename to memblock
From: Kumar Gala @ 2010-08-31 21:40 UTC (permalink / raw)
To: Kumar Gala; +Cc: linuxppc-dev
In-Reply-To: <1283272943-2454-1-git-send-email-galak@kernel.crashing.org>
On Aug 31, 2010, at 11:42 AM, Kumar Gala wrote:
> arch/powerpc/platforms/85xx/p1022_ds.c:22:23: error: linux/lmb.h: No =
such file or directory
> arch/powerpc/platforms/85xx/p1022_ds.c: In function =
'p1022_ds_setup_arch':
> arch/powerpc/platforms/85xx/p1022_ds.c:100: error: implicit =
declaration of function 'memblock_end_of_DRAM'
> arch/powerpc/platforms/85xx/p1022_ds.c: At top level:
> arch/powerpc/platforms/85xx/p1022_ds.c:147: error: 'udbg_progress' =
undeclared here (not in a function)
> make[2]: *** [arch/powerpc/platforms/85xx/p1022_ds.o] Error 1
>=20
> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
> ---
> arch/powerpc/platforms/85xx/p1022_ds.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
applied to merge
- k=
^ permalink raw reply
* Re: [PATCH] arch/powerpc/platforms/83xx/mpc837x_mds.c: Add missing iounmap
From: Kumar Gala @ 2010-08-31 21:39 UTC (permalink / raw)
To: Julia Lawall
Cc: devicetree-discuss, kernel-janitors, linux-kernel, Paul Mackerras,
linuxppc-dev
In-Reply-To: <1283111238-3129-1-git-send-email-julia@diku.dk>
On Aug 29, 2010, at 2:47 PM, Julia Lawall wrote:
> The function of_iomap returns the result of calling ioremap, so iounmap
> should be called on the result in the error handling code, as done in the
> normal exit of the function.
>
> The sematic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @r exists@
> local idexpression x;
> expression E,E1;
> identifier l;
> statement S;
> @@
>
> *x = of_iomap(...);
> ... when != iounmap(x)
> when != if (...) { ... iounmap(x); ... }
> when != E = x
> when any
> (
> if (x == NULL) S
> |
> if (...) {
> ... when != iounmap(x)
> when != if (...) { ... iounmap(x); ... }
> (
> return <+...x...+>;
> |
> * return ...;
> )
> }
> )
> ... when != x = E1
> when any
> iounmap(x);
> // </smpl>
>
> Signed-off-by: Julia Lawall <julia@diku.dk>
>
> ---
> arch/powerpc/platforms/83xx/mpc837x_mds.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
applied to merge
- k
^ permalink raw reply
* Re: [PATCH v2 1/4] fsl_rio: fix compile errors
From: Kumar Gala @ 2010-08-31 21:39 UTC (permalink / raw)
To: Li Yang; +Cc: linuxppc-dev
In-Reply-To: <1276842263-4186-1-git-send-email-leoli@freescale.com>
On Jun 18, 2010, at 1:24 AM, Li Yang wrote:
> Fixes the following compile problem on E500 platforms:
> arch/powerpc/sysdev/fsl_rio.c: In function 'fsl_rio_mcheck_exception':
> arch/powerpc/sysdev/fsl_rio.c:248: error: 'MCSR_MASK' undeclared =
(first use in this function)
>=20
> Also fixes the compile problem on non-E500 platforms.
>=20
> Signed-off-by: Li Yang <leoli@freescale.com>
> ---
> arch/powerpc/sysdev/fsl_rio.c | 6 +++++-
> 1 files changed, 5 insertions(+), 1 deletions(-)
applied to merge
- k=
^ permalink raw reply
* Re: [PATCH 3/3] PPC: Fix compilation of mpc85xx_mds.c
From: Kumar Gala @ 2010-08-31 21:24 UTC (permalink / raw)
To: Alexander Graf; +Cc: Linuxppc-dev, Anton Vorontsov
In-Reply-To: <1283220922-20369-4-git-send-email-agraf@suse.de>
On Aug 30, 2010, at 9:15 PM, Alexander Graf wrote:
> Commit 99d8238f berobbed the for_each loop of its iterator! Let's be
> nice and give it back, so it compiles for us.
>
> CC: Anton Vorontsov <avorontsov@mvista.com>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
> arch/powerpc/platforms/85xx/mpc85xx_mds.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
applied to merge
- k
^ permalink raw reply
* Re: [PATCH 2/3] PPC: Fix compilation of fsl_rio.c
From: Kumar Gala @ 2010-08-31 18:55 UTC (permalink / raw)
To: Scott Wood; +Cc: Linuxppc-dev, Alexandre Bounine, Alexander Graf, Thomas Moll
In-Reply-To: <20100831131055.01e844d3@schlenkerla.am.freescale.net>
On Aug 31, 2010, at 1:10 PM, Scott Wood wrote:
> On Tue, 31 Aug 2010 04:15:21 +0200
> Alexander Graf <agraf@suse.de> wrote:
>
>> Commit a52c8f52 introduced machine check magic for the RapidIO chip.
>> Unfortunately it was so magical that it used constants that aren't even
>> defined!
>>
>> This patch bluntly comments out the broken constant's usage. This
>> probably means that said functionality thus doesn't work, but at
>> least it makes it compile for me.
>
> The MCSR_MASK is actually completely unnecessary -- it doesn't change
> the result of testing bits that are within the mask.
>
> Multiple patches have been posted for this already, including:
> http://patchwork.ozlabs.org/patch/56135/
>
> Someone just needs to apply it. :-)
I've got a different version as I don't want to override the mcheck handler.
- k
^ permalink raw reply
* Re: [PATCH 0/8] v5 De-couple sysfs memory directories from memory sections
From: Dave Hansen @ 2010-08-31 18:12 UTC (permalink / raw)
To: Nathan Fontenot
Cc: linuxppc-dev, Greg KH, linux-kernel, linux-mm, Andrew Morton,
KAMEZAWA Hiroyuki
In-Reply-To: <4C694C60.6030207@austin.ibm.com>
On Mon, 2010-08-16 at 09:34 -0500, Nathan Fontenot wrote:
> > It's not an unresolvable issue, as this is a must-fix problem. But you
> > should tell us what your proposal is to prevent breakage of existing
> > installations. A Kconfig option would be good, but a boot-time kernel
> > command line option which selects the new format would be much better.
>
> This shouldn't break existing installations, unless an architecture chooses
> to do so. With my patch only the powerpc/pseries arch is updated such that
> what is seen in userspace is different.
Even if an arch defines the override for the sysfs dir size, I still
don't think this breaks anything (it shouldn't). We move _all_ of the
directories over, all at once, to a single, uniform size. The only
apparent change to a user moving kernels would be a larger
block_size_bytes (which is certainly not changing the ABI) and a new
sysfs file for the end of the section. The new sysfs file is
_completely_ redundant at this point.
The architecture is only supposed to bump up the directory size when it
*KNOWS* that all operations will be done at the larger section size,
such as if the specific hardware has physical DIMMs which are much
larger than SECTION_SIZE.
Let's say we have a system with 20MB of memory, SECTION_SIZE of 1MB and
a sysfs dir size of 4MB.
Before the patch, we have 20 directories: one for each section. After
this patch, we have 5 directories.
The thing that I think is the next step, but that we _will_ probably
need eventually is this, take the 5 sysfs dirs in the above case:
0->3, 4->7, 8->11, 12->15, 16->19
and turn that into a single one:
0->19
*That* will require changing the ABI, but we could certainly have some
bloated and slow, but backward-compatible mode.
-- Dave
^ permalink raw reply
* Re: [PATCH 2/3] PPC: Fix compilation of fsl_rio.c
From: Scott Wood @ 2010-08-31 18:10 UTC (permalink / raw)
To: Alexander Graf; +Cc: Linuxppc-dev, Thomas Moll, Alexandre Bounine
In-Reply-To: <1283220922-20369-3-git-send-email-agraf@suse.de>
On Tue, 31 Aug 2010 04:15:21 +0200
Alexander Graf <agraf@suse.de> wrote:
> Commit a52c8f52 introduced machine check magic for the RapidIO chip.
> Unfortunately it was so magical that it used constants that aren't even
> defined!
>
> This patch bluntly comments out the broken constant's usage. This
> probably means that said functionality thus doesn't work, but at
> least it makes it compile for me.
The MCSR_MASK is actually completely unnecessary -- it doesn't change
the result of testing bits that are within the mask.
Multiple patches have been posted for this already, including:
http://patchwork.ozlabs.org/patch/56135/
Someone just needs to apply it. :-)
-Scott
^ permalink raw reply
* Re: [PATCH] powerpc: mtmsrd not defined
From: Sean MacLennan @ 2010-08-31 17:46 UTC (permalink / raw)
To: Kumar Gala; +Cc: linuxppc-dev
In-Reply-To: <D9007E15-F764-450D-9B20-6506F0051BA7@kernel.crashing.org>
On Tue, 31 Aug 2010 11:17:17 -0500
Kumar Gala <galak@kernel.crashing.org> wrote:
> Can we add proper CONFIG_PPC_FPU into this file.
If nobody beats me to it.... I can try this evening.
Cheers,
Sean
^ permalink raw reply
* Re: [PATCH] gpiolib: Add 'struct gpio_chip' forward declaration for !GPIOLIB case
From: Anton Vorontsov @ 2010-08-31 17:07 UTC (permalink / raw)
To: Grant Likely
Cc: David Brownell, Andrew Morton, Benjamin Herrenschmidt,
linuxppc-dev, linux-kernel
In-Reply-To: <AANLkTi=EaHxDgwVfh-bhGANcBYWjHW7-aZt4K5ZZ7WRi@mail.gmail.com>
On Tue, Aug 31, 2010 at 10:44:14AM -0600, Grant Likely wrote:
> On Tue, Aug 31, 2010 at 2:37 AM, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> > On Tue, Aug 31, 2010 at 02:03:44AM -0600, Grant Likely wrote:
> >> On Tue, Aug 24, 2010 at 01:26:23PM +0400, Anton Vorontsov wrote:
> >> > With CONFIG_GPIOLIB=n, the 'struct gpio_chip' is not declared,
> >> > so the following pops up on PowerPC:
> >> >
> >> > cc1: warnings being treated as errors
> >> > In file included from arch/powerpc/platforms/52xx/mpc52xx_common.c:19:
> >> > include/linux/of_gpio.h:74: warning: 'struct gpio_chip' declared
> >> > inside parameter list
> >> > include/linux/of_gpio.h:74: warning: its scope is only this definition
> >> > or declaration, which is probably not what
> >> > you want
> >> > include/linux/of_gpio.h:75: warning: 'struct gpio_chip' declared
> >> > inside parameter list
> >> > make[2]: *** [arch/powerpc/platforms/52xx/mpc52xx_common.o] Error 1
> >> >
> >> > This patch fixes the issue by providing the proper forward declaration.
> >> >
> >> > Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>
> >>
> >> This doesn't actually solve the problem, and gpiochip should
> >> remain undefined when CONFIG_GPIOLIB=n to catch exactly these
> >> build failures. The real problem is that I merged a change
> >> into the mpc5200 code that required CONFIG_GPIOLIB be enabled
> >> without reflecting that requirement in Kconfig.
> >
> > No, look closer. The error is in of_gpio.h, and it's perfectly
> > fine to include it w/o GPIOLIB=y.
>
> Looking even closer, we're both wrong. You're right I didn't look
> carefully enough, and the error is in of_gpio.h, not the .c file.
>
> However, it is not okay to get the definitions from of_gpio.h when
> CONFIG_GPIOLIB=n. If GPIOLIB, or more specifically OF_GPIO isn't set,
> then the of_gpio.h definitions should either not be defined, or should
> be defined as empty stubs (where appropriate).
Grrr. Grant, look again, even closer than you did.
They are stubs!
#else /* CONFIG_OF_GPIO */ <<<<<< !OF_GPIO (or !GPIOLIB) case.
/* Drivers may not strictly depend on the GPIO support, so let them link. */
static inline int of_get_gpio_flags(struct device_node *np, int index,
enum of_gpio_flags *flags)
{
return -ENOSYS;
}
static inline unsigned int of_gpio_count(struct device_node *np)
{
return 0;
}
static inline void of_gpiochip_add(struct gpio_chip *gc) { }
static inline void of_gpiochip_remove(struct gpio_chip *gc) { }
#endif /* CONFIG_OF_GPIO */
The errors are triggered by the of_gpiochip_*() stubs, which
are needed by the drivers/gpio/gpiolib.c.
Do you want to add another #ifdef CONFIG_GPIOLIB around
of_gpiochip_*()? That would be ugly.
There's nothing wrong in providing the forward decls, you
can't dereference it anyway (so you would still catch the
bogus users). And at the same time it's will work great
for these stubs.
--
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
^ permalink raw reply
* Re: [PATCH] powerpc/fsl_soc: Search all global-utilities nodes for rstccr
From: Timur Tabi @ 2010-08-31 16:56 UTC (permalink / raw)
To: Matthew McClintock; +Cc: kumar.gala, linuxppc-dev
In-Reply-To: <09C49916-9A20-41CD-A7BA-2F467BFD24DA@freescale.com>
On Tue, Aug 31, 2010 at 11:26 AM, Matthew McClintock <msm@freescale.com> wr=
ote:
>> I'm not sure KERN_EMERG is warranted for this kind of error.
>
> I'm not sure either - I left it as it was before.
My vote is to change it to KERN_ERR, but it's your call.
>> So if a node has an fsl,rstcr property, but the of_iomap() fails, we
>> jump to the next global-utilities node? =A0Perhaps you need a 'break'
>> after the printk()?
>
> Or potentially a continue to be more robust? Or would two (or more) "has-=
rstcr" nodes be wrong?
My point is that if the of_iomap() call fails, looking for another
global-utilities node is not the right course of action. of_iomap()
failing is a serious error that should result in an immediate exit.
>> Wouldn't it make more sense to assign fsl_rstcr_restart to
>> ppc_md.restart only if we find a valid fsl,has-rstcr property?
>
> Again I'm not entirely sure, I left this as it was before. Is there anoth=
er way to reset the board if the rstcr node was not found correctly?
Not on our 85xx boards. On 83xx, there's mpc83xx_restart(). I just
don't like "ppc_md.restart =3D=3D fsl_rstcr_restart", because it assumes
that the define_machine() entry is incorrect.
--=20
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: [PATCH] gpiolib: Add 'struct gpio_chip' forward declaration for !GPIOLIB case
From: Grant Likely @ 2010-08-31 16:44 UTC (permalink / raw)
To: Anton Vorontsov
Cc: David Brownell, Andrew Morton, Benjamin Herrenschmidt,
linuxppc-dev, linux-kernel
In-Reply-To: <20100831083716.GA28362@oksana.dev.rtsoft.ru>
On Tue, Aug 31, 2010 at 2:37 AM, Anton Vorontsov <cbouatmailru@gmail.com> w=
rote:
> On Tue, Aug 31, 2010 at 02:03:44AM -0600, Grant Likely wrote:
>> On Tue, Aug 24, 2010 at 01:26:23PM +0400, Anton Vorontsov wrote:
>> > With CONFIG_GPIOLIB=3Dn, the 'struct gpio_chip' is not declared,
>> > so the following pops up on PowerPC:
>> >
>> > =A0 cc1: warnings being treated as errors
>> > =A0 In file included from arch/powerpc/platforms/52xx/mpc52xx_common.c=
:19:
>> > =A0 include/linux/of_gpio.h:74: warning: 'struct gpio_chip' declared
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 inside par=
ameter list
>> > =A0 include/linux/of_gpio.h:74: warning: its scope is only this defini=
tion
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 or declara=
tion, which is probably not what
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 you want
>> > =A0 include/linux/of_gpio.h:75: warning: 'struct gpio_chip' declared
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 inside par=
ameter list
>> > =A0 make[2]: *** [arch/powerpc/platforms/52xx/mpc52xx_common.o] Error =
1
>> >
>> > This patch fixes the issue by providing the proper forward declaration=
.
>> >
>> > Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>
>>
>> This doesn't actually solve the problem, and gpiochip should
>> remain undefined when CONFIG_GPIOLIB=3Dn to catch exactly these
>> build failures. =A0The real problem is that I merged a change
>> into the mpc5200 code that required CONFIG_GPIOLIB be enabled
>> without reflecting that requirement in Kconfig.
>
> No, look closer. The error is in of_gpio.h, and it's perfectly
> fine to include it w/o GPIOLIB=3Dy.
Looking even closer, we're both wrong. You're right I didn't look
carefully enough, and the error is in of_gpio.h, not the .c file.
However, it is not okay to get the definitions from of_gpio.h when
CONFIG_GPIOLIB=3Dn. If GPIOLIB, or more specifically OF_GPIO isn't set,
then the of_gpio.h definitions should either not be defined, or should
be defined as empty stubs (where appropriate).
So, instead of adding a forward declarations of the struct, the
correct thing I think to do is to #ifdef out the contents of of_gpio.h
when GPIOLIB isn't selected so that extra #includes are completely
benign. I've been doing the same thing on the other linux/of*.h
headers. I've got a patch in my tree that I'm testing right now and
I'll post later today.
g.
>
>> > ---
>> >
>> > On Tue, Aug 24, 2010 at 04:26:08PM +1000, Benjamin Herrenschmidt wrote=
:
>> > > I get that with my current stuff:
>> > >
>> > > cc1: warnings being treated as errors
>> > > In file included from [..]/mpc52xx_common.c:19:
>> > > of_gpio.h:74: error: =91struct gpio_chip=92 declared inside paramete=
r list
>> > [...]
>> > > make[3]: *** Waiting for unfinished jobs....
>> >
>> > That's because with GPIOCHIP=3Dn no one declares struct gpio_chip.
>> >
>> > It should be either of_gpio.h or gpio.h. Let's make it gpio.h, as
>> > this feels more generic, and we already have some !GPIOLIB handling
>> > in there.
>> >
>> > =A0include/linux/gpio.h | =A0 =A06 ++++++
>> > =A01 files changed, 6 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/include/linux/gpio.h b/include/linux/gpio.h
>> > index 03f616b..85207d2 100644
>> > --- a/include/linux/gpio.h
>> > +++ b/include/linux/gpio.h
>> > @@ -15,6 +15,12 @@
>> > =A0struct device;
>> >
>> > =A0/*
>> > + * Some code might rely on the declaration. Still, it is illegal
>> > + * to dereference it for !GPIOLIB case.
>> > + */
>> > +struct gpio_chip;
>> > +
>> > +/*
>> > =A0 * Some platforms don't support the GPIO programming interface.
>> > =A0 *
>> > =A0 * In case some driver uses it anyway (it should normally have
>> > --
>> > 1.7.0.5
>> >
>
> --
> Anton Vorontsov
> email: cbouatmailru@gmail.com
> irc://irc.freenode.net/bd2
>
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* [PATCH] powerpc/85xx: Fix compile issue with p1022_ds due to lmb rename to memblock
From: Kumar Gala @ 2010-08-31 16:42 UTC (permalink / raw)
To: linuxppc-dev
arch/powerpc/platforms/85xx/p1022_ds.c:22:23: error: linux/lmb.h: No such file or directory
arch/powerpc/platforms/85xx/p1022_ds.c: In function 'p1022_ds_setup_arch':
arch/powerpc/platforms/85xx/p1022_ds.c:100: error: implicit declaration of function 'memblock_end_of_DRAM'
arch/powerpc/platforms/85xx/p1022_ds.c: At top level:
arch/powerpc/platforms/85xx/p1022_ds.c:147: error: 'udbg_progress' undeclared here (not in a function)
make[2]: *** [arch/powerpc/platforms/85xx/p1022_ds.o] Error 1
Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---
arch/powerpc/platforms/85xx/p1022_ds.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/85xx/p1022_ds.c b/arch/powerpc/platforms/85xx/p1022_ds.c
index e1467c9..34e0090 100644
--- a/arch/powerpc/platforms/85xx/p1022_ds.c
+++ b/arch/powerpc/platforms/85xx/p1022_ds.c
@@ -19,7 +19,7 @@
#include <linux/pci.h>
#include <linux/of_platform.h>
-#include <linux/lmb.h>
+#include <linux/memblock.h>
#include <asm/mpic.h>
#include <asm/swiotlb.h>
@@ -97,7 +97,7 @@ static void __init p1022_ds_setup_arch(void)
#endif
#ifdef CONFIG_SWIOTLB
- if (lmb_end_of_DRAM() > max) {
+ if (memblock_end_of_DRAM() > max) {
ppc_swiotlb_enable = 1;
set_pci_dma_ops(&swiotlb_dma_ops);
ppc_md.pci_dma_dev_setup = pci_dma_dev_setup_swiotlb;
--
1.6.0.6
^ permalink raw reply related
* Re: [PATCH] powerpc/fsl_soc: Search all global-utilities nodes for rstccr
From: Matthew McClintock @ 2010-08-31 16:26 UTC (permalink / raw)
To: Timur Tabi; +Cc: kumar.gala, linuxppc-dev
In-Reply-To: <AANLkTimYz6v2tMJRYnki9Ph33HL9yGsOcpgJ78LTDCTG@mail.gmail.com>
On Aug 28, 2010, at 5:34 PM, Timur Tabi wrote:
>> <msm@freescale.com> wrote:
>=20
>> +
>> + for_each_node_by_name(np, "global-utilities") {
>> + if ((of_get_property(np, "fsl,has-rstcr", NULL))) {
>> + rstcr =3D of_iomap(np, 0) + 0xb0;
>> + if (!rstcr)
>> + printk (KERN_EMERG "Error: reset =
control "
>=20
> I'm not sure KERN_EMERG is warranted for this kind of error.
I'm not sure either - I left it as it was before.
>=20
>> + "register not =
mapped!\n");
>> + }
>=20
> So if a node has an fsl,rstcr property, but the of_iomap() fails, we
> jump to the next global-utilities node? Perhaps you need a 'break'
> after the printk()?
Or potentially a continue to be more robust? Or would two (or more) =
"has-rstcr" nodes be wrong?
>=20
>> + }
>> +
>> + if (!rstcr && ppc_md.restart =3D=3D fsl_rstcr_restart)
>=20
> Wouldn't it make more sense to assign fsl_rstcr_restart to
> ppc_md.restart only if we find a valid fsl,has-rstcr property?
Again I'm not entirely sure, I left this as it was before. Is there =
another way to reset the board if the rstcr node was not found =
correctly?
-M
>=20
> --=20
> Timur Tabi
> Linux kernel developer at Freescale
>=20
^ permalink raw reply
* Re: [PATCH 1/7] drivers/macintosh/via-pmu-led.c: Add of_node_put to avoid memory leak
From: Grant Likely @ 2010-08-31 16:33 UTC (permalink / raw)
To: Vasiliy Kulikov
Cc: devicetree-discuss, kernel-janitors, linux-kernel, Julia Lawall,
linuxppc-dev, walter harms
In-Reply-To: <20100831161644.GA14505@albatros>
On Tue, Aug 31, 2010 at 10:16 AM, Vasiliy Kulikov <segooon@gmail.com> wrote=
:
> On Tue, Aug 31, 2010 at 18:08 +0200, Julia Lawall wrote:
>> On Tue, 31 Aug 2010, walter harms wrote:
>> > > =A0 if (strncmp(model, "PowerBook", strlen("PowerBook")) !=3D 0 &&
>> > > =A0 =A0 =A0 strncmp(model, "iBook", strlen("iBook")) !=3D 0 &&
>> > > =A0 =A0 =A0 strcmp(model, "PowerMac7,2") !=3D 0 &&
>> > >
>> >
>> > is there any rule that says when to use strncmp ? it seems perfecly va=
lid to use strcpy here
>> > (what is done in the last cmp).
>>
>> Perhaps there are some characters after eg PowerBook that one doesn't wa=
nt
>> to compare with?
>
> It seems to me that model has no '\0' in the end. If model is got from
> the hardware then we should double check it - maybe harware is buggy.
> Otherwise we'll overflow model.
Model does have \0 at the end. This code is using strncmp to
purposefully ignore the model suffix.
> But why strcmp(model, "PowerMac7,2")? IMO it should be replaced
> with strncmp().
We use strcmp when parsing the device tree because the the length of
the model property string is unknown and in most cases we *must* match
the exact entire string, such as with this PowerMac7,2 example. Using
strncmp would also happen to match with something like
"PowerMac7,2345" which is not the desired behaviour.
g.
^ permalink raw reply
* Re: [PATCH] powerpc: mtmsrd not defined
From: Kumar Gala @ 2010-08-31 16:17 UTC (permalink / raw)
To: Sean MacLennan; +Cc: linuxppc-dev
In-Reply-To: <20100822184844.37c30d5e@lappy.seanm.ca>
On Aug 22, 2010, at 5:48 PM, Sean MacLennan wrote:
> On Mon, 23 Aug 2010 08:34:54 +1000
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
>> I'd rather have a macro somewhere in ppc_asm.h (MTMSR ?) that does the
>> right thing. We might even already have one...
>
> We do.... here is a new, improved patch.
>
> Replace the BOOK3S_64 specific mtmsrd with the generic MTMSRD macro.
>
> Signed-off-by: Sean MacLennan <smaclennan@pikatech.com>
> ---
Can we add proper CONFIG_PPC_FPU into this file.
- k
^ permalink raw reply
* Re: [PATCH 1/7] drivers/macintosh/via-pmu-led.c: Add of_node_put to avoid memory leak
From: Vasiliy Kulikov @ 2010-08-31 16:16 UTC (permalink / raw)
To: Julia Lawall
Cc: devicetree-discuss, kernel-janitors, linux-kernel, linuxppc-dev,
walter harms
In-Reply-To: <Pine.LNX.4.64.1008311807470.2668@ask.diku.dk>
On Tue, Aug 31, 2010 at 18:08 +0200, Julia Lawall wrote:
> On Tue, 31 Aug 2010, walter harms wrote:
>
> >
> >
> > Julia Lawall schrieb:
> > > Add a call to of_node_put in the error handling code following a call to
> > > of_find_node_by_path.
[...]
> > > --- a/drivers/macintosh/via-pmu-led.c
> > > +++ b/drivers/macintosh/via-pmu-led.c
> > > @@ -92,8 +92,10 @@ static int __init via_pmu_led_init(void)
> > > if (dt == NULL)
> > > return -ENODEV;
> > > model = of_get_property(dt, "model", NULL);
> > > - if (model == NULL)
> > > + if (model == NULL) {
> > > + of_node_put(dt);
> > > return -ENODEV;
> > > + }
> > > if (strncmp(model, "PowerBook", strlen("PowerBook")) != 0 &&
> > > strncmp(model, "iBook", strlen("iBook")) != 0 &&
> > > strcmp(model, "PowerMac7,2") != 0 &&
> > >
> >
> > is there any rule that says when to use strncmp ? it seems perfecly valid to use strcpy here
> > (what is done in the last cmp).
>
> Perhaps there are some characters after eg PowerBook that one doesn't want
> to compare with?
It seems to me that model has no '\0' in the end. If model is got from
the hardware then we should double check it - maybe harware is buggy.
Otherwise we'll overflow model.
But why strcmp(model, "PowerMac7,2")? IMO it should be replaced
with strncmp().
--
Vasiliy
^ permalink raw reply
* Re: [PATCH 1/7] drivers/macintosh/via-pmu-led.c: Add of_node_put to avoid memory leak
From: Grant Likely @ 2010-08-31 16:13 UTC (permalink / raw)
To: Julia Lawall
Cc: devicetree-discuss, kernel-janitors, linux-kernel, linuxppc-dev,
walter harms
In-Reply-To: <Pine.LNX.4.64.1008311807470.2668@ask.diku.dk>
On Tue, Aug 31, 2010 at 06:08:19PM +0200, Julia Lawall wrote:
> On Tue, 31 Aug 2010, walter harms wrote:
> > > @@ -92,8 +92,10 @@ static int __init via_pmu_led_init(void)
> > > if (dt == NULL)
> > > return -ENODEV;
> > > model = of_get_property(dt, "model", NULL);
> > > - if (model == NULL)
> > > + if (model == NULL) {
> > > + of_node_put(dt);
> > > return -ENODEV;
> > > + }
> > > if (strncmp(model, "PowerBook", strlen("PowerBook")) != 0 &&
> > > strncmp(model, "iBook", strlen("iBook")) != 0 &&
> > > strcmp(model, "PowerMac7,2") != 0 &&
> > >
> >
> > is there any rule that says when to use strncmp ? it seems perfecly valid to use strcpy here
> > (what is done in the last cmp).
>
> Perhaps there are some characters after eg PowerBook that one doesn't want
> to compare with?
Yes. The model property on powermacs always has the version number. strncmp makes sure that *all* PowerBooks and iBooks are matched.
g.
^ permalink raw reply
* Re: [PATCH 1/7] drivers/macintosh/via-pmu-led.c: Add of_node_put to avoid memory leak
From: Julia Lawall @ 2010-08-31 16:08 UTC (permalink / raw)
To: walter harms
Cc: devicetree-discuss, kernel-janitors, linux-kernel, linuxppc-dev
In-Reply-To: <4C7D247F.4060500@bfs.de>
On Tue, 31 Aug 2010, walter harms wrote:
>
>
> Julia Lawall schrieb:
> > Add a call to of_node_put in the error handling code following a call to
> > of_find_node_by_path.
> >
> > The semantic match that finds this problem is as follows:
> > (http://coccinelle.lip6.fr/)
> >
> > // <smpl>
> > @r exists@
> > local idexpression x;
> > expression E,E1;
> > statement S;
> > @@
> >
> > *x =
> > (of_find_node_by_path
> > |of_find_node_by_name
> > |of_find_node_by_phandle
> > |of_get_parent
> > |of_get_next_parent
> > |of_get_next_child
> > |of_find_compatible_node
> > |of_match_node
> > )(...);
> > ...
> > if (x == NULL) S
> > <... when != x = E
> > *if (...) {
> > ... when != of_node_put(x)
> > when != if (...) { ... of_node_put(x); ... }
> > (
> > return <+...x...+>;
> > |
> > * return ...;
> > )
> > }
> > ...>
> > of_node_put(x);
> > // </smpl>
> >
> > Signed-off-by: Julia Lawall <julia@diku.dk>
> >
> > ---
> > drivers/macintosh/via-pmu-led.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/macintosh/via-pmu-led.c b/drivers/macintosh/via-pmu-led.c
> > index d242976..19c3718 100644
> > --- a/drivers/macintosh/via-pmu-led.c
> > +++ b/drivers/macintosh/via-pmu-led.c
> > @@ -92,8 +92,10 @@ static int __init via_pmu_led_init(void)
> > if (dt == NULL)
> > return -ENODEV;
> > model = of_get_property(dt, "model", NULL);
> > - if (model == NULL)
> > + if (model == NULL) {
> > + of_node_put(dt);
> > return -ENODEV;
> > + }
> > if (strncmp(model, "PowerBook", strlen("PowerBook")) != 0 &&
> > strncmp(model, "iBook", strlen("iBook")) != 0 &&
> > strcmp(model, "PowerMac7,2") != 0 &&
> >
>
> is there any rule that says when to use strncmp ? it seems perfecly valid to use strcpy here
> (what is done in the last cmp).
Perhaps there are some characters after eg PowerBook that one doesn't want
to compare with?
julia
^ permalink raw reply
* Re: [PATCH 1/7] drivers/macintosh/via-pmu-led.c: Add of_node_put to avoid memory leak
From: walter harms @ 2010-08-31 15:49 UTC (permalink / raw)
To: Julia Lawall
Cc: devicetree-discuss, kernel-janitors, linux-kernel, linuxppc-dev
In-Reply-To: <1283075566-27441-2-git-send-email-julia@diku.dk>
Julia Lawall schrieb:
> Add a call to of_node_put in the error handling code following a call to
> of_find_node_by_path.
>
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @r exists@
> local idexpression x;
> expression E,E1;
> statement S;
> @@
>
> *x =
> (of_find_node_by_path
> |of_find_node_by_name
> |of_find_node_by_phandle
> |of_get_parent
> |of_get_next_parent
> |of_get_next_child
> |of_find_compatible_node
> |of_match_node
> )(...);
> ...
> if (x == NULL) S
> <... when != x = E
> *if (...) {
> ... when != of_node_put(x)
> when != if (...) { ... of_node_put(x); ... }
> (
> return <+...x...+>;
> |
> * return ...;
> )
> }
> ...>
> of_node_put(x);
> // </smpl>
>
> Signed-off-by: Julia Lawall <julia@diku.dk>
>
> ---
> drivers/macintosh/via-pmu-led.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/macintosh/via-pmu-led.c b/drivers/macintosh/via-pmu-led.c
> index d242976..19c3718 100644
> --- a/drivers/macintosh/via-pmu-led.c
> +++ b/drivers/macintosh/via-pmu-led.c
> @@ -92,8 +92,10 @@ static int __init via_pmu_led_init(void)
> if (dt == NULL)
> return -ENODEV;
> model = of_get_property(dt, "model", NULL);
> - if (model == NULL)
> + if (model == NULL) {
> + of_node_put(dt);
> return -ENODEV;
> + }
> if (strncmp(model, "PowerBook", strlen("PowerBook")) != 0 &&
> strncmp(model, "iBook", strlen("iBook")) != 0 &&
> strcmp(model, "PowerMac7,2") != 0 &&
>
is there any rule that says when to use strncmp ? it seems perfecly valid to use strcpy here
(what is done in the last cmp).
re,
wh
^ permalink raw reply
* [PATCH 4/4] arch/powerpc/platforms/chrp/nvram.c: Add of_node_put to avoid memory leak
From: Julia Lawall @ 2010-08-31 15:48 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: devicetree-discuss, kernel-janitors, linux-kernel, Paul Mackerras,
linuxppc-dev
In-Reply-To: <1283269738-14612-1-git-send-email-julia@diku.dk>
Add a call to of_node_put in the error handling code following a call to
of_find_node_by_type.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@r exists@
local idexpression x;
expression E,E1,E2;
statement S;
@@
*x =
(of_find_node_by_path
|of_find_node_by_name
|of_find_node_by_phandle
|of_get_parent
|of_get_next_parent
|of_get_next_child
|of_find_compatible_node
|of_match_node
|of_find_node_by_type
|of_find_node_with_property
|of_find_matching_node
|of_parse_phandle
)(...);
...
if (x == NULL) S
<... when != x = E
*if (...) {
... when != of_node_put(x)
when != if (...) { ... of_node_put(x); ... }
(
return <+...x...+>;
|
* return ...;
)
}
...>
(
E2 = x;
|
of_node_put(x);
)
// </smpl>
Signed-off-by: Julia Lawall <julia@diku.dk>
---
arch/powerpc/platforms/chrp/nvram.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/chrp/nvram.c b/arch/powerpc/platforms/chrp/nvram.c
index ba3588f..d3ceff0 100644
--- a/arch/powerpc/platforms/chrp/nvram.c
+++ b/arch/powerpc/platforms/chrp/nvram.c
@@ -74,8 +74,10 @@ void __init chrp_nvram_init(void)
return;
nbytes_p = of_get_property(nvram, "#bytes", &proplen);
- if (nbytes_p == NULL || proplen != sizeof(unsigned int))
+ if (nbytes_p == NULL || proplen != sizeof(unsigned int)) {
+ of_node_put(nvram);
return;
+ }
nvram_size = *nbytes_p;
^ permalink raw reply related
* [PATCH 1/4] drivers/serial/ucc_uart.c: Add of_node_put to avoid memory leak
From: Julia Lawall @ 2010-08-31 15:48 UTC (permalink / raw)
To: Timur Tabi
Cc: linuxppc-dev, devicetree-discuss, kernel-janitors, linux-kernel
In-Reply-To: <1283269738-14612-1-git-send-email-julia@diku.dk>
Add a call to of_node_put in the error handling code following a call to
of_find_compatible_node or of_find_node_by_type.
This patch also substantially reorganizes the error handling code in the
function, to that it is possible first to jump to code that frees qe_port
and then to jump to code that also puts np.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@r exists@
local idexpression x;
expression E,E1,E2;
statement S;
@@
*x =
(of_find_node_by_path
|of_find_node_by_name
|of_find_node_by_phandle
|of_get_parent
|of_get_next_parent
|of_get_next_child
|of_find_compatible_node
|of_match_node
|of_find_node_by_type
|of_find_node_with_property
|of_find_matching_node
|of_parse_phandle
)(...);
...
if (x == NULL) S
<... when != x = E
*if (...) {
... when != of_node_put(x)
when != if (...) { ... of_node_put(x); ... }
(
return <+...x...+>;
|
* return ...;
)
}
...>
(
E2 = x;
|
of_node_put(x);
)
// </smpl>
Signed-off-by: Julia Lawall <julia@diku.dk>
---
drivers/serial/ucc_uart.c | 67 ++++++++++++++++++++++++----------------------
1 file changed, 35 insertions(+), 32 deletions(-)
diff --git a/drivers/serial/ucc_uart.c b/drivers/serial/ucc_uart.c
index 3f4848e..38a5ef0 100644
--- a/drivers/serial/ucc_uart.c
+++ b/drivers/serial/ucc_uart.c
@@ -1270,13 +1270,12 @@ static int ucc_uart_probe(struct platform_device *ofdev,
ret = of_address_to_resource(np, 0, &res);
if (ret) {
dev_err(&ofdev->dev, "missing 'reg' property in device tree\n");
- kfree(qe_port);
- return ret;
+ goto out_free;
}
if (!res.start) {
dev_err(&ofdev->dev, "invalid 'reg' property in device tree\n");
- kfree(qe_port);
- return -EINVAL;
+ ret = -EINVAL;
+ goto out_free;
}
qe_port->port.mapbase = res.start;
@@ -1286,17 +1285,17 @@ static int ucc_uart_probe(struct platform_device *ofdev,
if (!iprop) {
iprop = of_get_property(np, "device-id", NULL);
if (!iprop) {
- kfree(qe_port);
dev_err(&ofdev->dev, "UCC is unspecified in "
"device tree\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto out_free;
}
}
if ((*iprop < 1) || (*iprop > UCC_MAX_NUM)) {
dev_err(&ofdev->dev, "no support for UCC%u\n", *iprop);
- kfree(qe_port);
- return -ENODEV;
+ ret = -ENODEV;
+ goto out_free;
}
qe_port->ucc_num = *iprop - 1;
@@ -1310,16 +1309,16 @@ static int ucc_uart_probe(struct platform_device *ofdev,
sprop = of_get_property(np, "rx-clock-name", NULL);
if (!sprop) {
dev_err(&ofdev->dev, "missing rx-clock-name in device tree\n");
- kfree(qe_port);
- return -ENODEV;
+ ret = -ENODEV;
+ goto out_free;
}
qe_port->us_info.rx_clock = qe_clock_source(sprop);
if ((qe_port->us_info.rx_clock < QE_BRG1) ||
(qe_port->us_info.rx_clock > QE_BRG16)) {
dev_err(&ofdev->dev, "rx-clock-name must be a BRG for UART\n");
- kfree(qe_port);
- return -ENODEV;
+ ret = -ENODEV;
+ goto out_free;
}
#ifdef LOOPBACK
@@ -1329,39 +1328,39 @@ static int ucc_uart_probe(struct platform_device *ofdev,
sprop = of_get_property(np, "tx-clock-name", NULL);
if (!sprop) {
dev_err(&ofdev->dev, "missing tx-clock-name in device tree\n");
- kfree(qe_port);
- return -ENODEV;
+ ret = -ENODEV;
+ goto out_free;
}
qe_port->us_info.tx_clock = qe_clock_source(sprop);
#endif
if ((qe_port->us_info.tx_clock < QE_BRG1) ||
(qe_port->us_info.tx_clock > QE_BRG16)) {
dev_err(&ofdev->dev, "tx-clock-name must be a BRG for UART\n");
- kfree(qe_port);
- return -ENODEV;
+ ret = -ENODEV;
+ goto out_free;
}
/* Get the port number, numbered 0-3 */
iprop = of_get_property(np, "port-number", NULL);
if (!iprop) {
dev_err(&ofdev->dev, "missing port-number in device tree\n");
- kfree(qe_port);
- return -EINVAL;
+ ret = -EINVAL;
+ goto out_free;
}
qe_port->port.line = *iprop;
if (qe_port->port.line >= UCC_MAX_UART) {
dev_err(&ofdev->dev, "port-number must be 0-%u\n",
UCC_MAX_UART - 1);
- kfree(qe_port);
- return -EINVAL;
+ ret = -EINVAL;
+ goto out_free;
}
qe_port->port.irq = irq_of_parse_and_map(np, 0);
if (qe_port->port.irq == NO_IRQ) {
dev_err(&ofdev->dev, "could not map IRQ for UCC%u\n",
qe_port->ucc_num + 1);
- kfree(qe_port);
- return -EINVAL;
+ ret = -EINVAL;
+ goto out_free;
}
/*
@@ -1373,8 +1372,8 @@ static int ucc_uart_probe(struct platform_device *ofdev,
np = of_find_node_by_type(NULL, "qe");
if (!np) {
dev_err(&ofdev->dev, "could not find 'qe' node\n");
- kfree(qe_port);
- return -EINVAL;
+ ret = -EINVAL;
+ goto out_free;
}
}
@@ -1382,8 +1381,8 @@ static int ucc_uart_probe(struct platform_device *ofdev,
if (!iprop) {
dev_err(&ofdev->dev,
"missing brg-frequency in device tree\n");
- kfree(qe_port);
- return -EINVAL;
+ ret = -EINVAL;
+ goto out_np;
}
if (*iprop)
@@ -1398,16 +1397,16 @@ static int ucc_uart_probe(struct platform_device *ofdev,
if (!iprop) {
dev_err(&ofdev->dev,
"missing QE bus-frequency in device tree\n");
- kfree(qe_port);
- return -EINVAL;
+ ret = -EINVAL;
+ goto out_np;
}
if (*iprop)
qe_port->port.uartclk = *iprop / 2;
else {
dev_err(&ofdev->dev,
"invalid QE bus-frequency in device tree\n");
- kfree(qe_port);
- return -EINVAL;
+ ret = -EINVAL;
+ goto out_np;
}
}
@@ -1445,8 +1444,7 @@ static int ucc_uart_probe(struct platform_device *ofdev,
if (ret) {
dev_err(&ofdev->dev, "could not add /dev/ttyQE%u\n",
qe_port->port.line);
- kfree(qe_port);
- return ret;
+ goto out_np;
}
dev_set_drvdata(&ofdev->dev, qe_port);
@@ -1460,6 +1458,11 @@ static int ucc_uart_probe(struct platform_device *ofdev,
SERIAL_QE_MINOR + qe_port->port.line);
return 0;
+out_np:
+ of_node_put(np);
+out_free:
+ kfree(qe_port);
+ return ret;
}
static int ucc_uart_remove(struct platform_device *ofdev)
^ permalink raw reply related
* Re: [PATCH] ucc_geth: fix ethtool set ring param bug
From: Ben Hutchings @ 2010-08-31 15:23 UTC (permalink / raw)
To: Liang Li; +Cc: netdev, avorontsov, linuxppc-dev, davem
In-Reply-To: <20100831151629.GA21442@localhost>
On Tue, 2010-08-31 at 23:16 +0800, Liang Li wrote:
> On Tue, Aug 31, 2010 at 03:41:22PM +0100, Ben Hutchings wrote:
> > On Mon, 2010-08-30 at 22:47 +0800, Liang Li wrote:
> > > It's common sense that when we should do change to driver ring
> > > desc/buffer etc only after 'stop/shutdown' the device. When we
> > > do change while devices/driver is running, kernel oops occur:
> > [...]
> > > diff --git a/drivers/net/ucc_geth_ethtool.c b/drivers/net/ucc_geth_ethtool.c
> > > index 6f92e48..1b37aaa 100644
> > > --- a/drivers/net/ucc_geth_ethtool.c
> > > +++ b/drivers/net/ucc_geth_ethtool.c
> > > @@ -255,13 +255,18 @@ uec_set_ringparam(struct net_device *netdev,
> > > return -EINVAL;
> > > }
> > >
> > > - ug_info->bdRingLenRx[queue] = ring->rx_pending;
> > > - ug_info->bdRingLenTx[queue] = ring->tx_pending;
> > > -
> > > if (netif_running(netdev)) {
> > > - /* FIXME: restart automatically */
> > > - printk(KERN_INFO
> > > - "Please re-open the interface.\n");
> > > + printk(KERN_INFO "Stopping interface %s.\n", netdev->name);
> > > + ucc_geth_close(netdev);
> > > +
> > > + ug_info->bdRingLenRx[queue] = ring->rx_pending;
> > > + ug_info->bdRingLenTx[queue] = ring->tx_pending;
> > > +
> > > + printk(KERN_INFO "Reactivating interface %s.\n", netdev->name);
> > > + ucc_geth_open(netdev);
> >
> > What if ucc_geth_open() fails?
>
> I did some runtime tests but did not witness the ucc_geth_open fail.
> Assume it may fail for some reason, then I tend to think give out
> warnings for request user to open/enable it mannually? Or we may need
> to keep the 'FIXME' line?
The easy way out is to allow changing the ring size only while the
interface is down. The hard way is to make the change in such a way
that you can roll back in case of failure.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH] ucc_geth: fix ethtool set ring param bug
From: Liang Li @ 2010-08-31 15:16 UTC (permalink / raw)
To: Ben Hutchings; +Cc: netdev, avorontsov, linuxppc-dev, davem
In-Reply-To: <1283265682.2244.0.camel@achroite.uk.solarflarecom.com>
On Tue, Aug 31, 2010 at 03:41:22PM +0100, Ben Hutchings wrote:
> On Mon, 2010-08-30 at 22:47 +0800, Liang Li wrote:
> > It's common sense that when we should do change to driver ring
> > desc/buffer etc only after 'stop/shutdown' the device. When we
> > do change while devices/driver is running, kernel oops occur:
> [...]
> > diff --git a/drivers/net/ucc_geth_ethtool.c b/drivers/net/ucc_geth_ethtool.c
> > index 6f92e48..1b37aaa 100644
> > --- a/drivers/net/ucc_geth_ethtool.c
> > +++ b/drivers/net/ucc_geth_ethtool.c
> > @@ -255,13 +255,18 @@ uec_set_ringparam(struct net_device *netdev,
> > return -EINVAL;
> > }
> >
> > - ug_info->bdRingLenRx[queue] = ring->rx_pending;
> > - ug_info->bdRingLenTx[queue] = ring->tx_pending;
> > -
> > if (netif_running(netdev)) {
> > - /* FIXME: restart automatically */
> > - printk(KERN_INFO
> > - "Please re-open the interface.\n");
> > + printk(KERN_INFO "Stopping interface %s.\n", netdev->name);
> > + ucc_geth_close(netdev);
> > +
> > + ug_info->bdRingLenRx[queue] = ring->rx_pending;
> > + ug_info->bdRingLenTx[queue] = ring->tx_pending;
> > +
> > + printk(KERN_INFO "Reactivating interface %s.\n", netdev->name);
> > + ucc_geth_open(netdev);
>
> What if ucc_geth_open() fails?
I did some runtime tests but did not witness the ucc_geth_open fail.
Assume it may fail for some reason, then I tend to think give out
warnings for request user to open/enable it mannually? Or we may need
to keep the 'FIXME' line?
Thanks,
-Liang Li
>
> Ben.
>
> > + } else {
> > + ug_info->bdRingLenRx[queue] = ring->rx_pending;
> > + ug_info->bdRingLenTx[queue] = ring->tx_pending;
> > }
> >
> > return ret;
>
> --
> Ben Hutchings, Senior Software Engineer, Solarflare Communications
> Not speaking for my employer; that's the marketing department's job.
> They asked us to note that Solarflare product names are trademarked.
>
^ 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