From: Liberman Igal <Igal.Liberman@freescale.com>
To: Paul Bolle <pebolle@tiscali.nl>,
        Madalin-Cristian Bucur <madalin.bucur@freescale.com>
CC: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
        Scott Wood <scottwood@freescale.com>
Subject: RE: [PATCH 08/12] fsl/fman: Add Frame Manager support
Thread-Topic: [PATCH 08/12] fsl/fman: Add Frame Manager support
Thread-Index: AQHQo5FfWGKUFxFg30GaYqj8iJ6S5p2nAbgAgAALtICABpcOAA==
Date: Mon, 15 Jun 2015 14:23:14 +0000
Message-ID: <DM2PR03MB383A5275392BBBD604FE71BE6B80@DM2PR03MB383.namprd03.prod.outlook.com>
References: <1433949712-5648-1-git-send-email-madalin.bucur@freescale.com>
	 <1433949712-5648-2-git-send-email-madalin.bucur@freescale.com>
	 <1433949712-5648-3-git-send-email-madalin.bucur@freescale.com>
	 <1433949712-5648-4-git-send-email-madalin.bucur@freescale.com>
	 <1434012956.2271.58.camel@x220> <1434015469.2271.83.camel@x220>
In-Reply-To: <1434015469.2271.83.camel@x220>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Jun 2015 14:23:14.6441
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 710a03f5-10f6-4d38-9ff4-a80b81da590d
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM2PR03MB557
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by nfs id t5FEcGCd028425
Content-Length: 2552
Lines: 86

Hi Paul,
Thank you very much for your feedback.

I'm planning to address the issues you've raised in the next submission.

Regards,
Igal Liberman.

> -----Original Message-----
> From: Paul Bolle [mailto:pebolle@tiscali.nl]
> Sent: Thursday, June 11, 2015 12:38 PM
> To: Bucur Madalin-Cristian-B32716
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linuxppc-
> dev@lists.ozlabs.org; Wood Scott-B07421; Liberman Igal-B31950
> Subject: Re: [PATCH 08/12] fsl/fman: Add Frame Manager support
> 
> 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

????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?