From: Paul Mundt <lethal@linux-sh.org> To: Alexey Charkov <alchark@gmail.com> Cc: linux-arm-kernel@lists.infradead.org, vt8500-wm8505-linux-kernel@googlegroups.com, Andrew Morton <akpm@linux-foundation.org>, Guennadi Liakhovetski <g.liakhovetski@gmx.de>, Florian Tobias Schandinat <FlorianSchandinat@gmx.de>, Ralf Baechle <ralf@linux-mips.org>, "David S. Miller" <davem@davemloft.net>, linux-kernel@vger.kernel.org Subject: Re: [PATCH 6/6 v3] ARM: Add support for the display controllers in VT8500 and WM8505 Date: Tue, 9 Nov 2010 06:30:08 +0900 [thread overview] Message-ID: <20101108213008.GB17708@linux-sh.org> (raw) In-Reply-To: <AANLkTikXffMT9ZiUpm2QAoW+KPJN3MBf55Dgruj1EJN0@mail.gmail.com> On Tue, Nov 09, 2010 at 12:15:19AM +0300, Alexey Charkov wrote: > 2010/11/8 Paul Mundt <lethal@linux-sh.org>: > >> + ?? ?? if (!fbi) > >> + ?? ?? ?? ?? ?? ?? return 0; > >> + > > I would kill this test as well. If this ever triggers, something horribly > > wrong has happened and you likely have bigger things to worry about. > > But a couple of extra instructions for error handling to hold in the > kernel binary should not hurt, should they? Are there benefits aside > from code compaction? > It's not a realistic situation. The only way this would trigger is if the pointer you got handed is one that didn't go through the probe path or was otherwise corrupted. If it was corrupted, you're going to notice regardless. The driver core does sensible refcounting already, there's no need to second guess it. > >> diff --git a/drivers/video/wmt_ge_rops.c b/drivers/video/wmt_ge_rops.c > >> new file mode 100644 > >> index 0000000..b201a60 > >> --- /dev/null > >> +++ b/drivers/video/wmt_ge_rops.c > >> +EXPORT_SYMBOL(wmt_ge_fillrect); > >> +EXPORT_SYMBOL(wmt_ge_copyarea); > >> +EXPORT_SYMBOL(wmt_ge_sync); > >> + > > ... > > > > Is there a particular reason why you are favouring EXPORT_SYMBOL? In > > general we prefer that new infrastructure patches and the like stick with > > EXPORT_SYMBOL_GPL, as this discourages use by non-GPLed modules going > > forward. > > > > Well, I have no personal preference towards these, so I just took what > was in cfb*.c as a guidance. If the *_GPL variant is more welcome, it > can be changed. > Those exports go back a ways. The idea with the _GPL exports was not to change symbol export behaviour retroactively, so the old ones stay the way they were and newer stuff can choose which way it wants to go. If you are not using this driver with an out-of-tree non-GPLed module then you are advised to use the GPL variants so others don't either. > >> +static int __devinit wmt_ge_rops_probe(struct platform_device *pdev) > >> +{ > > ... > >> + ?? ?? regbase = ioremap(res->start, resource_size(res)); > >> + ?? ?? if (regbase == NULL) { > >> + ?? ?? ?? ?? ?? ?? dev_err(&pdev->dev, "failed to map I/O memory\n"); > >> + ?? ?? ?? ?? ?? ?? ret = -EBUSY; > >> + ?? ?? ?? ?? ?? ?? goto error; > >> + ?? ?? } > >> + > > You might also want to do something like: > > > > ?? ?? ?? ??/* Only one ROP engine is presently supported. */ > > ?? ?? ?? ??if (unlikely(regbase)) { > > ?? ?? ?? ?? ?? ?? ?? ??WARN_ON(1); > > ?? ?? ?? ?? ?? ?? ?? ??return -EBUSY; > > ?? ?? ?? ??} > > > > ?? ?? ?? ??regbase = ioremap(...); > > ?? ?? ?? ??... > > > > But for that I'd have to initialize regbase to NULL (so as not to use > an uninitialized variable), wouldn't I? checkpatch.pl complains on > that... > No, regbase is BSS initialized, so it will already be cleared. > >> +static int __devexit wmt_ge_rops_remove(struct platform_device *pdev) > >> +{ > >> + ?? ?? iounmap(regbase); > >> + ?? ?? return 0; > >> +} > >> + > > You're missing a: > > > > ?? ?? ?? ??writel(0, regbase + GE_ENABLE_OFF); > > > > here? > > > > In fact, this module only uses a subset of GE functions, so I'm > somewhat reluctant to disable the hardware altogether when unloading > the module. And should the hardware really be disabled when the driver > is removed? > You're the only one who can answer that. I just noticed in your other drivers that this is the pattern that you opted for, so I thought that perhaps this was an oversight in the rop engine code. Having said that, the general expectation is that a remove will balance out the probe. If the probe is enabling random blocks then the remove should be disabling them. If this driver is primarily used as a client by the other drivers and you're concerned from it being ripped out underneath them, then a bit more thinking and refcounting is needed. This is however something that can be done later if its a direction you wish to head in.
WARNING: multiple messages have this Message-ID (diff)
From: lethal@linux-sh.org (Paul Mundt) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 6/6 v3] ARM: Add support for the display controllers in VT8500 and WM8505 Date: Tue, 9 Nov 2010 06:30:08 +0900 [thread overview] Message-ID: <20101108213008.GB17708@linux-sh.org> (raw) In-Reply-To: <AANLkTikXffMT9ZiUpm2QAoW+KPJN3MBf55Dgruj1EJN0@mail.gmail.com> On Tue, Nov 09, 2010 at 12:15:19AM +0300, Alexey Charkov wrote: > 2010/11/8 Paul Mundt <lethal@linux-sh.org>: > >> + ?? ?? if (!fbi) > >> + ?? ?? ?? ?? ?? ?? return 0; > >> + > > I would kill this test as well. If this ever triggers, something horribly > > wrong has happened and you likely have bigger things to worry about. > > But a couple of extra instructions for error handling to hold in the > kernel binary should not hurt, should they? Are there benefits aside > from code compaction? > It's not a realistic situation. The only way this would trigger is if the pointer you got handed is one that didn't go through the probe path or was otherwise corrupted. If it was corrupted, you're going to notice regardless. The driver core does sensible refcounting already, there's no need to second guess it. > >> diff --git a/drivers/video/wmt_ge_rops.c b/drivers/video/wmt_ge_rops.c > >> new file mode 100644 > >> index 0000000..b201a60 > >> --- /dev/null > >> +++ b/drivers/video/wmt_ge_rops.c > >> +EXPORT_SYMBOL(wmt_ge_fillrect); > >> +EXPORT_SYMBOL(wmt_ge_copyarea); > >> +EXPORT_SYMBOL(wmt_ge_sync); > >> + > > ... > > > > Is there a particular reason why you are favouring EXPORT_SYMBOL? In > > general we prefer that new infrastructure patches and the like stick with > > EXPORT_SYMBOL_GPL, as this discourages use by non-GPLed modules going > > forward. > > > > Well, I have no personal preference towards these, so I just took what > was in cfb*.c as a guidance. If the *_GPL variant is more welcome, it > can be changed. > Those exports go back a ways. The idea with the _GPL exports was not to change symbol export behaviour retroactively, so the old ones stay the way they were and newer stuff can choose which way it wants to go. If you are not using this driver with an out-of-tree non-GPLed module then you are advised to use the GPL variants so others don't either. > >> +static int __devinit wmt_ge_rops_probe(struct platform_device *pdev) > >> +{ > > ... > >> + ?? ?? regbase = ioremap(res->start, resource_size(res)); > >> + ?? ?? if (regbase == NULL) { > >> + ?? ?? ?? ?? ?? ?? dev_err(&pdev->dev, "failed to map I/O memory\n"); > >> + ?? ?? ?? ?? ?? ?? ret = -EBUSY; > >> + ?? ?? ?? ?? ?? ?? goto error; > >> + ?? ?? } > >> + > > You might also want to do something like: > > > > ?? ?? ?? ??/* Only one ROP engine is presently supported. */ > > ?? ?? ?? ??if (unlikely(regbase)) { > > ?? ?? ?? ?? ?? ?? ?? ??WARN_ON(1); > > ?? ?? ?? ?? ?? ?? ?? ??return -EBUSY; > > ?? ?? ?? ??} > > > > ?? ?? ?? ??regbase = ioremap(...); > > ?? ?? ?? ??... > > > > But for that I'd have to initialize regbase to NULL (so as not to use > an uninitialized variable), wouldn't I? checkpatch.pl complains on > that... > No, regbase is BSS initialized, so it will already be cleared. > >> +static int __devexit wmt_ge_rops_remove(struct platform_device *pdev) > >> +{ > >> + ?? ?? iounmap(regbase); > >> + ?? ?? return 0; > >> +} > >> + > > You're missing a: > > > > ?? ?? ?? ??writel(0, regbase + GE_ENABLE_OFF); > > > > here? > > > > In fact, this module only uses a subset of GE functions, so I'm > somewhat reluctant to disable the hardware altogether when unloading > the module. And should the hardware really be disabled when the driver > is removed? > You're the only one who can answer that. I just noticed in your other drivers that this is the pattern that you opted for, so I thought that perhaps this was an oversight in the rop engine code. Having said that, the general expectation is that a remove will balance out the probe. If the probe is enabling random blocks then the remove should be disabling them. If this driver is primarily used as a client by the other drivers and you're concerned from it being ripped out underneath them, then a bit more thinking and refcounting is needed. This is however something that can be done later if its a direction you wish to head in.
next prev parent reply other threads:[~2010-11-08 21:30 UTC|newest] Thread overview: 184+ messages / expand[flat|nested] mbox.gz Atom feed top 2010-11-07 16:28 [PATCH 1/6 v2] ARM: Add basic architecture support for VIA/WonderMedia 85xx SoC's Alexey Charkov 2010-11-07 16:28 ` Alexey Charkov 2010-11-07 16:28 ` [PATCH 2/6 v2] serial: Add support for UART on VIA VT8500 and compatibles Alexey Charkov 2010-11-07 16:28 ` Alexey Charkov 2010-11-07 23:08 ` Alan Cox 2010-11-07 23:08 ` Alan Cox 2010-11-08 0:58 ` [PATCH 2/6 v3] " Alexey Charkov 2010-11-08 0:58 ` Alexey Charkov 2010-11-08 10:46 ` Alan Cox 2010-11-08 10:46 ` Alan Cox 2010-11-08 17:33 ` [PATCH 2/6 v4] " Alexey Charkov 2010-11-08 17:33 ` Alexey Charkov 2010-11-07 16:28 ` [PATCH 3/6 v2] input: Add support for VIA VT8500 and compatibles in i8042 Alexey Charkov 2010-11-07 16:28 ` Alexey Charkov 2010-11-12 22:54 ` Alexey Charkov 2010-11-12 22:54 ` Alexey Charkov 2010-11-12 22:54 ` Alexey Charkov 2010-11-12 23:30 ` Dmitry Torokhov 2010-11-12 23:30 ` Dmitry Torokhov 2010-11-12 23:30 ` Dmitry Torokhov 2010-11-13 0:00 ` Alexey Charkov 2010-11-13 0:00 ` Alexey Charkov 2010-12-22 21:41 ` [PATCH 3/6 v3] " Alexey Charkov 2010-12-22 21:41 ` Alexey Charkov 2010-11-07 16:28 ` [PATCH 4/6 v2] usb: Add support for VIA VT8500 and compatibles in EHCI HCD Alexey Charkov 2010-11-07 16:28 ` Alexey Charkov 2010-11-07 16:28 ` [PATCH 5/6 v2] rtc: Add support for the RTC in VIA VT8500 and compatibles Alexey Charkov 2010-11-07 16:28 ` Alexey Charkov 2010-11-12 22:53 ` Alexey Charkov 2010-11-12 22:53 ` Alexey Charkov 2010-11-13 12:14 ` Lars-Peter Clausen 2010-11-13 12:14 ` Lars-Peter Clausen 2010-11-13 23:56 ` [PATCH 5/6 v3] " Alexey Charkov 2010-11-13 23:56 ` Alexey Charkov 2010-11-14 15:50 ` Lars-Peter Clausen 2010-11-14 15:50 ` Lars-Peter Clausen 2010-11-14 17:00 ` [PATCH 5/6 v4] " Alexey Charkov 2010-11-14 17:00 ` Alexey Charkov 2010-11-23 19:17 ` Alexey Charkov 2010-11-23 19:17 ` Alexey Charkov 2010-11-24 19:23 ` Lars-Peter Clausen 2010-11-24 19:23 ` Lars-Peter Clausen 2010-11-24 22:47 ` [PATCH 5/6 v5] " Alexey Charkov 2010-11-24 22:47 ` Alexey Charkov 2010-11-07 16:28 ` [PATCH 6/6 v2] ARM: Add support for the display controllers in VT8500 and WM8505 Alexey Charkov 2010-11-07 16:28 ` Alexey Charkov 2010-11-08 4:17 ` Paul Mundt 2010-11-08 4:17 ` Paul Mundt 2010-11-08 12:56 ` Alexey Charkov 2010-11-08 12:56 ` Alexey Charkov 2010-11-08 14:14 ` [PATCH 6/6 v3] " Alexey Charkov 2010-11-08 14:14 ` Alexey Charkov 2010-11-08 20:43 ` Paul Mundt 2010-11-08 20:43 ` Paul Mundt 2010-11-08 21:15 ` Alexey Charkov 2010-11-08 21:15 ` Alexey Charkov 2010-11-08 21:30 ` Paul Mundt [this message] 2010-11-08 21:30 ` Paul Mundt 2010-11-08 23:42 ` [PATCH 6/6 v4] " Alexey Charkov 2010-11-08 23:42 ` Alexey Charkov 2010-11-08 23:54 ` Paul Mundt 2010-11-08 23:54 ` Paul Mundt 2010-11-09 0:03 ` Alexey Charkov 2010-11-09 0:03 ` Alexey Charkov 2010-11-09 7:36 ` Guennadi Liakhovetski 2010-11-09 7:36 ` Guennadi Liakhovetski 2010-11-09 9:39 ` Paul Mundt 2010-11-09 9:39 ` Paul Mundt 2010-11-09 9:49 ` Alexey Charkov 2010-11-09 9:49 ` Alexey Charkov 2010-11-09 10:33 ` [PATCH 6/6 v3] " Russell King - ARM Linux 2010-11-09 10:33 ` Russell King - ARM Linux 2010-11-09 10:51 ` Paul Mundt 2010-11-09 10:51 ` Paul Mundt 2010-11-09 11:04 ` Russell King - ARM Linux 2010-11-09 11:04 ` Russell King - ARM Linux 2010-11-09 13:02 ` Geert Uytterhoeven 2010-11-09 13:02 ` Geert Uytterhoeven 2010-11-09 13:33 ` Arnd Bergmann 2010-11-09 13:33 ` Arnd Bergmann 2010-11-09 16:20 ` Paul Mundt 2010-11-09 16:20 ` Paul Mundt 2010-11-08 8:47 ` [PATCH 6/6 v2] " Arnd Bergmann 2010-11-08 8:47 ` Arnd Bergmann 2010-11-09 10:23 ` Alexey Charkov 2010-11-09 10:23 ` Alexey Charkov 2010-11-09 15:03 ` Arnd Bergmann 2010-11-09 15:03 ` Arnd Bergmann 2010-11-07 16:57 ` [PATCH 1/6 v2] ARM: Add basic architecture support for VIA/WonderMedia 85xx SoC's Russell King - ARM Linux 2010-11-07 16:57 ` Russell King - ARM Linux 2010-11-07 17:08 ` Alexey Charkov 2010-11-07 17:08 ` Alexey Charkov 2010-11-07 17:17 ` Russell King - ARM Linux 2010-11-07 17:17 ` Russell King - ARM Linux 2010-11-07 18:25 ` [PATCH 1/6 v3] " Alexey Charkov 2010-11-07 18:25 ` Alexey Charkov 2010-11-08 17:19 ` [PATCH 1/6 v4] " Alexey Charkov 2010-11-08 17:19 ` Alexey Charkov 2010-11-10 15:16 ` saeed bishara 2010-11-10 15:16 ` saeed bishara 2010-11-10 15:18 ` Russell King - ARM Linux 2010-11-10 15:18 ` Russell King - ARM Linux 2010-11-10 15:20 ` saeed bishara 2010-11-10 15:20 ` saeed bishara 2010-11-11 21:23 ` [PATCH 1/6 v5] " Alexey Charkov 2010-11-11 21:23 ` Alexey Charkov 2010-11-11 23:49 ` Russell King - ARM Linux 2010-11-11 23:49 ` Russell King - ARM Linux 2010-11-12 16:54 ` [PATCH 1/6 v6] " Alexey Charkov 2010-11-12 16:54 ` Alexey Charkov 2010-11-12 20:14 ` Alexey Charkov 2010-11-12 20:14 ` Alexey Charkov 2010-11-23 19:50 ` [PATCH 1/6 v7] " Alexey Charkov 2010-11-23 19:50 ` Alexey Charkov 2010-12-19 17:40 ` [PATCH 1/6 v8] " Alexey Charkov 2010-12-19 17:40 ` Alexey Charkov 2010-12-20 18:15 ` Arnd Bergmann 2010-12-20 18:15 ` Arnd Bergmann 2010-12-20 19:15 ` Russell King - ARM Linux 2010-12-20 19:15 ` Russell King - ARM Linux 2010-12-20 19:26 ` Alexey Charkov 2010-12-20 19:26 ` Alexey Charkov 2010-12-20 19:54 ` [PATCH 1/6 v9] " Alexey Charkov 2010-12-20 19:54 ` Alexey Charkov 2010-12-20 20:50 ` Ryan Mallon 2010-12-20 20:50 ` Ryan Mallon 2010-12-20 21:48 ` Alexey Charkov 2010-12-20 21:48 ` Alexey Charkov 2010-12-20 22:23 ` Ryan Mallon 2010-12-20 22:23 ` Ryan Mallon 2010-12-20 23:00 ` Alexey Charkov 2010-12-20 23:00 ` Alexey Charkov 2010-12-20 23:22 ` Ryan Mallon 2010-12-20 23:22 ` Ryan Mallon 2010-12-20 23:49 ` Alexey Charkov 2010-12-20 23:49 ` Alexey Charkov 2010-12-21 0:09 ` Alexey Charkov 2010-12-21 0:09 ` Alexey Charkov 2010-12-21 2:32 ` Ryan Mallon 2010-12-21 2:32 ` Ryan Mallon 2010-12-21 10:00 ` Alexey Charkov 2010-12-21 10:00 ` Alexey Charkov 2010-12-21 12:05 ` Arnd Bergmann 2010-12-21 12:05 ` Arnd Bergmann 2010-12-21 19:39 ` Ryan Mallon 2010-12-21 19:39 ` Ryan Mallon 2010-12-22 21:18 ` [PATCH 1/6 v10] " Alexey Charkov 2010-12-22 21:18 ` Alexey Charkov 2010-12-22 21:52 ` Ryan Mallon 2010-12-22 21:52 ` Ryan Mallon 2010-12-22 22:02 ` Alexey Charkov 2010-12-22 22:02 ` Alexey Charkov 2010-12-22 22:32 ` Ryan Mallon 2010-12-22 22:32 ` Ryan Mallon 2010-12-22 22:25 ` [PATCH 1/6 v11] " Alexey Charkov 2010-12-22 22:25 ` Alexey Charkov 2010-12-22 22:59 ` Ryan Mallon 2010-12-22 22:59 ` Ryan Mallon 2010-12-22 23:38 ` [PATCH 1/6 v12] " Alexey Charkov 2010-12-22 23:38 ` Alexey Charkov 2010-12-28 14:52 ` Alexey Charkov 2010-12-28 14:52 ` Alexey Charkov 2011-01-15 0:53 ` Alexey Charkov 2011-01-15 0:53 ` Alexey Charkov 2011-01-15 0:58 ` Russell King - ARM Linux 2011-01-15 0:58 ` Russell King - ARM Linux 2011-01-15 1:13 ` Alexey Charkov 2011-01-15 1:13 ` Alexey Charkov 2011-01-21 10:43 ` Russell King - ARM Linux 2011-01-21 10:43 ` Russell King - ARM Linux 2011-07-06 12:34 ` [PATCH 1/6 v8] " Russell King - ARM Linux 2011-07-06 12:34 ` Russell King - ARM Linux 2011-07-07 7:13 ` Alexey Charkov 2011-07-07 7:13 ` Alexey Charkov 2011-07-07 7:54 ` Arnd Bergmann 2011-07-07 7:54 ` Arnd Bergmann 2011-07-07 7:59 ` Russell King - ARM Linux 2011-07-07 7:59 ` Russell King - ARM Linux 2011-07-07 8:05 ` Arnd Bergmann 2011-07-07 8:05 ` Arnd Bergmann 2010-11-07 17:00 ` [PATCH 1/6 v2] " Russell King - ARM Linux 2010-11-07 17:00 ` Russell King - ARM Linux 2010-11-07 17:16 ` Alexey Charkov 2010-11-07 17:16 ` Alexey Charkov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20101108213008.GB17708@linux-sh.org \ --to=lethal@linux-sh.org \ --cc=FlorianSchandinat@gmx.de \ --cc=akpm@linux-foundation.org \ --cc=alchark@gmail.com \ --cc=davem@davemloft.net \ --cc=g.liakhovetski@gmx.de \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=ralf@linux-mips.org \ --cc=vt8500-wm8505-linux-kernel@googlegroups.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.