Re: [PATCH] irqchip/qcom-irq-combiner: Rename driver struct to end in _driver

Re: [PATCH] irqchip/qcom-irq-combiner: Rename driver struct to end in _driver

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3610 bytes --]

Hello,

[adding the people involved with scripts/mod to Cc:]

On Mon, Jun 30, 2025 at 08:01:53PM +0200, Thomas Gleixner wrote:
> On Mon, Jun 30 2025 at 19:23, Uwe Kleine-König wrote:
> > The modpost section mismatch checks are more lax for objects that have a
> > name that ends in "_probe". This is not justified here though, so rename
> 
> That's a truly bad design or lack of such.
> 
> Why can't this muck use foo_driver(name) foo_probe(name) annotations to
> make it entirely clear what is tested for instead of oracling it out of
> the name itself. That would make it too easy to understand and analyse.

I don't understand what you're suggesting here. Either I got it wrong or
it is insufficient because every object is checked, not only the driver
structs. That would result in more exceptions/special cases than we have
now.

Anyhow, I agree that depending on the name is unfortunate, maybe we can
come up with something more clever?

There are a few more candidates:

$ git grep -E '_driver\>\s*[a-z_0-9]*_(ops|probe|console)\>' next/master
next/master:Documentation/driver-api/usb/typec_bus.rst:   :functions: typec_altmode_driver typec_altmode_ops
next/master:drivers/char/virtio_console.c:static struct virtio_driver virtio_console = {
next/master:drivers/clocksource/timer-nxp-stm.c:static struct platform_driver nxp_stm_probe = {
next/master:drivers/irqchip/qcom-irq-combiner.c:static struct platform_driver qcom_irq_combiner_probe = {
next/master:drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c:static struct pci_driver aq_pci_ops = {
next/master:drivers/net/ethernet/dec/tulip/xircom_cb.c:static struct pci_driver xircom_ops = {
next/master:drivers/video/fbdev/omap2/omapfb/displays/panel-dpi.c:static struct omap_dss_driver panel_dpi_ops = {
next/master:drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c:static struct omap_dss_driver dsicm_ops = {
next/master:drivers/video/fbdev/omap2/omapfb/displays/panel-lgphilips-lb035q02.c:static struct omap_dss_driver lb035q02_ops = {
next/master:drivers/video/fbdev/omap2/omapfb/displays/panel-nec-nl8048hl11.c:static struct omap_dss_driver nec_8048_ops = {
next/master:drivers/video/fbdev/omap2/omapfb/displays/panel-sharp-ls037v7dw01.c:static struct omap_dss_driver sharp_ls_ops = {
next/master:drivers/video/fbdev/omap2/omapfb/displays/panel-sony-acx565akm.c:static struct omap_dss_driver acx565akm_ops = {
next/master:drivers/video/fbdev/omap2/omapfb/displays/panel-tpo-td028ttec1.c:static struct omap_dss_driver td028ttec1_ops = {
next/master:drivers/video/fbdev/omap2/omapfb/displays/panel-tpo-td043mtea1.c:static struct omap_dss_driver tpo_td043_ops = {

I already sent patches for the two networking drivers, we're discussing
the irqchip driver here and the omap2 framebuffer drivers are IMHO false
positives. I didn't look into the virtio_console driver, but I guess
that one is ok, too.

The clocksource driver is a bit more difficult, .probe() must be in
__init because it calls sched_clock_register() but using
platform_driver_probe() might not be easy. Markus currently fights with
a similar clocksource driver where the clocksource depends on an mbox
driver that is only probed later. In his case probing the schedclock
returns -EPROBE_DEFER but when the mbox driver is ready the device
cannot be reprobed again as the schedclock driver is already gone. Of
course the error message for -EPROBE_DEFER is suppressed, because that's
what you do for this type of message. So the machine dies with no
explaining output. We seem to have a yak to shave.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] irqchip/qcom-irq-combiner: Rename driver struct to end in _driver

[Thomas Gleixner]

On Mon, Jun 30 2025 at 21:40, Uwe Kleine-König wrote:
> On Mon, Jun 30, 2025 at 08:01:53PM +0200, Thomas Gleixner wrote:
>> On Mon, Jun 30 2025 at 19:23, Uwe Kleine-König wrote:
>> > The modpost section mismatch checks are more lax for objects that have a
>> > name that ends in "_probe". This is not justified here though, so rename
>> 
>> That's a truly bad design or lack of such.
>> 
>> Why can't this muck use foo_driver(name) foo_probe(name) annotations to
>> make it entirely clear what is tested for instead of oracling it out of
>> the name itself. That would make it too easy to understand and analyse.
>
> I don't understand what you're suggesting here. Either I got it wrong or
> it is insufficient because every object is checked, not only the driver
> structs. That would result in more exceptions/special cases than we have
> now.
>
> Anyhow, I agree that depending on the name is unfortunate, maybe we can
> come up with something more clever?

That's what I was referring to. Doing checks based on struct names is a
bad idea. Having distinct '...driver_probe(name)' and ...driver(name)'
macros to distinguish the functionality is the proper thing to do and
way simpler to analyse than names.

Re: [PATCH] irqchip/qcom-irq-combiner: Rename driver struct to end in _driver

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2071 bytes --]

On Tue, Jul 01, 2025 at 11:01:18AM +0200, Thomas Gleixner wrote:
> On Mon, Jun 30 2025 at 21:40, Uwe Kleine-König wrote:
> > On Mon, Jun 30, 2025 at 08:01:53PM +0200, Thomas Gleixner wrote:
> >> On Mon, Jun 30 2025 at 19:23, Uwe Kleine-König wrote:
> >> > The modpost section mismatch checks are more lax for objects that have a
> >> > name that ends in "_probe". This is not justified here though, so rename
> >> 
> >> That's a truly bad design or lack of such.
> >> 
> >> Why can't this muck use foo_driver(name) foo_probe(name) annotations to
> >> make it entirely clear what is tested for instead of oracling it out of
> >> the name itself. That would make it too easy to understand and analyse.
> >
> > I don't understand what you're suggesting here. Either I got it wrong or
> > it is insufficient because every object is checked, not only the driver
> > structs. That would result in more exceptions/special cases than we have
> > now.
> >
> > Anyhow, I agree that depending on the name is unfortunate, maybe we can
> > come up with something more clever?
> 
> That's what I was referring to. Doing checks based on struct names is a
> bad idea. Having distinct '...driver_probe(name)' and ...driver(name)'
> macros to distinguish the functionality is the proper thing to do and
> way simpler to analyse than names.

A driver struct should have no reference to .init.text (i.e. no callback
to a function marked with __init) no matter if it is registered using
module_platform_driver_probe() or module_platform_driver(). But even if
the requirements for those were different, how do you signal in the
binary if the driver was registered using the normal (i.e
platform_driver_register()) or the platform_driver_probe() way? Or do
you want to check the source file?

And note that if you have that, you covered only platform drivers, with
a bit of luck mostly all drivers (note that console drivers are special
and are allowed to have an __init callback). modpost checks all objects,
not only driver structs.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] irqchip/qcom-irq-combiner: Rename driver struct to end in _driver

[Thomas Gleixner]

On Tue, Jul 01 2025 at 15:58, Uwe Kleine-König wrote:
> On Tue, Jul 01, 2025 at 11:01:18AM +0200, Thomas Gleixner wrote:
>> > Anyhow, I agree that depending on the name is unfortunate, maybe we can
>> > come up with something more clever?
>> 
>> That's what I was referring to. Doing checks based on struct names is a
>> bad idea. Having distinct '...driver_probe(name)' and ...driver(name)'
>> macros to distinguish the functionality is the proper thing to do and
>> way simpler to analyse than names.
>
> A driver struct should have no reference to .init.text (i.e. no callback
> to a function marked with __init) no matter if it is registered using
> module_platform_driver_probe() or module_platform_driver().

I came from this wording in your change log:

 "The modpost section mismatch checks are more lax for objects that have a
  name that ends in "_probe". This is not justified here...."

That's the underlying problem. Using object names for deciding which
check rule to use is error prone and wrong to begin with. That what I
was referring to and there are obviously different rules for these
section checks, otherwise with your rename there would be no
difference, no?

I'm not talking about what this driver should do or not in it's
callbacks, that's a different problem, independent of this discussion.

I'm just horrified by the idea that such issues go unnoticed
because of a object name ending in _foo.

> But even if the requirements for those were different, how do you
> signal in the binary if the driver was registered using the normal
> (i.e platform_driver_register()) or the platform_driver_probe() way?
> Or do you want to check the source file?

No. We have a lot of magic already which puts annotations into sections
so that tools of all sorts can extract them for post processsing and not
make magic assumptions about names or their endings.

It's clearly simpler to do analyis based on:

     magic_driver_allowed_to_do_stupid_stuff($name);

than on

     default_driver_whatever($name);

both for tools when the magic macros emits section data and for humans
to grep, no?

Thanks,

        tglx