Re: [PATCH 07/23] pci, dmar: Update dmar units devices list during hotplug

[Bjorn Helgaas]

On Fri, Mar 9, 2012 at 12:06 AM, Yinghai Lu <yinghai@xxxxxxxxxx> wrote:
> On Thu, Mar 8, 2012 at 5:06 PM, Bjorn Helgaas <bhelgaas@xxxxxxxxxx> wrote:
>> On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@xxxxxxxxxx> wrote:
>>> When do pci remove/rescan on system that have more iommus, got
>>>
>>> [  894.089745] Set context mapping for c4:00.0
>>> [  894.110890] mpt2sas3: Allocated physical memory: size(4293 kB)
>>> [  894.112556] mpt2sas3: Current Controller Queue Depth(1883), Max Controller Queue Depth(2144)
>>> [  894.127278] mpt2sas3: Scatter Gather Elements per IO(128)
>>> [  894.361295] DRHD: handling fault status reg 2
>>> [  894.364053] DMAR:[DMA Read] Request device [c4:00.0] fault addr fffbe000
>>> [  894.364056] DMAR:[fault reason 02] Present bit in context entry is cl
>>>
>>> it turns out when remove/rescan, pci dev will be freed and will get another new dev.
>>> but drhd units still keep old one... so dmar_find_matched_drhd_unit will
>>> return wrong drhd and iommu for the device that is not on first iommu.
>>>
>>> So need to update devices in drhd_units during pci remove/rescan.
>>>
>>> Could save domain/bus/device/function aside in the list and according that info
>>> restore new dev to drhd_units later.
>>> Then dmar_find_matched_drdh_unit and device_to_iommu could return right drhd and iommu.
>>>
>>> Add remove_dev_from_drhd/restore_dev_to_drhd functions to do the real work.
>>> call them in device ADD_DEVICE and UNBOUND_DRIVER
>>>
>>> Need to do the samething to atsr.  (expose dmar_atsr_units and add atsru->segment)
>>>
>>> After patch, will right iommu for the new dev and will not get DMAR error any more.
>>>
>>> -v2: add pci_dev_put/pci_dev_get to make refcount consistent.
>>>
>>> Signed-off-by: Yinghai Lu <yinghai@xxxxxxxxxx>
>>> Cc: David Woodhouse <dwmw2@xxxxxxxxxxxxx>
>>> Cc: Vinod Koul <vinod.koul@xxxxxxxxx>
>>> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
>>> Cc: iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx
>>> ---
>>>  drivers/iommu/intel-iommu.c |  187 ++++++++++++++++++++++++++++++++++++++++---
>>>  include/linux/dmar.h        |    4 +
>>>  2 files changed, 181 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>>> index c9c6053..39ef2ce 100644
>>> --- a/drivers/iommu/intel-iommu.c
>>> +++ b/drivers/iommu/intel-iommu.c
>>> @@ -665,6 +665,159 @@ static struct intel_iommu *device_to_iommu(int segment, u8 bus, u8 devfn)
>>>        return NULL;
>>>  }
>>>
>>> +#ifdef CONFIG_HOTPLUG
>>> +struct dev_dmaru {
>>> +       struct list_head list;
>>> +       void *dmaru;
>>> +       int index;
>>> +       int segment;
>>> +       unsigned char bus;
>>> +       unsigned int devfn;
>>> +};
>>> +
>>> +static int
>>> +save_dev_dmaru(int segment, unsigned char bus, unsigned int devfn,
>>> +                void *dmaru, int index, struct list_head *lh)
>>
>> Follow the indentation style used elsewhere in the file: as much of
>> the declaration on one line as will fit (here and below).
>>
>> I think the callers would read more naturally if you passed in the
>> struct pci_dev * to save_dev_dmaru() and extracted the
>> segment/bus/devfn here rather than in the callers.
>
> Just want to keep save_dev_dmaru more consistent to get_dev_dmaru...

Well, it looks like you can change both save_dev_dmaru() *and*
get_dev_dmaru() to take a struct pci_dev *.  I assumed that would be
obvious.

Bjorn
--
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/