On Thu, May 31, 2018 at 04:51:17PM +0300, Mika Westerberg wrote:
> On Thu, May 31, 2018 at 08:12:02AM -0500, Bjorn Helgaas wrote:
> > Maybe I'm reading your patches wrong. It looks to me like shpchp will
> > claim GOLAM_7450, which means shpchp will register slots, program the
> > SHPC, handle hotplug interrupts, etc.
> >
> > But since shpchp_is_native() returns false, acpiphp thinks *it* should
> > handle hotplug. For example, I think that given some ACPI
> > prerequisites (_EJ0/_RMV/etc), both will call pci_hp_register():
> >
> > shpc_probe
> > is_shpc_capable # true for GOLAM_7450
> > init_slots
> > pci_hp_register
> >
> > acpi_pci_add_slots
> > acpiphp_enumerate_slots
> > acpi_walk_namespace(..., acpiphp_add_context)
> > acpiphp_add_context
> > hotplug_is_native # false for GOLAM_7450
> > acpiphp_register_hotplug_slot
> > pci_hp_register
> >
> > It is true that the same situation occurred before your patches, since
> > acpiphp_add_context() only checked pciehp_is_native(). In fact, with
> > the existing code, shpchp and acpiphp could both try to manage *any*
> > SHPC, not just GOLAM_7450.
> >
> > I think the current series fixes 99% of that problem and it seems like
> > we should try to do that last 1% at the same time so the SHPC code
> > makes more sense.
>
> Would the following fix the last 1% for you? Applies on top of this
> patch.
Yes, that's exactly what I was looking for! Thanks!
> diff --git a/drivers/pci/hotplug/shpchp.h b/drivers/pci/hotplug/shpchp.h
> index 9675ab757323..516e4835019c 100644
> --- a/drivers/pci/hotplug/shpchp.h
> +++ b/drivers/pci/hotplug/shpchp.h
> @@ -105,7 +105,6 @@ struct controller {
> };
>
> /* Define AMD SHPC ID */
> -#define PCI_DEVICE_ID_AMD_GOLAM_7450 0x7450
> #define PCI_DEVICE_ID_AMD_POGO_7458 0x7458
>
> /* AMD PCI-X bridge registers */
> diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c
> index 0f3711404c2b..e91be287f292 100644
> --- a/drivers/pci/hotplug/shpchp_core.c
> +++ b/drivers/pci/hotplug/shpchp_core.c
> @@ -270,22 +270,12 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value)
> return 0;
> }
>
> -static int is_shpc_capable(struct pci_dev *dev)
> -{
> - if (dev->vendor == PCI_VENDOR_ID_AMD &&
> - dev->device == PCI_DEVICE_ID_AMD_GOLAM_7450)
> - return 1;
> - if (acpi_get_hp_hw_control_from_firmware(dev))
> - return 0;
> - return 1;
> -}
> -
> static int shpc_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> {
> int rc;
> struct controller *ctrl;
>
> - if (!is_shpc_capable(pdev))
> + if (acpi_get_hp_hw_control_from_firmware(pdev))
> return -ENODEV;
>
> ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL);
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index bbc4ea70505a..fd1c0ee33805 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -408,6 +408,14 @@ bool shpchp_is_native(struct pci_dev *bridge)
> if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
> return false;
>
> + /*
> + * It is assumed that AMD GOLAM chips support SHPC but they do not
> + * have SHPC capability.
> + */
> + if (bridge->vendor == PCI_VENDOR_ID_AMD &&
> + bridge->device == PCI_DEVICE_ID_AMD_GOLAM_7450)
> + return true;
> +
> if (!pci_find_capability(bridge, PCI_CAP_ID_SHPC))
> return false;
>
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 883cb7bf78aa..5aace6cca0d7 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -561,6 +561,7 @@
> #define PCI_DEVICE_ID_AMD_OPUS_7443 0x7443
> #define PCI_DEVICE_ID_AMD_VIPER_7443 0x7443
> #define PCI_DEVICE_ID_AMD_OPUS_7445 0x7445
> +#define PCI_DEVICE_ID_AMD_GOLAM_7450 0x7450
> #define PCI_DEVICE_ID_AMD_8111_PCI 0x7460
> #define PCI_DEVICE_ID_AMD_8111_LPC 0x7468
> #define PCI_DEVICE_ID_AMD_8111_IDE 0x7469
