Re: [PATCH] omapfb: reduce stack usage
On Fri, Oct 18, 2019 at 05:30:04PM +0100, Sudip Mukherjee wrote:
> The build of xtensa allmodconfig is giving a warning of:
> In function 'dsi_dump_dsidev_irqs':
> warning: the frame size of 1120 bytes is larger than 1024 bytes
>
> Allocate the memory for 'struct dsi_irq_stats' dynamically instead
> of assigning it in stack.
So now function can fail silently, executes longer, code is sligthly
bigger... And all that to silent warning about exceeding frame size.
Is it really worth "fixing"?
> Signed-off-by: Sudip Mukherjee
> ---
> drivers/video/fbdev/omap2/omapfb/dss/dsi.c | 24 ++--
> 1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dsi.c
> b/drivers/video/fbdev/omap2/omapfb/dss/dsi.c
> index d620376216e1..43402467bf40 100644
> --- a/drivers/video/fbdev/omap2/omapfb/dss/dsi.c
> +++ b/drivers/video/fbdev/omap2/omapfb/dss/dsi.c
> @@ -1536,22 +1536,25 @@ static void dsi_dump_dsidev_irqs(struct
> platform_device *dsidev,
> {
> struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
> unsigned long flags;
> - struct dsi_irq_stats stats;
> + struct dsi_irq_stats *stats;
>
> + stats = kmalloc(sizeof(*stats), GFP_KERNEL);
> + if (!stats)
> + return;
> spin_lock_irqsave(&dsi->irq_stats_lock, flags);
>
> - stats = dsi->irq_stats;
> + memcpy(stats, &dsi->irq_stats, sizeof(*stats));
> memset(&dsi->irq_stats, 0, sizeof(dsi->irq_stats));
> dsi->irq_stats.last_reset = jiffies;
>
> spin_unlock_irqrestore(&dsi->irq_stats_lock, flags);
>
> seq_printf(s, "period %u ms\n",
> - jiffies_to_msecs(jiffies - stats.last_reset));
> + jiffies_to_msecs(jiffies - stats->last_reset));
>
> - seq_printf(s, "irqs %d\n", stats.irq_count);
> + seq_printf(s, "irqs %d\n", stats->irq_count);
> #define PIS(x) \
> - seq_printf(s, "%-20s %10d\n", #x, stats.dsi_irqs[ffs(DSI_IRQ_##x)-1]);
> + seq_printf(s, "%-20s %10d\n", #x, stats->dsi_irqs[ffs(DSI_IRQ_##x)-1]);
>
> seq_printf(s, "-- DSI%d interrupts --\n", dsi->module_id + 1);
> PIS(VC0);
> @@ -1575,10 +1578,10 @@ static void dsi_dump_dsidev_irqs(struct
> platform_device *dsidev,
>
> #define PIS(x) \
> seq_printf(s, "%-20s %10d %10d %10d %10d\n", #x, \
> - stats.vc_irqs[0][ffs(DSI_VC_IRQ_##x)-1], \
> - stats.vc_irqs[1][ffs(DSI_VC_IRQ_##x)-1], \
> - stats.vc_irqs[2][ffs(DSI_VC_IRQ_##x)-1], \
> - stats.vc_irqs[3][ffs(DSI_VC_IRQ_##x)-1]);
> + stats->vc_irqs[0][ffs(DSI_VC_IRQ_##x)-1], \
> + stats->vc_irqs[1][ffs(DSI_VC_IRQ_##x)-1], \
> + stats->vc_irqs[2][ffs(DSI_VC_IRQ_##x)-1], \
> + stats->vc_irqs[3][ffs(DSI_VC_IRQ_##x)-1]);
>
> seq_printf(s, "-- VC interrupts --\n");
> PIS(CS);
> @@ -1594,7 +1597,7 @@ static void dsi_dump_dsidev_irqs(struct platform_device
> *dsidev,
>
> #define PIS(x) \
> seq_printf(s, "%-20s %10d\n", #x, \
> - stats.cio_irqs[ffs(DSI_CIO_IRQ_##x)-1]);
> + stats->cio_irqs[ffs(DSI_CIO_IRQ_##x)-1]);
>
> seq_printf(s, "-- CIO interrupts --\n");
> PIS(ERRSYNCESC1);
> @@ -1618,6 +1621,7 @@ static void dsi_dump_dsidev_irqs(struct platform_device
> *dsidev,
> PIS(ULPSACTIVENOT_ALL0);
> PIS(ULPSACTIVENOT_ALL1);
> #undef PIS
> + kfree(stats);
> }
>
> static void dsi1_dump_irqs(struct seq_file *s)
> --
> 2.11.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 08/21] udl-kms: avoid prefetch
On Tue, Jun 05, 2018 at 10:08:02AM +, Alexey Brodkin wrote:
> Hi Mikulas,
>
> On Sun, 2018-06-03 at 16:41 +0200, Mikulas Patocka wrote:
> > Modern processors can detect linear memory accesses and prefetch data
> > automatically, so there's no need to use prefetch.
>
> Not each and every CPU that's capable of running Linux has prefetch
> functionality :)
>
> Still read-on...
>
> > Signed-off-by: Mikulas Patocka
> >
> > ---
> > drivers/gpu/drm/udl/udl_transfer.c |7 ---
> > 1 file changed, 7 deletions(-)
> >
> > Index: linux-4.16.12/drivers/gpu/drm/udl/udl_transfer.c
> > ===
> > --- linux-4.16.12.orig/drivers/gpu/drm/udl/udl_transfer.c 2018-05-31
> > 14:48:12.0 +0200
> > +++ linux-4.16.12/drivers/gpu/drm/udl/udl_transfer.c2018-05-31
> > 14:48:12.0 +0200
> > @@ -13,7 +13,6 @@
> > #include
> > #include
> > #include
> > -#include
> > #include
> >
> > #include
> > @@ -51,9 +50,6 @@ static int udl_trim_hline(const u8 *bbac
> > int start = width;
> > int end = width;
> >
> > - prefetch((void *) front);
> > - prefetch((void *) back);
>
> AFAIK prefetcher fetches new data according to a known history... i.e. based
> on previously
> used pattern we'll trying to get the next batch of data.
>
> But the code above is in the very beginning of the data processing routine
> where
> prefetcher doesn't yet have any history to know what and where to prefetch.
>
> So I'd say this particular usage is good.
> At least those prefetches shouldn't hurt because typically it
> would be just 1 instruction if those exist or nothing if CPU/compiler doesn't
> support it.
>
> > for (j = 0; j < width; j++) {
> > if (back[j] != front[j]) {
> > start = j;
> > @@ -140,8 +136,6 @@ static void udl_compress_hline16(
> > const u8 *cmd_pixel_start, *cmd_pixel_end = NULL;
> > uint16_t pixel_val16;
> >
> > - prefetchw((void *) cmd); /* pull in one cache line at least */
> > -
>
> Pretty much the same is here. Obviously on the first iteration prefetcher
> dosn't
> know where to get "cmd". On the next iterations it might be better but given
> amount
> of operation happens further in the cycle (and in inner cycles) I won't be
> completely
> sure that each and every prefetcher will still keep track of "cmd".
>
> > *cmd++ = 0xaf;
> > *cmd++ = 0x6b;
> > *cmd++ = (uint8_t) ((dev_addr >> 16) & 0xFF);
> > @@ -158,7 +152,6 @@ static void udl_compress_hline16(
> > (unsigned long)(pixel_end - pixel) >>
> > log_bpp,
> > (unsigned long)(cmd_buffer_end - 1 -
> > cmd) / 2) << log_bpp);
> >
> > - prefetch_range((void *) pixel, cmd_pixel_end - pixel);
>
> Again I'm not sure what we gain removing that code in comparison of possible
> performance degradation on simpler CPUs.
>
> And essentially all the same is applicable to UDLFB patch.
Tested this one on AT91SAM9G20 SoC, but couldn't get any measurable difference.
Part of problem is probably that full speed USB is the bottleneck here.
However, the same applies on OMAP3630 based board with high speed USB.
As a side note, I didn't experience any problem those paches are fixing, so
perhaps testcases could be described briefly, preferably with some numbers
(I'm not running console on udlfb, but try to give it a try next week).
Thank you,
ladis
> -Alexey
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions
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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions
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.
textdata bss dec hex filename
5931923724184 65875 10153 dispc.o.orig
5917823724184 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) {
-
Re: [PATCH] omapfb/dss: Delete an error message for a failed memory allocation in three functions
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 > >>> 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 > >>> --- > >> > >> nak, unlike many others, these message give extra info on which > >> allocation failed, that can be useful. > > > > 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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions
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 23724184 65875 10153 dispc.o.orig
> 59178 23724184 65734 100c6 dispc.o
59095 23724184 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;
+
Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions
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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions
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");
-
Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions
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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: omapfb/dss: Delete an error message for a failed memory allocation in three functions
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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
