Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1488649 > unrolled thread

[PATCH v3 2/3] xen/pciback: avoid multiple entries in slot list

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 2/3] xen/pciback: avoid multiple entries in slot list Juergen Gross <jgross@suse.com> - 2016-09-22 10:50 +0200
    Re: [PATCH v3 2/3] xen/pciback: avoid multiple entries in slot list Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-09-22 23:10 +0200
      Re: [PATCH v3 2/3] xen/pciback: avoid multiple entries in slot list Juergen Gross <jgross@suse.com> - 2016-09-23 06:10 +0200
      Re: [Xen-devel] [PATCH v3 2/3] xen/pciback: avoid multiple entries in  slot list David Vrabel <david.vrabel@citrix.com> - 2016-09-23 12:20 +0200
        Re: [Xen-devel] [PATCH v3 2/3] xen/pciback: avoid multiple entries in  slot list Juergen Gross <jgross@suse.com> - 2016-09-28 19:30 +0200

#1488649 — [PATCH v3 2/3] xen/pciback: avoid multiple entries in slot list

The Xen pciback driver has a list of all pci devices it is ready to
seize. There is no check whether a to be added entry already exists.
While this might be no problem in the common case it might confuse
those which consume the list via sysfs.

Modify the handling of this list by not adding an entry which already
exists. As this will be needed later split out the list handling into
a separate function.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/xen-pciback/pci_stub.c | 39 ++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index 79a9e4d..0179333 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -478,6 +478,36 @@ static int __init pcistub_init_devices_late(void)
 	return 0;
 }
 
+static void pcistub_device_id_add_list(struct pcistub_device_id *new,
+				       int domain, int bus, unsigned int devfn)
+{
+	struct pcistub_device_id *pci_dev_id;
+	unsigned long flags;
+	int found = 0;
+
+	spin_lock_irqsave(&device_ids_lock, flags);
+
+	list_for_each_entry(pci_dev_id, &pcistub_device_ids, slot_list) {
+		if (pci_dev_id->domain == domain && pci_dev_id->bus == bus &&
+		    pci_dev_id->devfn == devfn) {
+			found = 1;
+			break;
+		}
+	}
+
+	if (!found) {
+		new->domain = domain;
+		new->bus = bus;
+		new->devfn = devfn;
+		list_add_tail(&new->slot_list, &pcistub_device_ids);
+	}
+
+	spin_unlock_irqrestore(&device_ids_lock, flags);
+
+	if (found)
+		kfree(new);
+}
+
 static int pcistub_seize(struct pci_dev *dev)
 {
 	struct pcistub_device *psdev;
@@ -1012,7 +1042,6 @@ static inline int str_to_quirk(const char *buf, int *domain, int *bus, int
 static int pcistub_device_id_add(int domain, int bus, int slot, int func)
 {
 	struct pcistub_device_id *pci_dev_id;
-	unsigned long flags;
 	int rc = 0, devfn = PCI_DEVFN(slot, func);
 
 	if (slot < 0) {
@@ -1042,16 +1071,10 @@ static int pcistub_device_id_add(int domain, int bus, int slot, int func)
 	if (!pci_dev_id)
 		return -ENOMEM;
 
-	pci_dev_id->domain = domain;
-	pci_dev_id->bus = bus;
-	pci_dev_id->devfn = devfn;
-
 	pr_debug("wants to seize %04x:%02x:%02x.%d\n",
 		 domain, bus, slot, func);
 
-	spin_lock_irqsave(&device_ids_lock, flags);
-	list_add_tail(&pci_dev_id->slot_list, &pcistub_device_ids);
-	spin_unlock_irqrestore(&device_ids_lock, flags);
+	pcistub_device_id_add_list(pci_dev_id, domain, bus, devfn);
 
 	return 0;
 }
-- 
2.6.6

[toc] | [next] | [standalone]

#1489551

On 09/22/2016 04:45 AM, Juergen Gross wrote:
> The Xen pciback driver has a list of all pci devices it is ready to
> seize. There is no check whether a to be added entry already exists.
> While this might be no problem in the common case it might confuse
> those which consume the list via sysfs.
>
> Modify the handling of this list by not adding an entry which already
> exists. As this will be needed later split out the list handling into
> a separate function.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  drivers/xen/xen-pciback/pci_stub.c | 39 ++++++++++++++++++++++++++++++--------
>  1 file changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
> index 79a9e4d..0179333 100644
> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
> @@ -478,6 +478,36 @@ static int __init pcistub_init_devices_late(void)
>  	return 0;
>  }
>  
> +static void pcistub_device_id_add_list(struct pcistub_device_id *new,
> +				       int domain, int bus, unsigned int devfn)
> +{
> +	struct pcistub_device_id *pci_dev_id;
> +	unsigned long flags;
> +	int found = 0;
> +
> +	spin_lock_irqsave(&device_ids_lock, flags);
> +
> +	list_for_each_entry(pci_dev_id, &pcistub_device_ids, slot_list) {
> +		if (pci_dev_id->domain == domain && pci_dev_id->bus == bus &&
> +		    pci_dev_id->devfn == devfn) {
> +			found = 1;
> +			break;
> +		}
> +	}
> +
> +	if (!found) {
> +		new->domain = domain;
> +		new->bus = bus;
> +		new->devfn = devfn;
> +		list_add_tail(&new->slot_list, &pcistub_device_ids);
> +	}
> +
> +	spin_unlock_irqrestore(&device_ids_lock, flags);
> +
> +	if (found)
> +		kfree(new);

I'd rather free 'new' in the caller (who allocated it) and return
something like -EEXIST if device is already on the list.

-boris

> +}
> +
>  static int pcistub_seize(struct pci_dev *dev)
>  {
>  	struct pcistub_device *psdev;
> @@ -1012,7 +1042,6 @@ static inline int str_to_quirk(const char *buf, int *domain, int *bus, int
>  static int pcistub_device_id_add(int domain, int bus, int slot, int func)
>  {
>  	struct pcistub_device_id *pci_dev_id;
> -	unsigned long flags;
>  	int rc = 0, devfn = PCI_DEVFN(slot, func);
>  
>  	if (slot < 0) {
> @@ -1042,16 +1071,10 @@ static int pcistub_device_id_add(int domain, int bus, int slot, int func)
>  	if (!pci_dev_id)
>  		return -ENOMEM;
>  
> -	pci_dev_id->domain = domain;
> -	pci_dev_id->bus = bus;
> -	pci_dev_id->devfn = devfn;
> -
>  	pr_debug("wants to seize %04x:%02x:%02x.%d\n",
>  		 domain, bus, slot, func);
>  
> -	spin_lock_irqsave(&device_ids_lock, flags);
> -	list_add_tail(&pci_dev_id->slot_list, &pcistub_device_ids);
> -	spin_unlock_irqrestore(&device_ids_lock, flags);
> +	pcistub_device_id_add_list(pci_dev_id, domain, bus, devfn);
>  
>  	return 0;
>  }

[toc] | [prev] | [next] | [standalone]

#1489739

On 22/09/16 23:02, Boris Ostrovsky wrote:
> On 09/22/2016 04:45 AM, Juergen Gross wrote:
>> The Xen pciback driver has a list of all pci devices it is ready to
>> seize. There is no check whether a to be added entry already exists.
>> While this might be no problem in the common case it might confuse
>> those which consume the list via sysfs.
>>
>> Modify the handling of this list by not adding an entry which already
>> exists. As this will be needed later split out the list handling into
>> a separate function.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  drivers/xen/xen-pciback/pci_stub.c | 39 ++++++++++++++++++++++++++++++--------
>>  1 file changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
>> index 79a9e4d..0179333 100644
>> --- a/drivers/xen/xen-pciback/pci_stub.c
>> +++ b/drivers/xen/xen-pciback/pci_stub.c
>> @@ -478,6 +478,36 @@ static int __init pcistub_init_devices_late(void)
>>  	return 0;
>>  }
>>  
>> +static void pcistub_device_id_add_list(struct pcistub_device_id *new,
>> +				       int domain, int bus, unsigned int devfn)
>> +{
>> +	struct pcistub_device_id *pci_dev_id;
>> +	unsigned long flags;
>> +	int found = 0;
>> +
>> +	spin_lock_irqsave(&device_ids_lock, flags);
>> +
>> +	list_for_each_entry(pci_dev_id, &pcistub_device_ids, slot_list) {
>> +		if (pci_dev_id->domain == domain && pci_dev_id->bus == bus &&
>> +		    pci_dev_id->devfn == devfn) {
>> +			found = 1;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (!found) {
>> +		new->domain = domain;
>> +		new->bus = bus;
>> +		new->devfn = devfn;
>> +		list_add_tail(&new->slot_list, &pcistub_device_ids);
>> +	}
>> +
>> +	spin_unlock_irqrestore(&device_ids_lock, flags);
>> +
>> +	if (found)
>> +		kfree(new);
> 
> I'd rather free 'new' in the caller (who allocated it) and return
> something like -EEXIST if device is already on the list.

Hmm, I thought of this, but with two callers after the following patch
having to deal with the situation I've chosen this way to do it.

The code is smaller this way.


Juergen

[toc] | [prev] | [next] | [standalone]

#1489922 — Re: [Xen-devel] [PATCH v3 2/3] xen/pciback: avoid multiple entries in slot list

On 22/09/16 22:02, Boris Ostrovsky wrote:
> On 09/22/2016 04:45 AM, Juergen Gross wrote:
>> The Xen pciback driver has a list of all pci devices it is ready to
>> seize. There is no check whether a to be added entry already exists.
>> While this might be no problem in the common case it might confuse
>> those which consume the list via sysfs.
>>
>> Modify the handling of this list by not adding an entry which already
>> exists. As this will be needed later split out the list handling into
>> a separate function.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  drivers/xen/xen-pciback/pci_stub.c | 39 ++++++++++++++++++++++++++++++--------
>>  1 file changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
>> index 79a9e4d..0179333 100644
>> --- a/drivers/xen/xen-pciback/pci_stub.c
>> +++ b/drivers/xen/xen-pciback/pci_stub.c
>> @@ -478,6 +478,36 @@ static int __init pcistub_init_devices_late(void)
>>  	return 0;
>>  }
>>  
>> +static void pcistub_device_id_add_list(struct pcistub_device_id *new,
>> +				       int domain, int bus, unsigned int devfn)

I think this should allocate the new pcistub_device_id if needed.  You
can pass in GFP flags if needed.

Then it can return the newly allocated one, or the existing one.

static struct pcistub_device_id *pcistub_device_id_add_list(
    int domain, int bus, unsigned int devfn)

David

[toc] | [prev] | [next] | [standalone]

#1492792 — Re: [Xen-devel] [PATCH v3 2/3] xen/pciback: avoid multiple entries in slot list

On 23/09/16 12:15, David Vrabel wrote:
> On 22/09/16 22:02, Boris Ostrovsky wrote:
>> On 09/22/2016 04:45 AM, Juergen Gross wrote:
>>> The Xen pciback driver has a list of all pci devices it is ready to
>>> seize. There is no check whether a to be added entry already exists.
>>> While this might be no problem in the common case it might confuse
>>> those which consume the list via sysfs.
>>>
>>> Modify the handling of this list by not adding an entry which already
>>> exists. As this will be needed later split out the list handling into
>>> a separate function.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>>  drivers/xen/xen-pciback/pci_stub.c | 39 ++++++++++++++++++++++++++++++--------
>>>  1 file changed, 31 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
>>> index 79a9e4d..0179333 100644
>>> --- a/drivers/xen/xen-pciback/pci_stub.c
>>> +++ b/drivers/xen/xen-pciback/pci_stub.c
>>> @@ -478,6 +478,36 @@ static int __init pcistub_init_devices_late(void)
>>>  	return 0;
>>>  }
>>>  
>>> +static void pcistub_device_id_add_list(struct pcistub_device_id *new,
>>> +				       int domain, int bus, unsigned int devfn)
> 
> I think this should allocate the new pcistub_device_id if needed.  You
> can pass in GFP flags if needed.
> 
> Then it can return the newly allocated one, or the existing one.
> 
> static struct pcistub_device_id *pcistub_device_id_add_list(
>     int domain, int bus, unsigned int devfn)

Patch 3 will be very nasty then: in case of an allocation failure
all of the actions done in pcistub_seize() will have to be undone
again. I'd really like to avoid that.


Juergen

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web