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

[Sui Jingfeng]

Hi,

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.


This has side effects, but they are not severe.

It boils down to the compilation time thing, while originally we want it 
to be a runtime thing.

Driver writers have to isolate PCI driver-related subroutines in a 
separate source file.

with the DRM_ETNAVIV_PCI_DRIVER option, guard callers of those subroutines,

to let them not get compiled when CONFIG_PCIis disabled.


> Bjorn
>
> [1] https://lore.kernel.org/all/20230530160643.2344551-6-suijingfeng@xxxxxxxxxxx/
>
> > Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202305301659.4guSLavL-lkp@xxxxxxxxx/
> > Signed-off-by: Sui Jingfeng <suijingfeng@xxxxxxxxxxx>
> > ---
> >   include/linux/pci.h | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index d0c19ff0c958..71c85380676c 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -1904,6 +1904,7 @@ static inline int pci_dev_present(const struct pci_device_id *ids)
> >   #define pci_dev_put(dev)	do { } while (0)
> >   
> >   static inline void pci_set_master(struct pci_dev *dev) { }
> > +static inline void pci_clear_master(struct pci_dev *dev) { }
> >   static inline int pci_enable_device(struct pci_dev *dev) { return -EIO; }
> >   static inline void pci_disable_device(struct pci_dev *dev) { }
> >   static inline int pcim_enable_device(struct pci_dev *pdev) { return -EIO; }
> > --
> > 2.25.1
--
Jingfeng