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

[Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On Tue, 2012-10-09 at 08:33 -0600, Shuah Khan wrote:
> Alex,
> 
> couple of comments in-lined:
> 
> On Mon, Oct 8, 2012 at 10:49 PM, Alex Williamson
> <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >
> >  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

It looks that way, but...

> >
> > -       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;

If you apply the patch, you'll see the end of this function is:

        ret = iommu_group_add_device(group, dev);

        iommu_group_put(group);

        return ret;
}

It's unfortunate that diff didn't make this easier to review.  Thanks,

Alex

> > +}
> > +
> > +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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/