Alex,
couple of comments in-lined:
On Mon, Oct 8, 2012 at 10:49 PM, Alex Williamson
<alex.william...@redhat.com> wrote:
> This needs to be broken apart, start with pulling all the IOMMU
> group init code into a new function.
>
> Signed-off-by: Alex Williamson <alex.william...@redhat.com>
> ---
>
> drivers/iommu/amd_iommu.c | 61
> ++++++++++++++++++++++++++++++++-------------
> 1 file changed, 43 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 55074cb..b65b377 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -276,39 +276,32 @@ static void swap_pci_ref(struct pci_dev **from, struct
> pci_dev *to)
>
> #define REQ_ACS_FLAGS (PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF)
>
> -static int iommu_init_device(struct device *dev)
> +static int init_iommu_group(struct device *dev)
> {
> - struct pci_dev *dma_pdev = NULL, *pdev = to_pci_dev(dev);
> struct iommu_dev_data *dev_data;
> struct iommu_group *group;
> - u16 alias;
> + struct pci_dev *dma_pdev = NULL;
> int ret;
I don't see ret get initialized
>
> - if (dev->archdata.iommu)
> + group = iommu_group_get(dev);
> + if (group) {
> + iommu_group_put(group);
> return 0;
> + }
>
> dev_data = find_dev_data(get_device_id(dev));
> if (!dev_data)
> return -ENOMEM;
>
> - alias = amd_iommu_alias_table[dev_data->devid];
> - if (alias != dev_data->devid) {
> - struct iommu_dev_data *alias_data;
> -
> - alias_data = find_dev_data(alias);
> - if (alias_data == NULL) {
> - pr_err("AMD-Vi: Warning: Unhandled device %s\n",
> - dev_name(dev));
> - free_dev_data(dev_data);
> - return -ENOTSUPP;
> - }
> - dev_data->alias_data = alias_data;
> + if (dev_data->alias_data) {
> + u16 alias;
>
> + alias = amd_iommu_alias_table[dev_data->devid];
> dma_pdev = pci_get_bus_and_slot(alias >> 8, alias & 0xff);
> }
>
> - if (dma_pdev == NULL)
> - dma_pdev = pci_dev_get(pdev);
> + if (!dma_pdev)
> + dma_pdev = pci_dev_get(to_pci_dev(dev));
>
> /* Account for quirked devices */
> swap_pci_ref(&dma_pdev, pci_get_dma_source(dma_pdev));
> @@ -358,6 +351,38 @@ root_bus:
>
> iommu_group_put(group);
>
> + return ret;
Do you even need ret - can this be simply return 0;
> +}
> +
> +static int iommu_init_device(struct device *dev)
> +{
> + struct pci_dev *pdev = to_pci_dev(dev);
> + struct iommu_dev_data *dev_data;
> + u16 alias;
> + int ret;
> +
> + if (dev->archdata.iommu)
> + return 0;
> +
> + dev_data = find_dev_data(get_device_id(dev));
> + if (!dev_data)
> + return -ENOMEM;
> +
> + alias = amd_iommu_alias_table[dev_data->devid];
> + if (alias != dev_data->devid) {
> + struct iommu_dev_data *alias_data;
> +
> + alias_data = find_dev_data(alias);
> + if (alias_data == NULL) {
> + pr_err("AMD-Vi: Warning: Unhandled device %s\n",
> + dev_name(dev));
> + free_dev_data(dev_data);
> + return -ENOTSUPP;
> + }
> + dev_data->alias_data = alias_data;
> + }
> +
> + ret = init_iommu_group(dev);
> if (ret)
> return ret;
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
