Re: IS_ERR_OR_NULL - please STOP telling people to use this on a whim
On 17/10/12 20:41 +0100, Russell King - ARM Linux wrote:
> People,
>
> This is not aimed at anyone specifically - but it is aimed at everyone
> who reviews patches to make them aware of this issue, and to modify their
> behaviour.
>
> I'm geting sick and tired of telling people about this. I've just
> floated the idea of removing IS_ERR_OR_NULL from the kernel tree because
> it's one of the most incorrectly used and abused macros we have in the
> source tree.
This makes me sad. I was responsible for its introduction, and my motive
was exactly yours in sending the above.
> It would be one thing if this was only being done by people who are
> submitting new code, but it's far worse than that. Reviewers who should
> know better are telling people to use it _incorrectly_.
>
> Reviewers really need to think about your review comments. Looking
> through the kernel tree today, I see lots of uses of IS_ERR_OR_NULL(),
> many of them are *buggy*.
>
> Take a moment to think about this:
>
> int error_value(struct device *dev, void *foo)
> {
> if (IS_ERR_OR_NULL(foo))
> return PTR_ERR(foo);
> return 0;
> }
>
> Consider the value this function returns for three arguments:
>
> 1. an errno encoded pointer
> 2. a NULL pointer.
> 3. a valid pointer.
>
> If you can't see the problem, then *do* *not* tell anyone to use
> IS_ERR_OR_NULL(), because you do *not* have the understanding necessary
> to make that judgement yourself - you're probably telling people to
> create buggy code.
The problem I saw was functions returning -ERRORs or NULL. There were
too many, and there was too much sloppy code inconsistently handling one
or either of the two, and not always both. I did consider trying to fix
some of the core functions that were returning -ERRORs or NULL to the
drivers I was involved in, but it seemed like there were too many, and
that would be too "brave". I imagined that my macro would help catch
that undesirable situation, and permit people to map the error onto
whatever was most appropriate to propagate on.
The idea of them propagating the undesirable problem up further in the call
chain is the exact antithesis of what I intended.
Thank you for highlighting the issue I didn't foresee (neither did my
colleagues at Nokia, they made good use of it fairly quickly) and in
such unambiguous terms. Better to nip it in the bud, certainly.
> Here's the list so far of what looks like buggy uses specific to ARM.
> There _are_ others elsewhere in the kernel.
>
> drivers/media/video/s5p-mfc/s5p_mfc.c: if
> (IS_ERR_OR_NULL(dev->alloc_ctx[0])) {
> drivers/media/video/s5p-mfc/s5p_mfc.c- ret =
> PTR_ERR(dev->alloc_ctx[0]);
> drivers/media/video/s5p-mfc/s5p_mfc.c- goto err_res;
> drivers/media/video/s5p-mfc/s5p_mfc.c- }
...
:-(
So, what to do? It can and has been used sensibly, so I don't think removing
it is the best option.
Phil
"pcarm...@nokia.com" is no more, I'm now "pc+l...@asdf.org"
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH 00/11] ARM: s3c64xx: Let amba-pl08x driver handle DMA
(Apologies if this is mangled, fighting both Outlook and remote desktop :-( ) linux-kernel-ow...@vger.kernel.org wrote on Behalf Of Mark Brown > On Wed, Jun 19, 2013 at 08:26:12PM +0200, Tomasz Figa wrote: > > On Wednesday 19 of June 2013 18:40:47 Mark Brown wrote: > > > > - ret = pd->get_signal(plchan->cd); > > > + ret = (pd->get_signal)(plchan->cd); > > > Hmm, that's strange. The former is a completely valid piece of > code... > > I know, hence... > > > > to get it to build which makes me suspect the compiler a bit as > well... > > ...my comment about suspecting the compiler. Can you just make that minimal change, and diff the objdump of the two .o's? It would be worth a bug-report against the toolchain if different code was being generated. If objdump spews huge numbers of diffs (due to one address changing and pushing everything else out of kilter), then feel free to forward both .o's or both objdumps to me, and I can run a script over them, which knows to ignore unimportant address changes. Praying that this mail is readable to you, as it's not readable as I write it, Phil -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH 00/11] ARM: s3c64xx: Let amba-pl08x driver handle DMA
> -Original Message-
> On Thu, Jun 20, 2013 at 12:24:47PM +0300, Phil Carmody wrote:
> > Can you just make that minimal change, and diff the objdump of the
> two .o's?
> > It would be worth a bug-report against the toolchain if different
> code
> > was being generated. If objdump spews huge numbers of diffs (due to
> > one address changing and pushing everything else out of kilter), then
> > feel free to forward both .o's or both objdumps to me, and I can run
> a
> > script over them, which knows to ignore unimportant address changes.
>
> See Arnd's followup - this looks like a collision with the get_signal
> macro in signal.h.
With my language-lawyer hat on, I'd suggest ``(get_signal)'' to prevent the
macro expansion:
/tmp$ cat crap.c
#define fnlikemacro(foo) foo+
int x(int y) {
int (fnlikemacro) = y;
return fnlikemacro(y)(fnlikemacro);
}
/tmp$ gcc -E crap.c
int x(int y) {
int (fnlikemacro) = y;
return y+(fnlikemacro);
}
(and yes, that compiles.)
However, it's more tempting (i.e. sensible) to just rename the
one with the weaker claim to the name.
Phil
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2 6/8] ARM: dts: Add camera subsystem nodes to exynos4x12.dtsi
Sylwester Nawrocki wrote:
Add common camera node and Exynos4212/4412 specific nodes for
FIMC, MIPI-CSIS, FIMC-LITE and FIMC-IS devices.
Signed-off-by: Sylwester Nawrocki
Signed-off-by: Kyungmin Park
---
arch/arm/boot/dts/exynos4x12.dtsi | 118 +
1 file changed, 118 insertions(+)
diff --git a/arch/arm/boot/dts/exynos4x12.dtsi
b/arch/arm/boot/dts/exynos4x12.dtsi
index 0e24d85..f8cc1d0 100644
--- a/arch/arm/boot/dts/exynos4x12.dtsi
+++ b/arch/arm/boot/dts/exynos4x12.dtsi
@@ -26,6 +26,8 @@
pinctrl1 = &pinctrl_1;
pinctrl2 = &pinctrl_2;
pinctrl3 = &pinctrl_3;
+ fimc-lite0 = &fimc_lite_0;
+ fimc-lite1 = &fimc_lite_1;
};
pd_isp: isp-power-domain@10023CA0 {
@@ -78,4 +80,120 @@
clock-names = "sclk_fimg2d", "fimg2d";
status = "disabled";
};
+
+ camera {
+ clocks = <&clock 132>, <&clock 133>, <&clock 351>, <&clock 352>,
+<&clock 388>, <&clock 389>, <&clock 17>;
+ clock-names = "sclk_cam0", "sclk_cam1", "pxl_async0",
+ "pxl_async1", "mux_cam0", "mux_cam1", "parent";
+
+ fimc_0: fimc@1180 {
+ compatible = "samsung,exynos4212-fimc";
+ clocks = <&clock 256>, <&clock 128>, <&clock 384>,
<&clock 17>;
+ clock-names = "fimc", "sclk_fimc", "mux", "parent";
+ samsung,pix-limits = <4224 8192 1920 4224>;
+ samsung,mainscaler-ext;
+ samsung,isp-wb;
+ samsung,cam-if;
+ };
+
+ fimc_1: fimc@1181 {
+ compatible = "samsung,exynos4212-fimc";
+ clocks = <&clock 257>, <&clock 129>, <&clock 385>,
<&clock 17>;
+ clock-names = "fimc", "sclk_fimc", "mux", "parent";
+ samsung,pix-limits = <4224 8192 1920 4224>;
+ samsung,mainscaler-ext;
+ samsung,isp-wb;
+ samsung,cam-if;
+ };
+
+ fimc_2: fimc@1182 {
+ compatible = "samsung,exynos4212-fimc";
+ clocks = <&clock 258>, <&clock 130>, <&clock 386>,
<&clock 17>;
+ clock-names = "fimc", "sclk_fimc", "mux", "parent";
+ samsung,pix-limits = <4224 8192 1920 4224>;
+ samsung,mainscaler-ext;
+ samsung,isp-wb;
+ samsung,lcd-wb;
+ samsung,cam-if;
+ };
+
+ fimc_3: fimc@1183 {
+ compatible = "samsung,exynos4212-fimc";
+ clocks = <&clock 259>, <&clock 131>, <&clock 387>,
<&clock 17>;
+ clock-names = "fimc", "sclk_fimc", "mux", "parent";
+ samsung,pix-limits = <1920 8192 1366 1920>;
+ samsung,rotators = <0>;
+ samsung,mainscaler-ext;
+ samsung,isp-wb;
+ samsung,lcd-wb;
+ };
+
+ csis_0: csis@1188 {
+ clocks = <&clock 260>, <&clock 134>, <&clock 390>,
<&clock 17>;
+ clock-names = "csis", "sclk_csis", "mux", "parent";
+ };
+
+ csis_1: csis@1189 {
+ clocks = <&clock 261>, <&clock 135>, <&clock 391>,
<&clock 17>;
+ clock-names = "csis", "sclk_csis", "mux", "parent";
+ };
+
+ fimc_lite_0: fimc-lite@1239 {
+ compatible = "samsung,exynos4212-fimc-lite";
+ reg = <0x1239 0x1000>;
+ interrupts = <0 105 0>;
+ samsung,power-domain = <&pd_isp>;
+ clocks = <&clock 353>;
+ clock-names = "flite";
+ status = "disabled";
+ };
+
+ fimc_lite_1: fimc-lite@123A {
+ compatible = "samsung,exynos4212-fimc-lite";
+ reg = <0x123A 0x1000>;
+ interrupts = <0 106 0>;
+ samsung,power-domain = <&pd_isp>;
+ clocks = <&clock 354>;
+ clock-names = "flite";
+ status = "disabled";
+ };
+
+ fimc_is: fimc-is@1200 {
+ compatible = "samsung,exynos4212-fimc-is", "simple-bus";
+ reg = <0x1200 0x26>;
+ interrupts = <0 90 0>, <0 95 0>;
+ samsung,power-domain = <&pd_isp>;
+ clocks = <&clock 353>, <&clock 354>, <&clock 355>,
+ <&clock 356>
