Patchwork [1/2] ARM: mx28evk: remove flexcan_pdata __initconst attribute

login
register
mail settings
Submitter Dong Aisheng
Date Nov. 11, 2011, 3:25 p.m.
Message ID <65EE16ACC360FA4D99C96DC085B3F7722CECED@039-SN1MPN1-002.039d.mgd.msft.net>
Download mbox | patch
Permalink /patch/125190/
State New
Headers show

Comments

Dong Aisheng - Nov. 11, 2011, 3:25 p.m.
> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de]
> Sent: Friday, November 11, 2011 11:06 PM
> To: Dong Aisheng-B29396
> Cc: linux-arm-kernel@lists.infradead.org; kernel@pengutronix.de;
> s.hauer@pengutronix.de; w.sang@pengutronix.de; Guo Shawn-R65073
> Subject: Re: [PATCH 1/2] ARM: mx28evk: remove flexcan_pdata __initconst
> attribute
> 
> On Fri, Nov 11, 2011 at 11:12:39PM +0800, Dong Aisheng wrote:
> > The flexcan driver still uses it after init.
> As "it" is a copy of flexcan_pdata it's completely OK for the driver to
> use it.
> 
> What is the exact problem you want to address?
>
Sorry, I just checked the code and found platform_device_add_data will
Add a copy of platform specific data.
Originally I thought the driver may access the freed pdata after init.

So, does it mean that every pdata of pdevice added
by mxs_add_platform_device_dmamask can be prefixed by __init*?

And, below change seems correct?
Regards
Dong Aisheng
Russell King - ARM Linux - Nov. 11, 2011, 3:27 p.m.
On Fri, Nov 11, 2011 at 03:25:01PM +0000, Dong Aisheng-B29396 wrote:
> -static struct fec_platform_data tx28_fec0_data = {
> +static struct fec_platform_data __initconst tx28_fec0_data = {

No.  It's spelt:

static const struct fec_platform_data tx28_fec0_data __initconst = {
Dong Aisheng - Nov. 11, 2011, 3:30 p.m.
> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> Sent: Friday, November 11, 2011 11:28 PM
> To: Dong Aisheng-B29396
> Cc: Uwe Kleine-König; Guo Shawn-R65073; s.hauer@pengutronix.de;
> kernel@pengutronix.de; linux-arm-kernel@lists.infradead.org;
> w.sang@pengutronix.de
> Subject: Re: [PATCH 1/2] ARM: mx28evk: remove flexcan_pdata __initconst
> attribute
> 
> On Fri, Nov 11, 2011 at 03:25:01PM +0000, Dong Aisheng-B29396 wrote:
> > -static struct fec_platform_data tx28_fec0_data = {
> > +static struct fec_platform_data __initconst tx28_fec0_data = {
> 
> No.  It's spelt:
> 
> static const struct fec_platform_data tx28_fec0_data __initconst = {
Thanks for reminder, I missed the first const. :-)

Regards
Dong Aisheng
Russell King - ARM Linux - Nov. 11, 2011, 3:33 p.m.
On Fri, Nov 11, 2011 at 03:30:04PM +0000, Dong Aisheng-B29396 wrote:
> > -----Original Message-----
> > From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> > Sent: Friday, November 11, 2011 11:28 PM
> > To: Dong Aisheng-B29396
> > Cc: Uwe Kleine-König; Guo Shawn-R65073; s.hauer@pengutronix.de;
> > kernel@pengutronix.de; linux-arm-kernel@lists.infradead.org;
> > w.sang@pengutronix.de
> > Subject: Re: [PATCH 1/2] ARM: mx28evk: remove flexcan_pdata __initconst
> > attribute
> > 
> > On Fri, Nov 11, 2011 at 03:25:01PM +0000, Dong Aisheng-B29396 wrote:
> > > -static struct fec_platform_data tx28_fec0_data = {
> > > +static struct fec_platform_data __initconst tx28_fec0_data = {
> > 
> > No.  It's spelt:
> > 
> > static const struct fec_platform_data tx28_fec0_data __initconst = {
> Thanks for reminder, I missed the first const. :-)

That's not the only thing.  Note the placement of the __initconst too.
Dong Aisheng - Nov. 11, 2011, 3:36 p.m.
> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> Sent: Friday, November 11, 2011 11:33 PM
> To: Dong Aisheng-B29396
> Cc: Uwe Kleine-König; Guo Shawn-R65073; s.hauer@pengutronix.de;
> kernel@pengutronix.de; linux-arm-kernel@lists.infradead.org;
> w.sang@pengutronix.de
> Subject: Re: [PATCH 1/2] ARM: mx28evk: remove flexcan_pdata __initconst
> attribute
> 
> On Fri, Nov 11, 2011 at 03:30:04PM +0000, Dong Aisheng-B29396 wrote:
> > > -----Original Message-----
> > > From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> > > Sent: Friday, November 11, 2011 11:28 PM
> > > To: Dong Aisheng-B29396
> > > Cc: Uwe Kleine-König; Guo Shawn-R65073; s.hauer@pengutronix.de;
> > > kernel@pengutronix.de; linux-arm-kernel@lists.infradead.org;
> > > w.sang@pengutronix.de
> > > Subject: Re: [PATCH 1/2] ARM: mx28evk: remove flexcan_pdata
> > > __initconst attribute
> > >
> > > On Fri, Nov 11, 2011 at 03:25:01PM +0000, Dong Aisheng-B29396 wrote:
> > > > -static struct fec_platform_data tx28_fec0_data = {
> > > > +static struct fec_platform_data __initconst tx28_fec0_data = {
> > >
> > > No.  It's spelt:
> > >
> > > static const struct fec_platform_data tx28_fec0_data __initconst = {
> > Thanks for reminder, I missed the first const. :-)
> 
> That's not the only thing.  Note the placement of the __initconst too.

Got it, thanks for your carefully review. :-)

Regards
Dong Aisheng
Uwe Kleine-König - Nov. 11, 2011, 3:42 p.m.
On Fri, Nov 11, 2011 at 03:25:01PM +0000, Dong Aisheng-B29396 wrote:
> > -----Original Message-----
> > From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de]
> > Sent: Friday, November 11, 2011 11:06 PM
> > To: Dong Aisheng-B29396
> > Cc: linux-arm-kernel@lists.infradead.org; kernel@pengutronix.de;
> > s.hauer@pengutronix.de; w.sang@pengutronix.de; Guo Shawn-R65073
> > Subject: Re: [PATCH 1/2] ARM: mx28evk: remove flexcan_pdata __initconst
> > attribute
> > 
> > On Fri, Nov 11, 2011 at 11:12:39PM +0800, Dong Aisheng wrote:
> > > The flexcan driver still uses it after init.
> > As "it" is a copy of flexcan_pdata it's completely OK for the driver to
> > use it.
> > 
> > What is the exact problem you want to address?
> >
> Sorry, I just checked the code and found platform_device_add_data will
> Add a copy of platform specific data.
> Originally I thought the driver may access the freed pdata after init.
> 
> So, does it mean that every pdata of pdevice added
> by mxs_add_platform_device_dmamask can be prefixed by __init*?
> 
> And, below change seems correct?
> diff --git a/arch/arm/mach-mxs/module-tx28.c b/arch/arm/mach-mxs/module-tx28.c
> index 0fcff47..a096cca 100644
> --- a/arch/arm/mach-mxs/module-tx28.c
> +++ b/arch/arm/mach-mxs/module-tx28.c
> @@ -66,11 +66,11 @@ static const iomux_cfg_t tx28_fec1_pads[] __initconst = {
>         MX28_PAD_ENET0_CRS__ENET1_RX_EN,
>  };
> 
> -static struct fec_platform_data tx28_fec0_data = {
> +static struct fec_platform_data __initconst tx28_fec0_data = {
>         .phy = PHY_INTERFACE_MODE_RMII,
>  };
> 
> -static struct fec_platform_data tx28_fec1_data = {
> +static struct fec_platform_data __initconst tx28_fec1_data = {
>         .phy = PHY_INTERFACE_MODE_RMII,
>  };
Apart from Russell's objections, yes.

Best regards
Uwe

Patch

diff --git a/arch/arm/mach-mxs/module-tx28.c b/arch/arm/mach-mxs/module-tx28.c
index 0fcff47..a096cca 100644
--- a/arch/arm/mach-mxs/module-tx28.c
+++ b/arch/arm/mach-mxs/module-tx28.c
@@ -66,11 +66,11 @@  static const iomux_cfg_t tx28_fec1_pads[] __initconst = {
        MX28_PAD_ENET0_CRS__ENET1_RX_EN,
 };

-static struct fec_platform_data tx28_fec0_data = {
+static struct fec_platform_data __initconst tx28_fec0_data = {
        .phy = PHY_INTERFACE_MODE_RMII,
 };

-static struct fec_platform_data tx28_fec1_data = {
+static struct fec_platform_data __initconst tx28_fec1_data = {
        .phy = PHY_INTERFACE_MODE_RMII,
 };