| Message ID | 1511328675-21981-31-git-send-email-okaya@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On 11/21/17 11:31 PM, Sinan Kaya wrote: > Use pci_get_domain_bus_and_slot() with a domain number of 0 where we can't > extract the domain number. Other places, use the actual domain number from > the device. > > Now that all users of pci_get_bus_and_slot() switched to > pci_get_domain_bus_and_slot(), it is now safe to remove this function. This doesn't really eliminate pci_get_bus_and_slot(), because it doesn't force developers to support non-zero domains. What's to stop a driver developer from doing this? #define pci_get_bus_and_slot(b, d) pci_get_domain_bus_and_slot(0, b, d) thereby completely ignoring what you're trying to do?
On 11/22/2017 12:45 AM, Timur Tabi wrote: > On 11/21/17 11:31 PM, Sinan Kaya wrote: >> Use pci_get_domain_bus_and_slot() with a domain number of 0 where we can't >> extract the domain number. Other places, use the actual domain number from >> the device. >> >> Now that all users of pci_get_bus_and_slot() switched to >> pci_get_domain_bus_and_slot(), it is now safe to remove this function. > > This doesn't really eliminate pci_get_bus_and_slot(), because it doesn't force developers to support non-zero domains. What's to stop a driver developer from doing this? > > #define pci_get_bus_and_slot(b, d) pci_get_domain_bus_and_slot(0, b, d) > > thereby completely ignoring what you're trying to do? > Surely, the goal is not to eliminate all domain 0 users/assumptions but open the path for flexibility over time. There are patches in this series where I hard-coded a value of 0 because domain information was not available. For places where domain number information is available, I extracted domain number and added into pci_get_domain_bus_and_slot() call such as xen or bn drivers. This will allow these drivers to be used with non-zero segment numbers. These issues were missed until this refactoring took place. pci_get_domain_bus_and_slot() function makes the developer think about where to find the domain number as it is mandatory. I also double checked that all current users of pci_get_domain_bus_and_slot() are actually extracting the domain number correctly along with the bus, device, function. The assumption at this point is for pci_get_bus_and_slot() usages to be caught in code-review. This is a best-effort approach towards flexibility.
On 11/21/17 11:55 PM, Sinan Kaya wrote: > For places where domain number information is available, I extracted domain number > and added into pci_get_domain_bus_and_slot() call such as xen or bn drivers. My suggestion is that you restrict your first patch set to only these patches. > The assumption at this point is for pci_get_bus_and_slot() usages to be caught > in code-review. How about this: static inline struct pci_dev * __deprecated pci_get_bus_and_slot(unsigned int bus, unsigned int devfn) { return pci_get_domain_bus_and_slot(0, bus, devfn); }
On Wed, Nov 22, 2017 at 12:08:45AM -0600, Timur Tabi wrote: > On 11/21/17 11:55 PM, Sinan Kaya wrote: > > For places where domain number information is available, I extracted domain number > > and added into pci_get_domain_bus_and_slot() call such as xen or bn drivers. > > My suggestion is that you restrict your first patch set to only these > patches. > > > The assumption at this point is for pci_get_bus_and_slot() usages to be caught > > in code-review. > > How about this: > > static inline struct pci_dev * __deprecated pci_get_bus_and_slot(unsigned > int bus, > unsigned int devfn) > { > return pci_get_domain_bus_and_slot(0, bus, devfn); > } Ick, no, why? What is wrong with removing this function as is? Don't mark something as __depreciated if there are no in-kernel users, just delete it and move on. If you have out-of-tree drivers, then yes, they can make a wrapper for this function like this if they really feel the need, or they can get their code merged :) thanks, greg k-h
On 11/22/17 1:51 AM, Greg KH wrote: > Ick, no, why? What is wrong with removing this function as is? Don't > mark something as __depreciated if there are no in-kernel users, just > delete it and move on. > > If you have out-of-tree drivers, then yes, they can make a wrapper for > this function like this if they really feel the need, or they can get > their code merged:) Sorry, I guess I should have been clearer. My suggestion was to fix some of the drivers where the domain can be determined, and for the rest, just mark the old function as deprecated. If that's still a terrible idea, well, okay. I'm just unsure that simply hard-coding a 0 for the domain for some drivers is really a solution. Don't we really want all drivers to properly support all domains?
On Wed, Nov 22, 2017 at 08:42:35AM -0600, Timur Tabi wrote: > On 11/22/17 1:51 AM, Greg KH wrote: > > Ick, no, why? What is wrong with removing this function as is? Don't > > mark something as __depreciated if there are no in-kernel users, just > > delete it and move on. > > > > If you have out-of-tree drivers, then yes, they can make a wrapper for > > this function like this if they really feel the need, or they can get > > their code merged:) > > Sorry, I guess I should have been clearer. My suggestion was to fix some of > the drivers where the domain can be determined, and for the rest, just mark > the old function as deprecated. So the build now gets warnings? That's annoying, and then someone else will have to make the exact same patches that were created here? > If that's still a terrible idea, well, okay. I'm just unsure that simply > hard-coding a 0 for the domain for some drivers is really a solution. Don't > we really want all drivers to properly support all domains? I bet all of those drivers don't care because they are running only in systems with 1 domain, otherwise they would be broken today, right? But really, it shouldn't be that hard to get to the "real" PCI device to provide the correct pointer to the domain for most of these, as I pointed out in one patch review already. thanks, greg k-h
On 11/22/2017 9:49 AM, Greg KH wrote: >>> If you have out-of-tree drivers, then yes, they can make a wrapper for >>> this function like this if they really feel the need, or they can get >>> their code merged:) >> Sorry, I guess I should have been clearer. My suggestion was to fix some of >> the drivers where the domain can be determined, and for the rest, just mark >> the old function as deprecated. > So the build now gets warnings? That's annoying, and then someone else > will have to make the exact same patches that were created here? > >> If that's still a terrible idea, well, okay. I'm just unsure that simply >> hard-coding a 0 for the domain for some drivers is really a solution. Don't >> we really want all drivers to properly support all domains? > I bet all of those drivers don't care because they are running only in > systems with 1 domain, otherwise they would be broken today, right? But > really, it shouldn't be that hard to get to the "real" PCI device to > provide the correct pointer to the domain for most of these, as I > pointed out in one patch review already. I agree. Point is 95% drivers do support multiple segments. These are the exceptions. We should try to fix them as much as we can. (I'll take a look at Greg's suggestion) In the end, this API is a backdoor and workarounds the proper APIs and usual practices like carrying struct pci_dev pointer. I'm trying to shoot that API in the foot so that a developer thinks twice before putting number 0 there.
diff --git a/include/linux/pci.h b/include/linux/pci.h index d16a7c0..8c1b650 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -957,11 +957,6 @@ struct pci_dev *pci_get_subsys(unsigned int vendor, unsigned int device, struct pci_dev *pci_get_slot(struct pci_bus *bus, unsigned int devfn); struct pci_dev *pci_get_domain_bus_and_slot(int domain, unsigned int bus, unsigned int devfn); -static inline struct pci_dev *pci_get_bus_and_slot(unsigned int bus, - unsigned int devfn) -{ - return pci_get_domain_bus_and_slot(0, bus, devfn); -} struct pci_dev *pci_get_class(unsigned int class, struct pci_dev *from); int pci_dev_present(const struct pci_device_id *ids); @@ -1676,9 +1671,6 @@ static inline struct pci_bus *pci_find_next_bus(const struct pci_bus *from) static inline struct pci_dev *pci_get_slot(struct pci_bus *bus, unsigned int devfn) { return NULL; } -static inline struct pci_dev *pci_get_bus_and_slot(unsigned int bus, - unsigned int devfn) -{ return NULL; } static inline int pci_domain_nr(struct pci_bus *bus) { return 0; } static inline struct pci_dev *pci_dev_get(struct pci_dev *dev) { return NULL; }
pci_get_bus_and_slot() is restrictive such that it assumes domain=0 as where a PCI device is present. This restricts the device drivers to be reused for other domain numbers. Use pci_get_domain_bus_and_slot() with a domain number of 0 where we can't extract the domain number. Other places, use the actual domain number from the device. Now that all users of pci_get_bus_and_slot() switched to pci_get_domain_bus_and_slot(), it is now safe to remove this function. Signed-off-by: Sinan Kaya <okaya@codeaurora.org> --- include/linux/pci.h | 8 -------- 1 file changed, 8 deletions(-)