Message-ID:
 <SJ0PR18MB44293B3E1A4860D44DD5CD8FD95D2@SJ0PR18MB4429.namprd18.prod.outlook.com>
Date: Fri, 8 Nov 2024 12:17:22 +0000
From: Shijith Thotton <sthotton@...vell.com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "ilpo.jarvinen@...ux.intel.com" <ilpo.jarvinen@...ux.intel.com>,
        "Jonathan.Cameron@...wei.com" <Jonathan.Cameron@...wei.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "rafael@...nel.org"
	<rafael@...nel.org>,
        "scott@...amperecomputing.com"
	<scott@...amperecomputing.com>,
        Jerin Jacob <jerinj@...vell.com>,
        Srujana
 Challa <schalla@...vell.com>,
        Vamsi Krishna Attunuru <vattunuru@...vell.com>
Subject: Re: [PATCH v3] PCI: hotplug: Add OCTEON PCI hotplug controller driver

>> This patch introduces a PCI hotplug controller driver for the OCTEON
>> PCIe device, a multi-function PCIe device where the first function acts
>> as a hotplug controller. It is equipped with MSI-x interrupts to notify
>> the host of hotplug events from the OCTEON firmware.
>
>s/MSI-x/MSI-X/ to match spec and other uses

I will change in v4

>
>> The driver facilitates the hotplugging of non-controller functions
>> within the same device. During probe, non-controller functions are
>> removed and registered as PCI hotplug slots. The slots are added back
>> only upon request from the device firmware. The driver also allows the
>> enabling and disabling of the slots via sysfs slot entries, provided by
>> the PCI hotplug framework.
>>
>> Signed-off-by: Shijith Thotton <sthotton@...vell.com>
>> Co-developed-by: Vamsi Attunuru <vattunuru@...vell.com>
>> Signed-off-by: Vamsi Attunuru <vattunuru@...vell.com>
>
>This order is incorrect because the handler (Shijith in this case)
>should be last in the chain; see
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=v6.11#n396
>

I will change in v4.

>> This patch introduces a PCI hotplug controller driver for OCTEON PCIe hotplug
>> controller. The OCTEON PCIe device is a multi-function device where the first
>> function acts as a PCI hotplug controller.
>>
>>               +--------------------------------+
>>               |           Root Port            |
>>               +--------------------------------+
>>                               |
>>                              PCIe
>>                               |
>> +---------------------------------------------------------------+
>> |              OCTEON PCIe Multifunction Device                 |
>> +---------------------------------------------------------------+
>>             |                    |              |            |
>>             |                    |              |            |
>> +---------------------+  +----------------+  +-----+  +----------------+
>> |      Function 0     |  |   Function 1   |  | ... |  |   Function 7   |
>> | (Hotplug controller)|  | (Hotplug slot) |  |     |  | (Hotplug slot) |
>> +---------------------+  +----------------+  +-----+  +----------------+
>>             |
>>             |
>> +-------------------------+
>> |   Controller Firmware   |
>> +-------------------------+
>>
>> The hotplug controller driver facilitates the hotplugging of non-controller
>> functions in the same device. During the probe of the driver, the non-
>controller
>> function are removed and registered as PCI hotplug slots. They are added
>back
>> only upon request from the device firmware. The driver also allows the user
>to
>> enable/disable the functions using sysfs slot entries provided by PCI hotplug
>> framework.
>
>I think the diagram and this text would be useful in the commit log.
>
>I would rephrase "functions are removed ..." as "the driver removes
>functions" to make it clear that this is purely a software thing and
>there's no PCIe-level change.  Similar for adding them back.
>

I will update the commit log with the diagram and more wordings from above.

>> This solution adopts a hardware + software approach for several reasons:
>>
>> 1. To reduce hardware implementation cost. Supporting complete hotplug
>>    capability within the card would require a PCI switch implemented within.
>>
>> 2. In the multi-function device, non-controller functions can act as emulated
>>    devices. The firmware can dynamically enable or disable them at runtime.
>>
>> 3. Not all root ports support PCI hotplug. This approach provides greater
>>    flexibility and compatibility across different hardware configurations.
>
>The downside of all this is the need for special-purpose software,
>which slows things down (you need to develop it, others need to review
>it) and burdens everybody with ongoing maintenance, e.g., changes to
>PCI device removal, resource assignment, slot registration, etc. now
>have to consider another case.
>
>> The hotplug controller function is lightweight and is equipped with MSI-x
>> interrupts to notify the host about hotplug events. Upon receiving an
>> interrupt, the hotplug register is read, and the required function is enabled
>> or disabled.
>>
>> This driver will be beneficial for managing PCI hotplug events on the OCTEON
>> PCIe device without requiring complex hardware solutions.
>
>> +config HOTPLUG_PCI_OCTEONEP
>> +	bool "OCTEON PCI device Hotplug controller driver"
>
>s/Marvell PCI device/Cavium OCTEON PCI/ to match other entries.
>

Cavium was acquired by Marvell, but we are still using the PCI vendor ID  
`PCI_VENDOR_ID_CAVIUM`. I hope using Marvell is acceptable; please let me  
know if this poses any issues.

>This Kconfig file isn't completely sorted, but if you move this above
>SHPC, it will at least be closer.
>

I will move above SHPC.

>> +static int octep_hp_pci_probe(struct pci_dev *pdev,
>> +			      const struct pci_device_id *id)
>> +{
>> +	struct octep_hp_controller *hp_ctrl;
>> +	struct pci_dev *tmp_pdev = NULL;
>> +	struct octep_hp_slot *hp_slot;
>> +	u16 slot_number = 0;
>> +	int ret;
>> +
>> +	hp_ctrl = devm_kzalloc(&pdev->dev, sizeof(*hp_ctrl), GFP_KERNEL);
>> +	if (!hp_ctrl)
>> +		return -ENOMEM;
>> +
>> +	ret = octep_hp_controller_setup(pdev, hp_ctrl);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/*
>> +	 * Register all hotplug slots. Hotplug controller is the first function
>> +	 * of the PCI device. The hotplug slots are the remaining functions of
>> +	 * the PCI device. They are removed from the bus and are added back
>when
>> +	 * the hotplug event is triggered.
>
>"Logically removed," I guess?  Obviously the PCIe Link remains up
>since function 0 is still alive, and I assume these other functions
>would still respond to config accesses.
>

Yes, it's just a software removal. I will update the text.

>> +	 */
>> +	for_each_pci_dev(tmp_pdev) {
>
>Ugh.  We should avoid for_each_pci_dev() when possible.
>
>IIUC you only care about other functions of *this* device, and those
>functions should all be on the bus->devices list.  There are many
>instances of this, which I think would be sufficient:
>
>  list_for_each_entry(dev, &bus->devices, bus_list)
>

Okay. I will replace for_each_pci_dev() with list_for_each_entry_safe().

>> +		if (!octep_hp_is_slot(hp_ctrl, tmp_pdev))
>> +			continue;
>
>Which would make this check mostly unnecessary.

Yes, I will replace it with the check below to skip the controller function.
  if (tmp_pdev == pdev)
      continue;

>
>> +		hp_slot = octep_hp_register_slot(hp_ctrl, tmp_pdev,
>slot_number);
>> +		if (IS_ERR(hp_slot))
>> +			return dev_err_probe(&pdev->dev, PTR_ERR(hp_slot),
>> +					     "Failed to register hotplug slot
>%u\n",
>> +					     slot_number);
>> +
>> +		ret = devm_add_action(&pdev->dev, octep_hp_deregister_slot,
>> +				      hp_slot);
>> +		if (ret)
>> +			return dev_err_probe(&pdev->dev, ret,
>> +					     "Failed to add action for
>deregistering slot %u\n",
>> +					     slot_number);
>> +		slot_number++;
>> +	}
>
>AFAICS this driver logs nothing at all in dmesg (except for errors and
>a few dev_dbg() uses).  Seems like it might be useful to have some
>hints there about the hotplug controller existence, possibly the
>connection between the slot name used in sysfs and the PCI function,
>and maybe even hot-add and hot-remove events.
>

Okay. I’ll add more informative prints as mentioned above.

>> +	return 0;
>> +}
>> +
>> +#define OCTEP_DEVID_HP_CONTROLLER 0xa0e3
>
>Even though this is a private #define, I think something like
>PCI_DEVICE_ID_CAVIUM_OCTEP_HP_CTLR would be nice because that's the
>typical pattern of include/linux/pci_ids.h.
>

The same reason mentioned above for not using the name CAVIUM.
Is it  okay to use `PCI_DEVICE_ID_MARVELL_OCTEP_HP_CTLR`?

>> +static struct pci_device_id octep_hp_pci_map[] = {
>> +	{ PCI_DEVICE(PCI_VENDOR_ID_CAVIUM,
>OCTEP_DEVID_HP_CONTROLLER) },
>> +	{ },
>> +};

Thanks,
Shijith

Powered by blists - more mailing lists