Re: [RFT PATCH] PCI: provide helper to reset all domain for host drivers

[Brian Norris <briannorris@chromium.org>]

Hi Shawn,

I might be mistaken here, but I don't think this is wise:

On Fri, Mar 10, 2017 at 03:58:26PM +0800, Shawn Lin wrote:
> Otherwise the domain_nr will be increased if doing ubind and
> bind host drives. I don't find any useful routine to reset it
> from current code, so add this helper. This patch is based on patchset[1]
> from Brian.
> 
> [1]: https://patchwork.kernel.org/patch/9614517/
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> 
> ---
> Hi Brian,
> 
> I wasn't able to run my platfrom with linux-next but I test this for
> my local kernel-v4.4, and I just send this for test!
> 
>  drivers/pci/host/pcie-rockchip.c | 1 +
>  drivers/pci/pci.c                | 6 ++++++
>  include/linux/pci.h              | 3 +++
>  3 files changed, 10 insertions(+)
> 
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index bd6df72..61787e7 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -1435,6 +1435,7 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
>  	pci_remove_root_bus(rockchip->root_bus);
>  	pci_unmap_iospace(rockchip->io);
>  	irq_domain_remove(rockchip->irq_domain);
> +	pci_reset_all_domain();

You don't want to reset *all* domains here; what if some other driver
created another domain? You're essentially cloberring it here.

>  	phy_power_off(rockchip->phy);
>  	phy_exit(rockchip->phy);
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 3ec24877..7c7c63b 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5181,6 +5181,12 @@ int pci_get_new_domain_nr(void)
>  	return atomic_inc_return(&__domain_nr);
>  }
>  
> +void pci_reset_all_domain(void)
> +{
> +	atomic_set(&__domain_nr, -1);

IIUC, instead of completely resetting here, it seems like we'd need to
rework the entire domain allocation code, so that it can properly
hotplug domains, and track the potential "holes" in indexing that might
arise.

Brian

> +}
> +EXPORT_SYMBOL(pci_reset_all_domain);
> +
>  #ifdef CONFIG_PCI_DOMAINS_GENERIC
>  static int of_pci_bus_find_domain_nr(struct device *parent)
>  {
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index eb3da1a..c9bd939 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1456,11 +1456,13 @@ static inline int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
>  #ifdef CONFIG_PCI_DOMAINS
>  extern int pci_domains_supported;
>  int pci_get_new_domain_nr(void);
> +void pci_reset_all_domain(void);
>  #else
>  enum { pci_domains_supported = 0 };
>  static inline int pci_domain_nr(struct pci_bus *bus) { return 0; }
>  static inline int pci_proc_domain(struct pci_bus *bus) { return 0; }
>  static inline int pci_get_new_domain_nr(void) { return -ENOSYS; }
> +static inline void pci_reset_all_domain(void) {}
>  #endif /* CONFIG_PCI_DOMAINS */
>  
>  /*
> @@ -1617,6 +1619,7 @@ static inline struct pci_dev *pci_get_bus_and_slot(unsigned int bus,
>  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; }
>  static inline int pci_get_new_domain_nr(void) { return -ENOSYS; }
> +static inline void pci_reset_all_domain(void) {};
>  
>  #define dev_is_pci(d) (false)
>  #define dev_is_pf(d) (false)
> -- 
> 1.9.1
> 
>