Re: [PATCH] linux/pci.h: add a dummy implement for pci_clear_master()

[Geert Uytterhoeven]

Hi Sui,

On Wed, May 31, 2023 at 7:46 PM Sui Jingfeng <suijingfeng@xxxxxxxxxxx> wrote:
> On 2023/6/1 01:16, Bjorn Helgaas wrote:
> > On Wed, May 31, 2023 at 12:25:10PM +0800, Sui Jingfeng wrote:
> >> On 2023/5/31 04:11, Bjorn Helgaas wrote:
> >>> On Tue, May 30, 2023 at 06:16:55PM +0800, Sui Jingfeng wrote:
> >>>> As some arch(m68k for example) doesn't have config_pci enabled, drivers[1]
> >>>> call pci_clear_master() without config_pci guard can not built.
> >>>>
> >>>>      drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c:
> >>>>      In function 'etnaviv_gpu_pci_fini':
> >>>>>> drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c:32:9:
> >>>>      error: implicit declaration of function 'pci_clear_master';
> >>>>      did you mean 'pci_set_master'? [-Werror=implicit-function-declaration]
> >>>>         32 |         pci_clear_master(pdev);
> >>>>            |         ^~~~~~~~~~~~~~~~
> >>>>            |         pci_set_master
> >>>>      cc1: some warnings being treated as errors
> >>>>
> >>>> [1] https://patchwork.freedesktop.org/patch/539977/?series=118522&rev=1
> >>> I don't mind adding a stub if it's needed, but I don't understand why
> >>> it's needed here.
> >> For a single driver that supports both platform devices and PCI devices,
> >>
> >> Sometimes there is no way to separate the PCI driver part and the platform
> >> driver part cleanly and clearly.
> >>
> >> For example, the module_init() and module_exit() functions,
> >>
> >> where we have to register PCI drivers and platform drivers there.
> >>
> >> We can't simply let the entire driver depend on PCI in Kconfig,
> >>
> >> This will make this driver unable to compile, which it's originally could.
> >>
> >> The PCI core could do such a thing for us, and
> >>
> >> There is no need to introduce a driver-specific guard then.
> >>
> >>
> >> There is already a dummy stub for pci_set_master().
> >>
> >> Therefore, pci_clear_master() should also have a counterpart.
> >>
> >> They should emerge in pairs.
> >>
> >> This could probably eliminate pain for PCI driver writers,
> >>
> >> This patch is still useful.
> >>
> >>
> >>>    The caller is in etnaviv_pci_drv.c, and if I
> >>> understand the patch at [1], etnaviv_pci_drv.c is only compiled when
> >>> CONFIG_PCI=y.
> >> Yes, you are right. This is the right thing to do for the driver, though.
> >>
> >> Pure PCI device driver does not need to worry about this.
> >>
> >> Like drm/ast, drm/amdgpu, drm/radeon, etc.
> >>
> >> But drm/etnaviv is special; it's a platform driver that could pass the
> >> compile test originally.
> >>
> >>
> >> When patching it (Etnaviv) with PCI device driver support,
> >>
> >> This forces the PCI driver writer to add another config option.
> >>
> >> (which depends on the PCI config option.) in the Kconfig.
> >>
> >> For my case, it's theDRM_ETNAVIV_PCI_DRIVER config option.
> > So if I understand correctly, you would prefer not to add the
> > DRM_ETNAVIV_PCI_DRIVER config option, and if we add this stub, you
> > won't need to add it?
> >
> > That's a good reason to add this patch.
>
> Yes, please add this patch.
>
> Otherwise, other people may suffer from the same issue someday.

People already have, several years ago, cfr.
https://lore.kernel.org/all/20160309003955.GA1589@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds