Re: [PATCH RFC v4 12/21] PCI: Don't allow hotplugged devices to steal resources

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 3/26/19 11:55 PM, Bjorn Helgaas wrote:
> On Mon, Mar 11, 2019 at 04:31:13PM +0300, Sergey Miroshnichenko wrote:
>> When movable BARs are enabled, the PCI subsystem at first releases
>> all the bridge windows and then performs an attempt to assign new
>> requested resources and re-assign the existing ones.
> 
> s/performs an attempt/attempts/
> 
> I guess "new requested resources" means "resources to newly hotplugged
> devices"?
> 

Yes, that's exactly what I've tried to express :) Will rephrase that in v5.

>> If a hotplugged device gets its resources first, there could be no
>> space left to re-assign resources of already working devices, which
>> is unacceptable. If this happens, this patch marks one of the new
>> devices with the new introduced flag PCI_DEV_IGNORE and retries the
>> resource assignment.
>>
>> This patch adds a new res_mask bitmask to the struct pci_dev for
>> storing the indices of assigned resources.
>>
>> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@xxxxxxxxx>
>> ---
>>  drivers/pci/bus.c       |   5 ++
>>  drivers/pci/pci.h       |  11 +++++
>>  drivers/pci/probe.c     | 100 +++++++++++++++++++++++++++++++++++++++-
>>  drivers/pci/setup-bus.c |  15 ++++++
>>  include/linux/pci.h     |   1 +
>>  5 files changed, 130 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
>> index 5cb40b2518f9..a9784144d6f2 100644
>> --- a/drivers/pci/bus.c
>> +++ b/drivers/pci/bus.c
>> @@ -311,6 +311,11 @@ void pci_bus_add_device(struct pci_dev *dev)
>>  {
>>  	int retval;
>>  
>> +	if (pci_dev_is_ignored(dev)) {
>> +		pci_warn(dev, "%s: don't enable the ignored device\n", __func__);
>> +		return;
> 
> I'm not sure about this.  Even if we're unable to assign space for all
> the device's BARs, it still should respond to config accesses, and I
> think it should show up in sysfs and lspci.
> 

I agree, that would be better.

Also, this patch introduces a new issue to think about: how to recover BARs for such
devices when their neighbors was removed and it's enough space now.

>> +	}
>> +
>>  	/*
>>  	 * Can not put in pci_device_add yet because resources
>>  	 * are not assigned yet for some devices.
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index e06e8692a7b1..56b905068ac5 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -366,6 +366,7 @@ static inline bool pci_dev_is_disconnected(const struct pci_dev *dev)
>>  
>>  /* pci_dev priv_flags */
>>  #define PCI_DEV_ADDED 0
>> +#define PCI_DEV_IGNORE 1
>>  
>>  static inline void pci_dev_assign_added(struct pci_dev *dev, bool added)
>>  {
>> @@ -377,6 +378,16 @@ static inline bool pci_dev_is_added(const struct pci_dev *dev)
>>  	return test_bit(PCI_DEV_ADDED, &dev->priv_flags);
>>  }
>>  
>> +static inline void pci_dev_ignore(struct pci_dev *dev, bool ignore)
>> +{
>> +	assign_bit(PCI_DEV_IGNORE, &dev->priv_flags, ignore);
>> +}
>> +
>> +static inline bool pci_dev_is_ignored(const struct pci_dev *dev)
>> +{
>> +	return test_bit(PCI_DEV_IGNORE, &dev->priv_flags);
>> +}
>> +
>>  #ifdef CONFIG_PCIEAER
>>  #include <linux/aer.h>
>>  
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index 692752c71f71..62f4058a001f 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -3248,6 +3248,23 @@ unsigned int pci_rescan_bus_bridge_resize(struct pci_dev *bridge)
>>  	return max;
>>  }
>>  
>> +static unsigned int pci_dev_res_mask(struct pci_dev *dev)
>> +{
>> +	unsigned int res_mask = 0;
>> +	int i;
>> +
>> +	for (i = 0; i < PCI_BRIDGE_RESOURCES; i++) {
>> +		struct resource *r = &dev->resource[i];
>> +
>> +		if (!r->flags || (r->flags & IORESOURCE_UNSET) || !r->parent)
>> +			continue;
>> +
>> +		res_mask |= (1 << i);
>> +	}
>> +
>> +	return res_mask;
>> +}
>> +
>>  static void pci_bus_rescan_prepare(struct pci_bus *bus)
>>  {
>>  	struct pci_dev *dev;
>> @@ -3257,6 +3274,8 @@ static void pci_bus_rescan_prepare(struct pci_bus *bus)
>>  	list_for_each_entry(dev, &bus->devices, bus_list) {
>>  		struct pci_bus *child = dev->subordinate;
>>  
>> +		dev->res_mask = pci_dev_res_mask(dev);
>> +
>>  		if (child) {
>>  			pci_bus_rescan_prepare(child);
>>  		} else if (dev->driver &&
>> @@ -3318,6 +3337,84 @@ static void pci_setup_bridges(struct pci_bus *bus)
>>  		pci_setup_bridge(bus);
>>  }
>>  
>> +static struct pci_dev *pci_find_next_new_device(struct pci_bus *bus)
>> +{
>> +	struct pci_dev *dev;
>> +
>> +	if (!bus)
>> +		return NULL;
>> +
>> +	list_for_each_entry(dev, &bus->devices, bus_list) {
>> +		struct pci_bus *child_bus = dev->subordinate;
>> +
>> +		if (!pci_dev_is_added(dev) && !pci_dev_is_ignored(dev))
>> +			return dev;
>> +
>> +		if (child_bus) {
>> +			struct pci_dev *next_new_dev;
>> +
>> +			next_new_dev = pci_find_next_new_device(child_bus);
>> +			if (next_new_dev)
>> +				return next_new_dev;
>> +		}
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +static bool pci_bus_validate_resources(struct pci_bus *bus)
> 
> The name of this function should tell us what the return value means.
> Just from the name "pci_bus_validate_resources", I can't tell whether we
> call it for side-effects, or whether true or false indicates success.
> 

Sure, now I realize this too. Would the pci_bus_check_all_bars_reassigned() be better choice?

>> +{
>> +	struct pci_dev *dev;
>> +	bool ret = true;
>> +
>> +	if (!bus)
>> +		return false;
>> +
>> +	list_for_each_entry(dev, &bus->devices, bus_list) {
>> +		struct pci_bus *child = dev->subordinate;
>> +		unsigned int res_mask = pci_dev_res_mask(dev);
>> +
>> +		if (pci_dev_is_ignored(dev))
>> +			continue;
>> +
>> +		if (dev->res_mask & ~res_mask) {
>> +			pci_err(dev, "%s: Non-re-enabled resources found: 0x%x -> 0x%x\n",
>> +				__func__, dev->res_mask, res_mask);
> 
> I don't think __func__ really tells users anything useful, so I would
> just omit them.  Searching for the text of the message is almost as
> good.
> 

Ok, I'll drop __func__'s.

Serge

>> +			ret = false;
>> +		}
>> +
>> +		if (child && !pci_bus_validate_resources(child))
>> +			ret = false;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static void pci_reassign_root_bus_resources(struct pci_bus *root)
>> +{
>> +	do {
>> +		struct pci_dev *next_new_dev;
>> +
>> +		pci_bus_release_root_bridge_resources(root);
>> +		pci_assign_unassigned_root_bus_resources(root);
>> +
>> +		if (pci_bus_validate_resources(root))
>> +			break;
>> +
>> +		next_new_dev = pci_find_next_new_device(root);
>> +		if (!next_new_dev) {
>> +			dev_err(&root->dev, "%s: failed to re-assign resources even after ignoring all the hotplugged devices\n",
>> +				__func__);
>> +			break;
>> +		}
>> +
>> +		dev_warn(&root->dev, "%s: failed to re-assign resources, disable the next hotplugged device %s and retry\n",
>> +			 __func__, dev_name(&next_new_dev->dev));
>> +
>> +		pci_dev_ignore(next_new_dev, true);
>> +	} while (true);
>> +}
>> +
>>  /**
>>   * pci_rescan_bus - Scan a PCI bus for devices
>>   * @bus: PCI bus to scan
>> @@ -3341,8 +3438,7 @@ unsigned int pci_rescan_bus(struct pci_bus *bus)
>>  
>>  		max = pci_scan_child_bus(root);
>>  
>> -		pci_bus_release_root_bridge_resources(root);
>> -		pci_assign_unassigned_root_bus_resources(root);
>> +		pci_reassign_root_bus_resources(root);
>>  
>>  		pci_setup_bridges(root);
>>  		pci_bus_rescan_done(root);
>> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
>> index 36a1907d9509..551108f48df7 100644
>> --- a/drivers/pci/setup-bus.c
>> +++ b/drivers/pci/setup-bus.c
>> @@ -131,6 +131,9 @@ static void pdev_sort_resources(struct pci_dev *dev, struct list_head *head)
>>  {
>>  	int i;
>>  
>> +	if (pci_dev_is_ignored(dev))
>> +		return;
>> +
>>  	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
>>  		struct resource *r;
>>  		struct pci_dev_resource *dev_res, *tmp;
>> @@ -181,6 +184,9 @@ static void __dev_sort_resources(struct pci_dev *dev,
>>  {
>>  	u16 class = dev->class >> 8;
>>  
>> +	if (pci_dev_is_ignored(dev))
>> +		return;
>> +
>>  	/* Don't touch classless devices or host bridges or ioapics.  */
>>  	if (class == PCI_CLASS_NOT_DEFINED || class == PCI_CLASS_BRIDGE_HOST)
>>  		return;
>> @@ -284,6 +290,9 @@ static void assign_requested_resources_sorted(struct list_head *head,
>>  	int idx;
>>  
>>  	list_for_each_entry(dev_res, head, list) {
>> +		if (pci_dev_is_ignored(dev_res->dev))
>> +			continue;
>> +
>>  		res = dev_res->res;
>>  		idx = res - &dev_res->dev->resource[0];
>>  		if (resource_size(res) &&
>> @@ -991,6 +1000,9 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
>>  	list_for_each_entry(dev, &bus->devices, bus_list) {
>>  		int i;
>>  
>> +		if (pci_dev_is_ignored(dev))
>> +			continue;
>> +
>>  		for (i = 0; i < PCI_NUM_RESOURCES; i++) {
>>  			struct resource *r = &dev->resource[i];
>>  			resource_size_t r_size;
>> @@ -1353,6 +1365,9 @@ void __pci_bus_assign_resources(const struct pci_bus *bus,
>>  	pbus_assign_resources_sorted(bus, realloc_head, fail_head);
>>  
>>  	list_for_each_entry(dev, &bus->devices, bus_list) {
>> +		if (pci_dev_is_ignored(dev))
>> +			continue;
>> +
>>  		pdev_assign_fixed_resources(dev);
>>  
>>  		b = dev->subordinate;
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 3d52f5538282..26aa59cb6220 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -369,6 +369,7 @@ struct pci_dev {
>>  	 */
>>  	unsigned int	irq;
>>  	struct resource resource[DEVICE_COUNT_RESOURCE]; /* I/O and memory regions + expansion ROMs */
>> +	unsigned int	res_mask;		/* Bitmask of assigned resources */
>>  
>>  	bool		match_driver;		/* Skip attaching driver */
>>  
>> -- 
>> 2.20.1
>>



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux