diff mbox series

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

Message ID 20231008200143.196369-2-u.kleine-koenig@pengutronix.de (mailing list archive)
State New, archived
Headers show
Series mtd: Convert to platform remove callback returning void | expand

Commit Message

Uwe Kleine-König Oct. 8, 2023, 8:01 p.m. UTC
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
CONFIG_MTD_NAND_TXX9NDFMC=m:

	WARNING: modpost: drivers/mtd/nand/raw/txx9ndfmc: section mismatch in reference: txx9ndfmc_driver+0x4 (section: .data) -> txx9ndfmc_remove (section: .exit.text)

Fixes: 64fb65baffa5 ("[MTD] TXx9 SoC NAND Flash Memory Controller driver")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

note this warning only triggers with f177cd0c15fc ("modpost: Don't let
"driver"s reference .exit.*") that currently waits in next for the next
merge window + building with W=1.

Best regards
Uwe

 drivers/mtd/nand/raw/txx9ndfmc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Masahiro Yamada Oct. 9, 2023, 7:22 a.m. UTC | #1
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
> CONFIG_MTD_NAND_TXX9NDFMC=m:
>
>         WARNING: modpost: drivers/mtd/nand/raw/txx9ndfmc: section mismatch in reference: txx9ndfmc_driver+0x4 (section: .data) -> txx9ndfmc_remove (section: .exit.text)
>
> Fixes: 64fb65baffa5 ("[MTD] TXx9 SoC NAND Flash Memory Controller driver")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
>
> note this warning only triggers with f177cd0c15fc ("modpost: Don't let
> "driver"s reference .exit.*") that currently waits in next for the next
> merge window + building with W=1.
>
> Best regards
> Uwe
>
>  drivers/mtd/nand/raw/txx9ndfmc.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/nand/raw/txx9ndfmc.c b/drivers/mtd/nand/raw/txx9ndfmc.c
> index eddcc0728a67..aff9d6f16851 100644
> --- a/drivers/mtd/nand/raw/txx9ndfmc.c
> +++ b/drivers/mtd/nand/raw/txx9ndfmc.c
> @@ -406,7 +406,13 @@ static int txx9ndfmc_resume(struct platform_device *dev)
>  #define txx9ndfmc_resume NULL
>  #endif
>
> -static struct platform_driver txx9ndfmc_driver = {
> +/*
> + * txx9ndfmc_remove() lives in .exit.text. For drivers registered via
> + * module_platform_driver_probe() this is ok because they cannot get unbound at
> + * runtime. So mark the driver struct with __refdata to prevent modpost
> + * triggering a section mismatch warning.
> + */
> +static struct platform_driver txx9ndfmc_driver __refdata = {
>         .remove         = __exit_p(txx9ndfmc_remove),
>         .resume         = txx9ndfmc_resume,
>         .driver         = {
> --
> 2.40.1
>


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.

I added Greg and Arnd to CC.



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.



--
Best Regards
Masahiro Yamada
Arnd Bergmann Oct. 9, 2023, 8:43 a.m. UTC | #2
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.

This is the list of module_platform_driver_probe() instances
I found that have a .remove callback:

drivers/ata/pata_falcon.c-	.remove = __exit_p(pata_falcon_remove_one),
drivers/ata/pata_gayle.c-	.remove = __exit_p(pata_gayle_remove_one),
drivers/char/hw_random/mxc-rnga.c-	.remove = __exit_p(mxc_rnga_remove),
drivers/hwmon/mc13783-adc.c-	.remove_new	= mc13783_adc_remove,
drivers/input/mouse/amimouse.c-	.remove = __exit_p(amimouse_remove),
drivers/input/serio/q40kbd.c-	.remove_new	= q40kbd_remove,
drivers/input/touchscreen/mc13783_ts.c-	.remove_new	= mc13783_ts_remove,
drivers/leds/leds-mc13783.c-	.remove_new	= mc13xxx_led_remove,
drivers/media/platform/renesas/sh_vou.c:	.remove_new = sh_vou_remove,
drivers/media/rc/meson-ir-tx.c-	.remove_new = meson_irtx_remove,
drivers/memory/emif.c-	.remove		= __exit_p(emif_remove),
drivers/memory/ti-aemif.c-	.remove = aemif_remove,
drivers/memory/ti-emif-pm.c-	.remove = ti_emif_remove,
drivers/mtd/devices/docg3.c-	.remove		= docg3_release,
drivers/net/ethernet/broadcom/tg3.c-	.remove		= tg3_remove_one,
drivers/rapidio/switches/idt_gen3.c-	.remove = idtg3_remove,
drivers/video/fbdev/cg3.c-	.remove_new	= cg3_remove,
drivers/mtd/nand/raw/fsmc_nand.c-	.remove_new = fsmc_nand_remove,
drivers/mtd/nand/raw/orion_nand.c-	.remove_new	= orion_nand_remove,
drivers/mtd/nand/raw/sh_flctl.c-	.remove_new	= flctl_remove,
drivers/mtd/nand/raw/txx9ndfmc.c-	.remove		= __exit_p(txx9ndfmc_remove),
drivers/net/ethernet/cirrus/cs89x0.c-	.remove_new = cs89x0_platform_remove,
drivers/parport/parport_amiga.c-	.remove = __exit_p(amiga_parallel_remove),
drivers/power/reset/at91-poweroff.c-	.remove = __exit_p(at91_poweroff_remove),
drivers/power/reset/at91-reset.c-	.remove = __exit_p(at91_reset_remove),
drivers/power/reset/at91-sama5d2_shdwc.c-	.remove = __exit_p(at91_shdwc_remove),
drivers/rtc/rtc-at91rm9200.c-	.remove		= __exit_p(at91_rtc_remove),
drivers/rtc/rtc-at91sam9.c-	.remove_new	= at91_rtc_remove,
drivers/rtc/rtc-ftrtc010.c-	.remove_new	= ftrtc010_rtc_remove,
drivers/rtc/rtc-imxdi.c-	.remove = __exit_p(dryice_rtc_remove),
drivers/rtc/rtc-mc13xxx.c-	.remove_new = mc13xxx_rtc_remove,
drivers/rtc/rtc-mv.c-	.remove		= __exit_p(mv_rtc_remove),
drivers/rtc/rtc-pcap.c-	.remove = __exit_p(pcap_rtc_remove),
drivers/rtc/rtc-pxa.c-	.remove		= __exit_p(pxa_rtc_remove),
drivers/rtc/rtc-sh.c-	.remove		= __exit_p(sh_rtc_remove),
drivers/scsi/a3000.c-	.remove = __exit_p(amiga_a3000_scsi_remove),
drivers/scsi/a4000t.c-	.remove = __exit_p(amiga_a4000t_scsi_remove),
drivers/scsi/atari_scsi.c-	.remove = __exit_p(atari_scsi_remove),
drivers/scsi/mac_scsi.c-	.remove = __exit_p(mac_scsi_remove),
drivers/scsi/sun3_scsi.c-	.remove = __exit_p(sun3_scsi_remove),
drivers/tty/amiserial.c-	.remove = __exit_p(amiga_serial_remove),
drivers/usb/gadget/udc/at91_udc.c-	.remove		= at91udc_remove,
drivers/staging/emxx_udc/emxx_udc.c-	.remove_new	= nbu2ss_drv_remove,
drivers/usb/gadget/udc/aspeed_udc.c-	.remove			= ast_udc_remove,
drivers/usb/gadget/udc/at91_udc.c-	.remove		= at91udc_remove,
drivers/usb/gadget/udc/atmel_usba_udc.c-	.remove_new	= usba_udc_remove,
drivers/usb/gadget/udc/bcm63xx_udc.c-	.remove_new	= bcm63xx_udc_remove,
drivers/usb/gadget/udc/dummy_hcd.c-	.remove_new	= dummy_udc_remove,
drivers/usb/gadget/udc/fsl_qe_udc.c-	.remove_new     = qe_udc_remove,
drivers/usb/gadget/udc/fsl_udc_core.c-	.remove		= fsl_udc_remove,
drivers/usb/gadget/udc/lpc32xx_udc.c-	.remove		= lpc32xx_udc_remove,
drivers/usb/gadget/udc/mv_udc_core.c-	.remove_new	= mv_udc_remove,
drivers/usb/gadget/udc/omap_udc.c-	.remove_new	= omap_udc_remove,
drivers/usb/gadget/udc/pch_udc.c-	.remove =	pch_udc_remove,
drivers/usb/gadget/udc/pxa25x_udc.c-	.remove		= pxa25x_udc_remove,
drivers/usb/gadget/udc/pxa27x_udc.c-	.remove_new	= pxa_udc_remove,
drivers/usb/gadget/udc/renesas_usbf.c-	.remove_new     = usbf_remove,
drivers/usb/gadget/udc/tegra-xudc.c-	.remove_new = tegra_xudc_remove,
drivers/usb/gadget/udc/udc-xilinx.c-	.remove_new = xudc_remove,
drivers/usb/usbip/vudc_main.c-	.remove		= vudc_remove,
drivers/usb/gadget/udc/fusb300_udc.c-	.remove_new =	fusb300_remove,
drivers/usb/gadget/udc/lpc32xx_udc.c-	.remove		= lpc32xx_udc_remove,
drivers/usb/gadget/udc/m66592-udc.c-	.remove_new =	m66592_remove,
drivers/usb/gadget/udc/r8a66597-udc.c-	.remove_new =	r8a66597_remove,
drivers/usb/host/r8a66597-hcd.c-	.remove_new =	r8a66597_remove,
drivers/video/fbdev/amifb.c-	.remove = __exit_p(amifb_remove),
drivers/video/fbdev/atmel_lcdfb.c-	.remove		= __exit_p(atmel_lcdfb_remove),
drivers/video/fbdev/omap2/omapfb/vrfb.c-	.remove		= __exit_p(vrfb_remove),
drivers/virt/coco/sev-guest/sev-guest.c-	.remove		= __exit_p(sev_guest_remove),
drivers/watchdog/at91rm9200_wdt.c-	.remove_new	= at91wdt_remove,
drivers/watchdog/at91sam9_wdt.c-	.remove		= __exit_p(at91wdt_remove),
drivers/watchdog/txx9wdt.c-	.remove = __exit_p(txx9wdt_remove),

Out of these, only 29 have the .remove callback in the .exit.text section,
and they all use __exit_p:

drivers/ata/pata_falcon.c:static int __exit pata_falcon_remove_one(struct platform_device *pdev)
drivers/ata/pata_gayle.c:static int __exit pata_gayle_remove_one(struct platform_device *pdev)
drivers/char/hw_random/mxc-rnga.c:static int __exit mxc_rnga_remove(struct platform_device *pdev)
drivers/input/mouse/amimouse.c:static int __exit amimouse_remove(struct platform_device *pdev)
drivers/memory/emif.c:static int __exit emif_remove(struct platform_device *pdev)
drivers/mtd/nand/raw/txx9ndfmc.c:static int __exit txx9ndfmc_remove(struct platform_device *dev)
drivers/parport/parport_amiga.c:static int __exit amiga_parallel_remove(struct platform_device *pdev)
drivers/power/reset/at91-poweroff.c:static int __exit at91_poweroff_remove(struct platform_device *pdev)
drivers/power/reset/at91-reset.c:static int __exit at91_reset_remove(struct platform_device *pdev)
drivers/power/reset/at91-sama5d2_shdwc.c:static int __exit at91_shdwc_remove(struct platform_device *pdev)
drivers/rtc/rtc-at91rm9200.c:static int __exit at91_rtc_remove(struct platform_device *pdev)
drivers/rtc/rtc-imxdi.c:static int __exit dryice_rtc_remove(struct platform_device *pdev)
drivers/rtc/rtc-mv.c:static int __exit mv_rtc_remove(struct platform_device *pdev)
drivers/rtc/rtc-pcap.c:static int __exit pcap_rtc_remove(struct platform_device *pdev)
drivers/rtc/rtc-pxa.c:static int __exit pxa_rtc_remove(struct platform_device *pdev)
drivers/rtc/rtc-sh.c:static int __exit sh_rtc_remove(struct platform_device *pdev)
drivers/scsi/a3000.c:static int __exit amiga_a3000_scsi_remove(struct platform_device *pdev)
drivers/scsi/a4000t.c:static int __exit amiga_a4000t_scsi_remove(struct platform_device *pdev)
drivers/scsi/atari_scsi.c:static int __exit atari_scsi_remove(struct platform_device *pdev)
drivers/scsi/mac_scsi.c:static int __exit mac_scsi_remove(struct platform_device *pdev)
drivers/scsi/sun3_scsi.c:static int __exit sun3_scsi_remove(struct platform_device *pdev)
drivers/tty/amiserial.c:static int __exit amiga_serial_remove(struct platform_device *pdev)
drivers/video/fbdev/amifb.c:static int __exit amifb_remove(struct platform_device *pdev)
drivers/video/fbdev/atmel_lcdfb.c:static int __exit atmel_lcdfb_remove(struct platform_device *pdev)
drivers/video/fbdev/omap2/omapfb/vrfb.c:static void __exit vrfb_remove(struct platform_device *pdev)
drivers/virt/coco/sev-guest/sev-guest.c:static int __exit sev_guest_remove(struct platform_device *pdev)
drivers/watchdog/at91sam9_wdt.c:static int __exit at91wdt_remove(struct platform_device *pdev)
drivers/watchdog/at91sam9_wdt.c:static int __exit at91wdt_remove(struct platform_device *pdev)
drivers/watchdog/txx9wdt.c:static int __exit txx9wdt_remove(struct platform_device *dev)

These are mostly ancient drivers, and half of them don't even have
DT support, so I think annotating them with __refdata is probably
fine, but removing the __exit_p() and __exit annotations would
also work, depending on the individual maintainer's preferences.

There are three more drivers that set .suppress_bind_addrs=true
and have a __exit_p .remove callback (coresight-etm4x-core.c,
pcie-kirin.c, omapfb), they would also need to make the same
decision to shut up the warning.

All other drivers that have an __exit annotated .remove callback
are in probably broken and need a proper fix.

      Arnd
Uwe Kleine-König Oct. 9, 2023, 10:30 a.m. UTC | #3
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.

Best regards
Uwe
Miquel Raynal Oct. 9, 2023, 12:46 p.m. UTC | #4
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
Arnd Bergmann Oct. 9, 2023, 1:01 p.m. UTC | #5
On Mon, Oct 9, 2023, at 14:46, Miquel Raynal wrote:
>> 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> > 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.

It was changed to module_platform_driver_probe() in commit
3a2a13fa902d2 ("mtd: txx9ndfmc: use module_platform_driver_probe()")
with a short changelog text:

commit 3a2a13fa902d232a1e56582647aed6cb2591349b
Author: Jingoo Han <jg1.han@samsung.com>
Date:   Tue Mar 5 13:31:24 2013 +0900

    mtd: txx9ndfmc: use module_platform_driver_probe()
    
    This patch uses module_platform_driver_probe() macro which makes
    the code smaller and simpler.
    
    Signed-off-by: Jingoo Han <jg1.han@samsung.com>
    Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
    Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>

Instead of just simplifying the code, I think that was actually
a bugfix because it prevented both the probe and remove callbacks
from getting called after getting dropped (deferred probe or
unbind/rebind). Using module_platform_driver() is probably
even better here, but then we need to remove both the __init
and __exit annotations.

      Arnd
Miquel Raynal Oct. 9, 2023, 1:37 p.m. UTC | #6
Hi Arnd,

arnd@arndb.de wrote on Mon, 09 Oct 2023 15:01:17 +0200:

> On Mon, Oct 9, 2023, at 14:46, Miquel Raynal wrote:
> >> 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> > 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.  
> 
> It was changed to module_platform_driver_probe() in commit
> 3a2a13fa902d2 ("mtd: txx9ndfmc: use module_platform_driver_probe()")
> with a short changelog text:
> 
> commit 3a2a13fa902d232a1e56582647aed6cb2591349b
> Author: Jingoo Han <jg1.han@samsung.com>
> Date:   Tue Mar 5 13:31:24 2013 +0900
> 
>     mtd: txx9ndfmc: use module_platform_driver_probe()
>     
>     This patch uses module_platform_driver_probe() macro which makes
>     the code smaller and simpler.
>     
>     Signed-off-by: Jingoo Han <jg1.han@samsung.com>
>     Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>     Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> 
> Instead of just simplifying the code, I think that was actually
> a bugfix because it prevented both the probe and remove callbacks
> from getting called after getting dropped (deferred probe or
> unbind/rebind). Using module_platform_driver() is probably
> even better here, but then we need to remove both the __init
> and __exit annotations.

Agreed.

Thanks,
Miquèl
Miquel Raynal Oct. 16, 2023, 8:57 a.m. UTC | #7
Hello,

miquel.raynal@bootlin.com wrote on Mon, 9 Oct 2023 15:37:41 +0200:

> Hi Arnd,
> 
> arnd@arndb.de wrote on Mon, 09 Oct 2023 15:01:17 +0200:
> 
> > On Mon, Oct 9, 2023, at 14:46, Miquel Raynal wrote:  
> > >> 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> > 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.    
> > 
> > It was changed to module_platform_driver_probe() in commit
> > 3a2a13fa902d2 ("mtd: txx9ndfmc: use module_platform_driver_probe()")
> > with a short changelog text:
> > 
> > commit 3a2a13fa902d232a1e56582647aed6cb2591349b
> > Author: Jingoo Han <jg1.han@samsung.com>
> > Date:   Tue Mar 5 13:31:24 2013 +0900
> > 
> >     mtd: txx9ndfmc: use module_platform_driver_probe()
> >     
> >     This patch uses module_platform_driver_probe() macro which makes
> >     the code smaller and simpler.
> >     
> >     Signed-off-by: Jingoo Han <jg1.han@samsung.com>
> >     Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> >     Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> > 
> > Instead of just simplifying the code, I think that was actually
> > a bugfix because it prevented both the probe and remove callbacks
> > from getting called after getting dropped (deferred probe or
> > unbind/rebind). Using module_platform_driver() is probably
> > even better here, but then we need to remove both the __init
> > and __exit annotations.  
> 
> Agreed.
> 
> Thanks,
> Miquèl

FYI I've collected all the patches in this series except the txx9ndfmc
ones.

Thanks,
Miquèl
Arnd Bergmann Oct. 16, 2023, 9:25 a.m. UTC | #8
On Mon, Oct 9, 2023, at 12:30, Uwe Kleine-König wrote:
> On Mon, Oct 09, 2023 at 10:43:46AM +0200, Arnd Bergmann wrote:
>
> 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.

Hi Uwe,

Based on a few days of randconfig build testing, the patch
below addresses the remaining warnings I get for arm, arm64 and
x86 on linux-next. This is a shorter list than the ones that
I found in theory, possibly because some of the other ones
are only used in built-in code, or because they are never used
on these three architectures.

Have you already sent patches for (some of) these?

      Arnd

 drivers/char/hw_random/mxc-rnga.c                                 | 2 +-
 drivers/gpu/drm/bridge/ti-tpd12s015.c                             | 4 ++--
 drivers/hwmon/smsc47m1.c                                          | 2 +-
 drivers/hwtracing/coresight/coresight-etm4x-core.c                | 8 ++++----
 drivers/media/i2c/et8ek8/et8ek8_driver.c                          | 4 ++--
 drivers/memory/emif.c                                             | 2 +-
 drivers/mmc/host/davinci_mmc.c                                    | 2 +-
 drivers/mtd/nand/raw/txx9ndfmc.c                                  | 2 +-
 drivers/pci/controller/dwc/pci-exynos.c                           | 4 ++--
 drivers/pci/controller/dwc/pcie-kirin.c                           | 4 ++--
 drivers/power/reset/at91-poweroff.c                               | 2 +-
 drivers/power/reset/at91-reset.c                                  | 2 +-
 drivers/power/reset/at91-sama5d2_shdwc.c                          | 2 +-
 drivers/rtc/rtc-at91rm9200.c                                      | 2 +-
 drivers/rtc/rtc-imxdi.c                                           | 2 +-
 drivers/rtc/rtc-mv.c                                              | 2 +-
 drivers/rtc/rtc-pxa.c                                             | 2 +-
 drivers/rtc/rtc-sh.c                                              | 2 +-
 drivers/video/fbdev/atmel_lcdfb.c                                 | 2 +-
 drivers/video/fbdev/omap2/omapfb/displays/connector-analog-tv.c   | 2 +-
 drivers/video/fbdev/omap2/omapfb/displays/connector-dvi.c         | 2 +-
 drivers/video/fbdev/omap2/omapfb/displays/connector-hdmi.c        | 2 +-
 drivers/video/fbdev/omap2/omapfb/displays/encoder-opa362.c        | 2 +-
 drivers/video/fbdev/omap2/omapfb/displays/encoder-tfp410.c        | 2 +-
 drivers/video/fbdev/omap2/omapfb/displays/encoder-tpd12s015.c     | 2 +-
 drivers/video/fbdev/omap2/omapfb/displays/panel-dpi.c             | 2 +-
 drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c          | 2 +-
 .../video/fbdev/omap2/omapfb/displays/panel-sharp-ls037v7dw01.c   | 2 +-
 drivers/virt/coco/sev-guest/sev-guest.c                           | 2 +-
 drivers/watchdog/at91sam9_wdt.c                                   | 2 +-
 sound/soc/codecs/tlv320adc3xxx.c                                  | 4 ++--
 31 files changed, 39 insertions(+), 39 deletions(-)
diff --git a/drivers/char/hw_random/mxc-rnga.c b/drivers/char/hw_random/mxc-rnga.c
index 008763c988ed8..fc23777fffc90 100644
--- a/drivers/char/hw_random/mxc-rnga.c
+++ b/drivers/char/hw_random/mxc-rnga.c
@@ -194,7 +194,7 @@ static const struct of_device_id mxc_rnga_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, mxc_rnga_of_match);
 
-static struct platform_driver mxc_rnga_driver = {
+static struct platform_driver mxc_rnga_driver __refdata = {
 	.driver = {
 		.name = "mxc_rnga",
 		.of_match_table = mxc_rnga_of_match,
diff --git a/drivers/gpu/drm/bridge/ti-tpd12s015.c b/drivers/gpu/drm/bridge/ti-tpd12s015.c
index e0e015243a602..ecd0e0db68af3 100644
--- a/drivers/gpu/drm/bridge/ti-tpd12s015.c
+++ b/drivers/gpu/drm/bridge/ti-tpd12s015.c
@@ -179,7 +179,7 @@ static int tpd12s015_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int __exit tpd12s015_remove(struct platform_device *pdev)
+static int tpd12s015_remove(struct platform_device *pdev)
 {
 	struct tpd12s015_device *tpd = platform_get_drvdata(pdev);
 
@@ -197,7 +197,7 @@ MODULE_DEVICE_TABLE(of, tpd12s015_of_match);
 
 static struct platform_driver tpd12s015_driver = {
 	.probe	= tpd12s015_probe,
-	.remove	= __exit_p(tpd12s015_remove),
+	.remove	= tpd12s015_remove,
 	.driver	= {
 		.name	= "tpd12s015",
 		.of_match_table = tpd12s015_of_match,
diff --git a/drivers/hwmon/smsc47m1.c b/drivers/hwmon/smsc47m1.c
index 37531b5c82547..ff454f4fb0ffb 100644
--- a/drivers/hwmon/smsc47m1.c
+++ b/drivers/hwmon/smsc47m1.c
@@ -850,7 +850,7 @@ static int __exit smsc47m1_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static struct platform_driver smsc47m1_driver = {
+static struct platform_driver smsc47m1_driver __refdata = {
 	.driver = {
 		.name	= DRVNAME,
 	},
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 77b0271ce6eb9..69e292ca49f0a 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -2224,7 +2224,7 @@ static void clear_etmdrvdata(void *info)
 	per_cpu(delayed_probe, cpu) = NULL;
 }
 
-static void __exit etm4_remove_dev(struct etmv4_drvdata *drvdata)
+static void etm4_remove_dev(struct etmv4_drvdata *drvdata)
 {
 	bool had_delayed_probe;
 	/*
@@ -2253,7 +2253,7 @@ static void __exit etm4_remove_dev(struct etmv4_drvdata *drvdata)
 	}
 }
 
-static void __exit etm4_remove_amba(struct amba_device *adev)
+static void etm4_remove_amba(struct amba_device *adev)
 {
 	struct etmv4_drvdata *drvdata = dev_get_drvdata(&adev->dev);
 
@@ -2261,7 +2261,7 @@ static void __exit etm4_remove_amba(struct amba_device *adev)
 		etm4_remove_dev(drvdata);
 }
 
-static int __exit etm4_remove_platform_dev(struct platform_device *pdev)
+static int etm4_remove_platform_dev(struct platform_device *pdev)
 {
 	struct etmv4_drvdata *drvdata = dev_get_drvdata(&pdev->dev);
 
@@ -2305,7 +2305,7 @@ static const struct amba_id etm4_ids[] = {
 
 MODULE_DEVICE_TABLE(amba, etm4_ids);
 
-static struct amba_driver etm4x_amba_driver = {
+static struct amba_driver etm4x_amba_driver __refdata = {
 	.drv = {
 		.name   = "coresight-etm4x",
 		.owner  = THIS_MODULE,
diff --git a/drivers/media/i2c/et8ek8/et8ek8_driver.c b/drivers/media/i2c/et8ek8/et8ek8_driver.c
index d6fc843f9368e..0d6f0f8506f76 100644
--- a/drivers/media/i2c/et8ek8/et8ek8_driver.c
+++ b/drivers/media/i2c/et8ek8/et8ek8_driver.c
@@ -1460,7 +1460,7 @@ static int et8ek8_probe(struct i2c_client *client)
 	return ret;
 }
 
-static void __exit et8ek8_remove(struct i2c_client *client)
+static void et8ek8_remove(struct i2c_client *client)
 {
 	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
 	struct et8ek8_sensor *sensor = to_et8ek8_sensor(subdev);
@@ -1502,7 +1502,7 @@ static struct i2c_driver et8ek8_i2c_driver = {
 		.of_match_table	= et8ek8_of_table,
 	},
 	.probe		= et8ek8_probe,
-	.remove		= __exit_p(et8ek8_remove),
+	.remove		= et8ek8_remove,
 	.id_table	= et8ek8_id_table,
 };
 
diff --git a/drivers/memory/emif.c b/drivers/memory/emif.c
index f305643209f03..b0378dca13b85 100644
--- a/drivers/memory/emif.c
+++ b/drivers/memory/emif.c
@@ -1184,7 +1184,7 @@ static const struct of_device_id emif_of_match[] = {
 MODULE_DEVICE_TABLE(of, emif_of_match);
 #endif
 
-static struct platform_driver emif_driver = {
+static struct platform_driver emif_driver __refdata = {
 	.remove		= __exit_p(emif_remove),
 	.shutdown	= emif_shutdown,
 	.driver = {
diff --git a/drivers/mmc/host/davinci_mmc.c b/drivers/mmc/host/davinci_mmc.c
index ee3b1a4e08485..f35436e3ffc1e 100644
--- a/drivers/mmc/host/davinci_mmc.c
+++ b/drivers/mmc/host/davinci_mmc.c
@@ -1391,7 +1391,7 @@ static const struct dev_pm_ops davinci_mmcsd_pm = {
 #define davinci_mmcsd_pm_ops NULL
 #endif
 
-static struct platform_driver davinci_mmcsd_driver = {
+static struct platform_driver davinci_mmcsd_driver __refdata = {
 	.driver		= {
 		.name	= "davinci_mmc",
 		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
diff --git a/drivers/mtd/nand/raw/txx9ndfmc.c b/drivers/mtd/nand/raw/txx9ndfmc.c
index eddcc0728a676..cb8bd5324e1d3 100644
--- a/drivers/mtd/nand/raw/txx9ndfmc.c
+++ b/drivers/mtd/nand/raw/txx9ndfmc.c
@@ -406,7 +406,7 @@ static int txx9ndfmc_resume(struct platform_device *dev)
 #define txx9ndfmc_resume NULL
 #endif
 
-static struct platform_driver txx9ndfmc_driver = {
+static struct platform_driver txx9ndfmc_driver __refdata = {
 	.remove		= __exit_p(txx9ndfmc_remove),
 	.resume		= txx9ndfmc_resume,
 	.driver		= {
diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
index 6319082301d68..c6bede3469320 100644
--- a/drivers/pci/controller/dwc/pci-exynos.c
+++ b/drivers/pci/controller/dwc/pci-exynos.c
@@ -375,7 +375,7 @@ static int exynos_pcie_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int __exit exynos_pcie_remove(struct platform_device *pdev)
+static int exynos_pcie_remove(struct platform_device *pdev)
 {
 	struct exynos_pcie *ep = platform_get_drvdata(pdev);
 
@@ -431,7 +431,7 @@ static const struct of_device_id exynos_pcie_of_match[] = {
 
 static struct platform_driver exynos_pcie_driver = {
 	.probe		= exynos_pcie_probe,
-	.remove		= __exit_p(exynos_pcie_remove),
+	.remove		= exynos_pcie_remove,
 	.driver = {
 		.name	= "exynos-pcie",
 		.of_match_table = exynos_pcie_of_match,
diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index d93bc29069502..2ee146767971c 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -741,7 +741,7 @@ static int kirin_pcie_power_on(struct platform_device *pdev,
 	return ret;
 }
 
-static int __exit kirin_pcie_remove(struct platform_device *pdev)
+static int kirin_pcie_remove(struct platform_device *pdev)
 {
 	struct kirin_pcie *kirin_pcie = platform_get_drvdata(pdev);
 
@@ -818,7 +818,7 @@ static int kirin_pcie_probe(struct platform_device *pdev)
 
 static struct platform_driver kirin_pcie_driver = {
 	.probe			= kirin_pcie_probe,
-	.remove	        	= __exit_p(kirin_pcie_remove),
+	.remove	        	= kirin_pcie_remove,
 	.driver			= {
 		.name			= "kirin-pcie",
 		.of_match_table		= kirin_pcie_match,
diff --git a/drivers/power/reset/at91-poweroff.c b/drivers/power/reset/at91-poweroff.c
index dd5399785b691..061e3e2af751e 100644
--- a/drivers/power/reset/at91-poweroff.c
+++ b/drivers/power/reset/at91-poweroff.c
@@ -223,7 +223,7 @@ static const struct of_device_id at91_poweroff_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, at91_poweroff_of_match);
 
-static struct platform_driver at91_poweroff_driver = {
+static struct platform_driver at91_poweroff_driver __refdata = {
 	.remove = __exit_p(at91_poweroff_remove),
 	.driver = {
 		.name = "at91-poweroff",
diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c
index aa9b012d3d00b..5750e58b7daf9 100644
--- a/drivers/power/reset/at91-reset.c
+++ b/drivers/power/reset/at91-reset.c
@@ -427,7 +427,7 @@ static int __exit at91_reset_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static struct platform_driver at91_reset_driver = {
+static struct platform_driver at91_reset_driver __refdata = {
 	.remove = __exit_p(at91_reset_remove),
 	.driver = {
 		.name = "at91-reset",
diff --git a/drivers/power/reset/at91-sama5d2_shdwc.c b/drivers/power/reset/at91-sama5d2_shdwc.c
index e76b102b57b1f..0a6bc0ad2310d 100644
--- a/drivers/power/reset/at91-sama5d2_shdwc.c
+++ b/drivers/power/reset/at91-sama5d2_shdwc.c
@@ -441,7 +441,7 @@ static int __exit at91_shdwc_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static struct platform_driver at91_shdwc_driver = {
+static struct platform_driver at91_shdwc_driver __refdata = {
 	.remove = __exit_p(at91_shdwc_remove),
 	.driver = {
 		.name = "at91-shdwc",
diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
index add4f71d7b3b9..04eed377c7eb0 100644
--- a/drivers/rtc/rtc-at91rm9200.c
+++ b/drivers/rtc/rtc-at91rm9200.c
@@ -635,7 +635,7 @@ static int at91_rtc_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(at91_rtc_pm_ops, at91_rtc_suspend, at91_rtc_resume);
 
-static struct platform_driver at91_rtc_driver = {
+static struct platform_driver at91_rtc_driver __refdata = {
 	.remove		= __exit_p(at91_rtc_remove),
 	.shutdown	= at91_rtc_shutdown,
 	.driver		= {
diff --git a/drivers/rtc/rtc-imxdi.c b/drivers/rtc/rtc-imxdi.c
index 4b712e5ab08a0..05f26c5fcc955 100644
--- a/drivers/rtc/rtc-imxdi.c
+++ b/drivers/rtc/rtc-imxdi.c
@@ -851,7 +851,7 @@ static const struct of_device_id dryice_dt_ids[] = {
 
 MODULE_DEVICE_TABLE(of, dryice_dt_ids);
 
-static struct platform_driver dryice_rtc_driver = {
+static struct platform_driver dryice_rtc_driver __refdata = {
 	.driver = {
 		   .name = "imxdi_rtc",
 		   .of_match_table = dryice_dt_ids,
diff --git a/drivers/rtc/rtc-mv.c b/drivers/rtc/rtc-mv.c
index 6c526e2ec56d8..0d6226f9a4b46 100644
--- a/drivers/rtc/rtc-mv.c
+++ b/drivers/rtc/rtc-mv.c
@@ -303,7 +303,7 @@ static const struct of_device_id rtc_mv_of_match_table[] = {
 MODULE_DEVICE_TABLE(of, rtc_mv_of_match_table);
 #endif
 
-static struct platform_driver mv_rtc_driver = {
+static struct platform_driver mv_rtc_driver __refdata = {
 	.remove		= __exit_p(mv_rtc_remove),
 	.driver		= {
 		.name	= "rtc-mv",
diff --git a/drivers/rtc/rtc-pxa.c b/drivers/rtc/rtc-pxa.c
index e400c78252e82..be0b36dd9d150 100644
--- a/drivers/rtc/rtc-pxa.c
+++ b/drivers/rtc/rtc-pxa.c
@@ -403,7 +403,7 @@ static int pxa_rtc_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(pxa_rtc_pm_ops, pxa_rtc_suspend, pxa_rtc_resume);
 
-static struct platform_driver pxa_rtc_driver = {
+static struct platform_driver pxa_rtc_driver __refdata = {
 	.remove		= __exit_p(pxa_rtc_remove),
 	.driver		= {
 		.name	= "pxa-rtc",
diff --git a/drivers/rtc/rtc-sh.c b/drivers/rtc/rtc-sh.c
index 613a5a3a09cf3..96dd3a73707d6 100644
--- a/drivers/rtc/rtc-sh.c
+++ b/drivers/rtc/rtc-sh.c
@@ -668,7 +668,7 @@ static const struct of_device_id sh_rtc_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, sh_rtc_of_match);
 
-static struct platform_driver sh_rtc_platform_driver = {
+static struct platform_driver sh_rtc_platform_driver __refdata = {
 	.driver		= {
 		.name	= DRV_NAME,
 		.pm	= &sh_rtc_pm_ops,
diff --git a/drivers/video/fbdev/atmel_lcdfb.c b/drivers/video/fbdev/atmel_lcdfb.c
index a908db2334098..aad6efae964ad 100644
--- a/drivers/video/fbdev/atmel_lcdfb.c
+++ b/drivers/video/fbdev/atmel_lcdfb.c
@@ -1300,7 +1300,7 @@ static int atmel_lcdfb_resume(struct platform_device *pdev)
 #define atmel_lcdfb_resume	NULL
 #endif
 
-static struct platform_driver atmel_lcdfb_driver = {
+static struct platform_driver atmel_lcdfb_driver __refdata = {
 	.remove		= __exit_p(atmel_lcdfb_remove),
 	.suspend	= atmel_lcdfb_suspend,
 	.resume		= atmel_lcdfb_resume,
diff --git a/drivers/video/fbdev/omap2/omapfb/displays/connector-analog-tv.c b/drivers/video/fbdev/omap2/omapfb/displays/connector-analog-tv.c
index 0daaf9f89bab5..3f555b65e81a0 100644
--- a/drivers/video/fbdev/omap2/omapfb/displays/connector-analog-tv.c
+++ b/drivers/video/fbdev/omap2/omapfb/displays/connector-analog-tv.c
@@ -245,7 +245,7 @@ static const struct of_device_id tvc_of_match[] = {
 
 MODULE_DEVICE_TABLE(of, tvc_of_match);
 
-static struct platform_driver tvc_connector_driver = {
+static struct platform_driver tvc_connector_driver __refdata = {
 	.probe	= tvc_probe,
 	.remove	= __exit_p(tvc_remove),
 	.driver	= {
diff --git a/drivers/video/fbdev/omap2/omapfb/displays/connector-dvi.c b/drivers/video/fbdev/omap2/omapfb/displays/connector-dvi.c
index c8ad3ef42bd31..dfcfba9b6efa7 100644
--- a/drivers/video/fbdev/omap2/omapfb/displays/connector-dvi.c
+++ b/drivers/video/fbdev/omap2/omapfb/displays/connector-dvi.c
@@ -328,7 +328,7 @@ static const struct of_device_id dvic_of_match[] = {
 
 MODULE_DEVICE_TABLE(of, dvic_of_match);
 
-static struct platform_driver dvi_connector_driver = {
+static struct platform_driver dvi_connector_driver __refdata = {
 	.probe	= dvic_probe,
 	.remove	= __exit_p(dvic_remove),
 	.driver	= {
diff --git a/drivers/video/fbdev/omap2/omapfb/displays/connector-hdmi.c b/drivers/video/fbdev/omap2/omapfb/displays/connector-hdmi.c
index 8f9ff9fb4ca4c..2ed3103005034 100644
--- a/drivers/video/fbdev/omap2/omapfb/displays/connector-hdmi.c
+++ b/drivers/video/fbdev/omap2/omapfb/displays/connector-hdmi.c
@@ -272,7 +272,7 @@ static const struct of_device_id hdmic_of_match[] = {
 
 MODULE_DEVICE_TABLE(of, hdmic_of_match);
 
-static struct platform_driver hdmi_connector_driver = {
+static struct platform_driver hdmi_connector_driver __refdata = {
 	.probe	= hdmic_probe,
 	.remove	= __exit_p(hdmic_remove),
 	.driver	= {
diff --git a/drivers/video/fbdev/omap2/omapfb/displays/encoder-opa362.c b/drivers/video/fbdev/omap2/omapfb/displays/encoder-opa362.c
index dd29dc5c77ec8..cc19eef9a4609 100644
--- a/drivers/video/fbdev/omap2/omapfb/displays/encoder-opa362.c
+++ b/drivers/video/fbdev/omap2/omapfb/displays/encoder-opa362.c
@@ -258,7 +258,7 @@ static const struct of_device_id opa362_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, opa362_of_match);
 
-static struct platform_driver opa362_driver = {
+static struct platform_driver opa362_driver __refdata = {
 	.probe	= opa362_probe,
 	.remove	= __exit_p(opa362_remove),
 	.driver	= {
diff --git a/drivers/video/fbdev/omap2/omapfb/displays/encoder-tfp410.c b/drivers/video/fbdev/omap2/omapfb/displays/encoder-tfp410.c
index 7bac420169a69..4900edaf1068d 100644
--- a/drivers/video/fbdev/omap2/omapfb/displays/encoder-tfp410.c
+++ b/drivers/video/fbdev/omap2/omapfb/displays/encoder-tfp410.c
@@ -245,7 +245,7 @@ static const struct of_device_id tfp410_of_match[] = {
 
 MODULE_DEVICE_TABLE(of, tfp410_of_match);
 
-static struct platform_driver tfp410_driver = {
+static struct platform_driver tfp410_driver __refdata = {
 	.probe	= tfp410_probe,
 	.remove	= __exit_p(tfp410_remove),
 	.driver	= {
diff --git a/drivers/video/fbdev/omap2/omapfb/displays/encoder-tpd12s015.c b/drivers/video/fbdev/omap2/omapfb/displays/encoder-tpd12s015.c
index 67f0c9250e9e4..648bb1367dc5a 100644
--- a/drivers/video/fbdev/omap2/omapfb/displays/encoder-tpd12s015.c
+++ b/drivers/video/fbdev/omap2/omapfb/displays/encoder-tpd12s015.c
@@ -311,7 +311,7 @@ static const struct of_device_id tpd_of_match[] = {
 
 MODULE_DEVICE_TABLE(of, tpd_of_match);
 
-static struct platform_driver tpd_driver = {
+static struct platform_driver tpd_driver __refdata = {
 	.probe	= tpd_probe,
 	.remove	= __exit_p(tpd_remove),
 	.driver	= {
diff --git a/drivers/video/fbdev/omap2/omapfb/displays/panel-dpi.c b/drivers/video/fbdev/omap2/omapfb/displays/panel-dpi.c
index 9790053c5877c..e39becd4d8ecf 100644
--- a/drivers/video/fbdev/omap2/omapfb/displays/panel-dpi.c
+++ b/drivers/video/fbdev/omap2/omapfb/displays/panel-dpi.c
@@ -234,7 +234,7 @@ static const struct of_device_id panel_dpi_of_match[] = {
 
 MODULE_DEVICE_TABLE(of, panel_dpi_of_match);
 
-static struct platform_driver panel_dpi_driver = {
+static struct platform_driver panel_dpi_driver __refdata = {
 	.probe = panel_dpi_probe,
 	.remove = __exit_p(panel_dpi_remove),
 	.driver = {
diff --git a/drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c b/drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c
index 77fce1223a640..5d8b6daeb199b 100644
--- a/drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c
+++ b/drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c
@@ -1280,7 +1280,7 @@ static const struct of_device_id dsicm_of_match[] = {
 
 MODULE_DEVICE_TABLE(of, dsicm_of_match);
 
-static struct platform_driver dsicm_driver = {
+static struct platform_driver dsicm_driver __refdata = {
 	.probe = dsicm_probe,
 	.remove = __exit_p(dsicm_remove),
 	.driver = {
diff --git a/drivers/video/fbdev/omap2/omapfb/displays/panel-sharp-ls037v7dw01.c b/drivers/video/fbdev/omap2/omapfb/displays/panel-sharp-ls037v7dw01.c
index cc30758300e25..9181008c5c79c 100644
--- a/drivers/video/fbdev/omap2/omapfb/displays/panel-sharp-ls037v7dw01.c
+++ b/drivers/video/fbdev/omap2/omapfb/displays/panel-sharp-ls037v7dw01.c
@@ -315,7 +315,7 @@ static const struct of_device_id sharp_ls_of_match[] = {
 
 MODULE_DEVICE_TABLE(of, sharp_ls_of_match);
 
-static struct platform_driver sharp_ls_driver = {
+static struct platform_driver sharp_ls_driver __refdata = {
 	.probe = sharp_ls_probe,
 	.remove = __exit_p(sharp_ls_remove),
 	.driver = {
diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index 97dbe715e96ad..5b299af5b1867 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -852,7 +852,7 @@ static int __exit sev_guest_remove(struct platform_device *pdev)
  * support any SEV guest API. As such, even though it has been introduced
  * with the SEV-SNP support, it is named "sev-guest".
  */
-static struct platform_driver sev_guest_driver = {
+static struct platform_driver sev_guest_driver __refdata = {
 	.remove		= __exit_p(sev_guest_remove),
 	.driver		= {
 		.name = "sev-guest",
diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
index b111b28acb948..b55ae7998a8d3 100644
--- a/drivers/watchdog/at91sam9_wdt.c
+++ b/drivers/watchdog/at91sam9_wdt.c
@@ -392,7 +392,7 @@ static const struct of_device_id at91_wdt_dt_ids[] = {
 MODULE_DEVICE_TABLE(of, at91_wdt_dt_ids);
 #endif
 
-static struct platform_driver at91wdt_driver = {
+static struct platform_driver at91wdt_driver __refdata = {
 	.remove		= __exit_p(at91wdt_remove),
 	.driver		= {
 		.name	= "at91_wdt",
diff --git a/sound/soc/codecs/tlv320adc3xxx.c b/sound/soc/codecs/tlv320adc3xxx.c
index 420bbf588efea..e100cc9f5c192 100644
--- a/sound/soc/codecs/tlv320adc3xxx.c
+++ b/sound/soc/codecs/tlv320adc3xxx.c
@@ -1429,7 +1429,7 @@ static int adc3xxx_i2c_probe(struct i2c_client *i2c)
 	return ret;
 }
 
-static void __exit adc3xxx_i2c_remove(struct i2c_client *client)
+static void adc3xxx_i2c_remove(struct i2c_client *client)
 {
 	struct adc3xxx *adc3xxx = i2c_get_clientdata(client);
 
@@ -1452,7 +1452,7 @@ static struct i2c_driver adc3xxx_i2c_driver = {
 		   .of_match_table = tlv320adc3xxx_of_match,
 		  },
 	.probe = adc3xxx_i2c_probe,
-	.remove = __exit_p(adc3xxx_i2c_remove),
+	.remove = adc3xxx_i2c_remove,
 	.id_table = adc3xxx_i2c_id,
 };
Uwe Kleine-König Oct. 16, 2023, 10:21 a.m. UTC | #9
On Mon, Oct 16, 2023 at 11:25:44AM +0200, Arnd Bergmann wrote:
> On Mon, Oct 9, 2023, at 12:30, Uwe Kleine-König wrote:
> > On Mon, Oct 09, 2023 at 10:43:46AM +0200, Arnd Bergmann wrote:
> >
> > 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.
> 
> Hi Uwe,
> 
> Based on a few days of randconfig build testing, the patch
> below addresses the remaining warnings I get for arm, arm64 and
> x86 on linux-next. This is a shorter list than the ones that
> I found in theory, possibly because some of the other ones
> are only used in built-in code, or because they are never used
> on these three architectures.
> 
> Have you already sent patches for (some of) these?
> 
>       Arnd
> 
>  drivers/char/hw_random/mxc-rnga.c                                 | 2 +-
>  drivers/gpu/drm/bridge/ti-tpd12s015.c                             | 4 ++--
>  drivers/hwmon/smsc47m1.c                                          | 2 +-
>  drivers/hwtracing/coresight/coresight-etm4x-core.c                | 8 ++++----
>  drivers/media/i2c/et8ek8/et8ek8_driver.c                          | 4 ++--
>  drivers/memory/emif.c                                             | 2 +-
>  drivers/mmc/host/davinci_mmc.c                                    | 2 +-
>  drivers/mtd/nand/raw/txx9ndfmc.c                                  | 2 +-

The txx9ndfmc driver was fixed as part of this series, but Miquèl didn't
take the patch, I guess because he wants it to be converted to
module_platform_driver().

>  drivers/pci/controller/dwc/pci-exynos.c                           | 4 ++--
>  drivers/pci/controller/dwc/pcie-kirin.c                           | 4 ++--

These two drivers were already covered in 20231001170254.2506508-1-u.kleine-koenig@pengutronix.de
where I also addressed a similar problem in the keystone driver.

>  drivers/power/reset/at91-poweroff.c                               | 2 +-
>  drivers/power/reset/at91-reset.c                                  | 2 +-
>  drivers/power/reset/at91-sama5d2_shdwc.c                          | 2 +-
>  drivers/rtc/rtc-at91rm9200.c                                      | 2 +-
>  drivers/rtc/rtc-imxdi.c                                           | 2 +-
>  drivers/rtc/rtc-mv.c                                              | 2 +-
>  drivers/rtc/rtc-pxa.c                                             | 2 +-
>  drivers/rtc/rtc-sh.c                                              | 2 +-

These rtc drivers are fixed in 20231002080529.2535610-1-u.kleine-koenig@pengutronix.de

>  drivers/video/fbdev/atmel_lcdfb.c                                 | 2 +-
>  drivers/video/fbdev/omap2/omapfb/displays/connector-analog-tv.c   | 2 +-
>  drivers/video/fbdev/omap2/omapfb/displays/connector-dvi.c         | 2 +-
>  drivers/video/fbdev/omap2/omapfb/displays/connector-hdmi.c        | 2 +-
>  drivers/video/fbdev/omap2/omapfb/displays/encoder-opa362.c        | 2 +-
>  drivers/video/fbdev/omap2/omapfb/displays/encoder-tfp410.c        | 2 +-
>  drivers/video/fbdev/omap2/omapfb/displays/encoder-tpd12s015.c     | 2 +-
>  drivers/video/fbdev/omap2/omapfb/displays/panel-dpi.c             | 2 +-
>  drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c          | 2 +-
>  .../video/fbdev/omap2/omapfb/displays/panel-sharp-ls037v7dw01.c   | 2 +-
>  drivers/virt/coco/sev-guest/sev-guest.c                           | 2 +-
>  drivers/watchdog/at91sam9_wdt.c                                   | 2 +-
>  sound/soc/codecs/tlv320adc3xxx.c                                  | 4 ++--

I sent 20231004111624.2667753-1-u.kleine-koenig@pengutronix.de to fix
a/drivers/platform/x86/hp/hp-wmi.c, that's already applied so doesn't
occur in your patch. The others I didn't address yet.

I assume your question means you want to push that forward. That is very
welcome, the I remove this from my todo list.

Best regards
Uwe
Masahiro Yamada Oct. 17, 2023, 10:20 a.m. UTC | #10
On Mon, Oct 16, 2023 at 7:21 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Mon, Oct 16, 2023 at 11:25:44AM +0200, Arnd Bergmann wrote:
> > On Mon, Oct 9, 2023, at 12:30, Uwe Kleine-König wrote:
> > > On Mon, Oct 09, 2023 at 10:43:46AM +0200, Arnd Bergmann wrote:
> > >
> > > 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.
> >
> > Hi Uwe,
> >
> > Based on a few days of randconfig build testing, the patch
> > below addresses the remaining warnings I get for arm, arm64 and
> > x86 on linux-next. This is a shorter list than the ones that
> > I found in theory, possibly because some of the other ones
> > are only used in built-in code, or because they are never used
> > on these three architectures.
> >
> > Have you already sent patches for (some of) these?
> >
> >       Arnd
> >
> >  drivers/char/hw_random/mxc-rnga.c                                 | 2 +-
> >  drivers/gpu/drm/bridge/ti-tpd12s015.c                             | 4 ++--
> >  drivers/hwmon/smsc47m1.c                                          | 2 +-
> >  drivers/hwtracing/coresight/coresight-etm4x-core.c                | 8 ++++----
> >  drivers/media/i2c/et8ek8/et8ek8_driver.c                          | 4 ++--
> >  drivers/memory/emif.c                                             | 2 +-
> >  drivers/mmc/host/davinci_mmc.c                                    | 2 +-
> >  drivers/mtd/nand/raw/txx9ndfmc.c                                  | 2 +-
>
> The txx9ndfmc driver was fixed as part of this series, but Miquèl didn't
> take the patch, I guess because he wants it to be converted to
> module_platform_driver().


So, there are two ways for fixing, and it is
up to subsystem maintainers?




A question is, is module_platform_driver_probe()
still worth supporting?
Uwe Kleine-König Oct. 17, 2023, 1:20 p.m. UTC | #11
Hello,

On Tue, Oct 17, 2023 at 07:20:19PM +0900, Masahiro Yamada wrote:
> On Mon, Oct 16, 2023 at 7:21 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Mon, Oct 16, 2023 at 11:25:44AM +0200, Arnd Bergmann wrote:
> > > Based on a few days of randconfig build testing, the patch
> > > below addresses the remaining warnings I get for arm, arm64 and
> > > x86 on linux-next. This is a shorter list than the ones that
> > > I found in theory, possibly because some of the other ones
> > > are only used in built-in code, or because they are never used
> > > on these three architectures.
> > >
> > > Have you already sent patches for (some of) these?
> > >
> > >       Arnd
> > >
> > >  drivers/char/hw_random/mxc-rnga.c                                 | 2 +-
> > >  drivers/gpu/drm/bridge/ti-tpd12s015.c                             | 4 ++--
> > >  drivers/hwmon/smsc47m1.c                                          | 2 +-
> > >  drivers/hwtracing/coresight/coresight-etm4x-core.c                | 8 ++++----
> > >  drivers/media/i2c/et8ek8/et8ek8_driver.c                          | 4 ++--
> > >  drivers/memory/emif.c                                             | 2 +-
> > >  drivers/mmc/host/davinci_mmc.c                                    | 2 +-
> > >  drivers/mtd/nand/raw/txx9ndfmc.c                                  | 2 +-
> >
> > The txx9ndfmc driver was fixed as part of this series, but Miquèl didn't
> > take the patch, I guess because he wants it to be converted to
> > module_platform_driver().
> 
> 
> So, there are two ways for fixing, and it is
> up to subsystem maintainers?

Yes, either you use module_platform_driver_probe() and benefit from
.probe in __init and .remove in __exit. Or you use
module_platform_driver() and benefit from more flexible bind/unbind
support (probing devices that appear only after boot, hotplugging,
binding/unbinding via sysfs)

> A question is, is module_platform_driver_probe()
> still worth supporting?

If you ask me, module_platform_driver_probe is a thing from the past and
hardly relevant any more.

The effect of converting drivers/mtd/nand/raw/txx9ndfmc.c (on ARCH=arm
allmodconfig) is:

add/remove: 0/0 grow/shrink: 1/2 up/down: 12/-16 (-4)
Function                                     old     new   delta
txx9ndfmc_remove                             228     240     +12
txx9ndfmc_driver_init                         48      40      -8
init_module                                   48      40      -8
Total: Before=5496, After=5492, chg -0.07%

I didn't try to understand why the remove callback got bigger.

The section sizes were changed as follows:

	.text:			0xe94 -> 0xf84	( +0xf0)
	.ARM.exidx:		 0x58 ->  0x60  (  +0x8)
	.init.text:		0x444 -> 0x43c  (  -0x8)
	.exit.text		 0xfc ->  0x18  ( -0xe4)
	.ARM.exidx.exit.text:	0x10 ->    0x8  (  -0x8)

So we're talking about less than 250 bytes that cannot be discarded any
more with a builtin-driver after boot.

Still I'd expect some resistance if we deprecate it and work on removing
it.

OTOH:

$ git grep -l module_platform_driver_probe next/master | wc -l
74

with 19 in drivers/rtc (I added Alexandre to Cc:), 4 in drivers/mtd, 6
in drivers/usb and otherwise here and there a driver. That doesn't look
insurmountable.

Best regards
Uwe
Greg Kroah-Hartman Oct. 17, 2023, 1:45 p.m. UTC | #12
On Tue, Oct 17, 2023 at 03:20:45PM +0200, Uwe Kleine-König wrote:
> with 19 in drivers/rtc (I added Alexandre to Cc:), 4 in drivers/mtd, 6
> in drivers/usb and otherwise here and there a driver. That doesn't look
> insurmountable.

I'll gladly take patches to change this for drivers/usb/ stuff.

thanks,

greg k-h
diff mbox series

Patch

diff --git a/drivers/mtd/nand/raw/txx9ndfmc.c b/drivers/mtd/nand/raw/txx9ndfmc.c
index eddcc0728a67..aff9d6f16851 100644
--- a/drivers/mtd/nand/raw/txx9ndfmc.c
+++ b/drivers/mtd/nand/raw/txx9ndfmc.c
@@ -406,7 +406,13 @@  static int txx9ndfmc_resume(struct platform_device *dev)
 #define txx9ndfmc_resume NULL
 #endif
 
-static struct platform_driver txx9ndfmc_driver = {
+/*
+ * txx9ndfmc_remove() lives in .exit.text. For drivers registered via
+ * module_platform_driver_probe() this is ok because they cannot get unbound at
+ * runtime. So mark the driver struct with __refdata to prevent modpost
+ * triggering a section mismatch warning.
+ */
+static struct platform_driver txx9ndfmc_driver __refdata = {
 	.remove		= __exit_p(txx9ndfmc_remove),
 	.resume		= txx9ndfmc_resume,
 	.driver		= {