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 <[email protected]>
>>> 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 <[email protected]>
>>> ---
>>
>> 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:
>>>
From 1585257522393938674@xxx Mon Nov 27 21:49:43 +0000 2017
X-GM-THRID: 1585156039807309629
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
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 <[email protected]>
> >>> 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 <[email protected]>
> >>> ---
> >>
> >> 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
From 1585248449629949648@xxx Mon Nov 27 19:25:31 +0000 2017
X-GM-THRID: 1585156039807309629
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread