lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Thu, 3 Jan 2013 12:05:20 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Tony Prisk <linux@...sktech.co.nz>
Cc:	Dan Carpenter <error27@...il.com>,
	Sylwester Nawrocki <sylvester.nawrocki@...il.com>,
	Sergei Shtylyov <sshtylyov@...sta.com>,
	kernel-janitors@...r.kernel.org,
	Tomasz Stanislawski <t.stanislaws@...sung.com>,
	linux-kernel@...r.kernel.org,
	Kyungmin Park <kyungmin.park@...sung.com>,
	linux-arm-kernel@...ts.infradead.org, linux-media@...r.kernel.org
Subject: Re: [PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of
 IS_ERR_OR_NULL

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists