| Message ID | 1355852048-23188-7-git-send-email-linux@prisktech.co.nz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello. On 18-12-2012 21:34, Tony Prisk wrote: > Resend to include mailing lists. Such remarks should be placed under --- tear line, not in the changelog. > Replace IS_ERR_OR_NULL with IS_ERR on clk_get results. > Signed-off-by: Tony Prisk <linux@prisktech.co.nz> > CC: Kyungmin Park <kyungmin.park@samsung.com> > CC: Tomasz Stanislawski <t.stanislaws@samsung.com> > CC: linux-media@vger.kernel.org WBR, Sergei
On 12/22/2012 10:53 PM, Sergei Shtylyov wrote: > Hello. > > On 18-12-2012 21:34, Tony Prisk wrote: > >> Resend to include mailing lists. > > Such remarks should be placed under --- tear line, not in the changelog. > >> Replace IS_ERR_OR_NULL with IS_ERR on clk_get results. > >> Signed-off-by: Tony Prisk <linux@prisktech.co.nz> >> CC: Kyungmin Park <kyungmin.park@samsung.com> >> CC: Tomasz Stanislawski <t.stanislaws@samsung.com> >> CC: linux-media@vger.kernel.org I've applied first version of this patch to my tree for 3.9, thanks.
clk_get() returns NULL if CONFIG_HAVE_CLK is disabled. I told Tony about this but everyone has been gone with end of year holidays so it hasn't been addressed. Tony, please fix it so people don't apply these patches until clk_get() is updated to not return NULL. It sucks to have to revert patches. regards, dan carpenter
On Wed, 2013-01-02 at 08:10 +0300, Dan Carpenter wrote: > clk_get() returns NULL if CONFIG_HAVE_CLK is disabled. > > I told Tony about this but everyone has been gone with end of year > holidays so it hasn't been addressed. > > Tony, please fix it so people don't apply these patches until > clk_get() is updated to not return NULL. It sucks to have to revert > patches. > > regards, > dan carpenter I posted the query to Mike Turquette, linux-kernel and linux-arm-kernel mailing lists, regarding the return of NULL when HAVE_CLK is undefined. Short Answer: A return value of NULL is valid and not an error therefore we should be using IS_ERR, not IS_ERR_OR_NULL on clk_get results. I see the obvious problem this creates, and asked this question: If the driver can't operate with a NULL clk, it should use a IS_ERR_OR_NULL test to test for failure, rather than IS_ERR. And Russell's answer: Why should a _consumer_ of a clock care? It is _very_ important that people get this idea - to a consumer, the struct clk is just an opaque cookie. The fact that it appears to be a pointer does _not_ mean that the driver can do any kind of dereferencing on that pointer - it should never do so. Thread can be viewed here: https://lkml.org/lkml/2012/12/20/105 Regards Tony Prisk
On Wed, Jan 02, 2013 at 08:10:36AM +0300, Dan Carpenter wrote: > clk_get() returns NULL if CONFIG_HAVE_CLK is disabled. > > I told Tony about this but everyone has been gone with end of year > holidays so it hasn't been addressed. > > Tony, please fix it so people don't apply these patches until > clk_get() is updated to not return NULL. It sucks to have to revert > patches. How about people stop using IS_ERR_OR_NULL for stuff which it shouldn't be used for?
On Wed, 2 Jan 2013, Russell King - ARM Linux wrote: > On Wed, Jan 02, 2013 at 08:10:36AM +0300, Dan Carpenter wrote: > > clk_get() returns NULL if CONFIG_HAVE_CLK is disabled. > > > > I told Tony about this but everyone has been gone with end of year > > holidays so it hasn't been addressed. > > > > Tony, please fix it so people don't apply these patches until > > clk_get() is updated to not return NULL. It sucks to have to revert > > patches. > > How about people stop using IS_ERR_OR_NULL for stuff which it shouldn't > be used for? Perhaps the cases where clk_get returns NULL could have a comment indicating that NULL does not represent a failure? In 3.7.1, it looks like it might have been possible for NULL to be returned by clk_get in arch/mips/loongson1/common/clock.c, but that definition seems to be gone in a recent linux-next. The remaining definitions look OK. julia
On Wed, Jan 02, 2013 at 10:44:32AM +0100, Julia Lawall wrote: > On Wed, 2 Jan 2013, Russell King - ARM Linux wrote: > > > On Wed, Jan 02, 2013 at 08:10:36AM +0300, Dan Carpenter wrote: > > > clk_get() returns NULL if CONFIG_HAVE_CLK is disabled. > > > > > > I told Tony about this but everyone has been gone with end of year > > > holidays so it hasn't been addressed. > > > > > > Tony, please fix it so people don't apply these patches until > > > clk_get() is updated to not return NULL. It sucks to have to revert > > > patches. > > > > How about people stop using IS_ERR_OR_NULL for stuff which it shouldn't > > be used for? > > Perhaps the cases where clk_get returns NULL could have a comment > indicating that NULL does not represent a failure? No. More documentation is never the answer to people not reading existing documentation. > In 3.7.1, it looks like it might have been possible for NULL to be > returned by clk_get in arch/mips/loongson1/common/clock.c, but that > definition seems to be gone in a recent linux-next. The remaining > definitions look OK. How about people just read the API and comply with it rather than doing their own thing all the time? We've already had at least one instance where someone has tried using IS_ERR() with the ioremap() return value. Really, if you're going to program kernel space, it is very important that you *know* the interfaces that you're using and you comply with them. Otherwise, you have no business saying that you're a kernel programmer. Yes, the odd mistake happens, but that's no excuse for the constant blatent mistakes with stuff like IS_ERR_OR_NULL() with clk_get() which just comes from total laziness on the part of the coder to understand the interfaces being used. Hell, it's even documented in linux/clk.h - that just shows how many people read the documentation which has been around since the clk API came about.
On 01/02/2013 06:10 AM, Dan Carpenter wrote: > clk_get() returns NULL if CONFIG_HAVE_CLK is disabled. It's not a problem for this driver, as it never dereferences what's returned from clk_get(). It would have to include <plat/clock.h>, which it doesn't and which would have clearly indicated abuse of the clock API. Moreover, this driver now depends on architectures that select HAVE_CLK, so it couldn't be build when CONFIG_HAVE_CLK is disabled. > I told Tony about this but everyone has been gone with end of year > holidays so it hasn't been addressed. > > Tony, please fix it so people don't apply these patches until > clk_get() is updated to not return NULL. It sucks to have to revert > patches. As explained by Russell many times, the clock API users should not care whether the value returned from clk_get() is NULL or not. It should only be tested with IS_ERR(). The patches look fine to me, no need to do anything. --- Regards, Sylwester
On Wed, Jan 02, 2013 at 06:31:53PM +1300, Tony Prisk wrote: > On Wed, 2013-01-02 at 08:10 +0300, Dan Carpenter wrote: > > clk_get() returns NULL if CONFIG_HAVE_CLK is disabled. > > > > I told Tony about this but everyone has been gone with end of year > > holidays so it hasn't been addressed. > > > > Tony, please fix it so people don't apply these patches until > > clk_get() is updated to not return NULL. It sucks to have to revert > > patches. > > > > regards, > > dan carpenter > > I posted the query to Mike Turquette, linux-kernel and linux-arm-kernel > mailing lists, regarding the return of NULL when HAVE_CLK is undefined. > > Short Answer: A return value of NULL is valid and not an error therefore > we should be using IS_ERR, not IS_ERR_OR_NULL on clk_get results. > > I see the obvious problem this creates, and asked this question: > > If the driver can't operate with a NULL clk, it should use a > IS_ERR_OR_NULL test to test for failure, rather than IS_ERR. > > > And Russell's answer: > > Why should a _consumer_ of a clock care? It is _very_ important that > people get this idea - to a consumer, the struct clk is just an opaque > cookie. The fact that it appears to be a pointer does _not_ mean that > the driver can do any kind of dereferencing on that pointer - it should > never do so. > > Thread can be viewed here: > https://lkml.org/lkml/2012/12/20/105 > Ah. Grand. Thanks... Btw. The documentation for clk_get() really should include some of this information. I know Russell thinks that the driver authors are stupid and lazy, and it's probably true. But if everyone makes the same mistake over and over, then it probably means we could put a special note: "Do not check this with IS_ERR_OR_NULL(). Null values are not an error. Drivers should treat the return value as an opaque cookie and they should not dereference it." This is probably there in the file somewhere else, but I searched for "opaque", "cookie", and "dereference" and I didn't find anything. I'm not saying the documentation isn't perfect, just that driver authors are lazy and stupid but we can't kill them so we have to live with them. regards, dan carpenter
On Thu, 3 Jan 2013, Dan Carpenter wrote: > On Wed, Jan 02, 2013 at 06:31:53PM +1300, Tony Prisk wrote: > > On Wed, 2013-01-02 at 08:10 +0300, Dan Carpenter wrote: > > > clk_get() returns NULL if CONFIG_HAVE_CLK is disabled. > > > > > > I told Tony about this but everyone has been gone with end of year > > > holidays so it hasn't been addressed. > > > > > > Tony, please fix it so people don't apply these patches until > > > clk_get() is updated to not return NULL. It sucks to have to revert > > > patches. > > > > > > regards, > > > dan carpenter > > > > I posted the query to Mike Turquette, linux-kernel and linux-arm-kernel > > mailing lists, regarding the return of NULL when HAVE_CLK is undefined. > > > > Short Answer: A return value of NULL is valid and not an error therefore > > we should be using IS_ERR, not IS_ERR_OR_NULL on clk_get results. > > > > I see the obvious problem this creates, and asked this question: > > > > If the driver can't operate with a NULL clk, it should use a > > IS_ERR_OR_NULL test to test for failure, rather than IS_ERR. > > > > > > And Russell's answer: > > > > Why should a _consumer_ of a clock care? It is _very_ important that > > people get this idea - to a consumer, the struct clk is just an opaque > > cookie. The fact that it appears to be a pointer does _not_ mean that > > the driver can do any kind of dereferencing on that pointer - it should > > never do so. > > > > Thread can be viewed here: > > https://lkml.org/lkml/2012/12/20/105 > > > > Ah. Grand. Thanks... > > Btw. The documentation for clk_get() really should include some of > this information. I know Russell thinks that the driver authors are > stupid and lazy, and it's probably true. But if everyone makes the > same mistake over and over, then it probably means we could put a > special note: > > "Do not check this with IS_ERR_OR_NULL(). Null values are not an > error. Drivers should treat the return value as an opaque cookie > and they should not dereference it." > > This is probably there in the file somewhere else, but I searched > for "opaque", "cookie", and "dereference" and I didn't find > anything. I'm not saying the documentation isn't perfect, just that > driver authors are lazy and stupid but we can't kill them so we have > to live with them. I still think it would also be helpful for the definition that returns NULL to have some documentation associated with it. Having a feature disabled and then trying to use the feature could reasonably considered to lead to a failure, so it is not obvious what the NULL represents. julia
On Thu, Jan 03, 2013 at 12:05:20PM +0300, Dan Carpenter wrote: > On Wed, Jan 02, 2013 at 06:31:53PM +1300, Tony Prisk wrote: > > Why should a _consumer_ of a clock care? It is _very_ important that > > people get this idea - to a consumer, the struct clk is just an opaque > > cookie. The fact that it appears to be a pointer does _not_ mean that > > the driver can do any kind of dereferencing on that pointer - it should > > never do so. > > > > Thread can be viewed here: > > https://lkml.org/lkml/2012/12/20/105 > > > > Ah. Grand. Thanks... > > Btw. The documentation for clk_get() really should include some of > this information. It *does* contain this information. The problem is that driver authors _ARE_ stupid, lazy morons who don't bother to read documentation. /** * clk_get - lookup and obtain a reference to a clock producer. * @dev: device for clock "consumer" * @id: clock consumer ID * * Returns a struct clk corresponding to the clock producer, or * valid IS_ERR() condition containing errno. The implementation * uses @dev and @id to determine the clock consumer, and thereby * the clock producer. (IOW, @id may be identical strings, but * clk_get may return different clock producers depending on @dev.) * * Drivers must assume that the clock source is not enabled. * * clk_get should not be called from within interrupt context. */
On Thu, Jan 03, 2013 at 10:14:13AM +0100, Julia Lawall wrote: > On Thu, 3 Jan 2013, Dan Carpenter wrote: > > > On Wed, Jan 02, 2013 at 06:31:53PM +1300, Tony Prisk wrote: > > > On Wed, 2013-01-02 at 08:10 +0300, Dan Carpenter wrote: > > > > clk_get() returns NULL if CONFIG_HAVE_CLK is disabled. > > > > > > > > I told Tony about this but everyone has been gone with end of year > > > > holidays so it hasn't been addressed. > > > > > > > > Tony, please fix it so people don't apply these patches until > > > > clk_get() is updated to not return NULL. It sucks to have to revert > > > > patches. > > > > > > > > regards, > > > > dan carpenter > > > > > > I posted the query to Mike Turquette, linux-kernel and linux-arm-kernel > > > mailing lists, regarding the return of NULL when HAVE_CLK is undefined. > > > > > > Short Answer: A return value of NULL is valid and not an error therefore > > > we should be using IS_ERR, not IS_ERR_OR_NULL on clk_get results. > > > > > > I see the obvious problem this creates, and asked this question: > > > > > > If the driver can't operate with a NULL clk, it should use a > > > IS_ERR_OR_NULL test to test for failure, rather than IS_ERR. > > > > > > > > > And Russell's answer: > > > > > > Why should a _consumer_ of a clock care? It is _very_ important that > > > people get this idea - to a consumer, the struct clk is just an opaque > > > cookie. The fact that it appears to be a pointer does _not_ mean that > > > the driver can do any kind of dereferencing on that pointer - it should > > > never do so. > > > > > > Thread can be viewed here: > > > https://lkml.org/lkml/2012/12/20/105 > > > > > > > Ah. Grand. Thanks... > > > > Btw. The documentation for clk_get() really should include some of > > this information. I know Russell thinks that the driver authors are > > stupid and lazy, and it's probably true. But if everyone makes the > > same mistake over and over, then it probably means we could put a > > special note: > > > > "Do not check this with IS_ERR_OR_NULL(). Null values are not an > > error. Drivers should treat the return value as an opaque cookie > > and they should not dereference it." > > > > This is probably there in the file somewhere else, but I searched > > for "opaque", "cookie", and "dereference" and I didn't find > > anything. I'm not saying the documentation isn't perfect, just that > > driver authors are lazy and stupid but we can't kill them so we have > > to live with them. > > I still think it would also be helpful for the definition that returns > NULL to have some documentation associated with it. Having a feature > disabled and then trying to use the feature could reasonably considered to > lead to a failure, so it is not obvious what the NULL represents. /** * clk_get - lookup and obtain a reference to a clock producer. * @dev: device for clock "consumer" * @id: clock consumer ID * * Returns a struct clk corresponding to the clock producer, or * valid IS_ERR() condition containing errno. The implementation * uses @dev and @id to determine the clock consumer, and thereby * the clock producer. (IOW, @id may be identical strings, but * clk_get may return different clock producers depending on @dev.) * * Drivers must assume that the clock source is not enabled. * * clk_get should not be called from within interrupt context. */
Come on... Don't say we haven't read comment. Obviously, the first thing we did was read that comment. I've read it many times at this point and I still think we should add in a bit which says: "NOTE: Drivers should treat the return value as an opaque cookie and not dereference it. NULL returns don't imply an error so don't use IS_ERR_OR_NULL() to check for errors." regards, dan carpenter
On Thu, Jan 03, 2013 at 02:10:40PM +0300, Dan Carpenter wrote: > Come on... Don't say we haven't read comment. Obviously, the first > thing we did was read that comment. I've read it many times at this > point and I still think we should add in a bit which says: So where does it give you in that comment permission to treat NULL any differently to any other non-IS_ERR() return value? It is very clear: values where IS_ERR() is true are considered errors. Everything else is considered valid. > "NOTE: Drivers should treat the return value as an opaque cookie > and not dereference it. NULL returns don't imply an error so don't > use IS_ERR_OR_NULL() to check for errors." No. The one thing I've learnt through maintaining www.arm.linux.org.uk is that the more of these kinds of "lets add to documentation" suggestions you get, the more _unclear_ the documentation becomes, and the more it is open to bad interpretation, and the more suggestions to add more words you receive. Concise documentation is the only way to go. And what we have there today is concise and to the point. It specifies it very clearly: * Returns a struct clk corresponding to the clock producer, or * valid IS_ERR() condition containing errno. That one sentence gives you all the information you need about it's return value. It gives you two choices. (1) a return value where IS_ERR() is true, which is an error, and (2) a return value where IS_ERR() is false, which is a valid cookie. Maybe you don't realise, but IS_ERR(NULL) is false. Therefore, this falls into category (2). You can't get clearer than that, unless you don't understand the IS_ERR() and associated macro. Moreover, it tells you the function to use to check the return value for errors. IS_ERR(). It doesn't say IS_ERR_OR_NULL(), it says IS_ERR(). All it takes is for people to engage their grey cells and read the documentation as it stands, rather than trying to weasel their way around it and invent crap that it doesn't say.
On Thu, Jan 03, 2013 at 11:21:02AM +0000, Russell King - ARM Linux wrote: > Maybe you don't realise, but IS_ERR(NULL) is false. Therefore, this falls > into category (2). No, obviously, I know the difference between IS_ERR() and IS_ERR_OR_NULL(). That's how we started this thread. *shrug*. regards, dan carpenter
On Thu, Jan 03, 2013 at 04:45:54PM +0300, Dan Carpenter wrote: > On Thu, Jan 03, 2013 at 11:21:02AM +0000, Russell King - ARM Linux wrote: > > Maybe you don't realise, but IS_ERR(NULL) is false. Therefore, this falls > > into category (2). > > No, obviously, I know the difference between IS_ERR() and > IS_ERR_OR_NULL(). That's how we started this thread. Well, if you know that, then how is the documentation anything _but_ clear?
diff --git a/drivers/media/platform/s5p-g2d/g2d.c b/drivers/media/platform/s5p-g2d/g2d.c index 1bfbc32..dcd5335 100644 --- a/drivers/media/platform/s5p-g2d/g2d.c +++ b/drivers/media/platform/s5p-g2d/g2d.c @@ -715,7 +715,7 @@ static int g2d_probe(struct platform_device *pdev) } dev->clk = clk_get(&pdev->dev, "sclk_fimg2d"); - if (IS_ERR_OR_NULL(dev->clk)) { + if (IS_ERR(dev->clk)) { dev_err(&pdev->dev, "failed to get g2d clock\n"); return -ENXIO; } @@ -727,7 +727,7 @@ static int g2d_probe(struct platform_device *pdev) } dev->gate = clk_get(&pdev->dev, "fimg2d"); - if (IS_ERR_OR_NULL(dev->gate)) { + if (IS_ERR(dev->gate)) { dev_err(&pdev->dev, "failed to get g2d clock gate\n"); ret = -ENXIO; goto unprep_clk;
Resend to include mailing lists. Replace IS_ERR_OR_NULL with IS_ERR on clk_get results. Signed-off-by: Tony Prisk <linux@prisktech.co.nz> CC: Kyungmin Park <kyungmin.park@samsung.com> CC: Tomasz Stanislawski <t.stanislaws@samsung.com> CC: linux-media@vger.kernel.org --- drivers/media/platform/s5p-g2d/g2d.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)