Message-ID:
 <PH0PR18MB4425BE8D3F5F9C5114D8C289D98F2@PH0PR18MB4425.namprd18.prod.outlook.com>
Date: Thu, 22 Aug 2024 14:49:39 +0000
From: Shijith Thotton <sthotton@...vell.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
CC: "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "linux-pci@...r.kernel.org"
	<linux-pci@...r.kernel.org>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        Jerin Jacob <jerinj@...vell.com>,
        Srujana
 Challa <schalla@...vell.com>,
        Vamsi Krishna Attunuru <vattunuru@...vell.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        D Scott Phillips
	<scott@...amperecomputing.com>,
        Philipp Stanner <pstanner@...hat.com>
Subject: Re: [PATCH] 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.
>>
>> 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>
>> Signed-off-by: Vamsi Attunuru <vattunuru@...vell.com>
>I was curious, so drive by review.
>
>What was Vamsi's involvement?  Given Shijith sent this
>I'd guess co developer?  In which Co-developed-by tag
>should be used.
>

Will add the co-developer tag.

>Various random comments on things inline.
>
>> ---
>>
>> 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.
>>
>> 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 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.
>>
>>  MAINTAINERS                    |   6 +
>>  drivers/pci/hotplug/Kconfig    |  10 +
>>  drivers/pci/hotplug/Makefile   |   1 +
>>  drivers/pci/hotplug/octep_hp.c | 351
>+++++++++++++++++++++++++++++++++
>>  4 files changed, 368 insertions(+)
>>  create mode 100644 drivers/pci/hotplug/octep_hp.c
>>
>
>
>> diff --git a/drivers/pci/hotplug/octep_hp.c b/drivers/pci/hotplug/octep_hp.c
>> new file mode 100644
>> index 000000000000..efeb542d4993
>> --- /dev/null
>> +++ b/drivers/pci/hotplug/octep_hp.c
>> @@ -0,0 +1,351 @@
>
>> +struct octep_hp_controller {
>> +	void __iomem *base;
>> +	struct pci_dev *pdev;
>> +	struct work_struct work;
>> +	struct list_head slot_list;
>> +	struct mutex slot_lock; /* Protects slot_list */
>> +	struct list_head hp_cmd_list;
>> +	spinlock_t hp_cmd_lock; /* Protects hp_cmd_list */
>> +};
>
>> +static void octep_hp_disable_pdev(struct octep_hp_controller *hp_ctrl,
>> +				  struct octep_hp_slot *hp_slot)
>> +{
>> +	mutex_lock(&hp_ctrl->slot_lock);
>
>guard(mutex)(&hp_ctrl->slot_lock);
>
>Same for other simple cases where we can avoid explicit unlock in error paths
>+ main path.
>
>

Will change to use guard.

>> +	if (!hp_slot->hp_pdev) {
>> +		dev_dbg(&hp_ctrl->pdev->dev, "Slot %u already disabled\n",
>hp_slot->slot_number);
>> +		mutex_unlock(&hp_ctrl->slot_lock);
>> +		return;
>> +	}
>> +
>> +	dev_dbg(&hp_slot->hp_pdev->dev, "Disabling slot %u\n", hp_slot-
>>slot_number);
>> +
>> +	/* Remove the device from the bus */
>> +	pci_stop_and_remove_bus_device_locked(hp_slot->hp_pdev);
>> +	hp_slot->hp_pdev = NULL;
>> +	mutex_unlock(&hp_ctrl->slot_lock);
>> +}
>
>> +#define SLOT_NAME_SIZE 16
>> +static int octep_hp_register_slot(struct octep_hp_controller *hp_ctrl, struct
>pci_dev *pdev,
>> +				  u16 slot_number)
>> +{
>> +	char slot_name[SLOT_NAME_SIZE];
>> +	struct octep_hp_slot *hp_slot;
>> +	int ret;
>> +
>> +	hp_slot = kzalloc(sizeof(*hp_slot), GFP_KERNEL);
>> +	if (!hp_slot)
>> +		return -ENOMEM;
>> +
>> +	hp_slot->ctrl = hp_ctrl;
>> +	hp_slot->hp_pdev = pdev;
>> +	hp_slot->hp_devfn = pdev->devfn;
>> +	hp_slot->slot_number = slot_number;
>> +	hp_slot->slot.ops = &octep_hp_slot_ops;
>> +
>> +	snprintf(slot_name, SLOT_NAME_SIZE, "octep_hp_%u", slot_number);
>> +	ret = pci_hp_register(&hp_slot->slot, hp_ctrl->pdev->bus,
>PCI_SLOT(pdev->devfn), slot_name);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "Failed to register hotplug slot %u\n",
>slot_number);
>> +		kfree(hp_slot);
>> +		return ret;
>		return dev_err_probe() in here as well.
>

Will change.

>> +	}
>> +
>> +	octep_hp_disable_pdev(hp_ctrl, hp_slot);
>> +	list_add_tail(&hp_slot->list, &hp_ctrl->slot_list);
>> +
>> +	return 0;
>> +}
>> +
>> +static bool octep_hp_slot(struct octep_hp_controller *hp_ctrl, struct pci_dev
>*pdev)
>> +{
>> +	/* Check if the PCI device can be hotplugged */
>> +	return pdev != hp_ctrl->pdev && pdev->bus == hp_ctrl->pdev->bus &&
>> +		PCI_SLOT(pdev->devfn) == PCI_SLOT(hp_ctrl->pdev->devfn);
>> +}
>> +
>> +static void octep_hp_cmd_handler(struct octep_hp_controller *hp_ctrl,
>struct octep_hp_cmd *hp_cmd)
>
>Line break as that's getting long for little reason.
>

Okay. Will also change other places that cross  the 80 mark.

>> +{
>> +	struct octep_hp_slot *hp_slot;
>> +
>> +	/* Enable or disable the slots based on the slot mask */
>> +	list_for_each_entry(hp_slot, &hp_ctrl->slot_list, list) {
>> +		if (hp_cmd->slot_mask & BIT(hp_slot->slot_number)) {
>> +			if (hp_cmd->vec_type == OCTEP_HP_VEC_ENA)
>> +				octep_hp_enable_pdev(hp_ctrl, hp_slot);
>> +			else
>> +				octep_hp_disable_pdev(hp_ctrl, hp_slot);
>> +		}
>> +	}
>> +}
>> +
>> +static void octep_hp_work_handler(struct work_struct *work)
>> +{
>> +	struct octep_hp_controller *hp_ctrl = container_of(work, struct
>octep_hp_controller, work);
>
>Add a line break.
>

Okay.

>> +	struct octep_hp_cmd *hp_cmd;
>> +	unsigned long flags;
>
>...
>
>> +
>> +static void octep_hp_deregister_slots(struct octep_hp_controller *hp_ctrl)
>> +{
>> +	struct octep_hp_slot *hp_slot, *tmp;
>> +
>> +	/* Deregister all the hotplug slots */
>> +	list_for_each_entry_safe(hp_slot, tmp, &hp_ctrl->slot_list, list) {
>> +		pci_hp_deregister(&hp_slot->slot);
>> +		octep_hp_enable_pdev(hp_ctrl, hp_slot);
>> +		list_del(&hp_slot->list);
>> +		kfree(hp_slot);
>
>As below, maybe just register cleanup for one slot at a time.
>That will need a bit of magic to get form the slot to the hp_ctrl though.
>
>Plus point is simpler logic to follow.
>
>

Will change.

>> +	}
>> +}
>> +
>> +static void octep_hp_controller_cleanup(void *data)
>> +{
>> +	struct octep_hp_controller *hp_ctrl = data;
>> +
>> +	pci_free_irq_vectors(hp_ctrl->pdev);
>> +	flush_work(&hp_ctrl->work);
>I'd prefer to see this broken up so we have
>simple steps.
>1. Add action 1 in probe.
>2. Add cleanup for action 1 in probe
>3. Add action 2 in probe etc.
>
>Otherwise it can be a bit tricky to review.
>

Will break the cleanup to IRQ and slot specific.

>> +	octep_hp_deregister_slots(hp_ctrl);
>
>This is particular is no where near the place where the
>slots are registered.
>May be better to register one cleanup function call
>per slot as then it will also be nice and simple to follow.
>
>

Will change to cleanup per slot.

>> +}
>> +
>> +static int octep_hp_controller_setup(struct pci_dev *pdev, struct
>octep_hp_controller *hp_ctrl)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	int ret;
>> +
>> +	ret = pcim_enable_device(pdev);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to enable PCI device\n");
>Only called from probe, so
>		return dev_err_probe()
>
>> +		return ret;
>> +	}
>> +
>> +	ret = pcim_iomap_regions(pdev, BIT(0), OCTEP_HP_DRV_NAME);
>Given there is a patch on list deprecating this, reconsider.
> https://lore.kernel.org/all/20240807083018.8734-2-pstanner@redhat.com/ 
>+CC Philipp
>

Okay. The API is available in origin/devres branch.  Will rebase on top of it.

>> +	if (ret) {
>> +		dev_err(dev, "Failed to request MMIO region\n");
>> +		return ret;
>
>Same thing on return dev_err_probe here and later.
>

Will change.

>> +	}
>> +
>> +	pci_set_master(pdev);
>> +	pci_set_drvdata(pdev, hp_ctrl);
>> +
>> +	INIT_LIST_HEAD(&hp_ctrl->slot_list);
>> +	INIT_LIST_HEAD(&hp_ctrl->hp_cmd_list);
>> +	mutex_init(&hp_ctrl->slot_lock);
>> +	spin_lock_init(&hp_ctrl->hp_cmd_lock);
>> +	INIT_WORK(&hp_ctrl->work, octep_hp_work_handler);
>> +
>> +	hp_ctrl->pdev = pdev;
>> +	hp_ctrl->base = pcim_iomap_table(pdev)[0];
>> +	if (!hp_ctrl->base) {
>> +		dev_err(dev, "Failed to get device resource map\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	ret = pci_alloc_irq_vectors(pdev, 1,
>OCTEP_HP_INTR_VECTOR(OCTEP_HP_VEC_DIS) + 1,
>> +				    PCI_IRQ_MSIX);
>> +	if (ret < 0) {
>> +		dev_err(dev, "Failed to alloc MSI-X vectors");
>> +		return ret;
>> +	}
>> +
>> +	ret = devm_add_action(dev, octep_hp_controller_cleanup, hp_ctrl);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to add action for controller cleanup\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = devm_request_irq(dev, pci_irq_vector(pdev,
>OCTEP_HP_INTR_VECTOR(OCTEP_HP_VEC_ENA)),
>> +			       octep_hp_intr_handler, 0, "octep_hp_ena",
>hp_ctrl);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to register slot enable interrupt
>handle\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = devm_request_irq(dev, pci_irq_vector(pdev,
>OCTEP_HP_INTR_VECTOR(OCTEP_HP_VEC_DIS)),
>> +			       octep_hp_intr_handler, 0, "octep_hp_dis", hp_ctrl);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to register slot disable interrupt
>handle\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +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;
>> +	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) {
>> +		dev_err(&pdev->dev, "Failed to setup octep controller\n");
>> +		return ret;
>return dev_err_probe()
>actually no. Drop it. There are more informative error prints for all the
>conditions. We probably don't care that the end result is this specific
>function fails (more than an irq request failed etc).
>

Okay.

>
>> +	}
>> +
>> +	/*
>> +	 * 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.
>> +	 */
>> +	for_each_pci_dev(tmp_pdev) {
>> +		if (octep_hp_slot(hp_ctrl, tmp_pdev)) {
>What IIpo said on this.  No one likes deep indent ;)
>> +			ret = octep_hp_register_slot(hp_ctrl, tmp_pdev,
>slot_number++);
>> +			if (ret) {
>> +				dev_err(&pdev->dev, "Failed to register
>hotplug slots.\n");
>> +				return ret;
>
>return dev_err_probe();
>
>Cleaner in general even if no chance of deferral.
>
>
>
>> +			}
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +#define OCTEP_DEVID_HP_CONTROLLER 0xa0e3
>> +static struct pci_device_id octep_hp_pci_map[] = {
>> +	{ PCI_DEVICE(PCI_VENDOR_ID_CAVIUM,
>OCTEP_DEVID_HP_CONTROLLER) },
>> +	{ 0 },
>
>{ }
>Is sufficient. I can't remember if that's common style in PCI or not though.
>

Will change.

>> +};

Thanks for the review.
Shijith

Powered by blists - more mailing lists