Re: [PATCH 01/20] mtd: rawnand: txx9ndfmc: Mark driver struct with __refdata to prevent section mismatch warning

From: Miquel Raynal <miquel.raynal@bootlin.com>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Arnd Bergmann <arnd@arndb.de>,
	Masahiro Yamada <masahiroy@kernel.org>,
	David Woodhouse <dwmw2@infradead.org>,
	linux-kbuild@vger.kernel.org, Richard Weinberger <richard@nod.at>,
	Atsushi Nemoto <anemo@mba.ocn.ne.jp>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-mtd@lists.infradead.org,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	Andrew Morton <akpm@linux-foundation.org>,
	Vignesh Raghavendra <vigneshr@ti.com>
Subject: Re: [PATCH 01/20] mtd: rawnand: txx9ndfmc: Mark driver struct with __refdata to prevent section mismatch warning
Date: Mon, 9 Oct 2023 14:46:10 +0200	[thread overview]
Message-ID: <20231009144610.4ff82afa@xps-13> (raw)
In-Reply-To: <20231009103037.j44gkzqv7cpn4zpu@pengutronix.de>

Hello,

u.kleine-koenig@pengutronix.de wrote on Mon, 9 Oct 2023 12:30:37 +0200:

> Hello,
> 
> [Changed email address for David Woodhouse from intel to infradead]
> 
> On Mon, Oct 09, 2023 at 10:43:46AM +0200, Arnd Bergmann wrote:
> > On Mon, Oct 9, 2023, at 09:22, Masahiro Yamada wrote:  
> > > On Mon, Oct 9, 2023 at 5:02 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:  
> > >>
> > >> As described in the added code comment, a reference to .exit.text is ok
> > >> for drivers registered via module_platform_driver_probe(). Make this
> > >> explicit to prevent a section mismatch warning with  
> >   
> > >
> > > We have thousands of module_platform_drivers.
> > > I would be scared if they started to add __refdata.
> > >
> > > I am not sure if this is the right direction.  
> > 
> > For a normal module_platform_driver(), this would indeed be
> > wrong, but as Uwe said above there is a special case for
> > module_platform_driver_probe(), which implicitly sets the
> > drv->driver.suppress_bind_attrs=true flag.
> >   
> > > In my understanding of the current DT overlay,
> > > there is no way to create/remove a platform device dynamically.
> > > I do not know if that will happen in the future.  
> > 
> > For drivers without suppress_bind_attrs, you can manually
> > unbind the device from a driver, which in case of a loadable
> > module ends up calling the .remove callback (this is fine),
> > but in a built-in driver this would use a NULL pointer for
> > .remove and cause unexpected behavior.  
> 
> only a slight correction: As not having a remove callback can be fine
> and platform_remove() only calls .remove (or .remove_new) when non-NULL
> we're not hitting a NULL pointer dereference in the presence of
> 
> 	.remove = __exit_p(somefunc),
> 
> But a problem can arise later if some resource isn't properly freed and
> so it might be used at a later point in time which then most likely
> oopses.
> 
> I didn't double check Arnd's list, but otherwise I agree to his
> analysis.

Can we instead question the use of module_platform_driver_probe()?
I don't have the history in mind, but why not just switch to regular
module_platform_driver() registration instead? It seems like the
original authors just did not care about the remove path and were
happy to skip its implementation.

On mtd devices one can argue that the flash underlying stores the
rootfs and thus cannot be removed, but I believe today this is a
questionable (software) design.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/