On Wed, Jun 27, 2018 at 6:19 PM, Lukas Wunner <lukas@xxxxxxxxx> wrote: > Hi Hari, > > please cc the list when replying so that others can comment. > apologize. My mistake. > On Wed, Jun 27, 2018 at 03:45:26PM +0530, Hari Vyas wrote: >> On Tue, Jun 26, 2018 at 5:23 PM, Lukas Wunner <lukas@xxxxxxxxx> wrote: >> > While a spinlock may do the job, I think a better solution would be >> > to move is_added to the priv_flags bitmap in struct pci_dev. The >> > is_added flag is internal to the PCI core and anything outside has >> > no business dealing with it. >> > >> > (Assuming arch/powerpc/kernel/pci-common.c can also be considered >> > part of the PCI core.) >> >> Simple grep is showing that >> is_added is being used outside in 1) arch/powerpc/platforms/powernv/ >> pci-ioda.c 2) arch/powerpc/platforms/pseries/setup.c files. > > I'd #include "../../../../drivers/pci/pci.h" in the few arch-specific > files, there's some precedent for that, see > > git grep '#include .*\.\./\.\./\.\./\.\.' -- :/arch > > (I'd mention that precedent in the commit message or below the three > dashes, in the latter case it doesn't become part of the commit message > but it helps reviewers understand what's going on.) > >>> > The flags in priv_flags are defined in drivers/pci/pci.h, so far >> > there's only one for PCI_DEV_DISCONNECTED which was introduced by >> > 89ee9f768. That commit also introduced accessors, personally I >> > don't think that's necessary for the few places in the PCI core >> > that the new PCI_DEV_ADDED flag would be used and I'd just update >> > those sites to set or test the bit directly. >> > >> Agree. Issue needs to be just fixed in good way. Let me know if priv_flags >> approach needs to be taken for is_added flag. > > I'd definitely recommend using priv_flags and the atomic bitops > test_bit() and set_bit() (see Documentation/atomic_bitops.txt), > it's less code than using a spinlock and nominally reduces the > size of struct pci_dev by 1 bit (modulo padding by the compiler). > spinlock is for when you have several function calls which need > to be executed atomically. > Okay. Will update code (move is_added:1 to priv_flags), test and revert back. Should fix concern but better to verify. > > Thanks, > > Lukas
