| Message ID | 1398150453-28141-18-git-send-email-jiang.liu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Tuesday, April 22, 2014 03:07:28 PM Jiang Liu wrote: > Finally enhance pci_root driver to support DMAR device hotplug when > hot-plugging PCI host bridges. > > Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> > --- > drivers/acpi/pci_root.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index d388f13d48b4..aa8f549869f3 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -33,6 +33,7 @@ > #include <linux/pci.h> > #include <linux/pci-acpi.h> > #include <linux/pci-aspm.h> > +#include <linux/dmar.h> > #include <linux/acpi.h> > #include <linux/slab.h> > #include <acpi/apei.h> /* for acpi_hest_init() */ > @@ -511,6 +512,7 @@ static int acpi_pci_root_add(struct acpi_device *device, > struct acpi_pci_root *root; > acpi_handle handle = device->handle; > int no_aspm = 0, clear_aspm = 0; > + bool hotadd = (system_state != SYSTEM_BOOTING); The parens are not necessary in the above instruction. > root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL); > if (!root) > @@ -557,6 +559,11 @@ static int acpi_pci_root_add(struct acpi_device *device, > strcpy(acpi_device_class(device), ACPI_PCI_ROOT_CLASS); > device->driver_data = root; > > + if (hotadd && dmar_device_hotplug(handle, true)) { > + result = -ENXIO; > + goto end; > + } > + > pr_info(PREFIX "%s [%s] (domain %04x %pR)\n", > acpi_device_name(device), acpi_device_bid(device), > root->segment, &root->secondary); > @@ -583,7 +590,7 @@ static int acpi_pci_root_add(struct acpi_device *device, > root->segment, (unsigned int)root->secondary.start); > device->driver_data = NULL; > result = -ENODEV; > - goto end; > + goto remove_dmar; > } > > if (clear_aspm) { > @@ -597,7 +604,7 @@ static int acpi_pci_root_add(struct acpi_device *device, > if (device->wakeup.flags.run_wake) > device_set_run_wake(root->bus->bridge, true); > > - if (system_state != SYSTEM_BOOTING) { > + if (hotadd) { > pcibios_resource_survey_bus(root->bus); > pci_assign_unassigned_root_bus_resources(root->bus); > } > @@ -607,6 +614,9 @@ static int acpi_pci_root_add(struct acpi_device *device, > pci_unlock_rescan_remove(); > return 1; > > +remove_dmar: > + if (hotadd) > + dmar_device_hotplug(handle, false); I suppose that works if dmar_device_hotplug() returned false before? > end: > kfree(root); > return result; > @@ -625,6 +635,8 @@ static void acpi_pci_root_remove(struct acpi_device *device) > > pci_remove_root_bus(root->bus); > > + dmar_device_hotplug(device->handle, false); > + > pci_unlock_rescan_remove(); > > kfree(root); >
On Tue, Apr 22, 2014 at 03:07:28PM +0800, Jiang Liu wrote: > Finally enhance pci_root driver to support DMAR device hotplug when > hot-plugging PCI host bridges. > > Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> > --- > drivers/acpi/pci_root.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index d388f13d48b4..aa8f549869f3 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -33,6 +33,7 @@ > #include <linux/pci.h> > #include <linux/pci-acpi.h> > #include <linux/pci-aspm.h> > +#include <linux/dmar.h> > #include <linux/acpi.h> > #include <linux/slab.h> > #include <acpi/apei.h> /* for acpi_hest_init() */ > @@ -511,6 +512,7 @@ static int acpi_pci_root_add(struct acpi_device *device, > struct acpi_pci_root *root; > acpi_handle handle = device->handle; > int no_aspm = 0, clear_aspm = 0; > + bool hotadd = (system_state != SYSTEM_BOOTING); > > root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL); > if (!root) > @@ -557,6 +559,11 @@ static int acpi_pci_root_add(struct acpi_device *device, > strcpy(acpi_device_class(device), ACPI_PCI_ROOT_CLASS); > device->driver_data = root; > > + if (hotadd && dmar_device_hotplug(handle, true)) { Apparently "dmar_device_hotplug(handle, true)" means "add a DMAR device," and "dmar_device_hotplug(device->handle, false)" means "remove a DMAR device." I'm not really a fan of interfaces where one of the arguments selects between two completely different actions, because it's harder for a casual reader to see what's going on. I see how it simplifies your implementation a little bit, but I think it's more important to simplify for the reader. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014/4/25 1:33, Bjorn Helgaas wrote: > On Tue, Apr 22, 2014 at 03:07:28PM +0800, Jiang Liu wrote: >> Finally enhance pci_root driver to support DMAR device hotplug when >> hot-plugging PCI host bridges. >> >> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> >> --- >> drivers/acpi/pci_root.c | 16 ++++++++++++++-- >> 1 file changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c >> index d388f13d48b4..aa8f549869f3 100644 >> --- a/drivers/acpi/pci_root.c >> +++ b/drivers/acpi/pci_root.c >> @@ -33,6 +33,7 @@ >> #include <linux/pci.h> >> #include <linux/pci-acpi.h> >> #include <linux/pci-aspm.h> >> +#include <linux/dmar.h> >> #include <linux/acpi.h> >> #include <linux/slab.h> >> #include <acpi/apei.h> /* for acpi_hest_init() */ >> @@ -511,6 +512,7 @@ static int acpi_pci_root_add(struct acpi_device *device, >> struct acpi_pci_root *root; >> acpi_handle handle = device->handle; >> int no_aspm = 0, clear_aspm = 0; >> + bool hotadd = (system_state != SYSTEM_BOOTING); >> >> root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL); >> if (!root) >> @@ -557,6 +559,11 @@ static int acpi_pci_root_add(struct acpi_device *device, >> strcpy(acpi_device_class(device), ACPI_PCI_ROOT_CLASS); >> device->driver_data = root; >> >> + if (hotadd && dmar_device_hotplug(handle, true)) { > > Apparently "dmar_device_hotplug(handle, true)" means "add a DMAR device," > and "dmar_device_hotplug(device->handle, false)" means "remove a DMAR > device." I'm not really a fan of interfaces where one of the arguments > selects between two completely different actions, because it's harder for a > casual reader to see what's going on. > > I see how it simplifies your implementation a little bit, but I think it's > more important to simplify for the reader. Thanks Bjorn, I will rename it to dmar_device_add/remove() instead. > > Bjorn > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014/4/22 17:49, Rafael J. Wysocki wrote: > On Tuesday, April 22, 2014 03:07:28 PM Jiang Liu wrote: >> Finally enhance pci_root driver to support DMAR device hotplug when >> hot-plugging PCI host bridges. >> >> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> >> --- >> drivers/acpi/pci_root.c | 16 ++++++++++++++-- >> 1 file changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c >> index d388f13d48b4..aa8f549869f3 100644 >> --- a/drivers/acpi/pci_root.c >> +++ b/drivers/acpi/pci_root.c >> @@ -33,6 +33,7 @@ >> #include <linux/pci.h> >> #include <linux/pci-acpi.h> >> #include <linux/pci-aspm.h> >> +#include <linux/dmar.h> >> #include <linux/acpi.h> >> #include <linux/slab.h> >> #include <acpi/apei.h> /* for acpi_hest_init() */ >> @@ -511,6 +512,7 @@ static int acpi_pci_root_add(struct acpi_device *device, >> struct acpi_pci_root *root; >> acpi_handle handle = device->handle; >> int no_aspm = 0, clear_aspm = 0; >> + bool hotadd = (system_state != SYSTEM_BOOTING); > > The parens are not necessary in the above instruction. Hi Rafael, Thanks! Will fix it in next version. > >> root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL); >> if (!root) >> @@ -557,6 +559,11 @@ static int acpi_pci_root_add(struct acpi_device *device, >> strcpy(acpi_device_class(device), ACPI_PCI_ROOT_CLASS); >> device->driver_data = root; >> >> + if (hotadd && dmar_device_hotplug(handle, true)) { >> + result = -ENXIO; >> + goto end; >> + } >> + >> pr_info(PREFIX "%s [%s] (domain %04x %pR)\n", >> acpi_device_name(device), acpi_device_bid(device), >> root->segment, &root->secondary); >> @@ -583,7 +590,7 @@ static int acpi_pci_root_add(struct acpi_device *device, >> root->segment, (unsigned int)root->secondary.start); >> device->driver_data = NULL; >> result = -ENODEV; >> - goto end; >> + goto remove_dmar; >> } >> >> if (clear_aspm) { >> @@ -597,7 +604,7 @@ static int acpi_pci_root_add(struct acpi_device *device, >> if (device->wakeup.flags.run_wake) >> device_set_run_wake(root->bus->bridge, true); >> >> - if (system_state != SYSTEM_BOOTING) { >> + if (hotadd) { >> pcibios_resource_survey_bus(root->bus); >> pci_assign_unassigned_root_bus_resources(root->bus); >> } >> @@ -607,6 +614,9 @@ static int acpi_pci_root_add(struct acpi_device *device, >> pci_unlock_rescan_remove(); >> return 1; >> >> +remove_dmar: >> + if (hotadd) >> + dmar_device_hotplug(handle, false); > > I suppose that works if dmar_device_hotplug() returned false before? Yes, it's for error recover only. > >> end: >> kfree(root); >> return result; >> @@ -625,6 +635,8 @@ static void acpi_pci_root_remove(struct acpi_device *device) >> >> pci_remove_root_bus(root->bus); >> >> + dmar_device_hotplug(device->handle, false); >> + >> pci_unlock_rescan_remove(); >> >> kfree(root); >> > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index d388f13d48b4..aa8f549869f3 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -33,6 +33,7 @@ #include <linux/pci.h> #include <linux/pci-acpi.h> #include <linux/pci-aspm.h> +#include <linux/dmar.h> #include <linux/acpi.h> #include <linux/slab.h> #include <acpi/apei.h> /* for acpi_hest_init() */ @@ -511,6 +512,7 @@ static int acpi_pci_root_add(struct acpi_device *device, struct acpi_pci_root *root; acpi_handle handle = device->handle; int no_aspm = 0, clear_aspm = 0; + bool hotadd = (system_state != SYSTEM_BOOTING); root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL); if (!root) @@ -557,6 +559,11 @@ static int acpi_pci_root_add(struct acpi_device *device, strcpy(acpi_device_class(device), ACPI_PCI_ROOT_CLASS); device->driver_data = root; + if (hotadd && dmar_device_hotplug(handle, true)) { + result = -ENXIO; + goto end; + } + pr_info(PREFIX "%s [%s] (domain %04x %pR)\n", acpi_device_name(device), acpi_device_bid(device), root->segment, &root->secondary); @@ -583,7 +590,7 @@ static int acpi_pci_root_add(struct acpi_device *device, root->segment, (unsigned int)root->secondary.start); device->driver_data = NULL; result = -ENODEV; - goto end; + goto remove_dmar; } if (clear_aspm) { @@ -597,7 +604,7 @@ static int acpi_pci_root_add(struct acpi_device *device, if (device->wakeup.flags.run_wake) device_set_run_wake(root->bus->bridge, true); - if (system_state != SYSTEM_BOOTING) { + if (hotadd) { pcibios_resource_survey_bus(root->bus); pci_assign_unassigned_root_bus_resources(root->bus); } @@ -607,6 +614,9 @@ static int acpi_pci_root_add(struct acpi_device *device, pci_unlock_rescan_remove(); return 1; +remove_dmar: + if (hotadd) + dmar_device_hotplug(handle, false); end: kfree(root); return result; @@ -625,6 +635,8 @@ static void acpi_pci_root_remove(struct acpi_device *device) pci_remove_root_bus(root->bus); + dmar_device_hotplug(device->handle, false); + pci_unlock_rescan_remove(); kfree(root);
Finally enhance pci_root driver to support DMAR device hotplug when hot-plugging PCI host bridges. Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> --- drivers/acpi/pci_root.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)