From: Paul Bolle <pebolle@tiscali.nl>
To: madalin.bucur@freescale.com
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, scottwood@freescale.com,
Igal Liberman <Igal.Liberman@freescale.com>
Subject: Re: [PATCH 08/12] fsl/fman: Add Frame Manager support
Date: Thu, 11 Jun 2015 11:37:49 +0200 [thread overview]
Message-ID: <1434015469.2271.83.camel@x220> (raw)
In-Reply-To: <1434012956.2271.58.camel@x220>
So I couldn't help having yet another look at the code, just to drive
home my point.
On Thu, 2015-06-11 at 10:55 +0200, Paul Bolle wrote:
> > +void *fm_drv_init(void)
>
> static.
>
> > +{
> > + memset(&fm_drvs, 0, sizeof(fm_drvs));
fm_drvs is an external variable. It is guaranteed to be zero, isn't it?
> > + mutex_init(&fm_drv_mutex);
> > +
> > + /* Register to the DTB for basic FM API */
> > + platform_driver_register(&fm_driver);
> > +
> > + return &fm_drvs;
You're returning a pointer to external variable. How's that useful?
And note this is the last time we'll ever see fm_drvs. So I think that
all this variable does for the code is getting initialized to zero,
twice.
> > +}
> > +
> > +int fm_drv_free(void *p_fm_drv)
>
> static.
>
> > +{
> > + platform_driver_unregister(&fm_driver);
> > + mutex_destroy(&fm_drv_mutex);
> > +
> > + return 0;
This function has one caller, which doesn't check the return value. So
this should be a function returning void. Of course, a wrapper of two
lines called only once means you should actually not put this into a
separate function.
> > +}
> > +static void *p_fm_drv;
>
> > +static int __init __cold fm_load(void)
> > +{
> > + p_fm_drv = fm_drv_init();
> > + if (!p_fm_drv) {
fm_drv_init() returns a pointer to an external variable. So how can this
happen?
> > + pr_err("Failed to init FM wrapper!\n");
> > + return -ENODEV;
> > + }
> > +
> > + pr_info("Freescale FM module\n");
> > + return 0;
> > +}
This is all rather basic. It must be, otherwise I wouldn't spot it.
So I keep spotting these basic oddities, with every cup of coffee I
treat myself to while reading through this, wherever I look. By now I'm
sure there's no need for the netdev people to look at this, not yet.
Paul Bolle
next prev parent reply other threads:[~2015-06-11 9:37 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-10 15:21 [PATCH 00/12] Freescale DPAA FMan Madalin Bucur
2015-06-10 15:21 ` [PATCH 07/12] fsl/fman: Add FMan MURAM support Madalin Bucur
2015-06-10 15:21 ` [PATCH 01/12] fsl/fman: Add the FMan FLIB headers Madalin Bucur
2015-06-10 15:21 ` [PATCH 08/12] fsl/fman: Add Frame Manager support Madalin Bucur
2015-06-10 15:21 ` [PATCH 02/12] fsl/fman: Add the FMan FLIB Madalin Bucur
2015-06-10 15:21 ` [PATCH 09/12] fsl/fman: Add FMan MAC support Madalin Bucur
2015-06-10 15:21 ` [PATCH 03/12] fsl/fman: Add the FMan port FLIB headers Madalin Bucur
2015-06-10 15:21 ` [PATCH 10/12] fsl/fman: Add FMan SP support Madalin Bucur
2015-06-10 15:21 ` [PATCH 04/12] fsl/fman: Add the FMan port FLIB Madalin Bucur
2015-06-10 15:21 ` [PATCH 11/12] fsl/fman: Add FMan Port Support Madalin Bucur
2015-06-10 15:21 ` [PATCH 05/12] fsl/fman: Add the FMan MAC FLIB headers Madalin Bucur
2015-06-10 15:21 ` [PATCH 12/12] fsl/fman: Add FMan MAC driver Madalin Bucur
2015-06-10 15:21 ` [PATCH 06/12] fsl/fman: Add the FMan MAC FLIB Madalin Bucur
2015-06-11 8:55 ` [PATCH 08/12] fsl/fman: Add Frame Manager support Paul Bolle
2015-06-11 9:37 ` Paul Bolle [this message]
2015-06-15 14:23 ` Liberman Igal
2015-06-16 3:42 ` Bob Cochran
2015-06-16 4:33 ` Scott Wood
2015-06-10 18:54 ` [PATCH 01/12] fsl/fman: Add the FMan FLIB headers Scott Wood
2015-06-17 14:59 ` Liberman Igal
2015-06-17 17:57 ` Scott Wood
2015-06-10 18:52 ` [PATCH 00/12] Freescale DPAA FMan Scott Wood
-- strict thread matches above, loose matches on Subject: below --
2015-06-10 8:03 [PATCH 08/12] fsl/fman: Add Frame Manager support Igal.Liberman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1434015469.2271.83.camel@x220 \
--to=pebolle@tiscali.nl \
--cc=Igal.Liberman@freescale.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=madalin.bucur@freescale.com \
--cc=netdev@vger.kernel.org \
--cc=scottwood@freescale.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).