linux-kernel - Re: [PATCH 1/2] Add a common struct clk

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] [day] [month] [year] [list]

Message-ID: <1278901153.9235.6.camel@pororo.lan>
Date:	Mon, 12 Jul 2010 10:19:13 +0800
From:	Jeremy Kerr <jeremy.kerr@...onical.com>
To:	MyungJoo Ham <myungjoo.ham@...il.com>
Cc:	linux-kernel@...r.kernel.org,
	Ben Herrenchmidt <benh@...nel.crashing.org>,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 1/2] Add a common struct clk

Hi MyungJoo,

> > +int clk_enable(struct clk *clk)
> > +{
> > +       int ret = 0;
> > +
> > +       if (!clk->ops->enable)
> > +               return 0;
> 
> Wouldn't it be better (safer?) to check "clk" and "clk->ops" before
> accessing clk->ops->enable?
> For example,
> 
>         if (IS_ERR_OR_NULL(clk))
>                 return -EINVAL;
> 
>         if (!clk->ops || !clk->ops->enable)
>                 return 0;
> 
> Or, do you think it'd be better not to check and save some time?
> 
> Anyway, if we intend to check the input, the patch for the patch
> including kernel/clk.c would be...

I think that we should leave it as-is; although it may be 'safer', it
may mean more subtle bugs can arise because we've been handed an invalid
clock pointer.

I'd rather the code oops on first usage so that the developer realises
that something is broken, rather than fail with an error code.

BTW - nice work on the samsung implementation, I will check it out soon.

Cheers,


Jeremy

--
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/