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

Groups > linux.kernel > #1303512 > unrolled thread

Re: [PATCH 5/6] iommu/amd: Add support for non-pci devices

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

  Re: [PATCH 5/6] iommu/amd: Add support for non-pci devices Joerg Roedel <joro@8bytes.org> - 2016-01-07 13:10 +0100
    Re: [PATCH 5/6] iommu/amd: Add support for non-pci devices Wan ZongShun <mcuos.com@gmail.com> - 2016-01-08 04:20 +0100
      Re: [PATCH 5/6] iommu/amd: Add support for non-pci devices Joerg Roedel <joro@8bytes.org> - 2016-01-08 13:20 +0100
        Re: [PATCH 5/6] iommu/amd: Add support for non-pci devices Joerg Roedel <joro@8bytes.org> - 2016-01-08 18:10 +0100
          Re: [PATCH 5/6] iommu/amd: Add support for non-pci devices Wan Zongshun <vw@iommu.org> - 2016-01-09 11:10 +0100
        Re: [PATCH 5/6] iommu/amd: Add support for non-pci devices Wan Zongshun <vw@iommu.org> - 2016-01-08 19:10 +0100

#1303512 — Re: [PATCH 5/6] iommu/amd: Add support for non-pci devices

On Tue, Jan 05, 2016 at 05:07:23AM -0500, Wan Zongshun wrote:
> -static inline u16 get_device_id(struct device *dev)
> +static inline int match_hid_uid(struct device *dev,
> +				struct acpihid_map_entry *entry)
> +{
> +	const char *hid, *uid;
> +
> +	hid = acpi_device_hid(ACPI_COMPANION(dev));
> +	uid = acpi_device_uid(ACPI_COMPANION(dev));
> +
> +	if (!hid || !(*hid))
> +		return -ENODEV;
> +
> +	if (!uid || !(*uid))
> +		return strcmp(hid, entry->hid);
> +
> +	if (!(*entry->uid))
> +		return strcmp(hid, entry->hid);
> +
> +	return -ENODEV;
> +}
> +
> +static inline u16 get_pci_device_id(struct device *dev)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
>  
>  	return PCI_DEVID(pdev->bus->number, pdev->devfn);
>  }
>  
> +static inline int get_acpihid_device_id(struct device *dev,
> +					struct acpihid_map_entry **entry)
> +{
> +	struct acpihid_map_entry *p;
> +
> +	list_for_each_entry(p, &acpihid_map, list) {
> +		if (!match_hid_uid(dev, p)) {
> +			if (entry)
> +				*entry = p;
> +			return p->devid;
> +		}
> +	}
> +	return -EINVAL;
> +}
> +
> +static inline u16 get_device_id(struct device *dev)
> +{
> +	if (dev_is_pci(dev))
> +		return get_pci_device_id(dev);
> +	else
> +		return get_acpihid_device_id(dev, NULL);
> +}

This is not robust, get_acpihid_device_id() returns int and can return a
negative value. This gets lost when converting it to u16 here. So either
you add error handling for get_acpihid_device_id() in get_device_id() or
you change get_device_id() to return int too and handle the error at the
callers of get_device_id().


	Joerg

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1304150

2016-01-07 20:04 GMT+08:00 Joerg Roedel <joro@8bytes.org>:
> On Tue, Jan 05, 2016 at 05:07:23AM -0500, Wan Zongshun wrote:
>> -static inline u16 get_device_id(struct device *dev)
>> +static inline int match_hid_uid(struct device *dev,
>> +                             struct acpihid_map_entry *entry)
>> +{
>> +     const char *hid, *uid;
>> +
>> +     hid = acpi_device_hid(ACPI_COMPANION(dev));
>> +     uid = acpi_device_uid(ACPI_COMPANION(dev));
>> +
>> +     if (!hid || !(*hid))
>> +             return -ENODEV;
>> +
>> +     if (!uid || !(*uid))
>> +             return strcmp(hid, entry->hid);
>> +
>> +     if (!(*entry->uid))
>> +             return strcmp(hid, entry->hid);
>> +
>> +     return -ENODEV;
>> +}
>> +
>> +static inline u16 get_pci_device_id(struct device *dev)
>>  {
>>       struct pci_dev *pdev = to_pci_dev(dev);
>>
>>       return PCI_DEVID(pdev->bus->number, pdev->devfn);
>>  }
>>
>> +static inline int get_acpihid_device_id(struct device *dev,
>> +                                     struct acpihid_map_entry **entry)
>> +{
>> +     struct acpihid_map_entry *p;
>> +
>> +     list_for_each_entry(p, &acpihid_map, list) {
>> +             if (!match_hid_uid(dev, p)) {
>> +                     if (entry)
>> +                             *entry = p;
>> +                     return p->devid;
>> +             }
>> +     }
>> +     return -EINVAL;
>> +}
>> +
>> +static inline u16 get_device_id(struct device *dev)
>> +{
>> +     if (dev_is_pci(dev))
>> +             return get_pci_device_id(dev);
>> +     else
>> +             return get_acpihid_device_id(dev, NULL);
>> +}
>
> This is not robust, get_acpihid_device_id() returns int and can return a
> negative value. This gets lost when converting it to u16 here. So either
> you add error handling for get_acpihid_device_id() in get_device_id() or
> you change get_device_id() to return int too and handle the error at the
> callers of get_device_id().

Joerg,

Please see the following function, since I judge this
'get_acpihid_device_id(dev, NULL) < 0'  in the front of
'get_device_id', so your concern should not exist. I have already
filtered the negative situation in check_device firstly, do you think
it is ok?


static bool check_device(struct device *dev)
{
        u16 devid;
......

        /* No PCI device */
        if (!dev_is_pci(dev) && (get_acpihid_device_id(dev, NULL) < 0))
                return false;

        devid = get_device_id(dev);

.....

        return true;
}


>
>
>         Joerg
>
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu



-- 
---
Vincent Wan(Zongshun)
www.mcuos.com

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

#1304469

On Fri, Jan 08, 2016 at 11:15:37AM +0800, Wan ZongShun wrote:
> Please see the following function, since I judge this
> 'get_acpihid_device_id(dev, NULL) < 0'  in the front of
> 'get_device_id', so your concern should not exist. I have already
> filtered the negative situation in check_device firstly, do you think
> it is ok?
> 
> 
> static bool check_device(struct device *dev)
> {
>         u16 devid;
> ......
> 
>         /* No PCI device */
>         if (!dev_is_pci(dev) && (get_acpihid_device_id(dev, NULL) < 0))
>                 return false;
> 
>         devid = get_device_id(dev);

That is true for this case, but the other call-sites of get_device_id()
still have to care about a potential negative return value, right?



	Joerg

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

#1304737

On Fri, Jan 08, 2016 at 10:52:59PM +0800, Wan Zongshun wrote:
> Actually I am supposing the '.add_device' will be the first called
> in iommu initializing stage, so I think as long as having no error
> of check device here, any call-sites of get_device_id() will be
> fine, because adding device successfully should be the pre-condition
> of any iommu function can be performed, please correct me.
> 
> static int amd_iommu_add_device(struct device *dev)
> {
> 	struct iommu_dev_data *dev_data;
> 	struct iommu_domain *domain;
> 	struct amd_iommu *iommu;
> 	u16 devid;
> 	int ret;
> 
> 	if (!check_device(dev) || get_dev_data(dev))
> 		return 0;
> 
> 	devid = get_device_id(dev);
> 	iommu = amd_iommu_rlookup_table[devid];

There are places in the interrupt remapping code that call get_device_id
without calling check_device first. See get_irq_domain and get_devid.


	Joerg

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

#1305201

-------- Original Message --------
> On Fri, Jan 08, 2016 at 10:52:59PM +0800, Wan Zongshun wrote:
>> Actually I am supposing the '.add_device' will be the first called
>> in iommu initializing stage, so I think as long as having no error
>> of check device here, any call-sites of get_device_id() will be
>> fine, because adding device successfully should be the pre-condition
>> of any iommu function can be performed, please correct me.
>>
>> static int amd_iommu_add_device(struct device *dev)
>> {
>> 	struct iommu_dev_data *dev_data;
>> 	struct iommu_domain *domain;
>> 	struct amd_iommu *iommu;
>> 	u16 devid;
>> 	int ret;
>>
>> 	if (!check_device(dev) || get_dev_data(dev))
>> 		return 0;
>>
>> 	devid = get_device_id(dev);
>> 	iommu = amd_iommu_rlookup_table[devid];
>
> There are places in the interrupt remapping code that call get_device_id
> without calling check_device first. See get_irq_domain and get_devid.
>

Okay, I will change this get_device_id return to int, and judge this 
return value in caller of this function like get_devid style.

If so we will modify some existing amd iommu driver codes, and Can I 
merge those into this patch 5/6? or I will create another dedicated 
patch to take this action?

Vincent.

>
> 	Joerg
>
>

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

#1304789

>>
>>
>> static bool check_device(struct device *dev)
>> {
>>          u16 devid;
>> ......
>>
>>          /* No PCI device */
>>          if (!dev_is_pci(dev) && (get_acpihid_device_id(dev, NULL) < 0))
>>                  return false;
>>
>>          devid = get_device_id(dev);
>
> That is true for this case, but the other call-sites of get_device_id()
> still have to care about a potential negative return value, right?
>

Actually I am supposing the '.add_device' will be the first called in 
iommu initializing stage, so I think as long as having no error of check 
device here, any call-sites of get_device_id() will be fine, because 
adding device successfully should be the pre-condition of any iommu 
function can be performed, please correct me.

static int amd_iommu_add_device(struct device *dev)
{
	struct iommu_dev_data *dev_data;
	struct iommu_domain *domain;
	struct amd_iommu *iommu;
	u16 devid;
	int ret;

	if (!check_device(dev) || get_dev_data(dev))
		return 0;

	devid = get_device_id(dev);
	iommu = amd_iommu_rlookup_table[devid];


Vincent.
>
> 	Joerg
>
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web