Re: [RFC PATCH 1/5] amd_iommu: Split IOMMU group initialization

[Shuah Khan]

Alex,

couple of comments in-lined:

On Mon, Oct 8, 2012 at 10:49 PM, Alex Williamson
<alex.williamson@xxxxxxxxxx> 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.williamson@xxxxxxxxxx>
> ---
>
>  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 majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/