[PATCH] omapfb/dss: Delete an error message for a failed memory allocation in three functions

[PATCH] omapfb/dss: Delete an error message for a failed memory allocation in three functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 26 Nov 2017 19:46:09 +0100

Omit an extra message for a memory allocation failure in these functions.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/video/fbdev/omap2/omapfb/dss/dispc.c    | 4 +---
 drivers/video/fbdev/omap2/omapfb/dss/dss.c      | 4 +---
 drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c | 4 +---
 3 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dispc.c b/drivers/video/fbdev/omap2/omapfb/dss/dispc.c
index 7a75dfda9845..10164a3bae4a 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/dispc.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/dispc.c
@@ -3982,10 +3982,8 @@ static int dispc_init_features(struct platform_device *pdev)
 	struct dispc_features *dst;
 
 	dst = devm_kzalloc(&pdev->dev, sizeof(*dst), GFP_KERNEL);
-	if (!dst) {
-		dev_err(&pdev->dev, "Failed to allocate DISPC Features\n");
+	if (!dst)
 		return -ENOMEM;
-	}
 
 	switch (omapdss_get_version()) {
 	case OMAPDSS_VER_OMAP24xx:
diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dss.c b/drivers/video/fbdev/omap2/omapfb/dss/dss.c
index 48c6500c24e1..a5de13777e2b 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/dss.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/dss.c
@@ -893,10 +893,8 @@ static int dss_init_features(struct platform_device *pdev)
 	struct dss_features *dst;
 
 	dst = devm_kzalloc(&pdev->dev, sizeof(*dst), GFP_KERNEL);
-	if (!dst) {
-		dev_err(&pdev->dev, "Failed to allocate local DSS Features\n");
+	if (!dst)
 		return -ENOMEM;
-	}
 
 	switch (omapdss_get_version()) {
 	case OMAPDSS_VER_OMAP24xx:
diff --git a/drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c b/drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c
index 9a13c35fd6d8..d25eea10c665 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c
@@ -195,10 +195,8 @@ static int hdmi_phy_init_features(struct platform_device *pdev)
 	const struct hdmi_phy_features *src;
 
 	dst = devm_kzalloc(&pdev->dev, sizeof(*dst), GFP_KERNEL);
-	if (!dst) {
-		dev_err(&pdev->dev, "Failed to allocate HDMI PHY Features\n");
+	if (!dst)
 		return -ENOMEM;
-	}
 
 	switch (omapdss_get_version()) {
 	case OMAPDSS_VER_OMAP4430_ES1:
-- 
2.15.0

Re: [PATCH] omapfb/dss: Delete an error message for a failed memory allocation in three functions

[Andrew F. Davis]

On 11/26/2017 12:55 PM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 26 Nov 2017 19:46:09 +0100
> 
> Omit an extra message for a memory allocation failure in these functions.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---

nak, unlike many others, these message give extra info on which
allocation failed, that can be useful.

>  drivers/video/fbdev/omap2/omapfb/dss/dispc.c    | 4 +---
>  drivers/video/fbdev/omap2/omapfb/dss/dss.c      | 4 +---
>  drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c | 4 +---
>  3 files changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dispc.c b/drivers/video/fbdev/omap2/omapfb/dss/dispc.c
> index 7a75dfda9845..10164a3bae4a 100644
> --- a/drivers/video/fbdev/omap2/omapfb/dss/dispc.c
> +++ b/drivers/video/fbdev/omap2/omapfb/dss/dispc.c
> @@ -3982,10 +3982,8 @@ static int dispc_init_features(struct platform_device *pdev)
>  	struct dispc_features *dst;
>  
>  	dst = devm_kzalloc(&pdev->dev, sizeof(*dst), GFP_KERNEL);
> -	if (!dst) {
> -		dev_err(&pdev->dev, "Failed to allocate DISPC Features\n");
> +	if (!dst)
>  		return -ENOMEM;
> -	}
>  
>  	switch (omapdss_get_version()) {
>  	case OMAPDSS_VER_OMAP24xx:
> diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dss.c b/drivers/video/fbdev/omap2/omapfb/dss/dss.c
> index 48c6500c24e1..a5de13777e2b 100644
> --- a/drivers/video/fbdev/omap2/omapfb/dss/dss.c
> +++ b/drivers/video/fbdev/omap2/omapfb/dss/dss.c
> @@ -893,10 +893,8 @@ static int dss_init_features(struct platform_device *pdev)
>  	struct dss_features *dst;
>  
>  	dst = devm_kzalloc(&pdev->dev, sizeof(*dst), GFP_KERNEL);
> -	if (!dst) {
> -		dev_err(&pdev->dev, "Failed to allocate local DSS Features\n");
> +	if (!dst)
>  		return -ENOMEM;
> -	}
>  
>  	switch (omapdss_get_version()) {
>  	case OMAPDSS_VER_OMAP24xx:
> diff --git a/drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c b/drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c
> index 9a13c35fd6d8..d25eea10c665 100644
> --- a/drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c
> +++ b/drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c
> @@ -195,10 +195,8 @@ static int hdmi_phy_init_features(struct platform_device *pdev)
>  	const struct hdmi_phy_features *src;
>  
>  	dst = devm_kzalloc(&pdev->dev, sizeof(*dst), GFP_KERNEL);
> -	if (!dst) {
> -		dev_err(&pdev->dev, "Failed to allocate HDMI PHY Features\n");
> +	if (!dst)
>  		return -ENOMEM;
> -	}
>  
>  	switch (omapdss_get_version()) {
>  	case OMAPDSS_VER_OMAP4430_ES1:
> 

Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions

[SF Markus Elfring]

>> Omit an extra message for a memory allocation failure in these functions.
…
> nak, unlike many others, these message give extra info on which
> allocation failed, that can be useful.

Can a default allocation failure report provide the information
which you might expect so far?

Regards,
Markus

Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions

[Ladislav Michl]

On Mon, Nov 27, 2017 at 06:27:08PM +0100, SF Markus Elfring wrote:
> >> Omit an extra message for a memory allocation failure in these functions.
> …
> > nak, unlike many others, these message give extra info on which
> > allocation failed, that can be useful.
> 
> Can a default allocation failure report provide the information
> which you might expect so far?

You should be able to answer that question yourself. And if you are
unable to do so, just do not sent changes pointed by any code analysis
tools.

As a side note, if you look at whole call chain, you'll find quite some
room for optimizations - look how dev and pdev are used. That also applies
to other patches.

Best regards,
	ladis

Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions

[SF Markus Elfring]

>> Can a default allocation failure report provide the information
>> which you might expect so far?
> 
> You should be able to answer that question yourself.

I can not answer this detail completely myself because my knowledge
is incomplete about your concrete expectations for the exception handling
which can lead to different views for the need of additional error messages.


> And if you are unable to do so, just do not sent changes pointed
> by any code analysis tools.

They can point aspects out for further software development considerations,
can't they?


> As a side note, if you look at whole call chain, you'll find quite some
> room for optimizations - look how dev and pdev are used. That also applies
> to other patches.

Would you prefer to improve the source code in other ways?

Regards,
Markus

Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions

[Geert Uytterhoeven]

Hi Markus,

On Mon, Nov 27, 2017 at 7:12 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>>> Can a default allocation failure report provide the information
>>> which you might expect so far?
>>
>> You should be able to answer that question yourself.
>
> I can not answer this detail completely myself because my knowledge
> is incomplete about your concrete expectations for the exception handling
> which can lead to different views for the need of additional error messages.

It may be a good idea to try to trigger an out-of-memory condition yourself,
and see what happens?

>> And if you are unable to do so, just do not sent changes pointed
>> by any code analysis tools.
>
> They can point aspects out for further software development considerations,
> can't they?

Sure. But I think it is a good experience to witness what can happen if you
"violate" these "coding standards written by other people", and learn to
understand why they were written, increasing your own knowledge.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions

[Ladislav Michl]

On Mon, Nov 27, 2017 at 07:12:50PM +0100, SF Markus Elfring wrote:
> >> Can a default allocation failure report provide the information
> >> which you might expect so far?
> > 
> > You should be able to answer that question yourself.
> 
> I can not answer this detail completely myself because my knowledge
> is incomplete about your concrete expectations for the exception handling
> which can lead to different views for the need of additional error messages.

The rule is to be able to get idea what failed without recompiling kernel.
If you think you can point to failed allocation with your changes, then
you might be able to convice Andrew your change is an improvement.

> > And if you are unable to do so, just do not sent changes pointed
> > by any code analysis tools.
> 
> They can point aspects out for further software development considerations,
> can't they?

So?

> > As a side note, if you look at whole call chain, you'll find quite some
> > room for optimizations - look how dev and pdev are used. That also applies
> > to other patches.
> 
> Would you prefer to improve the source code in other ways?

I would prefer you to look at code as a whole, not as an isolated set
of lines which can be changes to shut up some random warning obtained
from code analysis tool.

Behold, here we saved over 100 bytes just by minor clean up. Patch
is a mess, should be probably polished and splitted, but you'll get
an idea. That said, we saved more than by deleting one error message
and if you can prove we will not loose information _what_ failed
with your patch, we can save even more.

   text	   data	    bss	    dec	    hex	filename
  59319	   2372	   4184	  65875	  10153	dispc.o.orig
  59178	   2372	   4184	  65734	  100c6	dispc.o

There is intentionally no signoff...

diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dispc.c b/drivers/video/fbdev/omap2/omapfb/dss/dispc.c
index 7a75dfda9845..4c8463957634 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/dispc.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/dispc.c
@@ -3976,52 +3976,33 @@ static const struct dispc_features omap54xx_dispc_feats = {
 	.has_writeback		=	true,
 };
 
-static int dispc_init_features(struct platform_device *pdev)
+static const struct dispc_features* dispc_get_features(void)
 {
-	const struct dispc_features *src;
-	struct dispc_features *dst;
-
-	dst = devm_kzalloc(&pdev->dev, sizeof(*dst), GFP_KERNEL);
-	if (!dst) {
-		dev_err(&pdev->dev, "Failed to allocate DISPC Features\n");
-		return -ENOMEM;
-	}
-
 	switch (omapdss_get_version()) {
 	case OMAPDSS_VER_OMAP24xx:
-		src = &omap24xx_dispc_feats;
-		break;
+		return &omap24xx_dispc_feats;
 
 	case OMAPDSS_VER_OMAP34xx_ES1:
-		src = &omap34xx_rev1_0_dispc_feats;
-		break;
+		return &omap34xx_rev1_0_dispc_feats;
 
 	case OMAPDSS_VER_OMAP34xx_ES3:
 	case OMAPDSS_VER_OMAP3630:
 	case OMAPDSS_VER_AM35xx:
 	case OMAPDSS_VER_AM43xx:
-		src = &omap34xx_rev3_0_dispc_feats;
-		break;
+		return &omap34xx_rev3_0_dispc_feats;
 
 	case OMAPDSS_VER_OMAP4430_ES1:
 	case OMAPDSS_VER_OMAP4430_ES2:
 	case OMAPDSS_VER_OMAP4:
-		src = &omap44xx_dispc_feats;
-		break;
+		return &omap44xx_dispc_feats;
 
 	case OMAPDSS_VER_OMAP5:
 	case OMAPDSS_VER_DRA7xx:
-		src = &omap54xx_dispc_feats;
-		break;
+		return &omap54xx_dispc_feats;
 
 	default:
-		return -ENODEV;
+		return NULL;
 	}
-
-	memcpy(dst, src, sizeof(*dst));
-	dispc.feat = dst;
-
-	return 0;
 }
 
 static irqreturn_t dispc_irq_handler(int irq, void *arg)
@@ -4068,54 +4049,61 @@ EXPORT_SYMBOL(dispc_free_irq);
 /* DISPC HW IP initialisation */
 static int dispc_bind(struct device *dev, struct device *master, void *data)
 {
-	struct platform_device *pdev = to_platform_device(dev);
 	u32 rev;
 	int r = 0;
+	const struct dispc_features *features;
 	struct resource *dispc_mem;
-	struct device_node *np = pdev->dev.of_node;
+	struct device_node *np = dev->of_node;
 
-	dispc.pdev = pdev;
+	dispc.pdev = to_platform_device(dev);
 
 	spin_lock_init(&dispc.control_lock);
 
-	r = dispc_init_features(dispc.pdev);
-	if (r)
-		return r;
+	features = dispc_get_features();
+	if (!features)
+		return -ENODEV;
+
+	dispc.feat = devm_kzalloc(dev, sizeof(*features), GFP_KERNEL);
+	if (dispc.feat) {
+		dev_err(dev, "Failed to allocate DISPC Features\n");
+		return -ENOMEM;
+	}
+	memcpy(dispc.feat, features, sizeof(*features));
 
 	dispc_mem = platform_get_resource(dispc.pdev, IORESOURCE_MEM, 0);
 	if (!dispc_mem) {
-		DSSERR("can't get IORESOURCE_MEM DISPC\n");
+		dev_err(dev, "can't get IORESOURCE_MEM DISPC\n");
 		return -EINVAL;
 	}
 
-	dispc.base = devm_ioremap(&pdev->dev, dispc_mem->start,
+	dispc.base = devm_ioremap(dev, dispc_mem->start,
 				  resource_size(dispc_mem));
 	if (!dispc.base) {
-		DSSERR("can't ioremap DISPC\n");
+		dev_err(dev, "can't ioremap DISPC\n");
 		return -ENOMEM;
 	}
 
 	dispc.irq = platform_get_irq(dispc.pdev, 0);
 	if (dispc.irq < 0) {
-		DSSERR("platform_get_irq failed\n");
+		dev_err(dev, "platform_get_irq failed\n");
 		return -ENODEV;
 	}
 
 	if (np && of_property_read_bool(np, "syscon-pol")) {
 		dispc.syscon_pol = syscon_regmap_lookup_by_phandle(np, "syscon-pol");
 		if (IS_ERR(dispc.syscon_pol)) {
-			dev_err(&pdev->dev, "failed to get syscon-pol regmap\n");
+			dev_err(dev, "failed to get syscon-pol regmap\n");
 			return PTR_ERR(dispc.syscon_pol);
 		}
 
 		if (of_property_read_u32_index(np, "syscon-pol", 1,
 				&dispc.syscon_pol_offset)) {
-			dev_err(&pdev->dev, "failed to get syscon-pol offset\n");
+			dev_err(dev, "failed to get syscon-pol offset\n");
 			return -EINVAL;
 		}
 	}
 
-	pm_runtime_enable(&pdev->dev);
+	pm_runtime_enable(dev);
 
 	r = dispc_runtime_get();
 	if (r)
@@ -4124,7 +4112,7 @@ static int dispc_bind(struct device *dev, struct device *master, void *data)
 	_omap_dispc_initial_config();
 
 	rev = dispc_read_reg(DISPC_REVISION);
-	dev_dbg(&pdev->dev, "OMAP DISPC rev %d.%d\n",
+	dev_dbg(dev, "OMAP DISPC rev %d.%d\n",
 	       FLD_GET(rev, 7, 4), FLD_GET(rev, 3, 0));
 
 	dispc_runtime_put();
@@ -4136,7 +4124,7 @@ static int dispc_bind(struct device *dev, struct device *master, void *data)
 	return 0;
 
 err_runtime_get:
-	pm_runtime_disable(&pdev->dev);
+	pm_runtime_disable(dev);
 	return r;
 }
 

Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions

[Ladislav Michl]

On Mon, Nov 27, 2017 at 08:22:51PM +0100, Ladislav Michl wrote:
> On Mon, Nov 27, 2017 at 07:12:50PM +0100, SF Markus Elfring wrote:
> > >> Can a default allocation failure report provide the information
> > >> which you might expect so far?
> > > 
> > > You should be able to answer that question yourself.
> > 
> > I can not answer this detail completely myself because my knowledge
> > is incomplete about your concrete expectations for the exception handling
> > which can lead to different views for the need of additional error messages.
> 
> The rule is to be able to get idea what failed without recompiling kernel.
> If you think you can point to failed allocation with your changes, then
> you might be able to convice Andrew your change is an improvement.
> 
> > > And if you are unable to do so, just do not sent changes pointed
> > > by any code analysis tools.
> > 
> > They can point aspects out for further software development considerations,
> > can't they?
> 
> So?
> 
> > > As a side note, if you look at whole call chain, you'll find quite some
> > > room for optimizations - look how dev and pdev are used. That also applies
> > > to other patches.
> > 
> > Would you prefer to improve the source code in other ways?
> 
> I would prefer you to look at code as a whole, not as an isolated set
> of lines which can be changes to shut up some random warning obtained
> from code analysis tool.
> 
> Behold, here we saved over 100 bytes just by minor clean up. Patch
> is a mess, should be probably polished and splitted, but you'll get
> an idea. That said, we saved more than by deleting one error message
> and if you can prove we will not loose information _what_ failed
> with your patch, we can save even more.

Well, if we remove allocation, we do not need to print error message...
And numbers are even better.

>    text	   data	    bss	    dec	    hex	filename
>   59319	   2372	   4184	  65875	  10153	dispc.o.orig
>   59178	   2372	   4184	  65734	  100c6	dispc.o
    59095	   2372	   4184	  65651	  10073	dispc.o.noalloc

Care to test this? It is a mess of unrelated changes, but I'm just
interested if it works...

diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dispc.c b/drivers/video/fbdev/omap2/omapfb/dss/dispc.c
index 7a75dfda9845..f6d631a68c70 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/dispc.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/dispc.c
@@ -3976,52 +3976,33 @@ static const struct dispc_features omap54xx_dispc_feats = {
 	.has_writeback		=	true,
 };
 
-static int dispc_init_features(struct platform_device *pdev)
+static const struct dispc_features* dispc_get_features(void)
 {
-	const struct dispc_features *src;
-	struct dispc_features *dst;
-
-	dst = devm_kzalloc(&pdev->dev, sizeof(*dst), GFP_KERNEL);
-	if (!dst) {
-		dev_err(&pdev->dev, "Failed to allocate DISPC Features\n");
-		return -ENOMEM;
-	}
-
 	switch (omapdss_get_version()) {
 	case OMAPDSS_VER_OMAP24xx:
-		src = &omap24xx_dispc_feats;
-		break;
+		return &omap24xx_dispc_feats;
 
 	case OMAPDSS_VER_OMAP34xx_ES1:
-		src = &omap34xx_rev1_0_dispc_feats;
-		break;
+		return &omap34xx_rev1_0_dispc_feats;
 
 	case OMAPDSS_VER_OMAP34xx_ES3:
 	case OMAPDSS_VER_OMAP3630:
 	case OMAPDSS_VER_AM35xx:
 	case OMAPDSS_VER_AM43xx:
-		src = &omap34xx_rev3_0_dispc_feats;
-		break;
+		return &omap34xx_rev3_0_dispc_feats;
 
 	case OMAPDSS_VER_OMAP4430_ES1:
 	case OMAPDSS_VER_OMAP4430_ES2:
 	case OMAPDSS_VER_OMAP4:
-		src = &omap44xx_dispc_feats;
-		break;
+		return &omap44xx_dispc_feats;
 
 	case OMAPDSS_VER_OMAP5:
 	case OMAPDSS_VER_DRA7xx:
-		src = &omap54xx_dispc_feats;
-		break;
+		return &omap54xx_dispc_feats;
 
 	default:
-		return -ENODEV;
+		return NULL;
 	}
-
-	memcpy(dst, src, sizeof(*dst));
-	dispc.feat = dst;
-
-	return 0;
 }
 
 static irqreturn_t dispc_irq_handler(int irq, void *arg)
@@ -4068,54 +4049,53 @@ EXPORT_SYMBOL(dispc_free_irq);
 /* DISPC HW IP initialisation */
 static int dispc_bind(struct device *dev, struct device *master, void *data)
 {
-	struct platform_device *pdev = to_platform_device(dev);
 	u32 rev;
 	int r = 0;
 	struct resource *dispc_mem;
-	struct device_node *np = pdev->dev.of_node;
+	struct device_node *np = dev->of_node;
 
-	dispc.pdev = pdev;
+	dispc.pdev = to_platform_device(dev);
 
 	spin_lock_init(&dispc.control_lock);
 
-	r = dispc_init_features(dispc.pdev);
-	if (r)
-		return r;
+	dispc.feat = dispc_get_features();
+	if (dispc.feat)
+		return -ENODEV;
 
 	dispc_mem = platform_get_resource(dispc.pdev, IORESOURCE_MEM, 0);
 	if (!dispc_mem) {
-		DSSERR("can't get IORESOURCE_MEM DISPC\n");
+		dev_err(dev, "can't get IORESOURCE_MEM DISPC\n");
 		return -EINVAL;
 	}
 
-	dispc.base = devm_ioremap(&pdev->dev, dispc_mem->start,
+	dispc.base = devm_ioremap(dev, dispc_mem->start,
 				  resource_size(dispc_mem));
 	if (!dispc.base) {
-		DSSERR("can't ioremap DISPC\n");
+		dev_err(dev, "can't ioremap DISPC\n");
 		return -ENOMEM;
 	}
 
 	dispc.irq = platform_get_irq(dispc.pdev, 0);
 	if (dispc.irq < 0) {
-		DSSERR("platform_get_irq failed\n");
+		dev_err(dev, "platform_get_irq failed\n");
 		return -ENODEV;
 	}
 
 	if (np && of_property_read_bool(np, "syscon-pol")) {
 		dispc.syscon_pol = syscon_regmap_lookup_by_phandle(np, "syscon-pol");
 		if (IS_ERR(dispc.syscon_pol)) {
-			dev_err(&pdev->dev, "failed to get syscon-pol regmap\n");
+			dev_err(dev, "failed to get syscon-pol regmap\n");
 			return PTR_ERR(dispc.syscon_pol);
 		}
 
 		if (of_property_read_u32_index(np, "syscon-pol", 1,
 				&dispc.syscon_pol_offset)) {
-			dev_err(&pdev->dev, "failed to get syscon-pol offset\n");
+			dev_err(dev, "failed to get syscon-pol offset\n");
 			return -EINVAL;
 		}
 	}
 
-	pm_runtime_enable(&pdev->dev);
+	pm_runtime_enable(dev);
 
 	r = dispc_runtime_get();
 	if (r)
@@ -4124,7 +4104,7 @@ static int dispc_bind(struct device *dev, struct device *master, void *data)
 	_omap_dispc_initial_config();
 
 	rev = dispc_read_reg(DISPC_REVISION);
-	dev_dbg(&pdev->dev, "OMAP DISPC rev %d.%d\n",
+	dev_dbg(dev, "OMAP DISPC rev %d.%d\n",
 	       FLD_GET(rev, 7, 4), FLD_GET(rev, 3, 0));
 
 	dispc_runtime_put();
@@ -4136,7 +4116,7 @@ static int dispc_bind(struct device *dev, struct device *master, void *data)
 	return 0;
 
 err_runtime_get:
-	pm_runtime_disable(&pdev->dev);
+	pm_runtime_disable(dev);
 	return r;
 }
 

Re: [PATCH] omapfb/dss: Delete an error message for a failed memory allocation in three functions

[Joe Perches]

On Mon, 2017-11-27 at 10:43 -0600, Andrew F. Davis wrote:
> On 11/26/2017 12:55 PM, SF Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Sun, 26 Nov 2017 19:46:09 +0100
> > 
> > Omit an extra message for a memory allocation failure in these functions.
> > 
> > This issue was detected by using the Coccinelle software.
> > 
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
> 
> nak, unlike many others, these message give extra info on which
> allocation failed, that can be useful.

<shrug>  Not really.  There are tradeoffs.

There is the generic stack dump on OOM so the module/line
is already known.

The existence of these messages increases code size which
also make the OOM condition slightly more likely.

These are generally used only at initialization and those
if you are OOM at initialization, bad things happen anyway
so where the specific OOM occurred doesn't really matter.

Markus' commit messages are always really poor descriptions
of why these removals are somewhat useful and the commit
could/should/might be applied.

Your choice.

> >  drivers/video/fbdev/omap2/omapfb/dss/dispc.c    | 4 +---
> >  drivers/video/fbdev/omap2/omapfb/dss/dss.c      | 4 +---
> >  drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c | 4 +---
> >  3 files changed, 3 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dispc.c b/drivers/video/fbdev/omap2/omapfb/dss/dispc.c
> > index 7a75dfda9845..10164a3bae4a 100644
> > --- a/drivers/video/fbdev/omap2/omapfb/dss/dispc.c
> > +++ b/drivers/video/fbdev/omap2/omapfb/dss/dispc.c
> > @@ -3982,10 +3982,8 @@ static int dispc_init_features(struct platform_device *pdev)
> >  	struct dispc_features *dst;
> >  
> >  	dst = devm_kzalloc(&pdev->dev, sizeof(*dst), GFP_KERNEL);
> > -	if (!dst) {
> > -		dev_err(&pdev->dev, "Failed to allocate DISPC Features\n");
> > +	if (!dst)
> >  		return -ENOMEM;
> > -	}
> >  
> >  	switch (omapdss_get_version()) {
> >  	case OMAPDSS_VER_OMAP24xx:
> > diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dss.c b/drivers/video/fbdev/omap2/omapfb/dss/dss.c
> > index 48c6500c24e1..a5de13777e2b 100644
> > --- a/drivers/video/fbdev/omap2/omapfb/dss/dss.c
> > +++ b/drivers/video/fbdev/omap2/omapfb/dss/dss.c
> > @@ -893,10 +893,8 @@ static int dss_init_features(struct platform_device *pdev)
> >  	struct dss_features *dst;
> >  
> >  	dst = devm_kzalloc(&pdev->dev, sizeof(*dst), GFP_KERNEL);
> > -	if (!dst) {
> > -		dev_err(&pdev->dev, "Failed to allocate local DSS Features\n");
> > +	if (!dst)
> >  		return -ENOMEM;
> > -	}
> >  
> >  	switch (omapdss_get_version()) {
> >  	case OMAPDSS_VER_OMAP24xx:
> > diff --git a/drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c b/drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c
> > index 9a13c35fd6d8..d25eea10c665 100644
> > --- a/drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c
> > +++ b/drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c
> > @@ -195,10 +195,8 @@ static int hdmi_phy_init_features(struct platform_device *pdev)
> >  	const struct hdmi_phy_features *src;
> >  
> >  	dst = devm_kzalloc(&pdev->dev, sizeof(*dst), GFP_KERNEL);
> > -	if (!dst) {
> > -		dev_err(&pdev->dev, "Failed to allocate HDMI PHY Features\n");
> > +	if (!dst)
> >  		return -ENOMEM;
> > -	}
> >  
> >  	switch (omapdss_get_version()) {
> >  	case OMAPDSS_VER_OMAP4430_ES1:
> > 

Re: [PATCH] omapfb/dss: Delete an error message for a failed memory allocation in three functions

[Andrew F. Davis]

On 11/27/2017 01:07 PM, Joe Perches wrote:
> On Mon, 2017-11-27 at 10:43 -0600, Andrew F. Davis wrote:
>> On 11/26/2017 12:55 PM, SF Markus Elfring wrote:
>>> From: Markus Elfring <elfring@users.sourceforge.net>
>>> Date: Sun, 26 Nov 2017 19:46:09 +0100
>>>
>>> Omit an extra message for a memory allocation failure in these functions.
>>>
>>> This issue was detected by using the Coccinelle software.
>>>
>>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>>> ---
>>
>> nak, unlike many others, these message give extra info on which
>> allocation failed, that can be useful.
> 
> <shrug>  Not really.  There are tradeoffs.
> 
> There is the generic stack dump on OOM so the module/line
> is already known.
> 

If that is the case then I have no strong feelings either way.

> The existence of these messages increases code size which
> also make the OOM condition slightly more likely.
> 
> These are generally used only at initialization and those
> if you are OOM at initialization, bad things happen anyway
> so where the specific OOM occurred doesn't really matter.
> 

True, these messages will probably only ever get displayed if someone is
messing with the allocated structs and accidentally balloons their size,
so these are more debug statements than anything.

> Markus' commit messages are always really poor descriptions
> of why these removals are somewhat useful and the commit
> could/should/might be applied.
> 
> Your choice.
> 
>>>  drivers/video/fbdev/omap2/omapfb/dss/dispc.c    | 4 +---
>>>  drivers/video/fbdev/omap2/omapfb/dss/dss.c      | 4 +---
>>>  drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c | 4 +---
>>>  3 files changed, 3 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dispc.c b/drivers/video/fbdev/omap2/omapfb/dss/dispc.c
>>> index 7a75dfda9845..10164a3bae4a 100644
>>> --- a/drivers/video/fbdev/omap2/omapfb/dss/dispc.c
>>> +++ b/drivers/video/fbdev/omap2/omapfb/dss/dispc.c
>>> @@ -3982,10 +3982,8 @@ static int dispc_init_features(struct platform_device *pdev)
>>>  	struct dispc_features *dst;
>>>  
>>>  	dst = devm_kzalloc(&pdev->dev, sizeof(*dst), GFP_KERNEL);
>>> -	if (!dst) {
>>> -		dev_err(&pdev->dev, "Failed to allocate DISPC Features\n");
>>> +	if (!dst)
>>>  		return -ENOMEM;
>>> -	}
>>>  
>>>  	switch (omapdss_get_version()) {
>>>  	case OMAPDSS_VER_OMAP24xx:
>>> diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dss.c b/drivers/video/fbdev/omap2/omapfb/dss/dss.c
>>> index 48c6500c24e1..a5de13777e2b 100644
>>> --- a/drivers/video/fbdev/omap2/omapfb/dss/dss.c
>>> +++ b/drivers/video/fbdev/omap2/omapfb/dss/dss.c
>>> @@ -893,10 +893,8 @@ static int dss_init_features(struct platform_device *pdev)
>>>  	struct dss_features *dst;
>>>  
>>>  	dst = devm_kzalloc(&pdev->dev, sizeof(*dst), GFP_KERNEL);
>>> -	if (!dst) {
>>> -		dev_err(&pdev->dev, "Failed to allocate local DSS Features\n");
>>> +	if (!dst)
>>>  		return -ENOMEM;
>>> -	}
>>>  
>>>  	switch (omapdss_get_version()) {
>>>  	case OMAPDSS_VER_OMAP24xx:
>>> diff --git a/drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c b/drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c
>>> index 9a13c35fd6d8..d25eea10c665 100644
>>> --- a/drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c
>>> +++ b/drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c
>>> @@ -195,10 +195,8 @@ static int hdmi_phy_init_features(struct platform_device *pdev)
>>>  	const struct hdmi_phy_features *src;
>>>  
>>>  	dst = devm_kzalloc(&pdev->dev, sizeof(*dst), GFP_KERNEL);
>>> -	if (!dst) {
>>> -		dev_err(&pdev->dev, "Failed to allocate HDMI PHY Features\n");
>>> +	if (!dst)
>>>  		return -ENOMEM;
>>> -	}
>>>  
>>>  	switch (omapdss_get_version()) {
>>>  	case OMAPDSS_VER_OMAP4430_ES1:
>>>

Re: [PATCH] omapfb/dss: Delete an error message for a failed memory allocation in three functions

[Ladislav Michl]

On Mon, Nov 27, 2017 at 03:33:13PM -0600, Andrew F. Davis wrote:
> On 11/27/2017 01:07 PM, Joe Perches wrote:
> > On Mon, 2017-11-27 at 10:43 -0600, Andrew F. Davis wrote:
> >> On 11/26/2017 12:55 PM, SF Markus Elfring wrote:
> >>> From: Markus Elfring <elfring@users.sourceforge.net>
> >>> Date: Sun, 26 Nov 2017 19:46:09 +0100
> >>>
> >>> Omit an extra message for a memory allocation failure in these functions.
> >>>
> >>> This issue was detected by using the Coccinelle software.
> >>>
> >>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> >>> ---
> >>
> >> nak, unlike many others, these message give extra info on which
> >> allocation failed, that can be useful.
> > 
> > <shrug>  Not really.  There are tradeoffs.
> > 
> > There is the generic stack dump on OOM so the module/line
> > is already known.
> > 
> 
> If that is the case then I have no strong feelings either way.
> 
> > The existence of these messages increases code size which
> > also make the OOM condition slightly more likely.
> > 
> > These are generally used only at initialization and those
> > if you are OOM at initialization, bad things happen anyway
> > so where the specific OOM occurred doesn't really matter.
> > 
> 
> True, these messages will probably only ever get displayed if someone is
> messing with the allocated structs and accidentally balloons their size,
> so these are more debug statements than anything.

All those messages are result of allocation failure. The memory allocated
is later used to hold duplicate of static const data. Do we need that
copy (and thus allocation) at all?

	ladis

Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions

[SF Markus Elfring]

> There is the generic stack dump on OOM so the module/line
> is already known.

Can such an implementation detail become better documented
than in C source code?


> The existence of these messages increases code size which
> also make the OOM condition slightly more likely.

Interesting view …


> Markus' commit messages are always really poor descriptions
> of why these removals are somewhat useful and the commit
> could/should/might be applied.

I agree that they could be improved for this transformation
pattern if other information sources would become clearer
for corresponding references.
It seems that I got no responses so far for clarification requests
according to the documentation in a direction I hoped for.

Regards,
Markus

Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions

[Joe Perches]

On Mon, 2017-11-27 at 22:48 +0100, SF Markus Elfring wrote:
> It seems that I got no responses so far for clarification requests
> according to the documentation in a direction I hoped for.

That's because you are pretty unresponsive to
direction.

You've gotten _many_ replies to your patches to
which you have seemingly decided to ignore.

Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions

[SF Markus Elfring]

>> It seems that I got no responses so far for clarification requests
>> according to the documentation in a direction I hoped for.
> 
> That's because you are pretty unresponsive to direction.

>From which places did you get this impression?


> You've gotten _many_ replies to your patches

I got the usual mixture of disagreements and acceptance for
my selection of change possibilities.
Some constructive feedback was also provided which might need
further software development considerations.
Is the situation improvable here?


> to which you have seemingly decided to ignore.

You might eventually notice that I react to special information
occasionally with a big delay.

For which concrete details are you still waiting for a better
response from me?

Regards,
Markus

Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions

[Julia Lawall]

On Tue, 28 Nov 2017, SF Markus Elfring wrote:

> >> It seems that I got no responses so far for clarification requests
> >> according to the documentation in a direction I hoped for.
> >
> > That's because you are pretty unresponsive to direction.
>
> From which places did you get this impression?

Perhaps from the text that you have written only four lines below.  All
comments are dismissed as "the usual mixture of disagreements and
acceptance".  If you look at the patches sent by others, who learn from
the feedback provided to them, there are not so many responses on the
disagreements side.  So the mixture is not usual.  Since you send lots of
patches on the same issues, there should be no disagreements at all at
this point.

julia

>
> > You've gotten _many_ replies to your patches
>
> I got the usual mixture of disagreements and acceptance for
> my selection of change possibilities.
> Some constructive feedback was also provided which might need
> further software development considerations.
> Is the situation improvable here?
>
>
> > to which you have seemingly decided to ignore.
>
> You might eventually notice that I react to special information
> occasionally with a big delay.
>
> For which concrete details are you still waiting for a better
> response from me?
>
> Regards,
> Markus
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions

[SF Markus Elfring]

>>>> It seems that I got no responses so far for clarification requests
>>>> according to the documentation in a direction I hoped for.
>>>
>>> That's because you are pretty unresponsive to direction.
>>
>> From which places did you get this impression?
> 
> Perhaps from the text that you have written only four lines below.
> All comments are dismissed as "the usual mixture of disagreements and acceptance".

A mixture will always evolve.

* Some acceptance might not need further considerations.

* But the disagreements are remembered differently.
  They have got a potential for further improvements in some areas.


> If you look at the patches sent by others, who learn from
> the feedback provided to them,

I am also learning to some degree continuously.


> there are not so many responses on the disagreements side.

How do you think about to look at the details for such an observation?


> So the mixture is not usual.

I find that it can be also a matter of statistics.


> Since you send lots of patches on the same issues,

Yes. - I am trying to fix some implementation details by the means
of source code analysis and corresponding transformation.
The patch count is still growing.


> there should be no disagreements at all at this point.

I got an other impression. The probability for disagreements is increasing
in relation to the number of contributors to which I show change possibilities.

There are also other open issues remaining which can get another
solution somehow.

Regards,
Markus

Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions

[Julia Lawall]

On Tue, 28 Nov 2017, SF Markus Elfring wrote:

> >>>> It seems that I got no responses so far for clarification requests
> >>>> according to the documentation in a direction I hoped for.
> >>>
> >>> That's because you are pretty unresponsive to direction.
> >>
> >> From which places did you get this impression?
> >
> > Perhaps from the text that you have written only four lines below.
> > All comments are dismissed as "the usual mixture of disagreements and acceptance".
>
> A mixture will always evolve.
>
> * Some acceptance might not need further considerations.
>
> * But the disagreements are remembered differently.
>   They have got a potential for further improvements in some areas.
>
>
> > If you look at the patches sent by others, who learn from
> > the feedback provided to them,
>
> I am also learning to some degree continuously.
>
>
> > there are not so many responses on the disagreements side.
>
> How do you think about to look at the details for such an observation?
>
>
> > So the mixture is not usual.
>
> I find that it can be also a matter of statistics.
>
>
> > Since you send lots of patches on the same issues,
>
> Yes. - I am trying to fix some implementation details by the means
> of source code analysis and corresponding transformation.
> The patch count is still growing.
>
>
> > there should be no disagreements at all at this point.
>
> I got an other impression. The probability for disagreements is increasing
> in relation to the number of contributors to which I show change possibilities.

No.  You should learn from the previous submissions what concerns people
have and address them up front.

julia

>
> There are also other open issues remaining which can get another
> solution somehow.
>
> Regards,
> Markus
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions

[SF Markus Elfring]

>> I got an other impression. The probability for disagreements is increasing
>> in relation to the number of contributors to which I show change possibilities.
> 
> No.  You should learn from the previous submissions what concerns people
> have and address them up front.

The concerns can vary as contributors and changes are different.

How would you like to “address” the data structure for a default allocation
failure report finally?

Regards,
Markus

Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions

[Joe Perches]

On Tue, 2017-11-28 at 08:41 +0100, SF Markus Elfring wrote:
> > > It seems that I got no responses so far for clarification requests
> > > according to the documentation in a direction I hoped for.
> > 
> > That's because you are pretty unresponsive to direction.
> 
> From which places did you get this impression?

How many times have I told you to include the reason for
your patches in
your proposed commit message?  Too often.

For instance, specific to this patch:

Many people do not know that a generic kmalloc does a
dump_stack() on OOM.  That information should be part
of the commit message.

Also removing the printk code reduces overall code size.
The actual size reduction should be described in the
commit message too.

Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions

[Ladislav Michl]

On Tue, Nov 28, 2017 at 12:04:04AM -0800, Joe Perches wrote:
> On Tue, 2017-11-28 at 08:41 +0100, SF Markus Elfring wrote:
> > > > It seems that I got no responses so far for clarification requests
> > > > according to the documentation in a direction I hoped for.
> > > 
> > > That's because you are pretty unresponsive to direction.
> > 
> > From which places did you get this impression?
> 
> How many times have I told you to include the reason for
> your patches in
> your proposed commit message?  Too often.
> 
> For instance, specific to this patch:
> 
> Many people do not know that a generic kmalloc does a
> dump_stack() on OOM.  That information should be part
> of the commit message.
> 
> Also removing the printk code reduces overall code size.
> The actual size reduction should be described in the
> commit message too.

Could we, please, return one step back and reevaluate need for
kmalloc. That would eliminate original "problem" as well.

	ladis

Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions

[SF Markus Elfring]

> How many times have I told you to include the reason for
> your patches in your proposed commit message?

It might be useful to look again.


> Too often.

I answered this feedback to some degree.


> Many people do not know that a generic kmalloc does a
> dump_stack() on OOM.

This is another interesting information, isn't it?

It is expected that the function “devm_kzalloc” has got a similar property.


> That information should be part of the commit message.

How do you think about to provide it also in any reference documentation
in a clearer way?

I would be more confident then to copy it into my messages.
Do you want that I take your current answer as an official note
for my work (instead of waiting for adjustments of other areas)?


> Also removing the printk code reduces overall code size.

Such an effect can be nice if the involved contributors can agree on
the deletion of questionable error messages at all.


> The actual size reduction should be described in the
> commit message too.

For which hardware and software combinations would you like to see
facts there?

Regards,
Markus

Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 936 bytes --]



On Tue, 28 Nov 2017, SF Markus Elfring wrote:

> > How many times have I told you to include the reason for
> > your patches in your proposed commit message?
>
> It might be useful to look again.
>
>
> > Too often.
>
> I answered this feedback to some degree.
>
>
> > Many people do not know that a generic kmalloc does a
> > dump_stack() on OOM.
>
> This is another interesting information, isn't it?
>
> It is expected that the function “devm_kzalloc” has got a similar property.


You don't have to expect this.  Go look at the definition of devm_kzalloc
and see whether it has the property or not.

> For which hardware and software combinations would you like to see
> facts there?

This is not for Joe to decide, it's for the person who receives the patch
to decide.  You could start with the ones for which the code actually
compiles, using the standard make file and no special options, and a
recent version of gcc.

julia

Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions

[SF Markus Elfring]

>>> Many people do not know that a generic kmalloc does a
>>> dump_stack() on OOM.
>>
>> This is another interesting information, isn't it?
>>
>> It is expected that the function “devm_kzalloc” has got a similar property.
> 
> 
> You don't have to expect this.  Go look at the definition of devm_kzalloc
> and see whether it has the property or not.

I find that the corresponding documentation of these programming interfaces
is incomplete for a desired format which could be different than C source code.

https://elixir.free-electrons.com/linux/v4.15-rc1/source/include/linux/device.h#L657
https://elixir.free-electrons.com/linux/v4.15-rc1/source/drivers/base/devres.c#L763
https://www.kernel.org/doc/html/latest/driver-api/basics.html#c.devm_kmalloc

Can the Coccinelle software help more to determine desired function properties?


>> For which hardware and software combinations would you like to see
>> facts there?
> 
> This is not for Joe to decide,

This view is fine in principle.


> it's for the person who receives the patch to decide.

I am curious on further comments from these contributors.


> You could start with the ones for which the code actually compiles,
> using the standard make file and no special options, and a
> recent version of gcc.

The variation space could become too big to handle for me (alone).
How will this aspect evolve further?

Regards,
Markus

Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions

[Ladislav Michl]

On Tue, Nov 28, 2017 at 11:15:37AM +0100, SF Markus Elfring wrote:
> >>> Many people do not know that a generic kmalloc does a
> >>> dump_stack() on OOM.
> >>
> >> This is another interesting information, isn't it?
> >>
> >> It is expected that the function “devm_kzalloc” has got a similar property.
> > 
> > 
> > You don't have to expect this.  Go look at the definition of devm_kzalloc
> > and see whether it has the property or not.
> 
> I find that the corresponding documentation of these programming interfaces
> is incomplete for a desired format which could be different than C source code.
> 
> https://elixir.free-electrons.com/linux/v4.15-rc1/source/include/linux/device.h#L657
> https://elixir.free-electrons.com/linux/v4.15-rc1/source/drivers/base/devres.c#L763
> https://www.kernel.org/doc/html/latest/driver-api/basics.html#c.devm_kmalloc
> 
> Can the Coccinelle software help more to determine desired function properties?
> 
> 
> >> For which hardware and software combinations would you like to see
> >> facts there?
> > 
> > This is not for Joe to decide,
> 
> This view is fine in principle.
> 
> 
> > it's for the person who receives the patch to decide.
> 
> I am curious on further comments from these contributors.
> 
> 
> > You could start with the ones for which the code actually compiles,
> > using the standard make file and no special options, and a
> > recent version of gcc.
> 
> The variation space could become too big to handle for me (alone).
> How will this aspect evolve further?

I do not follow. This is OMAP framebuffer driver, so in this case, there
is zero variation. Could you, please, review following patch and verify
is it satisfies your automated static code analysis test?

diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dispc.c b/drivers/video/fbdev/omap2/omapfb/dss/dispc.c
index 7a75dfda9845..6be13a106ece 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/dispc.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/dispc.c
@@ -3976,52 +3976,33 @@ static const struct dispc_features omap54xx_dispc_feats = {
 	.has_writeback		=	true,
 };
 
-static int dispc_init_features(struct platform_device *pdev)
+static const struct dispc_features* dispc_get_features(void)
 {
-	const struct dispc_features *src;
-	struct dispc_features *dst;
-
-	dst = devm_kzalloc(&pdev->dev, sizeof(*dst), GFP_KERNEL);
-	if (!dst) {
-		dev_err(&pdev->dev, "Failed to allocate DISPC Features\n");
-		return -ENOMEM;
-	}
-
 	switch (omapdss_get_version()) {
 	case OMAPDSS_VER_OMAP24xx:
-		src = &omap24xx_dispc_feats;
-		break;
+		return &omap24xx_dispc_feats;
 
 	case OMAPDSS_VER_OMAP34xx_ES1:
-		src = &omap34xx_rev1_0_dispc_feats;
-		break;
+		return &omap34xx_rev1_0_dispc_feats;
 
 	case OMAPDSS_VER_OMAP34xx_ES3:
 	case OMAPDSS_VER_OMAP3630:
 	case OMAPDSS_VER_AM35xx:
 	case OMAPDSS_VER_AM43xx:
-		src = &omap34xx_rev3_0_dispc_feats;
-		break;
+		return &omap34xx_rev3_0_dispc_feats;
 
 	case OMAPDSS_VER_OMAP4430_ES1:
 	case OMAPDSS_VER_OMAP4430_ES2:
 	case OMAPDSS_VER_OMAP4:
-		src = &omap44xx_dispc_feats;
-		break;
+		return &omap44xx_dispc_feats;
 
 	case OMAPDSS_VER_OMAP5:
 	case OMAPDSS_VER_DRA7xx:
-		src = &omap54xx_dispc_feats;
-		break;
+		return &omap54xx_dispc_feats;
 
 	default:
-		return -ENODEV;
+		return NULL;
 	}
-
-	memcpy(dst, src, sizeof(*dst));
-	dispc.feat = dst;
-
-	return 0;
 }
 
 static irqreturn_t dispc_irq_handler(int irq, void *arg)
@@ -4078,9 +4059,9 @@ static int dispc_bind(struct device *dev, struct device *master, void *data)
 
 	spin_lock_init(&dispc.control_lock);
 
-	r = dispc_init_features(dispc.pdev);
-	if (r)
-		return r;
+	dispc.feat = dispc_get_features();
+	if (!dispc.feat)
+		return -ENODEV;
 
 	dispc_mem = platform_get_resource(dispc.pdev, IORESOURCE_MEM, 0);
 	if (!dispc_mem) {
diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dss.c b/drivers/video/fbdev/omap2/omapfb/dss/dss.c
index 48c6500c24e1..9a255ffe77c5 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/dss.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/dss.c
@@ -887,58 +887,37 @@ static const struct dss_features dra7xx_dss_feats = {
 	.num_ports		=	ARRAY_SIZE(dra7xx_ports),
 };
 
-static int dss_init_features(struct platform_device *pdev)
+static const struct dss_features* dss_get_features(void)
 {
-	const struct dss_features *src;
-	struct dss_features *dst;
-
-	dst = devm_kzalloc(&pdev->dev, sizeof(*dst), GFP_KERNEL);
-	if (!dst) {
-		dev_err(&pdev->dev, "Failed to allocate local DSS Features\n");
-		return -ENOMEM;
-	}
-
 	switch (omapdss_get_version()) {
 	case OMAPDSS_VER_OMAP24xx:
-		src = &omap24xx_dss_feats;
-		break;
+		return &omap24xx_dss_feats;
 
 	case OMAPDSS_VER_OMAP34xx_ES1:
 	case OMAPDSS_VER_OMAP34xx_ES3:
 	case OMAPDSS_VER_AM35xx:
-		src = &omap34xx_dss_feats;
-		break;
+		return &omap34xx_dss_feats;
 
 	case OMAPDSS_VER_OMAP3630:
-		src = &omap3630_dss_feats;
-		break;
+		return &omap3630_dss_feats;
 
 	case OMAPDSS_VER_OMAP4430_ES1:
 	case OMAPDSS_VER_OMAP4430_ES2:
 	case OMAPDSS_VER_OMAP4:
-		src = &omap44xx_dss_feats;
-		break;
+		return &omap44xx_dss_feats;
 
 	case OMAPDSS_VER_OMAP5:
-		src = &omap54xx_dss_feats;
-		break;
+		return &omap54xx_dss_feats;
 
 	case OMAPDSS_VER_AM43xx:
-		src = &am43xx_dss_feats;
-		break;
+		return &am43xx_dss_feats;
 
 	case OMAPDSS_VER_DRA7xx:
-		src = &dra7xx_dss_feats;
-		break;
+		return &dra7xx_dss_feats;
 
 	default:
-		return -ENODEV;
+		return NULL;
 	}
-
-	memcpy(dst, src, sizeof(*dst));
-	dss.feat = dst;
-
-	return 0;
 }
 
 static void dss_uninit_ports(struct platform_device *pdev);
@@ -1104,9 +1083,9 @@ static int dss_bind(struct device *dev)
 
 	dss.pdev = pdev;
 
-	r = dss_init_features(dss.pdev);
-	if (r)
-		return r;
+	dss.feat = dss_get_features();
+	if (!dss.feat)
+		return -ENODEV;
 
 	dss_mem = platform_get_resource(dss.pdev, IORESOURCE_MEM, 0);
 	if (!dss_mem) {
diff --git a/drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c b/drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c
index 9a13c35fd6d8..07d46e14cea4 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c
@@ -189,47 +189,30 @@ static const struct hdmi_phy_features omap54xx_phy_feats = {
 	.max_phy	=	186000000,
 };
 
-static int hdmi_phy_init_features(struct platform_device *pdev)
+static const struct hdmi_phy_features* hdmi_phy_get_features(void)
 {
-	struct hdmi_phy_features *dst;
-	const struct hdmi_phy_features *src;
-
-	dst = devm_kzalloc(&pdev->dev, sizeof(*dst), GFP_KERNEL);
-	if (!dst) {
-		dev_err(&pdev->dev, "Failed to allocate HDMI PHY Features\n");
-		return -ENOMEM;
-	}
-
 	switch (omapdss_get_version()) {
 	case OMAPDSS_VER_OMAP4430_ES1:
 	case OMAPDSS_VER_OMAP4430_ES2:
 	case OMAPDSS_VER_OMAP4:
-		src = &omap44xx_phy_feats;
-		break;
+		return &omap44xx_phy_feats;
 
 	case OMAPDSS_VER_OMAP5:
 	case OMAPDSS_VER_DRA7xx:
-		src = &omap54xx_phy_feats;
-		break;
+		return &omap54xx_phy_feats;
 
 	default:
-		return -ENODEV;
+		return NULL;
 	}
-
-	memcpy(dst, src, sizeof(*dst));
-	phy_feat = dst;
-
-	return 0;
 }
 
 int hdmi_phy_init(struct platform_device *pdev, struct hdmi_phy_data *phy)
 {
-	int r;
 	struct resource *res;
 
-	r = hdmi_phy_init_features(pdev);
-	if (r)
-		return r;
+	phy_feat = hdmi_phy_get_features();
+	if (!phy_feat)
+		return -ENODEV;
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "phy");
 	if (!res) {

Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions

[SF Markus Elfring]

>> How will this aspect evolve further?
> 
> I do not follow.

Interesting …


> This is OMAP framebuffer driver, so in this case, there is zero variation.

How much are you interested to compare differences in build results
also for this software module because of varying parameters?

Which parameters were applied for your size comparisons so far?


> Could you, please, review following patch

I assume that other OMAP developers are in a better position to decide
about the deletion of extra memory allocations (instead of keeping
questionable error messages).


> and verify is it satisfies your automated static code analysis test?

I am not going to “verify” your update suggestion by my evolving approaches
around the semantic patch language (Coccinelle software) at the moment.

But I thank you for this contribution.
How will further feedback evolve for such an idea?

Regards,
Markus

Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions

[Ladislav Michl]

On Tue, Nov 28, 2017 at 11:50:14AM +0100, SF Markus Elfring wrote:
> >> How will this aspect evolve further?
> > 
> > I do not follow.
> 
> Interesting …
> 
> > This is OMAP framebuffer driver, so in this case, there is zero variation.
> 
> How much are you interested to compare differences in build results
> also for this software module because of varying parameters?
> 
> Which parameters were applied for your size comparisons so far?

It was just omap2plus_defconfig build using gcc-7.2.0

> > Could you, please, review following patch
> 
> I assume that other OMAP developers are in a better position to decide
> about the deletion of extra memory allocations (instead of keeping
> questionable error messages).
> 
> > and verify is it satisfies your automated static code analysis test?
> 
> I am not going to “verify” your update suggestion by my evolving approaches
> around the semantic patch language (Coccinelle software) at the moment.

As you are sending patches as Markus Elfring I would expect you take
Coccinelle's suggestion into account and actually try to understand code
before sending patch. That suggestion may lead to actual bug in code
which your patch just leaves unnoticed as it is not apparent from
the patch itself (no, not talking about this very patch it all started
with)

That said, I'm considering Markus Elfring being a human. If you do not like
reactions to your patches or are interested only in improving tool that
generates them, it would be better to just setup a "tip bot for Markus
Elfring" and let it send patches automatically. This way noone is going
to waste time on them as it would be clear those are purely machine only
generated and there's no point to reply.

The way you are sending patches makes impression (at least to me), that
you spent some time on fixing issue Coccinelle found and not just shut
the warning up.

> But I thank you for this contribution.
> How will further feedback evolve for such an idea?

And the idea is?

> Regards,
> Markus

Thank you,
	ladis

Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions

[SF Markus Elfring]

>> I am not going to “verify” your update suggestion by my evolving approaches
>> around the semantic patch language (Coccinelle software) at the moment.
> 
> As you are sending patches as Markus Elfring

I am contributing also some update suggestions.


> I would expect you take Coccinelle's suggestion into account

The proposed change is based on a semantic patch script which I developed
with the support of other well-known Linux contributors.


> and actually try to understand code before sending patch.

I concentrated my understanding on the concrete transformation pattern
in this use case.


> That suggestion may lead to actual bug in code which your patch just leaves
> unnoticed as it is not apparent from the patch itself

There can be other change possibilities left over as usual.


> (no, not talking about this very patch it all started with)

Thanks for your distinction.


> That said, I'm considering Markus Elfring being a human.

Thanks for this view.


> If you do not like reactions to your patches

I am looking for constructive responses. - Disagreements can trigger
special communication challenges.


> or are interested only in improving tool that generates them,

How do you think about to look at any more background information?

https://github.com/coccinelle/coccinelle/issues
https://systeme.lip6.fr/pipermail/cocci/


> it would be better to just setup a "tip bot for Markus
> Elfring" and let it send patches automatically.

There is already an other automatic source code analysis system active.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/scripts/coccinelle


> The way you are sending patches makes impression (at least to me),
> that you spent some time on fixing issue Coccinelle found

Yes. - This view is appropriate.


> and not just shut the warning up.

Additional improvement possibilities can be taken into account
after corresponding software development discussions, can't they?

Regards,
Markus

Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions

[Ladislav Michl]

On Tue, Nov 28, 2017 at 01:13:51PM +0100, SF Markus Elfring wrote:
> Additional improvement possibilities can be taken into account
> after corresponding software development discussions, can't they?

Sure, but that is in contrary to all you replies. I guess you are
familiar with Documentation/process/submitting-patches.rst chapter 8.
No matter that patch was generated or suggested by a tool, you sent
it and normal review procedure follows. And here you ignored _all_
suggestions and concentrate solely on improving Coccinelle scripts.

On kernel related lists suggestions to patch itself are discussed.
Whenever you take them into account while developing Coccinelle
is up to you (on the Cocci list).

Best regards,
	ladis

Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions

[SF Markus Elfring]

>> Additional improvement possibilities can be taken into account
>> after corresponding software development discussions, can't they?
> 
> Sure, but that is in contrary to all you replies.

Where do you see a contradiction in this case?


> I guess you are familiar with Documentation/process/submitting-patches.rst chapter 8.

I hope so in principle.


> No matter that patch was generated or suggested by a tool, you sent
> it and normal review procedure follows.

This is generally fine.


> And here you ignored _all_ suggestions

I did not integrate a few of them for my commit message so far
because it seems that there are open issues for further clarification.

Do you want that I send a second approach for this software module
before your own evolving update suggestion?


> and concentrate solely on improving Coccinelle scripts.

I hope not.


> On kernel related lists suggestions to patch itself are discussed.

This is usual.


> Whenever you take them into account while developing Coccinelle
> is up to you (on the Cocci list).

This is also happening, isn't it?

Regards,
Markus

Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions

[Joe Perches]

On Tue, 2017-11-28 at 11:23 +0100, Ladislav Michl wrote:
> I do not follow. This is OMAP framebuffer driver, so in this case, there
> is zero variation. Could you, please, review following patch and verify
> is it satisfies your automated static code analysis test?

Obviously a better patch.

I suggest submitting it properly and letting the 0-day
build bot have a go at it.

cheers, Joe

Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions

[SF Markus Elfring]

> How many times have I told you to include the reason for your patches
> in your proposed commit message?

Will it be useful to look again at the involved circumstances?


> Too often.

Did I answer any concerns partly?


> Many people do not know that a generic kmalloc does a dump_stack() on OOM.

Do you see a need to represent such information better?

Is it expected that the function “devm_kzalloc” has got a similar property?


> That information should be part of the commit message.

How do you think about to share it also from any reference documentation
in a clearer way?

Do we stumble on a target conflict in this case?

I am generally trying to improve the software situation to some degree.
I prefer then to work with safe information sources.
Unfortunately, I might have not reached a desired confidence level here
for a more detailed commit message. I assume that software development
efforts could increase in significant ways if something should be improved
further in a direction I hope. But this could mean that time frames will
grow for corresponding clarifications.

* Does such a situation block progress on the deletion of other remaining
  questionable error messages?

* Would you like to increase the software development attention anyhow?



By the way:
It seems that my update suggestion for the directory “omapfb/dss”
could be superseded by the patch “omapfb: dss: Do not duplicate features data”
from Ladislav Michl.
https://patchwork.kernel.org/patch/10082027/
https://lkml.kernel.org/r/<20171129123308.GA26578@lenoch>

Regards,
Markus